Prune image configs of manifest V2 schema 2 #10835

miminar · 2016-09-07T12:50:28Z

A follow-up for #10805.

Resolves #10375

miminar · 2016-09-07T12:51:45Z

I'm working on extended test now. Otherwise ready for review. /cc @soltysh, @legionus

[test]

soltysh · 2016-09-08T13:36:39Z

pkg/image/prune/prune.go

 	// ReferencedImageLayerEdgeKind defines an edge from an ImageStreamNode or an
-	// ImageNode to an ImageLayerNode.
+	// ImageNode to an ImageComponentNode.
 	ReferencedImageLayerEdgeKind = "ReferencedImageLayer"


If you're replacing all, this should be renamed to ReferencedImageComponent, right?

Actually on second thought, you renamed the ImageLayerNode in the comment unnecessarily.

Well, I'm differentiating whether the image refers to the component as its layer or as its config. I'm using different edges for both cases. The edges don't serve any purpose later on. So instead of two, we could have just one ReferencedImageComponent, as you suggest. I kind of like the differentiation though.

I don't mind the differentiation. The first comment was too quick, I've later noticed you've renamed the variable in the comment but all the rest is left. I'm just saying one should reflect the other.

soltysh · 2016-09-09T12:48:37Z

In the end I'm still wondering if having the separation between layers and configs worth the trouble. They both end up being treated in the same way, we just need to make sure when adding config to the graph not to added if a layer was already added, right?

miminar · 2016-09-09T14:36:46Z

In the end I'm still wondering if having the separation between layers and configs worth the trouble. They both end up being treated in the same way, we just need to make sure when adding config to the graph not to added if a layer was already added, right?

For debugging purposes, I'd like to know whether the digest being deleted is a layer or a config. Also in the future, we might prefer to keep the configs only in etcd. Keeping some level of separation now will make it easier.

miminar · 2016-09-09T14:53:34Z

Added extended test.

miminar · 2016-09-09T14:55:34Z

[testextended][extended:core(images)][extended:core(builds)][extended:core(imageapis)]

soltysh · 2016-09-21T14:11:38Z

test/extended/util/framework.go

+//  -1 if rc a is older than b
+//   1 if rc a is newer than b
+//   0 if their names are the same
+func cmpResourceControllerNames(a, b string) int {


Use full names compareResourceControllerNames

soltysh · 2016-09-21T14:12:44Z

test/extended/util/framework.go

@@ -870,6 +965,20 @@ var CheckPodIsSucceededFn = func(pod kapi.Pod) bool {
 	return pod.Status.Phase == kapi.PodSucceeded
 }

+// CheckPodIsReadyFn returns true if the pod's ready probe determined that the pod is ready.
+var CheckPodIsReadyFn = func(pod kapi.Pod) bool {


Can this be a regular function, like:

func CheckPodIsReadyFn(pod kapi.Pod) bool { ...

why this construct?

It could, I'm just following a convention used to define other callback function defined in this file. See

origin/test/extended/util/framework.go

Line 964 in 313c88c

var CheckPodIsSucceededFn = func(pod kapi.Pod) bool {

,

origin/test/extended/util/framework.go

Line 492 in 313c88c

var CheckBuildSuccessFn = func(b *buildapi.Build) bool {

,

origin/test/extended/util/framework.go

Line 600 in 313c88c

var CheckImageStreamLatestTagPopulatedFn = func(i *imageapi.ImageStream) bool {

as examples. I'm not really sure why, but I'd rather stay consistent.

soltysh · 2016-09-21T14:16:12Z

test/extended/images/prune.go

+		}
+	}
+
+	testPruneImagesOfSchema := func(version int) {


Turn this into regular function as well.

soltysh · 2016-09-21T14:16:38Z

test/extended/images/prune.go

+		o.Expect(err).NotTo(o.HaveOccurred())
+	})
+
+	tearDown := func(cleanUp *cleanUpContainer) {


Same - regular function.

soltysh · 2016-09-21T14:19:33Z

test/extended/images/prune.go

+		return false, err
+	}
+
+	return strings.Contains(env, "REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_ACCEPTSCHEMA2=true"), nil


Can you use a constant here?

I'm talking about this

origin/pkg/dockerregistry/server/repositorymiddleware.go

Line 51 in aa08313

AcceptSchema2EnvVar = "REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_ACCEPTSCHEMA2"

Sure, I'll use the constant.

soltysh

I've left some comments, mind addressing them?

soltysh · 2016-09-21T14:21:21Z

test/extended/images/prune.go

+		return err
+	}
+
+	value := fmt.Sprintf("REGISTRY_MIDDLEWARE_REPOSITORY_OPENSHIFT_ACCEPTSCHEMA2=%t", accept)


Same env, please.

soltysh · 2016-09-21T14:23:32Z

test/extended/images/prune.go

+// ensureRegistryAcceptsSchema2 checks whether the registry is configured to accept manifests V2 schema 2 or
+// not. If the result doesn't match given accept argument, registry's deployment config is updated accordingly
+// and the function blocks until the registry is re-deployed and ready for new requests.
+func ensureRegistryAcceptsSchema2(oc *exutil.CLI, accept bool) error {


The only worry I have here, is that we might leave the registry in a state different than at the beginning, you are aware that this might influence other tests in the future? How about reverting it back to is original?

See

origin/test/extended/images/prune.go

Line 170 in 313c88c

}

and

origin/test/extended/images/prune.go

Line 189 in 313c88c

}

.

miminar · 2016-09-22T09:08:24Z

I've left some comments, mind addressing them?

Of course! Always a pleasure.

LayerDeleter suggests it is usable to delete just image layers. It is suitable to delete image configs as well. In fact, it doesn't really remove the layers, it just removes layer links from desired repository. This LayerLinkDeleter is a more appropriate. Signed-off-by: Michal Minář <[email protected]>

Resolves openshift#10375 Signed-off-by: Michal Minář <[email protected]>

Renamed LayerNode to ComponentNode because it now identifies image configs as well. The ComponentNode still needs to contain an information whether it represents config or layer for proper logging. The ComponentNode can now be connected to ImageNode or ImageStreamNode using either ImageLayerEdge or ImageConfigEdge. Signed-off-by: Michal Minář <[email protected]>

Add a utility function WaitForRegistry to wait for a re-deployed registry pod to become ready. Also make sure to support more than 9 re-deployments. Signed-off-by: Michal Minář <[email protected]>

One test for manifest V2 schema 1 and one test for manifest V2 schema 2. Signed-off-by: Michal Minář <[email protected]>

openshift-bot · 2016-09-22T11:47:27Z

Evaluated for origin test up to 0bc9d63

openshift-bot · 2016-09-22T11:47:36Z

Evaluated for origin testextended up to 0bc9d63

soltysh

LGTM [merge]

openshift-bot · 2016-09-22T13:42:30Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9132/)

miminar · 2016-09-22T13:55:17Z

Flake #10987

openshift-bot · 2016-09-22T14:10:13Z

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/506/) (Extended Tests: core(images))

soltysh · 2016-09-22T14:49:47Z

[merge]

soltysh · 2016-09-26T12:10:32Z

Flake #11079.

[merge]

openshift-bot · 2016-09-26T12:12:24Z

Evaluated for origin merge up to 0bc9d63

openshift-bot · 2016-09-26T12:12:35Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9280/) (Image: devenv-rhel7_5075)

miminar · 2016-09-26T14:51:46Z

@soltysh you made it happen! 🎆 🎉

Sorry for the pain.

soltysh · 2016-09-26T16:52:38Z

miminar force-pushed the prune-manifest-config-of-schema2 branch from a28053f to a5bacbe Compare September 7, 2016 13:55

soltysh reviewed Sep 8, 2016
View reviewed changes

miminar force-pushed the prune-manifest-config-of-schema2 branch from a5bacbe to 55c8ea8 Compare September 9, 2016 14:52

miminar changed the title ~~[WIP] Prune image configs of manifest V2 schema 2~~ Prune image configs of manifest V2 schema 2 Sep 9, 2016

miminar force-pushed the prune-manifest-config-of-schema2 branch 4 times, most recently from 919493f to 27a86b6 Compare September 9, 2016 15:16

miminar mentioned this pull request Sep 9, 2016

Extended.[images] openshift limit range admission should deny a push of built image exceeding limit on openshift.io/images resource #10375

Closed

miminar force-pushed the prune-manifest-config-of-schema2 branch 3 times, most recently from d37c79f to 313c88c Compare September 10, 2016 14:36

soltysh reviewed Sep 21, 2016

View reviewed changes

soltysh suggested changes Sep 21, 2016

View reviewed changes

soltysh self-assigned this Sep 21, 2016

soltysh added component/imageregistry component/image labels Sep 21, 2016

miminar force-pushed the prune-manifest-config-of-schema2 branch from 313c88c to 6c36b2d Compare September 22, 2016 09:07

Michal Minář added 4 commits September 22, 2016 13:45

extended: Fix binary build helper

110ef69

Resolves openshift#10375 Signed-off-by: Michal Minář <[email protected]>

extended: Allow to re-deploy registry pod

2b62a8b

Add a utility function WaitForRegistry to wait for a re-deployed registry pod to become ready. Also make sure to support more than 9 re-deployments. Signed-off-by: Michal Minář <[email protected]>

extended: Added new image pruning test

0bc9d63

One test for manifest V2 schema 1 and one test for manifest V2 schema 2. Signed-off-by: Michal Minář <[email protected]>

miminar force-pushed the prune-manifest-config-of-schema2 branch from 6c36b2d to 0bc9d63 Compare September 22, 2016 11:45

soltysh approved these changes Sep 22, 2016

View reviewed changes

openshift-bot merged commit a400f5a into openshift:master Sep 26, 2016

miminar deleted the prune-manifest-config-of-schema2 branch November 10, 2016 15:40

Prune image configs of manifest V2 schema 2 #10835

Prune image configs of manifest V2 schema 2 #10835

Conversation

miminar commented Sep 7, 2016 • edited Loading

miminar commented Sep 7, 2016

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

soltysh commented Sep 9, 2016

miminar commented Sep 9, 2016

miminar commented Sep 9, 2016

miminar commented Sep 9, 2016

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

soltysh left a 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

miminar commented Sep 22, 2016

openshift-bot commented Sep 22, 2016

openshift-bot commented Sep 22, 2016

soltysh left a comment

Choose a reason for hiding this comment

openshift-bot commented Sep 22, 2016

miminar commented Sep 22, 2016

openshift-bot commented Sep 22, 2016

soltysh commented Sep 22, 2016

soltysh commented Sep 26, 2016

openshift-bot commented Sep 26, 2016

openshift-bot commented Sep 26, 2016 • edited Loading

miminar commented Sep 26, 2016

soltysh commented Sep 26, 2016

miminar commented Sep 7, 2016 •

edited

Loading

openshift-bot commented Sep 26, 2016 •

edited

Loading