Move from docker.io #8165

edsantiago · 2020-10-27T13:33:39Z

Followon to #7965 (mirror registry). mirror.gcr.io doesn't
cache all the images we need, and I can't find a way to
add to its cache, so let's just use quay.io for those
images that it can't serve.

Tools used:
skopeo copy --all docker://docker.io/library/alpine:3.10.2
docker://quay.io/libpod/alpine:3.10.2

...and also:

docker.io/library/alpine:3.2
docker.io/library/busybox:latest
docker.io/library/busybox:glibc
docker.io/library/busybox:1.30.1
docker.io/library/redis:alpine
docker.io/libpod/alpine-with-bogus-seccomp:label
docker.io/libpod/alpine-with-seccomp:label
docker.io/libpod/alpine_healthcheck:latest
docker.io/libpod/badhealthcheck:latest

Since most of those were new quay.io/libpod images, they required
going in through the quay.io GUI, image, settings, Make Public.

Signed-off-by: Ed Santiago santiago@redhat.com

openshift-ci-robot · 2020-10-27T13:33:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [edsantiago]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

edsantiago · 2020-10-27T13:35:19Z

Just trying to see what fails in CI, and what I need to focus on.

edsantiago · 2020-10-27T20:01:58Z

test/e2e/images_test.go

 		result.WaitWithDefaultTimeout()
 		Expect(result).Should(Exit(0))
-		Expect(len(result.OutputToStringArray())).To(Equal(2))
+		Expect(len(result.OutputToStringArray())).To(Equal(3))


Because nginx was already fetched from quay; and now alpine+busybox are as well.

edsantiago · 2020-10-27T20:02:59Z

test/e2e/ps_test.go

 		result.WaitWithDefaultTimeout()
 		Expect(result.ExitCode()).To(Equal(0))
+		Expect(result.OutputToString()).To(Equal(cid))


I've taken the liberty of making this an actual test, with output checking and all (not just "it exits zero")

edsantiago

I'm getting more and more uncomfortable with this whole situation. Here are some comments about sections I believe need special attention. More later.

edsantiago · 2020-10-27T20:06:15Z

test/e2e/images_test.go

+		Expect(session.LineInOutputContainsTag("quay.io/libpod/alpine", "latest")).To(BeTrue())
+		Expect(session.LineInOutputContainsTag("quay.io/libpod/busybox", "latest")).To(BeTrue())
+		Expect(session.LineInOutputContainsTag("localhost/foo", "a")).To(BeTrue())
+		Expect(session.LineInOutputContainsTag("localhost/foo", "b")).To(BeTrue())
+		Expect(session.LineInOutputContainsTag("localhost/foo", "c")).To(BeTrue())
+		Expect(session.LineInOutputContainsTag("localhost/bar", "a")).To(BeTrue())
+		Expect(session.LineInOutputContainsTag("localhost/bar", "b")).To(BeTrue())


This was a complete NOP. Plus, LineInOutputContainsTag() was broken. See below.

edsantiago · 2020-10-27T20:07:15Z

test/utils/utils.go

@@ -383,7 +379,10 @@ func tagOutputToMap(imagesOutput []string) map[string]string {
 		if len(tmp) < 2 {
 			continue
 		}
-		m[tmp[0]] = tmp[1]


This was completely broken: for a list containing foo:a, foo:b, foo:c, ... well, you can figure it out.

edsantiago · 2020-10-27T20:08:13Z

test/utils/utils.go

@@ -383,7 +379,10 @@ func tagOutputToMap(imagesOutput []string) map[string]string {
 		if len(tmp) < 2 {
 			continue
 		}
-		m[tmp[0]] = tmp[1]
+		if m[tmp[0]] == nil {


WARNING: please review carefully, because I don't speak Golang, and there is almost certainly a more idiomatic way to do this.

edsantiago · 2020-10-27T20:18:41Z

When I first heard about this mirroring approach, I thought it would be: "we will set up our own mirror, and cache what we need from docker.io".

What I now understand is (and please correct me if I'm wrong): "we will use mirror.gcr.io which is owned and set up by some random party out of our control, and it includes a small handful of the images we need, but not all of them, and we have no way to populate it with what we need, so we have to use quay anyway".

If the latter is true; and given that most of this PR is s/docker/quay/ because the gcr mirror is so useless, should we just skip the mirroring thing entirely and just go full-on with a quay replacement?

edsantiago · 2020-10-27T22:46:27Z

Well, miracle of miracles, tests are passing. I'm still not sure whether to proceed with a full quay.io replacement, but I would like feedback on this as it is now.

mheon · 2020-10-28T01:25:00Z

test/e2e/exists_test.go

@@ -39,6 +39,7 @@ var _ = Describe("Podman image|container exists", func() {
 		Expect(session).Should(Exit(0))
 	})
 	It("podman image exists in local storage by short name", func() {
+		Skip("FIXME-8165: shortnames don't seem to work with quay")


This seems... pretty large? Any idea what's going on?

I don't, not on this or any of the other two FIXMEs. They worry me too, but my goal was to get tests passing and then dive deeper. I lost too much time on the map problem. I can dive deeper into these other problems today, I just want to know if the approach in this PR is reasonable.

I have no issue with the approach - it seems like a quick and easy fix to get things migrated away from Dockerhub.

@mheon see #8176. Another one that was unexpectedly difficult to reproduce.

mheon · 2020-10-28T01:26:20Z

test/e2e/images_test.go

@@ -240,6 +240,7 @@ WORKDIR /test
 	})

 	It("podman pull by digest and list --all", func() {
+		Skip("FIXME-8165: 'rmi -af' fails with 'layer not known'")


This one makes me deeply uncomfortable - it's that easy to break our storage in such a way that we can't fix it with rmi?

This was much, much harder to reproduce than I had expected. Added script to #6510 (comment)

mheon · 2020-10-28T01:28:35Z

test/e2e/tree_test.go

@@ -35,7 +35,7 @@ var _ = Describe("Podman image tree", func() {

 	It("podman image tree", func() {
 		SkipIfRemote("Does not work on remote client")
-		dockerfile := `FROM docker.io/library/busybox:latest
+		dockerfile := `FROM quay.io/libpod/busybox:latest


I wonder if we don't have builds that are just using shortnames, not pre-pulled images.

If we completely rip docker.io out of registries.conf, I wonder how much breaks...

Signed-off-by: Chris Evich <cevich@redhat.com>

Followon to containers#7965 (mirror registry). mirror.gcr.io doesn't cache all the images we need, and I can't find a way to add to its cache, so let's just use quay.io for those images that it can't serve. Tools used: skopeo copy --all docker://docker.io/library/alpine:3.10.2 \ docker://quay.io/libpod/alpine:3.10.2 ...and also: docker.io/library/alpine:3.2 docker.io/library/busybox:latest docker.io/library/busybox:glibc docker.io/library/busybox:1.30.1 docker.io/library/redis:alpine docker.io/libpod/alpine-with-bogus-seccomp:label docker.io/libpod/alpine-with-seccomp:label docker.io/libpod/alpine_healthcheck:latest docker.io/libpod/badhealthcheck:latest Since most of those were new quay.io/libpod images, they required going in through the quay.io GUI, image, settings, Make Public. Signed-off-by: Ed Santiago <santiago@redhat.com>

edsantiago · 2020-10-28T21:18:24Z

@containers/podman-maintainers PTACL ("C" for "Careful"). Some particular points to note:

This is based on Cirrus: Use google mirror for docker.io #7965, but not entirely: I took the liberty of git rebase -i to remove one unnecessary commit and to combine two edits to the registries.conf file
- Speaking of Cirrus: Use google mirror for docker.io #7965, that one has a 'crun/runc' commit that I don't really understand why it's part of this. @cevich can you comment on that?
The search test still relies heavily on docker.io. Fixing that is left as an exercise for a future PR.
I would feel happier if someone fixed integration tests: images_test: images with multiple tags: is a NOP #8171 in a better way than mine, but maybe mine is good enough for now
There are two new Skip()s, both of them real podman bugs (podman rmi gives "layer not known" when executed in parallel #6510, podman image exists, with vfs.imagestore and image from quay, fails #8176) related to vfs.

November 1 is this weekend. I don't think this PR is perfect, but I think it's a major step toward getting our CI to pass once docker cuts us off.

I will be out all of tomorrow (Thursday) with no means of communication. I will likely check in Friday morning but cannot promise a full day at work.

rhatdan · 2020-10-29T10:27:00Z

LGTM
@containers/podman-maintainers PTAL
I wonder if we should add a job that looks for docker.io in PRs for tests, to make sure they don't leak back in.

vrothberg · 2020-10-29T10:33:15Z

LGTM
@containers/podman-maintainers PTAL
I wonder if we should add a job that looks for docker.io in PRs for tests, to make sure they don't leak back in.

That can't happen in the e2e tests (see 53fe386) as references to docker.io will either go to the gcr mirror or quay.io/libpod.

LGTM but a third pair of eyes would be appreciated before merging

TomSweeneyRedHat · 2020-10-29T15:07:20Z

test/e2e/exists_test.go

@@ -39,6 +39,7 @@ var _ = Describe("Podman image|container exists", func() {
 		Expect(session).Should(Exit(0))
 	})
 	It("podman image exists in local storage by short name", func() {
+		Skip("FIXME-8165: shortnames don't seem to work with quay (#8176)")


@vrothberg FYI for short names work

Thanks, Tom!

@TomSweeneyRedHat @vrothberg late followup: this does not seem to be related to shortnames. #8176 is too weird for me to describe in a helpful title.

TomSweeneyRedHat · 2020-10-29T15:14:13Z

test/e2e/systemd_test.go

@@ -59,7 +59,7 @@ WantedBy=multi-user.target
 			Expect(stop.ExitCode()).To(Equal(0))
 		}()

-		create := podmanTest.Podman([]string{"create", "--name", "redis", "redis"})
+		create := podmanTest.Podman([]string{"create", "--name", "redis", redis})


Just double-checking is removing the quotes here intended/valid?

should be yes

@TomSweeneyRedHat @baude, late followup, yes, unquoting "redis" is deliberate: that gives us a variable that references an FQIN.

TomSweeneyRedHat · 2020-10-29T15:18:00Z

Just one question, otherwise LGTM and it's all green. I'm fine with merging now and fixing later if necessary.

baude · 2020-10-29T16:56:18Z

/lgtm

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2020

edsantiago changed the title ~~Move from dockerio~~ DO NOT MERGE: Move from dockerio Oct 27, 2020

edsantiago added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 27, 2020

This was referenced Oct 27, 2020

Cirrus: Use google mirror for docker.io #7965

Closed

Add test cases to cover podman volume #8157

Merged

edsantiago force-pushed the move_from_dockerio branch 5 times, most recently from a7085c4 to c9274c1 Compare October 27, 2020 20:00

edsantiago commented Oct 27, 2020

View reviewed changes

edsantiago force-pushed the move_from_dockerio branch from c9274c1 to 31131fe Compare October 27, 2020 20:03

edsantiago commented Oct 27, 2020

View reviewed changes

mheon reviewed Oct 28, 2020

View reviewed changes

This was referenced Oct 28, 2020

integration tests: images_test: images with multiple tags: is a NOP #8171

Closed

podman image exists, with vfs.imagestore and image from quay, fails #8176

Closed

cevich added 2 commits October 28, 2020 12:24

Cirrus: Always record runc/crun versions

5d48606

Signed-off-by: Chris Evich <cevich@redhat.com>

Cirrus: Use google mirror for docker.io

53fe386

Signed-off-by: Chris Evich <cevich@redhat.com>

edsantiago force-pushed the move_from_dockerio branch from 31131fe to c17f888 Compare October 28, 2020 18:26

edsantiago force-pushed the move_from_dockerio branch from c17f888 to 20e1043 Compare October 28, 2020 19:17

edsantiago changed the title ~~DO NOT MERGE: Move from dockerio~~ Move from docker.io Oct 28, 2020

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 Oct 28, 2020

edsantiago removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2020

TomSweeneyRedHat reviewed Oct 29, 2020

View reviewed changes

openshift-ci-robot assigned baude Oct 29, 2020

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

openshift-merge-robot merged commit 6d72e76 into containers:master Oct 29, 2020

edsantiago deleted the move_from_dockerio branch November 1, 2020 17:28

edsantiago mentioned this pull request Feb 3, 2021

[v2.2.1-rhel] Backport fix for CVE-2021-20199 #9221

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move from docker.io #8165

Move from docker.io #8165

edsantiago commented Oct 27, 2020 •

edited

Loading

openshift-ci-robot commented Oct 27, 2020

edsantiago commented Oct 27, 2020

edsantiago Oct 27, 2020

edsantiago Oct 27, 2020

edsantiago left a comment

edsantiago Oct 27, 2020

edsantiago Oct 27, 2020

edsantiago Oct 27, 2020

edsantiago commented Oct 27, 2020

edsantiago commented Oct 27, 2020

mheon Oct 28, 2020

edsantiago Oct 28, 2020

mheon Oct 28, 2020

edsantiago Oct 28, 2020

mheon Oct 28, 2020

edsantiago Oct 28, 2020

mheon Oct 28, 2020

edsantiago commented Oct 28, 2020

rhatdan commented Oct 29, 2020

vrothberg commented Oct 29, 2020

TomSweeneyRedHat Oct 29, 2020

vrothberg Oct 29, 2020

edsantiago Oct 30, 2020

TomSweeneyRedHat Oct 29, 2020

baude Oct 29, 2020

edsantiago Oct 30, 2020

TomSweeneyRedHat commented Oct 29, 2020

baude commented Oct 29, 2020

Move from docker.io #8165

Move from docker.io #8165

Conversation

edsantiago commented Oct 27, 2020 • edited Loading

openshift-ci-robot commented Oct 27, 2020

edsantiago commented Oct 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago left a 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

edsantiago commented Oct 27, 2020

edsantiago commented Oct 27, 2020

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

edsantiago commented Oct 28, 2020

rhatdan commented Oct 29, 2020

vrothberg commented Oct 29, 2020

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

TomSweeneyRedHat commented Oct 29, 2020

baude commented Oct 29, 2020

edsantiago commented Oct 27, 2020 •

edited

Loading