Skip to content

[integration-cli] fix p/z HubPullSuite tests#34837

Merged
vdemeester merged 1 commit intomoby:masterfrom
tophj-ibm:switch-hub-test-to-alpine
Sep 14, 2017
Merged

[integration-cli] fix p/z HubPullSuite tests#34837
vdemeester merged 1 commit intomoby:masterfrom
tophj-ibm:switch-hub-test-to-alpine

Conversation

@tophj-ibm
Copy link
Contributor

@tophj-ibm tophj-ibm commented Sep 13, 2017

These two tests try and pull all the tags in the busybox repo and looks to see
if there were more than two images pulled. This was failing on
p/z due to the recent change to manifest lists, where one of the busybox
tags didn't have a p/z manifest in it's manifest list.

This error seems fine to me, so I changed the test to see if pull fails,
it fails with the "manifest not found" error.
That gets into a weird condition where one of the tests would fail depending on which image it downloaded first.

Also switched from busybox -> alpine a test-specific dockercore repo, because it has significantly less tags, and the images are significantly smaller.

Signed-off-by: Christopher Jones tophj@linux.vnet.ibm.com

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

We should probably be using an image that is specifically designated for this test, instead of hopping that a library image happens to retain the properties we expect.

@tophj-ibm
Copy link
Contributor Author

Yeah, a repo with 3 small images would definitely be ideal here. This isn't that far off though.

@tophj-ibm
Copy link
Contributor Author

@dnephin in further investigation into this, alpine only works because the oldest two tags it downloads are v1 images and weren't converted to manifest lists, so we should definitely make a repo with images for this test. Something like docker/hubpulltest or something. Do you know who can make such a thing?

@dnephin
Copy link
Member

dnephin commented Sep 13, 2017

I think dockercore/engine-pull-all-test-fixture would be good. I can get that setup.

Can we create the images by tagging tianon/true with different tags? Is this sufficient or do we need something more:

docker pull tianon/true
docker tag tianon/true dockercore/engine-pull-all-test-fixture:v1
docker tag tianon/true dockercore/engine-pull-all-test-fixture:v2
docker tag tianon/true dockercore/engine-pull-all-test-fixture:latest

@tophj-ibm
Copy link
Contributor Author

Those images are just amd64 and not manifest lists, so they will definitely work here just like the old test did, but we won't be testing ml pulls which might be something we want to do.

@dnephin
Copy link
Member

dnephin commented Sep 13, 2017

If you can tag some true images for other platforms on your hub repo I can build a manifest list with docker/cli#138 and tag that instead of the single image manifest.

Do those 3 tags work? I haven't really looked at the test so I'm not sure what it needs. I'm hoping we can remove a bunch of these conditions with the better fixtures.

@tophj-ibm
Copy link
Contributor Author

Yep those tags all worked when I tested it. I built / pushed tophj/true:s390x and tophj/true:ppc64le here https://hub.docker.com/r/tophj/true/tags/

@dnephin
Copy link
Member

dnephin commented Sep 13, 2017

Ok, this is setup: https://hub.docker.com/r/dockercore/engine-pull-all-test-fixture/tags/

Let me know if there are any problems

@tophj-ibm
Copy link
Contributor Author

Looks good from my end. I think we'll have to add an arm image too, let me see what happens with the CI.

yongtang added a commit to yongtang/docker that referenced this pull request Sep 14, 2017
This fix is an enhencement for `TestPullAllTagsFromCentralRegistry`.
The test needs to test `--all-tags=true`. However, it uses `busybox` which consists
of 75 tags (and increasing).

With moby#34837 (comment)
this fix changes `busybox` into `dockercore/engine-pull-all-test-fixture`
so that the test time could be substentially reduced.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@tophj-ibm tophj-ibm force-pushed the switch-hub-test-to-alpine branch from c00810e to 20b0822 Compare September 14, 2017 13:06
This test tries to pull all the tags in the busybox repo and looks to see
if there were more than two images pulled. This was failing on
p/z due to the recent change to manifest lists, where one of the busybox
tags didn't have a p/z manifest in it's manifest list.

This error seems fine to me, so I changed the test to see if pull fails,
it fails with the "manifest not found" error.

Also switched from busybox -> alpine, because it has significantly less tags,
and the images are close in size.

Signed-off-by: Christopher Jones <tophj@linux.vnet.ibm.com>
@tophj-ibm tophj-ibm force-pushed the switch-hub-test-to-alpine branch from 20b0822 to 5739ba1 Compare September 14, 2017 13:42

// pull a repository large enough to fill the mount point
pullOut, err := s.d.Cmd("pull", "registry:2")
pullOut, err := s.d.Cmd("pull", "debian:stretch")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

registry:2 didn't have a manifest list either, so I switched it to debian:stretch. The entire point of this test though is to run it out of memory so that seems fine to me

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐅

@vdemeester vdemeester merged commit 3a081f5 into moby:master Sep 14, 2017
@tophj-ibm tophj-ibm deleted the switch-hub-test-to-alpine branch September 14, 2017 16:04
@tophj-ibm
Copy link
Contributor Author

thanks for the help @dnephin!

@tophj-ibm
Copy link
Contributor Author

@dnephin I pushed tophj/true:armhf, which we should add to the manifest list so the arm CI doesn't have this problem

@tophj-ibm
Copy link
Contributor Author

you'll have to annotate the architecture to arm and the variant to v7 on this one.

docker manifest annotate dockercore/engine-pull-all-test-fixture:v1 tophj/true:armhf --os linux --architecture arm --variant v7
should do the trick, although I would copy tophj/true into dockercore if you aren't already doing that.

@dnephin
Copy link
Member

dnephin commented Sep 18, 2017

I've pushed the 3 tags again to include that manifest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants