Download support of images with multi-arch manifest #35772

arm64b · 2017-12-12T07:13:26Z

Currently we only support 'application/vnd.docker.distribution.manifest.v2+json'
manifest images download, with more multi-arch images used, we need to support
download images with 'application/vnd.docker.distribution.manifest.list.v2+json'
format(aka "fat manifest"), else we will fail to download those multi-arch images.

This PR adds 'application/vnd.docker.distribution.manifest.list.v2+json' manifest
support, thus we can download both multi-arch and legacy images.

Fix: #35841

Signed-off-by: Dennis Chen [email protected]

- What I did
Add 'application/vnd.docker.distribution.manifest.list.v2+json' manifest images download support.
- How I did it
Get the first level manifest, if it's a 'fat manifest', then get the 2nd level manifest which can match the run time arch.
- How to verify it
After apply the patch, make binary & make test-integration upon both legacy and multi-arch docker images, on both arm64 and amd64 platforms.
- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

arm64b · 2017-12-13T06:50:57Z

This PR doesn't affect the behavior of current implementation, but it adds 'fat manifest' support thus we can download those multi-arch ones. As a easy way to test it, we can change the 4 docker images on amd64 to the counterparts in #35773 , the download will still succeed.
/cc @tianon @dnephin , any comments? 🎉

tianon

Approach seems generally OK to me, just a few comments on the implementation.

tianon · 2017-12-13T17:41:05Z

contrib/download-frozen-image-v2.sh

Any arch detection based on uname makes me queasy (I too often build 32bit userspace on a 64bit kernel, for example), so I'd prefer if we used go env GOARCH or dpkg --print-architecture or something userspace-based for this sort of detection. One benefit of using go env GOARCH would be that it should return the expected values out-of-the-box, and only arm will be slightly complicated.

Absolutely 👍 "$(go env GOARCH)"

tianon · 2017-12-13T17:41:44Z

contrib/download-frozen-image-v2.sh

Please use local for all variables introduced/local to this function, as in fetch_blob.

tianon · 2017-12-13T17:42:31Z

contrib/download-frozen-image-v2.sh

Let's stick to a single variable naming format -- the rest of this file uses camelCase.

tianon · 2017-12-13T17:42:44Z

contrib/download-frozen-image-v2.sh

$m_arch here should be quoted too

tianon · 2017-12-13T17:43:36Z

contrib/download-frozen-image-v2.sh

Shouldn't this break so that we don't continue parsing entries? (I believe Docker's current implementation uses the first entry which matches)

Yes, should break the loop if find the first matched entry.

tianon · 2017-12-13T17:44:06Z

contrib/download-frozen-image-v2.sh

$found should be quoted here too, and if we initialize it to the empty string instead of 0, this could instead simply be [ -z "$found" ]

tianon · 2017-12-13T17:45:17Z

hack/make/.detect-daemon-osarch

Is this here simply to skip TEST_IMAGE_NAMESPACE? It seems like it'd be more appropriate to put an if in the * block here, wouldn't it? Don't we still need to set it to something, or is the empty value handled OK elsewhere?

The reason to skip TEST_IMAGE_NAMESPACE here is we've already switched to the multi-arch images on AArch64, so we don't need a name space like aarch64/ any more order to pass the test-integration. I think we will not need TEST_IMAGE_NAMESPACE if we switch all the legacy images to multi-arch ones in the future. But now let's it be and I add a if in the * block to handle AArch64 specific.

arm64b

Done!

arm64b · 2017-12-14T05:10:59Z

contrib/download-frozen-image-v2.sh

Yes, should break the loop if find the first matched entry.

Currently we only support 'application/vnd.docker.distribution.manifest.v2+json' manifest images download, with more multi-arch images used, we need to support download images with 'application/vnd.docker.distribution.manifest.list.v2+json' format(aka "fat manifest"), else we will fail to download those multi-arch ones. This PR adds 'application/vnd.docker.distribution.manifest.list.v2+json' manifest support, thus we can download both multi-arch and legacy images. Signed-off-by: Dennis Chen <[email protected]>

arm64b · 2017-12-14T05:38:22Z

Thanks @tianon for the careful review! Sincerely I want to give you a "赞"(a.k.a 👍 ) 😂😂
I've finished the changes according to your comments and will force a push soon.

thaJeztah · 2017-12-15T10:02:02Z

restarting janky; https://jenkins.dockerproject.org/job/Docker-PRs/47116/console

07:04:21 ----------------------------------------------------------------------
07:04:21 FAIL: check_test.go:366: DockerSwarmSuite.TearDownTest
07:04:21 
07:04:21 check_test.go:371:
07:04:21     d.Stop(c)
07:04:21 daemon/daemon.go:395:
07:04:21     t.Fatalf("Error while stopping the daemon %s : %v", d.id, err)
07:04:21 ... Error: Error while stopping the daemon d4cd7d275ce84 : exit status 130
07:04:21 
07:04:21 
07:04:21 ----------------------------------------------------------------------
07:04:21 PANIC: docker_api_swarm_service_test.go:201: DockerSwarmSuite.TestAPISwarmServicesUpdateStartFirst
07:04:21 
07:04:21 [d4cd7d275ce84] waiting for daemon to start
07:04:21 [d4cd7d275ce84] daemon started
07:04:21 
07:04:21 [d4cd7d275ce84] daemon started
07:04:21 Attempt #2: daemon is still running with pid 8906
07:04:21 Attempt #3: daemon is still running with pid 8906
07:04:21 Attempt #4: daemon is still running with pid 8906
07:04:21 [d4cd7d275ce84] exiting daemon
07:04:21 ... Panic: Fixture has panicked (see related PANIC)
07:04:21

arm64b · 2017-12-15T10:04:42Z

hmm, this is so nice 😂 I really don't want to see the 'red-cross' :) Thanks @thaJeztah !

arm64b · 2017-12-20T03:14:43Z

ping @tianon @dnephin @estesp @coolljt0725 , Hi guys, I know this is a holiday season(I am on vacation too now), but I still want to get connect to some of yous about this PR. I just kick of a new issue #35841 which is depended on this PR. I hope can get some feedback/comments from you guys and see how we can move forward for the next step.

seemethere · 2017-12-26T18:53:50Z

Can confirm that this fixes build issues on aarch64

tianon

Looks great to me generally, just one more point of clarification. I'm happy with it! 👍

tianon · 2017-12-26T19:35:12Z

hack/make/.detect-daemon-osarch

-		TEST_IMAGE_NAMESPACE="$PACKAGE_ARCH"
+		if [ "$PACKAGE_ARCH" != "aarch64" ]; then
+			TEST_IMAGE_NAMESPACE="$PACKAGE_ARCH"
+		fi


The idea here is that eventually we can remove this per-arch block entirely, right? (and possibly even use the same Dockerfile verbatim across arches?)

(Forgot to add this comment to my "review" hahaha 😇)

Right. With multi-arch images used across all the platforms supported, IMO it does make sense to unify the Dockerfile across arches, consequently TEST_IMAGE_NAMESPACE here should be removed in the future in that case(suppose I am not missing something for this reply😂)

estesp

LGTM

thaJeztah · 2018-01-03T18:03:28Z

looks like @tianon's question was answered, so let's merge 👍

arm64b requested review from dnephin and tianon as code owners December 12, 2017 07:13

GordonTheTurtle added the status/0-triage label Dec 12, 2017

tianon reviewed Dec 13, 2017

View reviewed changes

arm64b commented Dec 14, 2017

View reviewed changes

contrib/download-frozen-image-v2.sh Outdated

Copy link

Contributor Author

arm64b Dec 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, should break the loop if find the first matched entry.

arm64b force-pushed the multi-arch-manifest-support branch from 164a48a to 0af5db5 Compare December 14, 2017 05:38

arm64b mentioned this pull request Dec 15, 2017

Prevent potential panic in TestLogsAPIUntil #35808

Merged

thaJeztah added the rebuild/janky label Dec 15, 2017

GordonTheTurtle removed the rebuild/janky label Dec 15, 2017

thaJeztah added status/2-code-review and removed status/0-triage labels Dec 15, 2017

arm64b mentioned this pull request Dec 20, 2017

"unknown manifest mediaType" when build the binary on AArch64 #35841

Closed

tianon approved these changes Dec 26, 2017

View reviewed changes

tianon reviewed Dec 26, 2017

View reviewed changes

GordonTheTurtle assigned ehazlett Dec 27, 2017

estesp approved these changes Jan 3, 2018

View reviewed changes

thaJeztah merged commit fda9049 into moby:master Jan 3, 2018

andrewhsu mentioned this pull request Jan 4, 2018

[18.01] Download support of images with multi-arch manifest docker-archive/docker-ce#380

Merged

arm64b deleted the multi-arch-manifest-support branch January 4, 2018 06:21

arm64b mentioned this pull request Jan 5, 2018

Upgrade the frozen images to multi-arch ones #35929

Merged

dims mentioned this pull request Jul 25, 2018

GCE/GKE jobs need newer Docker versions for multi-arch images kubernetes/test-infra#8824

Closed

Download support of images with multi-arch manifest #35772

Download support of images with multi-arch manifest #35772

Conversation

arm64b commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arm64b commented Dec 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arm64b left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arm64b commented Dec 14, 2017

Uh oh!

thaJeztah commented Dec 15, 2017

Uh oh!

arm64b commented Dec 15, 2017

Uh oh!

arm64b commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seemethere commented Dec 26, 2017

Uh oh!

tianon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

estesp left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

arm64b commented Dec 12, 2017 •

edited

Loading

arm64b commented Dec 13, 2017 •

edited

Loading

arm64b commented Dec 20, 2017 •

edited

Loading