Fix pulling of images within buildah #1319

rhatdan · 2019-02-02T12:31:03Z

Change references to Transfer to transfer to make it internal only.
It should be determined from the image specification and only determined
in one place.

Make buildah.Pull use registries.conf

Currently buildah pull does not resolve images based on registries.conf
This does not match the behaviour of buildah from or buildah bud

Signed-off-by: Daniel J Walsh [email protected]

TomSweeneyRedHat · 2019-02-02T15:43:31Z

Does this resolve #1316?

TomSweeneyRedHat · 2019-02-02T16:42:31Z

pull.go

+			if options.ReportWriter != nil {
+				options.ReportWriter.Write([]byte("Pulling " + name + "\n"))
+			}
+			ref, err := pullImage(ctx, options.Store, name, options, systemContext)


Maybe I've not had enuf tea today, but it looks like you're only pulling images now when you have --all-tags involved. Does this still work for buildah pull alpine?

Yes.
srcRef, _, err := resolveImage(ctx, systemContext, options.Store, boptions)
Actually pulls the image. The dump-tags is just using the resolved first image name to pull all of the tags.

rhatdan · 2019-02-02T20:51:19Z

Fixes #1316

rhatdan · 2019-02-03T11:24:37Z

@nalind @umohnani8 @vrothberg @giuseppe @mtrmac PTAL

TomSweeneyRedHat · 2019-02-04T18:48:39Z

I'd like to take a deeper dive an run some test with this Tues morn. But if others OK this before, then fine by me.

pull.go

TomSweeneyRedHat · 2019-02-05T13:57:36Z

When an image is pulled, the imageID is not being printed with this patch as it had been prior.

# buildah images
# buildah pull docker.io/busybox
# buildah images
IMAGE NAME                                               IMAGE TAG            IMAGE ID             CREATED AT             SIZE
docker.io/library/busybox                                latest               3a093384ac30         Dec 31, 2018 20:21     1.42 MB

TomSweeneyRedHat · 2019-02-05T14:06:30Z

Not only no imageId at the end, but no progress bars.

rhatdan · 2019-02-05T18:55:50Z

Fixed imageid and progress problems.

rhatdan · 2019-02-14T21:15:51Z

@TomSweeneyRedHat Could you merge this one.

TomSweeneyRedHat · 2019-02-14T23:44:54Z

Code and testing LGTM, but I'd like to get a head nod from @mtrmac to make sure his concerns were addressed. I'm not quite sure about his second comment in the old code in particular having been addressed.

rhatdan · 2019-02-15T13:55:09Z

I would rather get his concerns addressed in a new effort to get this code rewritten into containers/image and then vendored into podman, Buildah and CRI-O to do it the same way in all packages.

TomSweeneyRedHat · 2019-02-15T17:56:29Z

I was hoping @mtrmac would be able to check in by now, but I'll go ahead and merge now. I agree the right place for this functionality in my book is containers/image and it looks like we're moving in that direction.

TomSweeneyRedHat · 2019-02-15T17:56:49Z

@rh-atomic-bot r+

rh-atomic-bot · 2019-02-15T17:56:50Z

📌 Commit 73361eb has been approved by TomSweeneyRedHat

rh-atomic-bot · 2019-02-15T17:56:56Z

⌛ Testing commit 73361eb with merge f183b09...

Currently buildah pull does not resolve images based on registries.conf This does not match the behaviour of buildah from or buildah bud This patch makes buildah pull use the same image resolver as the other two tools. Signed-off-by: Daniel J Walsh <[email protected]> Closes: #1319 Approved by: TomSweeneyRedHat

rh-atomic-bot · 2019-02-15T18:44:32Z

💔 Test failed - status-travis

mtrmac · 2019-02-15T20:36:05Z

Code and testing LGTM, but I'd like to get a head nod from @mtrmac to make sure his concerns were addressed. I'm not quite sure about his second comment in the old code in particular having been addressed.

AFAICS all of those review comments are still applicable.

In my experience it’s pretty much never worth it to knowingly introduce new bugs in order to get a new feature, especially if introducing those bugs is not at all essential for the feature (as is the case of
https://github.com/containers/buildah/pull/1319/files#r253626657 , which needs about three more lines; the other comments may require notably more code). But, *shrug*, I realize that’s a bit extremist approach, and there may not be time or, well, interest, to avoid such situations — again, not my call, but that decision did depend on me being happy with the PR, I’m afraid I’m still not.

On a more conceptual level, does it even make much sense to buildah pull --all-tags fedora? Does that mean “I want all fedora:* tags from all registries on the search list”, or “I want all fedora:* tags from the first registry on the search list that contains a fedora, but I don’t care at all which registry it is”? The latter seems a tiny bit more sane, but both seem, fundamentally, rather useless. When would anyone ever want to use either?

So, maybe, at the highest level, there should be a separate PullAllTags + Pull, with PullAllTags not using the search path at all (and minimally modified), and Pull modified roughly like this PR? That would also nicely bypass the ":" and the srcRef concerns (but not the other alltransports.ParseImageName semantics breakage).

OTOH doing that the easy way would cause PullAllTags to (continue to) use the docker.io/library normalization, which may be considered very undesirable.

mtrmac · 2019-02-15T20:38:29Z

I would rather get his concerns addressed in a new effort to get this code rewritten into containers/image and then vendored into podman, Buildah and CRI-O to do it the same way in all packages.

(BTW don’t expect miracles from this — as https://github.com/containers/libpod/blob/9c1b08fd79012c12d478edb13f6057a0414762db/libpod/image/parts.go#L54 shows, rewrites in c/image won’t free libpod/buildah from making decisions about semantics of the input; if anything, it would make it less avoidable for them to make such decisions.)

rhatdan · 2019-02-17T11:57:03Z

@mtrmac @nalind
I reworked this PR Heavily to remove the handling of Transports from the clients, and moved it into the library.

I made the Transports private to the library to prevent the client passing in a transport different then the image.

Transport=docker
ImageName=ociarchive:/tmp/foobar

Also added OCI As a valid format.

util.ResolveImage now returns the image name without the transport as well as the transport.

TomSweeneyRedHat · 2019-02-18T16:01:04Z

docs/buildah-pull.md

@@ -25,6 +25,9 @@ Multiple transports are supported:
  **docker-daemon:**_docker-reference_
  An image _docker-reference_ stored in the docker daemon's internal storage.  _docker-reference_ must include either a tag or a digest.  Alternatively, when reading images, the format can also be docker-daemon:algo:digest (an image ID).

+  **oci:**_path_**:**_tag_**
+  An image tag in a directory compliant with "Open Container Image Layout Specification" at path.


nit, s/path/path/

Ugh love GitHub auto md sometimes

s/path/_path_/

TomSweeneyRedHat · 2019-02-18T16:10:00Z

@nalind and/or @vrothberg PTAL

TomSweeneyRedHat · 2019-02-18T17:52:22Z

buildah.go

@@ -340,7 +340,7 @@ type BuilderOptions struct {
 	// needs to be pulled and the image name alone, or the image name and
 	// the registry together, can not be resolved to a reference to a
 	// source image.  No separator is implicitly added.
-	Transport string


Just wondering why you switched this to lower case? Also in pull.go. I understand the scoping, but none of the other variables in these structs are lowercase. In some of the code we have a number of variables named transport(s) being used and having this upper case helps it to stand out better.

I don't want callers to use this field. We use it to pass the information around withing buildah library, but specifying transport as well as imagename can cause conflicts.
Bottom line, I only want transport derived in one place and from ImageName. Before we had it being derived in different places,and not the same way.

mtrmac

I very much like dropping the reliance on setting Transport in the front-end.

This opens up quite a few other opportunities for clean up, mostly non-blocking.

OTOH the AllTags code motivating this PR is still conflicted about handling its input.

buildah.go

cmd/buildah/from.go

mtrmac · 2019-02-18T22:00:55Z

cmd/buildah/pull.go

-	if len(arr) == 2 {
-		if iopts.allTags {
-			return errors.Errorf("tag can't be used with --all-tags")
-		}


This check seems useful and worth preserving somewhere (though this implementation did not made little sense). AFAICS util.ResolveNames does not add the implicit :latest tag, so the check could be moved closer to the PullOptions.AllTags implementation .

This is tough to check, and seem fairly arbitrary. If I state I want to pull --alltags on alpine:latest, I don't see this as a big conflict.

imagebuildah/build.go

util/util.go

pull.go

mtrmac · 2019-02-18T22:43:03Z

pull.go

-		options.ReportWriter = nil // Turns off logging output
+		boptions.ReportWriter = nil // Turns off logging output
+	}
+	srcRef, transport, img, err := resolveImage(ctx, systemContext, options.Store, boptions)


Still #1319 (comment) . In addition, this will break for the AllTags case if an image with the :latest tag does not exist.

Maybe the AllTags case should be handled separately, and explicitly hard-code / enforce docker.Transport? See also the earlier question about what AllTags + search means, and others.

If I don't test for the conflict of TAG versus all-tag, then the user must specify a legal tag if the :latest does not exist to do a pull-all.

Is that adequate for now.

At the very least, s/srcRef/destRef/ or s/srcRef/storageRef/ to not be confusing about the semantics of that value, please.

WRT requiring :latest to exist:

Well, it’s a breaking change AFAICS; this used to work. And forcing users to specify a tag would make reintroducing the “--all-tags with a tag is invalid” error another breaking change.

*shrug* I don’t quite understand the utility of --all-tags (let alone search+--all-tags together) in the first place, so I guess it is acceptable.

pull.go

mtrmac · 2019-02-18T23:03:41Z

pull.go

@@ -215,13 +222,10 @@ func pullImage(ctx context.Context, store storage.Store, imageName string, optio
 	spec := imageName
 	srcRef, err := alltransports.ParseImageName(spec)
 	if err != nil {


The two callers of pullImage now explicitly know whether the input should be readable by ParseImageName or whether it needs the transport added (and using the other path can misinterpret the input); it would be nice to make this unambiguous.

pullAndFindImage AFAICS currently always submits input input with the “concatenate options.transport+imageName” format, while the AllTags path in Pull always submits input with the “use ParseImageName(imageName) semantics — so, the concatenation code could be moved to pullAndFindImage (or maybe even higher to resolveImage, and consolidated with the other part there), and pullImage could expect input in the ParseImageName format.

(This is a pre-existing situation, so non-blocking; the changes to ResolveName in this PR now allow making this unambiguous.)

Lets handle this in a separate PR

rh-atomic-bot · 2019-02-19T15:15:21Z

☔ The latest upstream changes (presumably e8d73b2) made this pull request unmergeable. Please resolve the merge conflicts.

util/util.go

mtrmac · 2019-02-19T23:46:42Z

pull.go

-		options.ReportWriter = nil // Turns off logging output
+		boptions.ReportWriter = nil // Turns off logging output
+	}
+	srcRef, transport, img, err := resolveImage(ctx, systemContext, options.Store, boptions)


At the very least, s/srcRef/destRef/ or s/srcRef/storageRef/ to not be confusing about the semantics of that value, please.

WRT requiring :latest to exist:

Well, it’s a breaking change AFAICS; this used to work. And forcing users to specify a tag would make reintroducing the “--all-tags with a tag is invalid” error another breaking change.

*shrug* I don’t quite understand the utility of --all-tags (let alone search+--all-tags together) in the first place, so I guess it is acceptable.

mtrmac

Please rename srcRef to not be misleading, that’s the last blocker for me.

The requirement to name an existing tag (or the default :latest) for --all-tags to work is problematic, but obscure enough, and it would require yet another non-trivial modification, that I can live with it.

mtrmac · 2019-02-19T23:52:40Z

pull.go

+			if options.ReportWriter != nil {
+				options.ReportWriter.Write([]byte("Pulling " + name + "\n"))
+			}
+			ref, err := pullImage(ctx, options.Store, transport, name, options, systemContext)


Non-blocking:

For consistency with the pullAndFindImage caller, it would be nice if name here did not start with transport:

repoName := whateverSrcRefIsRenamedTo.DockerReference().Name() srcRef, err := alltransports.ParseImageName(transport+repoName) // or srcRef, err := c/image/docker.NewReference(srcRef.DockerReference()) … name = repoName + ":" + tag

or so.

Change references to Transfer to transfer to make it internal only. It should be determined from the image specification and only determined in one place. Make buildah.Pull use registries.conf Currently buildah pull does not resolve images based on registries.conf This does not match the behaviour of buildah from or buildah bud Signed-off-by: Daniel J Walsh <[email protected]>

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan · 2019-02-20T17:36:21Z

@mtrmac Ready to merge. I will open another PR to further clean this up and eventually get it into podman and maybe add the functions to containers/image

@rh-atomic-bot r+

rh-atomic-bot · 2019-02-20T17:36:21Z

📌 Commit ea5f858 has been approved by rhatdan

mtrmac · 2019-02-20T18:56:54Z

CK. Thanks!

mtrmac · 2019-02-20T18:57:05Z

ACK, that is.

Signed-off-by: Daniel J Walsh <[email protected]> Closes: #1319 Approved by: rhatdan

rh-atomic-bot · 2019-02-20T19:31:13Z

⌛ Testing commit ea5f858 with merge 3531a2d...

Signed-off-by: Daniel J Walsh <[email protected]> Closes: #1319 Approved by: rhatdan

rh-atomic-bot · 2019-02-20T21:41:36Z

☀️ Test successful - status-papr, status-travis
Approved by: rhatdan
Pushing 3531a2d to master...

TomSweeneyRedHat reviewed Feb 2, 2019

View reviewed changes

mtrmac reviewed Feb 4, 2019

View reviewed changes

pull.go Show resolved Hide resolved

pull.go Show resolved Hide resolved

pull.go Outdated Show resolved Hide resolved

pull.go Outdated Show resolved Hide resolved

rhatdan force-pushed the pull branch from 8842b99 to 73361eb Compare February 5, 2019 18:53

rhatdan force-pushed the pull branch from 73361eb to 654dcd7 Compare February 17, 2019 11:43

rhatdan changed the title ~~Resolve buildah.Pull to use registries.conf~~ Fix pulling of images within buildah Feb 17, 2019

rhatdan force-pushed the pull branch 3 times, most recently from 6ce7646 to 19b3e8d Compare February 17, 2019 23:11

TomSweeneyRedHat reviewed Feb 18, 2019

View reviewed changes

rhatdan force-pushed the pull branch from 19b3e8d to 29f91c8 Compare February 18, 2019 21:05

mtrmac reviewed Feb 18, 2019

View reviewed changes

rhatdan force-pushed the pull branch from 29f91c8 to a8b4906 Compare February 19, 2019 13:37

rhatdan force-pushed the pull branch from a8b4906 to 6ef421e Compare February 19, 2019 22:00

mtrmac reviewed Feb 19, 2019

View reviewed changes

rhatdan added 4 commits February 20, 2019 07:04

Support oci layout format

1854716

Signed-off-by: Daniel J Walsh <[email protected]>

Make sure buildah pull --all-tags only works with docker transport

525cb47

Signed-off-by: Daniel J Walsh <[email protected]>

Remove quiet option from pull options

ea5f858

Signed-off-by: Daniel J Walsh <[email protected]>

rhatdan force-pushed the pull branch from 6ef421e to ea5f858 Compare February 20, 2019 12:04

rh-atomic-bot pushed a commit that referenced this pull request Feb 20, 2019

Support oci layout format

fa71977

Signed-off-by: Daniel J Walsh <[email protected]> Closes: #1319 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request Feb 20, 2019

Make sure buildah pull --all-tags only works with docker transport

57ec39d

Signed-off-by: Daniel J Walsh <[email protected]> Closes: #1319 Approved by: rhatdan

rh-atomic-bot pushed a commit that referenced this pull request Feb 20, 2019

Remove quiet option from pull options

3531a2d

Signed-off-by: Daniel J Walsh <[email protected]> Closes: #1319 Approved by: rhatdan

rh-atomic-bot closed this in f9e645a Feb 20, 2019

mtrmac mentioned this pull request Feb 21, 2019

Pull cleanups #1361

Closed

TomSweeneyRedHat mentioned this pull request Mar 28, 2019

podman exit(0) on pull failure containers/podman#2785

Closed

TomSweeneyRedHat mentioned this pull request Apr 9, 2019

buildah push triggers "layer not known" corruption for concurrent buildah commit #966

Closed

github-actions bot added the locked - please file new issue/PR label Oct 15, 2023

github-actions bot locked as resolved and limited conversation to collaborators Oct 15, 2023

Fix pulling of images within buildah #1319

Fix pulling of images within buildah #1319

Conversation

rhatdan commented Feb 2, 2019 • edited Loading

TomSweeneyRedHat commented Feb 2, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Feb 2, 2019

rhatdan commented Feb 3, 2019

TomSweeneyRedHat commented Feb 4, 2019

TomSweeneyRedHat commented Feb 5, 2019

TomSweeneyRedHat commented Feb 5, 2019

rhatdan commented Feb 5, 2019

rhatdan commented Feb 14, 2019

TomSweeneyRedHat commented Feb 14, 2019

rhatdan commented Feb 15, 2019

TomSweeneyRedHat commented Feb 15, 2019

TomSweeneyRedHat commented Feb 15, 2019

rh-atomic-bot commented Feb 15, 2019

rh-atomic-bot commented Feb 15, 2019

rh-atomic-bot commented Feb 15, 2019

mtrmac commented Feb 15, 2019

mtrmac commented Feb 15, 2019

rhatdan commented Feb 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac 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

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

rh-atomic-bot commented Feb 19, 2019

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Feb 20, 2019

rh-atomic-bot commented Feb 20, 2019

mtrmac commented Feb 20, 2019

mtrmac commented Feb 20, 2019

rh-atomic-bot commented Feb 20, 2019

rh-atomic-bot commented Feb 20, 2019

rhatdan commented Feb 2, 2019 •

edited

Loading