image-pruning: dereference ImageStreamTags #16821

miminar · 2017-10-12T12:00:55Z

Create strong references to images for each pod/bc/dc/etc that uses <host>/<repo>:tag reference.

Resolves bz#1498604 and https://bugzilla.redhat.com/show_bug.cgi?id=1386917

Images can manually removed using oc delete. Image stream tags having references to these images become obsolete - we may delete them.

To be sure that we don't remove reference to image that has just been created (and we don't know about it), make sure to honor --keep-younger-than.

miminar · 2017-10-12T12:03:58Z

Based on #16806 and #15807 to make registry extended tests work.

Please review only commits NOT prefixed by extended:.

Let's try it out:
/test extended_image_registry
/test

/assign @dmage @legionus
/cc @bparees

miminar · 2017-10-12T12:06:48Z

pkg/image/prune/prune.go

+		return err
+	}
+	if err := p.addBuildsToGraph(options.Builds); err != nil {
+		// TODO: either make the other methods error out on image reference parsing or ignore this error


@legionus what do you think?

i'd prefer we report a warning if we can't parse an image reference, but don't prevent pruning. I think we had this conversation before, but my view is that if you don't have valid reference, we shouldn't care what you were trying to reference.

i'd prefer we report a warning if we can't parse an image reference, but don't prevent pruning. I think we had this conversation before, but my view is that if you don't have valid reference, we shouldn't care what you were trying to reference.

I agree. Changed to warning.

I disagree.

There can be two cases:

Docker change their policy on references (for example, they may allow the + symbol in tags). And we can change our policy on tags as well. In this case the old version of oc will fail to parse this reference, and it will happily remove images with such references ("graceful fail", yep).

Someone able to create a resource with an invalid image reference. This state is bad enough to start investigations. We shouldn't ignore bugs in our code.

if it's not a reference we can parse, then there can be no pods using it, by definition, no?

Whether that's a bug in our code or not doesn't really matter, if we can't parse it, it's not a reference.

And while i agree that the ability to create invalid references is bad(if it were to happen), the fact is the code paths are different (validation vs resolution) and pruning is not the place to reconcile/debug that.

Pruning is a client-side operation and it may have an outdated parser.

Whether that's a bug in our code or not doesn't really matter, if we can't parse it, it's not a reference.

@bparees But if you can't parse reference then you don't add a dependency and you can remove what is used right now.

Is not that a problem?

@miminar It's NAK for me.

miminar · 2017-10-12T12:12:54Z

pkg/image/prune/prune.go

@@ -455,18 +475,36 @@ func addPodSpecToGraph(g graph.Graph, spec *kapi.PodSpec, predecessor gonum.Node
 		}

 		if len(ref.ID) == 0 {
+			ref = ref.DockerClientDefaults()
+			if p.registryURL.Host != ref.RegistryURL().Host {


This is very fragile IMHO. There are many possible registry hosts that can be used to fetch images from the integrated registry. Currently, this considers just the one determined by the algorithm or passed with --registry-url, which may be totally wrong.

Shall we collect all the possible registry hosts from all the images when we build the graph and match the url against the collected set?

how would you build the set of possible registry hosts that represent the internal registry?

Collecting all hosts from all the images is not enough. In general we can't know which DockerImageReference uses the internal registry. One can pull images through the router.

Even now I can use three names for the integrated registry: 172.30.1.1:5000, docker-registry.default.svc and docker-registry.default.svc.cluster.local. But only one can be specified in p.registryURL.

right, that's why i'm asking how you could build the set of all possible hosts that represent the registry? it doesn't seem possible (or at least reasonably possible) since anyone can create a service or route that points to it and then use that.

So i'm trying to understand what you're proposing doing when you say:

Shall we collect all the possible registry hosts from all the images when we build the graph and match the url against the collected set?

Most of the images pushed to the integrated registry will have $OPENSHIFT_DEFAULT_REGISTRY host name in their dockerImageReference attribute. All of them will be managed (and having managed annotation). There may be even some older managed images having some obsolete registry address. These addresses are most probably being used inside a cluster in deployments, pods, daemonsets, etc.

For the external ones, it's much harder. All we have (sometimes) is the optional --registry-url flag.

What I had in mind is iterating over all managed images and get a set of internal addresses, add the registry-url and docker-registry.default.svc.cluster.local and compare image references against this set.

miminar · 2017-10-12T12:13:40Z

pkg/image/prune/prune.go

 			glog.V(4).Infof("%q has no image ID", container.Image)
+			node := p.g.Find(imagegraph.ImageStreamTagNodeName(makeISTagWithReference(&ref)))
+			if node == nil {
+				glog.V(4).Infof("no image stream tag found for %q - skipping", container.Image, ref.RegistryURL().Host, p.registryURL.Host)


Looks like this won't pass verification.

dmage · 2017-10-12T12:19:34Z

pkg/image/prune/prune.go

+			tagUpdated = true
+			continue
+		}
+		glog.V(4).Infof("Image stream tag %s:%s revision %d - keeping because of age", streamName, tag, i)


But it isn't kept.

Can you extract this part into func tagEventIsPrunable(tagEvent image.TagEvent, g, prunableImageNodes, keepYoungerThan) (ok bool, reason string)?

But it isn't kept.

Good catch, extracted.

dmage · 2017-10-12T12:28:36Z

pkg/image/prune/prune.go

+func (p *pruner) buildGraph(options PrunerOptions) error {
+	p.g = graph.New()
+
+	p.addImagesToGraph(options.Images, p.algorithm)


p.algorithm is a part of pruner and can be removed from arguments.

p.algorithm is a part of pruner and can be removed from arguments.

Removed.

dmage · 2017-10-12T13:23:07Z

pkg/image/prune/prune.go

@@ -351,28 +366,33 @@ func addImageStreamsToGraph(g graph.Graph, streams *imageapi.ImageStreamList, li
 					}
 				}

+				if i == 0 {
+					glog.V(4).Infof("Adding edge (kind=%s) from %q to %q", kind, istNode.UniqueName(), imageNode.UniqueName())
+					p.g.AddEdge(istNode, imageNode, kind)


Why do we need this?

Why do we need this?

In order to dereference imagestreamtag to image, there must be some kind of mapping between the two. It could be regular map[istagname]image. But since we already have the graph, we can utilize it. The fact is that ImageStreamTagNode takes unnecessarily too much memory - we could make the istnode a lot smaller by making it contain just a single string (istagname). Or would you prefer regular map?

bparees · 2017-10-12T16:47:11Z

pkg/image/prune/prune.go

-			age := metav1.Now().Sub(pod.CreationTimestamp.Time)
-			if age >= algorithm.keepYoungerThan {
-				glog.V(4).Infof("Pod %s is not running nor pending and age exceeds keepYoungerThan (%v) - skipping", getName(pod), age)
+			if !pod.CreationTimestamp.Time.After(algorithm.keepYoungerThan) {


I would not have expected --keep-younger-than to apply to pod ages, just image ages. is this well documented?

--keep-younger-than=1h0m0s: Specify the minimum age of an image for it to be considered a candidate for pruning.

I understand the motivation, but i think the behavior would surprise people. I would prefer that we either said "stopped pods don't count as image references" or "prune your stopped pods before pruning your images" instead of this approach of overloading the keep-younger-than arg to "ignore some stopped pods, but count others"

It's poorly documented here. Our official docs say it better:

Do not prune any image that is younger than relative to the current time. Do not prune any image that is referenced by any other object that is younger than relative to the current time. (default 60m)

I'll fix it.

We shouldn't prune an image which is referenced by any other object, should we?

We shouldn't prune an image which is referenced by any other object, should we?

@dmage that's not what we've been doing. I agree that it would be the right thing to do to maintain the integrity. However, we don't have an easy to use pruning solution for all the objects that can reference images.

If we decide to keep the images referenced by any other object regardless of age, let's do it in some other PR. This PR makes it already harder to delete referenced images.

bparees · 2017-10-12T16:48:53Z

pkg/image/prune/prune.go

-			if age >= algorithm.keepYoungerThan {
-				glog.V(4).Infof("Pod %s is not running nor pending and age exceeds keepYoungerThan (%v) - skipping", getName(pod), age)
+			if !pod.CreationTimestamp.Time.After(algorithm.keepYoungerThan) {
+				glog.V(4).Infof("Pod %s is neither running nor pending and is too old", getName(pod))


it's not clear (out of context) what this message means. Update it to something like "Ignoring pod %s for image reference counting because it is not running/pending and is older than %age"

bparees · 2017-10-12T16:54:49Z

pkg/image/prune/prune.go

+		glog.V(4).Infof("Checking tag event %d with image %q", i, tagEvent.Image)
+
+		if ok, reason := tagEventIsPrunable(tagEvent, g, prunableImageNodes, keepYoungerThan); ok {
+			glog.V(4).Infof("Image stream tag %s:%s revision %d - removing because %s", streamName, tag, i, reason)


print the namespace for the imagestream too.

The namespace is already part of the streamName: https://github.com/openshift/origin/blob/4fc5ca3/pkg/image/prune/prune.go#L1077

bparees · 2017-10-12T16:55:35Z

pkg/image/prune/prune.go

+			glog.V(4).Infof("Image stream tag %s:%s revision %d - removing because %s", streamName, tag, i, reason)
+			tagUpdated = true
+		} else {
+			glog.V(4).Infof("Image stream tag %s:%s revision %d - keeping because %s", streamName, tag, i, reason)


print the IS namespace too.

bparees · 2017-10-12T17:00:29Z

pkg/apps/graph/nodes/nodes.go

 )

+// EnsureDaemonSetNode adds the provided deployment config to the graph if it does not exist


s/deployment config/daemon set/

bparees · 2017-10-12T17:04:32Z

pkg/apps/graph/nodes/nodes.go

@@ -92,3 +92,30 @@ func FindOrCreateSyntheticDeploymentConfigNode(g osgraph.MutableUniqueGraph, dc
 		},
 	).(*DeploymentConfigNode)
 }
+
+// EnsureReplicaSetNode adds the provided deployment config to the graph if it does not exist


s/deployment config/replicaset/

bparees · 2017-10-12T18:22:22Z

pkg/image/prune/prune.go

@@ -455,18 +475,36 @@ func addPodSpecToGraph(g graph.Graph, spec *kapi.PodSpec, predecessor gonum.Node
 		}

 		if len(ref.ID) == 0 {
+			ref = ref.DockerClientDefaults()
+			if p.registryURL.Host != ref.RegistryURL().Host {


how would you build the set of possible registry hosts that represent the internal registry?

bparees · 2017-10-12T18:41:55Z

pkg/image/prune/prune.go

@@ -455,18 +475,36 @@ func addPodSpecToGraph(g graph.Graph, spec *kapi.PodSpec, predecessor gonum.Node
 		}

 		if len(ref.ID) == 0 {
+			ref = ref.DockerClientDefaults()


please add a comment here explaining what is happening.

My understanding is something like:

If the pod container spec image reference does not contain a digest(e.g. it i just "docker.io/foo/bar:latest"), attempt to determine an image digest by looking for imagestreamtags that point to the image referenced by this pod container spec, and create a reference between the pod and that image to prevent the image from being pruned.

bparees · 2017-10-12T18:44:00Z

/unassign @knobunc
/assign

dmage

NAK

legionus

NAK

miminar · 2017-10-23T13:14:07Z

/hold

miminar · 2017-10-23T13:14:39Z

~~@dmage @legionus can you please elaborate?~~

Update: The point is to error out on invalid image reference. All errors should be collected and reported during the building of graph.

Reason: consider a more benevolent reference parser (e.g. allowing + in the reference). Now consider old oadm client running against newer cluster having images with such a reference. The pruner would ignore such references leading to undesired removals.

~~I'm reworking it. ~~
Update: reworked.

miminar · 2017-10-25T15:53:39Z

All parsing errors are now collected and reported during the graph build.

~~I still need to verify the error reporting once I find out, how to inject bad image references into etcd.~~
Update: I wrote a unit test to capture this.

miminar · 2017-10-26T17:25:55Z

/test extended_image_registry

miminar · 2017-10-27T05:35:49Z

/hold cancel

dmage · 2017-10-30T10:10:28Z

pkg/image/prune/prune_test.go

-var (
-	config1 = "sha256:2b8fd9751c4c0f5dd266fcae00707e67a2545ef34f9a29354585f93dac906749"
-	config2 = "sha256:8ddc19f16526912237dd8af81971d5e4dd0587907234be2b83e249518d5b673f"
+	_ "github.com/openshift/origin/pkg/apps/apis/apps/install"


Is it for getRef?

~~Yes, I'll put a comment there~~

Update: commented.

dmage · 2017-10-30T10:30:43Z

pkg/oc/admin/prune/images.go

+		if closed && err != nil {
+			return nil, nil, err
+		}
+	case <-time.After(time.Second):


Why do you need this timeout?

The code is inspiried by

origin/pkg/oc/cli/cmd/version.go

Line 95 in 50389c0

func (o VersionOptions) RunVersion() error {

. The oc version asks the master for its version with some default timeout.

This call will be much faster than most of the other calls asking for object lists. On the other hand, I don't want to further block error reporting once we know we have failed and master is busy.

Update: changed it to get the timeout from client config

Create strong references to images for each pod/bc/dc/etc that uses <host>/<repo>:tag reference. Resolves bug 1498604 Signed-off-by: Michal Minář <[email protected]>

dmage

/approve
but I want one more opinion for lgtm :)

bparees · 2017-10-30T22:00:25Z

/lgtm

openshift-merge-robot · 2017-10-30T22:00:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, dmage, miminar

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/apps/OWNERS~~ [bparees]
~~pkg/cmd/OWNERS~~ [bparees]
~~pkg/image/OWNERS~~ [bparees]
~~pkg/oc/OWNERS~~ [bparees]
~~test/testdata/OWNERS~~ [bparees]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-bot · 2017-10-31T02:38:17Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-merge-robot · 2017-10-31T06:58:04Z

Automatic merge from submit-queue.

smarterclayton · 2017-10-31T13:16:25Z

Did this start flaking in master? https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/17035/test_pull_request_origin_unit/5039/ On Oct 31, 2017, at 2:58 AM, OpenShift Merge Robot <[email protected]> wrote: Merged #16821 <#16821>. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16821 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p5EoZWfroSswPhSOr_VnBtT1g0XPks5sxsUAgaJpZM4P25ZY> .

miminar · 2017-10-31T15:13:20Z

Did this start flaking in master?

Yes, sorry about that. Here's a fix: #17105

openshift-merge-robot assigned knobunc and smarterclayton Oct 12, 2017

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 12, 2017

openshift-ci-robot assigned dmage and legionus Oct 12, 2017

openshift-ci-robot requested a review from bparees October 12, 2017 12:04

miminar commented Oct 12, 2017

View reviewed changes

dmage reviewed Oct 12, 2017

View reviewed changes

miminar changed the title ~~image-pruning: dereference ImageStreamTags~~ [WIP] image-pruning: dereference ImageStreamTags Oct 12, 2017

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 12, 2017

miminar force-pushed the image-pruning-dereference-istags branch 5 times, most recently from 3fc7551 to 52d9c44 Compare October 12, 2017 17:47

bparees reviewed Oct 12, 2017

View reviewed changes

openshift-ci-robot assigned bparees and unassigned knobunc Oct 12, 2017

miminar force-pushed the image-pruning-dereference-istags branch from 52d9c44 to b89cba8 Compare October 12, 2017 19:08

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 15, 2017

miminar force-pushed the image-pruning-dereference-istags branch from b89cba8 to 99e3193 Compare October 16, 2017 08:25

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 16, 2017

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 16, 2017

miminar force-pushed the image-pruning-dereference-istags branch from 99e3193 to d75b1dc Compare October 20, 2017 13:54

openshift-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 20, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2017

bparees added the kind/bug Categorizes issue or PR as related to a bug. label Oct 23, 2017

dmage suggested changes Oct 23, 2017

View reviewed changes

legionus suggested changes Oct 23, 2017

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 23, 2017

miminar force-pushed the image-pruning-dereference-istags branch from c1ae9c6 to 4267f2a Compare October 25, 2017 15:51

openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 25, 2017

miminar force-pushed the image-pruning-dereference-istags branch 2 times, most recently from cc695ab to 4d5a231 Compare October 26, 2017 15:51

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2017

dmage reviewed Oct 30, 2017

View reviewed changes

image-pruning: derefence imagestreamtags

1cda0bb

Create strong references to images for each pod/bc/dc/etc that uses <host>/<repo>:tag reference. Resolves bug 1498604 Signed-off-by: Michal Minář <[email protected]>

miminar force-pushed the image-pruning-dereference-istags branch from 4d5a231 to 1cda0bb Compare October 30, 2017 12:20

dmage approved these changes Oct 30, 2017

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 30, 2017

openshift-merge-robot merged commit 6b1a836 into openshift:master Oct 31, 2017

miminar mentioned this pull request Oct 31, 2017

unit-tests: fixed flake and race in image pruning #17105

Merged

miminar deleted the image-pruning-dereference-istags branch November 8, 2017 09:54

		)

		// EnsureDaemonSetNode adds the provided deployment config to the graph if it does not exist

image-pruning: dereference ImageStreamTags #16821

image-pruning: dereference ImageStreamTags #16821

Conversation

miminar commented Oct 12, 2017 • edited by bparees Loading

miminar commented Oct 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmage Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bparees commented Oct 12, 2017

dmage left a comment

Choose a reason for hiding this comment

legionus left a comment

Choose a reason for hiding this comment

miminar commented Oct 23, 2017

miminar commented Oct 23, 2017 • edited Loading

miminar commented Oct 25, 2017 • edited Loading

miminar commented Oct 26, 2017

miminar commented Oct 27, 2017

Choose a reason for hiding this comment

miminar Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miminar Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

dmage left a comment

Choose a reason for hiding this comment

bparees commented Oct 30, 2017

openshift-merge-robot commented Oct 30, 2017

openshift-bot commented Oct 31, 2017

openshift-merge-robot commented Oct 31, 2017

smarterclayton commented Oct 31, 2017 via email

miminar commented Oct 31, 2017

miminar commented Oct 12, 2017 •

edited by bparees

Loading

dmage Oct 23, 2017 •

edited

Loading

miminar commented Oct 23, 2017 •

edited

Loading

miminar commented Oct 25, 2017 •

edited

Loading

miminar Oct 30, 2017 •

edited

Loading

miminar Oct 30, 2017 •

edited

Loading