Take care of manifest config blob #10759

miminar · 2016-09-01T12:13:58Z

Resolves #10730

Manifest V2 schema 2 introduces a special blob called config which used to be emeedded in earlier schema. See upstream's spec for details. The config is stored as a regular blob on registry's storage. Thus the config needs to be treated similar to a regular layer:

the pullthrough middleware needs to be able to fetch it from remote repository
we need to increase a size of image for the size of its config
the config's digest needs to be cached as a regular layer inside a registry for subsequent lookups
image pruning needs to prune it the same way as regular layer

The PR addresses the first 3 items. I'd like to cover the pruning case in a follow-up.

miminar · 2016-09-01T12:15:41Z

Not verified and untested. I'm working on it. But ready for review (cc @legionus, @soltysh).

[test]

mfojtik · 2016-09-01T13:02:40Z

@liggitt can you help review this?

miminar · 2016-09-01T13:21:54Z

The pullthrough part appears to be working now. Verifying a pull of local image of schema 2 tagged to different namespace now.

miminar · 2016-09-01T13:43:35Z

And the second part (pull of local image of schema 2 tagged to different namespace) seems to be working as well. Writing tests now.

mfojtik · 2016-09-01T14:32:11Z

pkg/dockerregistry/server/pullthroughblobstore.go

+func (r *pullthroughBlobStore) Get(ctx context.Context, dgst digest.Digest) ([]byte, error) {
+	store, ok := r.digestToStore[dgst.String()]
+	if !ok {
+		data, getErr := r.BlobStore.Get(ctx, dgst)


you can just use err here

you can just use err here

OK, I'll rename.

mfojtik · 2016-09-01T14:34:57Z

this need tests (but I think you're already working on them)

liggitt · 2016-09-01T14:49:31Z

pkg/dockerregistry/server/pullthroughblobstore.go

@@ -130,6 +136,28 @@ func (r *pullthroughBlobStore) ServeBlob(ctx context.Context, w http.ResponseWri
 	return nil
 }

+// Get attempts to fetch the requested blob by digest using a remote proxy store if necessary.
+func (r *pullthroughBlobStore) Get(ctx context.Context, dgst digest.Digest) ([]byte, error) {
+	store, ok := r.digestToStore[dgst.String()]


return early in the ok case and unindent the rest of the function?

if store, ok := r.digestToStore[dgst.String()]; ok { return store.Get(ctx, desc.Digest) }

pullthroughBlobStore instances are per-request, right? just making sure we don't have to worry about locking/races on the digestToStore map, or about long-term growth/retention

return early in the ok case and unindent the rest of the function?

good suggestion, I'll rewrite

pullthroughBlobStore instances are per-request, right?

That's right.

liggitt · 2016-09-01T19:17:22Z

pkg/dockerregistry/server/repositorymiddleware.go

@@ -553,6 +557,10 @@ func (r *repository) rememberLayersOfImage(image *imageapi.Image, cacheName stri
 		for _, layer := range image.DockerImageLayers {
 			r.cachedLayers.RememberDigest(digest.Digest(layer.Name), r.blobrepositorycachettl, cacheName)
 		}
+		// remember reference to manifest config as well for schema 2
+		if image.DockerImageManifestMediaType == schema2.MediaTypeManifest && len(image.DockerImageMetadata.ID) > 0 {


should we only do this if len(image.DockerImageConfig) > 0?

should we only do this if len(image.DockerImageConfig) > 0?

To make it comply with the other conditions, I'll use this instead. image.DockerImageConfig is filled only for manifest schema 2.

soltysh · 2016-09-02T14:51:35Z

The changes LGTM, but tests is something I'd like to see before getting this merged.

Make sure to match image having config name equal to the wanted blob digest. Remember the image and allow access to its layers. Signed-off-by: Michal Minář <[email protected]>

Manifest configs are fetched using Get() method from blob store. Pullthrough middleware needs to override it as well to allow for pulling manifest v2 schema 2 images from remote repositories. Signed-off-by: Michal Minář <[email protected]>

miminar · 2016-09-02T16:15:32Z

I've got the e2e tests working finally.

miminar · 2016-09-02T17:48:58Z

Flake #9624, extended conformance flake:

with readiness probe should not be ready before initial delay and never restart [Conformance]

And

InsufficientInstanceCapacity => We currently do not have sufficient m4.large capacity in the Availability Zone you requested (us-east-1d). Our system will be working on provisioning additional capacity. You can currently get m4.large capacity by not specifying an Availability Zone in your request or choosing us-east-1b, us-east-1c.
+ echo ''\''vagrant up'\'' failed - retrying'

In https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/8662/.

liggitt · 2016-09-02T18:12:25Z

hack/util.sh

+		exit 1
+	fi
+	local podnamejs='{range .subsets[*]}{range .addresses[*]}{.targetRef.name},{end}{end}'
+	wait_for_command "oc get endpoints/docker-registry -o 'jsonpath=$podnamejs' --config='${ADMIN_KUBECONFIG}' | egrep -q '(^|,)${registrypod},'" $TIME_MIN


why is this necessary?

why is this necessary?

Not sure, I was just reluctant to remove the existing guards.

In my local test, this passes in no time:

[INFO] Success running command: 'oc get rc/docker-registry-1 --template "{{ index .metadata.annotations \"openshift.io/deployment.phase\" }}" --config='/tmp/openshift/test-end-to-end//openshift.local.config/master/admin.kubeconfig' | grep Complete' after 23 seconds [INFO] Waiting for command to finish: 'oc get endpoints/docker-registry -o 'jsonpath={range .subsets[*]}{range .addresses[*]}{.targetRef.name},{end}{end}' --config='/tmp/openshift/test-end-to-end//openshift.local.config/master/admin.kubeconfig' | egrep -q '(^|,)docker-registry-1-6mc18,''... [INFO] Success running command: 'oc get endpoints/docker-registry -o 'jsonpath={range .subsets[*]}{range .addresses[*]}{.targetRef.name},{end}{end}' --config='/tmp/openshift/test-end-to-end//openshift.local.config/master/admin.kubeconfig' | egrep -q '(^|,)docker-registry-1-6mc18,'' after 0 seconds [INFO] Waiting for command to finish: 'oc get 'pod/docker-registry-1-6mc18' -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}' --config='/tmp/openshift/test-end-to-end//openshift.local.config/master/admin.kubeconfig' | grep -qi true'... [INFO] Success running command: 'oc get 'pod/docker-registry-1-6mc18' -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}' --config='/tmp/openshift/test-end-to-end//openshift.local.config/master/admin.kubeconfig' | grep -qi true' after 0 seconds

@mfojtik, @Kargakis do you think it's safe to remove all the wait_for_command statements except for the first?

I meant all the additions to wait_for_registry, not just that line

I meant all the additions to wait_for_registry, not just that line

That's because I'm redeploying the registry. Until now, the wait_for_registry expected just rc version 1. Now it needs to deal with multiple versions of rcs, pods, etc.

I think it is fine to make sure that we are testing against the latest version of registry (in case the registry is re-deployed).

Here's a log from one of the recent runs:

[INFO] Waiting for command to finish: 'oc get rc/docker-registry-2 --template "{{ index .metadata.annotations \"openshift.io/deployment.phase\" }}" --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | grep Complete'... Complete [INFO] Success running command: 'oc get rc/docker-registry-2 --template "{{ index .metadata.annotations \"openshift.io/deployment.phase\" }}" --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | grep Complete' after 16 seconds [INFO] Waiting for command to finish: 'oc get endpoints/docker-registry -o 'jsonpath={range .subsets[*]}{range .addresses[*]}{.targetRef.name},{end}{end}' --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | egrep -q '(^|,)docker-registry-2-n8o46,''... [INFO] Success running command: 'oc get endpoints/docker-registry -o 'jsonpath={range .subsets[*]}{range .addresses[*]}{.targetRef.name},{end}{end}' --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | egrep -q '(^|,)docker-registry-2-n8o46,'' after 1 seconds [INFO] Waiting for command to finish: 'oc get pod -l deploymentconfig=docker-registry -o jsonpath='{.items[*].status.conditions[?(@.type=="Ready")].status}' --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | grep -qi true'... [INFO] Success running command: 'oc get pod -l deploymentconfig=docker-registry -o jsonpath='{.items[*].status.conditions[?(@.type=="Ready")].status}' --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | grep -qi true' after 0 seconds

The oc get endpoint needed additional 1 second to succeed after the deployment of dc-2 completed which IMHO justifies first two wait statements. I'm still not sure about the third (wait for a pod to become ready). I'd prefer to keep it though.

And, finally, here's a justification for the 3rd wait:

[INFO] Success running command: 'oc get endpoints/docker-registry -o 'jsonpath={range .subsets[*]}{range .addresses[*]}{.targetRef.name},{end}{end}' --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | egrep -q '(^|,)docker-registry-1-mwn9s,'' after 0 seconds [INFO] Waiting for command to finish: 'oc get 'pod/docker-registry-1-mwn9s' -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}' --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | grep -qi true'... [INFO] Success running command: 'oc get 'pod/docker-registry-1-mwn9s' -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}' --config='/tmp/openshift/test-end-to-end-docker//openshift.local.config/master/admin.kubeconfig' | grep -qi true' after 1 seconds

Yeah, if we're waiting for the version, the waits make sense, I was referring to all the additions in the helper function

I've simplified a lot the wait function. Kudos to @liggitt for the suggestions.

openshift-bot · 2016-09-05T16:02:27Z

Evaluated for origin merge up to ebc85ed

liggitt · 2016-09-05T16:14:55Z

[testextended][extended:core(builds)]

openshift-bot · 2016-09-05T16:17:19Z

Evaluated for origin testextended up to ebc85ed

openshift-bot · 2016-09-05T16:17:28Z

continuous-integration/openshift-jenkins/testextended Running (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/459/) (Extended Tests: core(builds), core(images))

stevekuznetsov · 2016-09-05T16:19:38Z

hack/util.sh

@@ -660,7 +660,9 @@ function install_registry() {
 readonly -f install_registry

 function wait_for_registry() {
-	wait_for_command "oc get endpoints docker-registry --template='{{ len .subsets }}' --config='${ADMIN_KUBECONFIG}' | grep -q '[1-9][0-9]*'" $((5*TIME_MIN))
+	local generation=$(oc get dc/docker-registry -o jsonpath='{.metadata.generation}')
+	local onereplicajs='{.status.observedGeneration},{.status.replicas},{.status.updatedReplicas},{.status.availableReplicas}'


Quote expansions

Sorry, thats for the line above this

openshift-bot · 2016-09-05T16:27:20Z

Evaluated for origin test up to ebc85ed

stevekuznetsov · 2016-09-05T16:28:53Z

test/end-to-end/core.sh

+os::cmd::expect_success 'oc login -u schema2-user -p pass'
+os::cmd::expect_success "oc new-project schema2tagged"
+os::cmd::expect_success "oc tag --source=istag schema2/busybox:latest busybox:latest"
+busybox_name=$(oc get -o 'jsonpath={.image.metadata.name}' istag busybox:latest)


stevekuznetsov · 2016-09-05T16:30:32Z

Since e2e was supposed to be a "user process" script... Is that the right place to put all these new tests?

liggitt · 2016-09-05T16:35:18Z

See comment about a follow up to split these into a registry extended test

openshift-bot · 2016-09-05T17:32:21Z

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

liggitt · 2016-09-05T17:33:40Z

Looks like this picked up a spurious --source arg

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/5787/consoleFull#16372028715762928fe4b031e1b25ced2a

Running test/end-to-end/core.sh:523: executing 'docker tag --source=docker busybox '172.30.169.212:5000/schema2/busybox'' expecting success...
FAILURE after 0.083s: test/end-to-end/core.sh:523: executing 'docker tag --source=docker busybox '172.30.169.212:5000/schema2/busybox'' expecting success: the command returned the wrong error code
There was no output from the command.
Standard error from the command:
flag provided but not defined: --source
See '/usr/bin/docker-current tag --help'.

openshift-bot · 2016-09-05T17:57:17Z

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

liggitt · 2016-09-05T18:26:13Z

superceded by #10805

mfojtik added this to the 1.3.0 milestone Sep 1, 2016

mfojtik added the priority/P1 label Sep 1, 2016

miminar force-pushed the take-care-of-manifest-config branch from 8d7be51 to 626b136 Compare September 1, 2016 13:20

miminar force-pushed the take-care-of-manifest-config branch from 626b136 to dc82904 Compare September 1, 2016 13:23

mfojtik reviewed Sep 1, 2016
View reviewed changes

liggitt self-assigned this Sep 1, 2016

liggitt reviewed Sep 1, 2016
View reviewed changes

miminar force-pushed the take-care-of-manifest-config branch from dc82904 to e22f4b2 Compare September 1, 2016 15:14

liggitt reviewed Sep 1, 2016
View reviewed changes

miminar force-pushed the take-care-of-manifest-config branch from e22f4b2 to 5c2786a Compare September 2, 2016 13:01

Michal Minář added 2 commits September 2, 2016 18:14

Remember image with matching config reference

c2f45fb

Make sure to match image having config name equal to the wanted blob digest. Remember the image and allow access to its layers. Signed-off-by: Michal Minář <[email protected]>

Pullthrough blobs using Get() as well

ed473e3

Manifest configs are fetched using Get() method from blob store. Pullthrough middleware needs to override it as well to allow for pulling manifest v2 schema 2 images from remote repositories. Signed-off-by: Michal Minář <[email protected]>

miminar force-pushed the take-care-of-manifest-config branch from 5c2786a to f8e5a5a Compare September 2, 2016 16:14

miminar changed the title ~~[WIP] Take care of manifest config blob~~ Take care of manifest config blob Sep 2, 2016

miminar force-pushed the take-care-of-manifest-config branch 2 times, most recently from d5dd631 to 43f6f95 Compare September 2, 2016 17:28

liggitt reviewed Sep 2, 2016
View reviewed changes

stevekuznetsov reviewed Sep 5, 2016
View reviewed changes

mfojtik mentioned this pull request Sep 5, 2016

Take care of manifest config blob #10805

Merged

liggitt closed this Sep 5, 2016

This was referenced Sep 6, 2016

Move manifest schema 1/2 end-to-end tests to extended tests #10813

Closed

test improvements: use schema 2 image we control #10814

Closed

miminar deleted the take-care-of-manifest-config branch February 7, 2017 10:53

Take care of manifest config blob #10759

Take care of manifest config blob #10759

Conversation

miminar commented Sep 1, 2016

miminar commented Sep 1, 2016

mfojtik commented Sep 1, 2016

miminar commented Sep 1, 2016

miminar commented Sep 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfojtik commented Sep 1, 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

soltysh commented Sep 2, 2016

miminar commented Sep 2, 2016

miminar commented Sep 2, 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

openshift-bot commented Sep 5, 2016

liggitt commented Sep 5, 2016 • edited Loading

openshift-bot commented Sep 5, 2016

openshift-bot commented Sep 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Sep 5, 2016

Choose a reason for hiding this comment

stevekuznetsov commented Sep 5, 2016

liggitt commented Sep 5, 2016

openshift-bot commented Sep 5, 2016

liggitt commented Sep 5, 2016 • edited Loading

openshift-bot commented Sep 5, 2016

liggitt commented Sep 5, 2016

liggitt commented Sep 5, 2016 •

edited

Loading

liggitt commented Sep 5, 2016 •

edited

Loading