Correct missing sizes for manifest schema 1 images #16325

miminar · 2017-09-13T14:48:23Z

Instead of filling metadata in the image object in the registry, send the manifest and config blobs to the master and let it do the job.

Resolves #16306
Resolves: bz#1491589

miminar · 2017-09-13T14:49:10Z

@dmage PTAL
@mfojtik FYI

miminar · 2017-09-13T14:55:58Z

The code is covered by extended_image_registry tests which will be part of a nightly job.

miminar · 2017-09-14T08:46:44Z

/retest

dmage · 2017-09-14T09:10:25Z

pkg/dockerregistry/server/manifestschema1handler.go

@@ -102,7 +102,7 @@ func (h *manifestSchema1Handler) FillImageMetadata(ctx context.Context, image *i
 	}
 	image.DockerImageMetadata.Object = meta

-	return nil
+	return encodeRawDockerImageMetadata(image, true)


above, imageMetadataFromManifest is called, which calls encodeRawDockerImageMetadata. Can't we merge the code above into imageMetadataFromManifest?

0xmichalis · 2017-09-14T10:01:07Z

/retest

smarterclayton · 2017-09-14T15:26:36Z

Add test so this doesn't regress

miminar · 2017-09-18T15:33:59Z

I moved the metadata filling into separate schema handlers since they differ a lot. I've also added few unit tests. I'll add some more coverage, especially for schema2.

@legionus, @dmage PTAL

dmage · 2017-09-18T19:40:59Z

pkg/dockerregistry/server/manifestschema1handler.go

@@ -53,56 +53,75 @@ type manifestSchema1Handler struct {
 var _ ManifestHandler = &manifestSchema1Handler{}

 func (h *manifestSchema1Handler) FillImageMetadata(ctx context.Context, image *imageapiv1.Image) error {


As it's our internal type, I'd like to propose to split this function into 3 parts:

Layers(ctx context.Context) (layers []imageapi.ImageLayer, annotations map[string]string, error) Signatures(ctx context.Context) ([][]byte, error) Metadata(ctx context.Context, layers []imageapi.ImageLayer) (imageapi.DockerImage, err error)

The output of these functions should depend only on the input manifest. And we do not need to parse manifest data in this functions, because we already have h.manifest (in this case we can remove DockerImageManifest in caller).

These functions can be reused in pkg/image/apis/image.ImageWithMetadata.

Good idea, I'll rework.

Reworked. I modified the declarations a bit. PTAL

smarterclayton · 2017-09-18T20:56:16Z

sgtm

…

On Mon, Sep 18, 2017 at 3:41 PM, Oleg Bulatov ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/dockerregistry/server/manifestschema1handler.go <#16325 (comment)>: > @@ -53,56 +53,75 @@ type manifestSchema1Handler struct { var _ ManifestHandler = &manifestSchema1Handler{} func (h *manifestSchema1Handler) FillImageMetadata(ctx context.Context, image *imageapiv1.Image) error { As it's our internal type, I'd like to propose to split this function into 3 parts: Layers(ctx context.Context) (layers []imageapi.ImageLayer, annotations map[string]string, error) Signatures(ctx context.Context) ([][]byte, error) Metadata(ctx context.Context, layers []imageapi.ImageLayer) (imageapi.DockerImage, err error) The output of these functions should depend only on the input manifest. And we do not need to parse manifest data in this functions, because we already have h.manifest (in this case we can remove DockerImageManifest in caller). These functions can be reused in pkg/image/apis/image.ImageWithMetadata. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#16325 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_pyWLOuxLD_ZKo0iF9SbhzyrTo5X1ks5sjsdQgaJpZM4PWPJK> .

miminar · 2017-09-21T14:30:08Z

Added more tests.
Instead of filling metadata to the image object in the registry, the manifest and config blobs are sent to master API, which should do the job.

dmage · 2017-09-22T16:39:56Z

pkg/dockerregistry/server/manifestservice.go

@@ -143,6 +148,7 @@ func (m *manifestService) Put(ctx context.Context, manifest distribution.Manifes
 			DockerImageReference:         fmt.Sprintf("%s/%s/%s@%s", m.repo.config.registryAddr, m.repo.namespace, m.repo.name, dgst.String()),
 			DockerImageManifest:          string(payload),
 			DockerImageManifestMediaType: mediaType,
+			DockerImageConfig:            string(config),


It will land in etcd, won't it?

It will land in etcd, won't it?

I've updated the image strategy to unset DockerImageConfig and DockerImageManifest.

Instead of filling metadata on image object in the registry, send manifest and config blobs to the master API and let it do the job. Signed-off-by: Michal Minář <miminar@redhat.com>

openshift-ci-robot · 2017-09-28T12:50:54Z

@miminar: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/openshift-jenkins/extended_image_registry	`7afd7f6`	link	`/test extended_image_registry`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

miminar · 2017-09-28T12:57:29Z

@stevekuznetsov

When I click on detail link of ci/openshift-jenkins/extended_image_registry, I get:

Unable to load build details from gs://origin-ci-test/pr-logs/pull/16325/test_pull_request_origin_extended_image_registry/24
This may mean the job has not yet uploaded any artifacts, or that it failed to upload. Take a look at the job in Jenkins for more information.

The job link works fine though. Is this openshift/test-infra issue?

miminar · 2017-09-28T12:57:35Z

/retest

miminar · 2017-09-28T13:00:11Z

Most of the failing registry extended tests will be addressed by @legionus' PR. An others by #15807

stevekuznetsov · 2017-09-28T15:05:03Z

@miminar yeah we had some issues yesterday... should be fixed from now on :)

miminar · 2017-09-29T08:21:39Z

@smarterclayton @dmage any other comments?

smarterclayton · 2017-10-01T22:38:55Z

Looks fine to me but I'll let @dmage have the final say

bparees · 2017-10-03T00:59:17Z

@dmage can you sign off on this? looks like this is the fix for a blocking bug.

bparees · 2017-10-03T01:14:13Z

(it also appears to be blocking #15807, it would be great to clear this logjam)

dmage · 2017-10-03T12:12:41Z

/lgtm

legionus · 2017-10-03T12:24:20Z

@miminar @dmage Should we add the imageapi.ImageManifestBlobStoredAnnotation annotation in the TagService ?

https://github.com/miminar/origin/blob/5ac2762a6e0874359683a32567350917f7321b19/pkg/dockerregistry/server/tagservice.go#L170-L177

legionus · 2017-10-03T12:27:33Z

Should we add the imageapi.ImageManifestBlobStoredAnnotation annotation in the TagService ?

I will answer myself. No. This should not be done because the image we get from the API.

/lgtm
/approve

miminar · 2017-10-03T12:38:07Z

/assign @bparees

bparees · 2017-10-03T18:14:28Z

/approve

openshift-merge-robot · 2017-10-03T18:14:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, dmage, legionus, 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/dockerregistry/OWNERS~~ [bparees,dmage,legionus,miminar]
~~pkg/image/OWNERS~~ [bparees]
~~test/extended/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-merge-robot · 2017-10-03T20:31:37Z

Automatic merge from submit-queue.

openshift-merge-robot assigned dmage and smarterclayton Sep 13, 2017

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

openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 13, 2017

miminar mentioned this pull request Sep 13, 2017

Images don't have size property #16306

Closed

miminar mentioned this pull request Sep 14, 2017

extended: fixed registry tests #15807

Merged

dmage reviewed Sep 14, 2017

View reviewed changes

miminar force-pushed the encode-dockerimagemetadata-for-schema1 branch from 8c96906 to 063f110 Compare September 18, 2017 15:09

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 18, 2017

miminar changed the title ~~Correct missing sizes for manifest schema 1 images~~ [WIP] Correct missing sizes for manifest schema 1 images Sep 18, 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 Sep 18, 2017

dmage reviewed Sep 18, 2017

View reviewed changes

miminar force-pushed the encode-dockerimagemetadata-for-schema1 branch from 063f110 to 101be87 Compare September 21, 2017 14:11

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

miminar changed the title ~~[WIP] Correct missing sizes for manifest schema 1 images~~ Correct missing sizes for manifest schema 1 images Sep 21, 2017

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

miminar force-pushed the encode-dockerimagemetadata-for-schema1 branch from 101be87 to 9044499 Compare September 21, 2017 14:28

dmage reviewed Sep 22, 2017

View reviewed changes

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 27, 2017

Fix missing sizes for manifest schema 1 images

5ac2762

Instead of filling metadata on image object in the registry, send manifest and config blobs to the master API and let it do the job. Signed-off-by: Michal Minář <miminar@redhat.com>

miminar force-pushed the encode-dockerimagemetadata-for-schema1 branch from 7afd7f6 to 5ac2762 Compare September 28, 2017 12:05

smarterclayton added api-approved and removed needs-api-review labels Oct 1, 2017

openshift-merge-robot added the needs-api-review label Oct 1, 2017

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

openshift-ci-robot assigned legionus Oct 3, 2017

openshift-ci-robot assigned bparees Oct 3, 2017

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

bparees added kind/bug Categorizes issue or PR as related to a bug. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 3, 2017

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

openshift-merge-robot merged commit 5471922 into openshift:master Oct 3, 2017

miminar deleted the encode-dockerimagemetadata-for-schema1 branch October 10, 2017 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct missing sizes for manifest schema 1 images #16325

Correct missing sizes for manifest schema 1 images #16325

miminar commented Sep 13, 2017 •

edited

Loading

miminar commented Sep 13, 2017

miminar commented Sep 13, 2017

miminar commented Sep 14, 2017

dmage Sep 14, 2017

0xmichalis commented Sep 14, 2017

smarterclayton commented Sep 14, 2017

miminar commented Sep 18, 2017

dmage Sep 18, 2017

miminar Sep 19, 2017

miminar Sep 21, 2017

smarterclayton commented Sep 18, 2017 via email

miminar commented Sep 21, 2017

dmage Sep 22, 2017

miminar Sep 27, 2017

openshift-ci-robot commented Sep 28, 2017 •

edited

Loading

miminar commented Sep 28, 2017

miminar commented Sep 28, 2017

miminar commented Sep 28, 2017

stevekuznetsov commented Sep 28, 2017

miminar commented Sep 29, 2017

smarterclayton commented Oct 1, 2017

bparees commented Oct 3, 2017

bparees commented Oct 3, 2017

dmage commented Oct 3, 2017

legionus commented Oct 3, 2017

legionus commented Oct 3, 2017

miminar commented Oct 3, 2017

bparees commented Oct 3, 2017

openshift-merge-robot commented Oct 3, 2017

openshift-merge-robot commented Oct 3, 2017

		@@ -53,56 +53,75 @@ type manifestSchema1Handler struct {
		var _ ManifestHandler = &manifestSchema1Handler{}

		func (h manifestSchema1Handler) FillImageMetadata(ctx context.Context, image imageapiv1.Image) error {

Correct missing sizes for manifest schema 1 images #16325

Correct missing sizes for manifest schema 1 images #16325

Conversation

miminar commented Sep 13, 2017 • edited Loading

miminar commented Sep 13, 2017

miminar commented Sep 13, 2017

miminar commented Sep 14, 2017

dmage Sep 14, 2017

Choose a reason for hiding this comment

0xmichalis commented Sep 14, 2017

smarterclayton commented Sep 14, 2017

miminar commented Sep 18, 2017

dmage Sep 18, 2017

Choose a reason for hiding this comment

miminar Sep 19, 2017

Choose a reason for hiding this comment

miminar Sep 21, 2017

Choose a reason for hiding this comment

smarterclayton commented Sep 18, 2017 via email

miminar commented Sep 21, 2017

dmage Sep 22, 2017

Choose a reason for hiding this comment

miminar Sep 27, 2017

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 28, 2017 • edited Loading

miminar commented Sep 28, 2017

miminar commented Sep 28, 2017

miminar commented Sep 28, 2017

stevekuznetsov commented Sep 28, 2017

miminar commented Sep 29, 2017

smarterclayton commented Oct 1, 2017

bparees commented Oct 3, 2017

bparees commented Oct 3, 2017

dmage commented Oct 3, 2017

legionus commented Oct 3, 2017

legionus commented Oct 3, 2017

miminar commented Oct 3, 2017

bparees commented Oct 3, 2017

openshift-merge-robot commented Oct 3, 2017

openshift-merge-robot commented Oct 3, 2017

miminar commented Sep 13, 2017 •

edited

Loading

openshift-ci-robot commented Sep 28, 2017 •

edited

Loading