New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 14, 2017

Conversation

Projects
None yet
5 participants
@tophj-ibm
Contributor

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

@dnephin

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

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 13, 2017

Contributor

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

Contributor

tophj-ibm commented Sep 13, 2017

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

@tophj-ibm

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 13, 2017

Contributor

@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?

Contributor

tophj-ibm commented Sep 13, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 13, 2017

Member

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
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

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 13, 2017

Contributor

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.

Contributor

tophj-ibm commented Sep 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 13, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 13, 2017

Contributor

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/

Contributor

tophj-ibm commented Sep 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 13, 2017

Member

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

Let me know if there are any problems

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

yongtang added a commit to yongtang/docker that referenced this pull request Sep 13, 2017

Replace busybox with for `TestPullAllTagsFromCentralRegistry`
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

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 13, 2017

Contributor

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

Contributor

tophj-ibm commented Sep 13, 2017

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

Replace busybox with for `TestPullAllTagsFromCentralRegistry`
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>
[integration-cli] fix p/z HubPullSuite tests
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>
@@ -1825,7 +1825,7 @@ func (s *DockerDaemonSuite) TestDaemonNoSpaceLeftOnDeviceError(c *check.C) {
defer s.d.Stop(c)
// 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")

This comment has been minimized.

@tophj-ibm

tophj-ibm Sep 14, 2017

Contributor

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

@tophj-ibm

tophj-ibm Sep 14, 2017

Contributor

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

@dnephin

LGTM

@vdemeester

LGTM 🐅

@vdemeester vdemeester merged commit 3a081f5 into moby:master Sep 14, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 36832 has succeeded
Details
janky Jenkins build Docker-PRs 45472 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 5871 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 17065 has succeeded
Details
z Jenkins build Docker-PRs-s390x 5650 has succeeded
Details

@tophj-ibm tophj-ibm deleted the tophj-ibm:switch-hub-test-to-alpine branch Sep 14, 2017

@tophj-ibm

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 14, 2017

Contributor

thanks for the help @dnephin!

Contributor

tophj-ibm commented Sep 14, 2017

thanks for the help @dnephin!

@tophj-ibm

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 18, 2017

Contributor

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

Contributor

tophj-ibm commented Sep 18, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@tophj-ibm

tophj-ibm Sep 18, 2017

Contributor

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.

Contributor

tophj-ibm commented Sep 18, 2017

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

This comment has been minimized.

Show comment
Hide comment
@dnephin

dnephin Sep 18, 2017

Member

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

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