-
Notifications
You must be signed in to change notification settings - Fork 18.6k
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
Add manifest command to docker cli #27455
Conversation
@tophj-ibm is working on tests as well. |
is this just a new command for building manifest lists or? |
@runcom building and also fetching/displaying them. When we'd talked to @RichardScothern @dmcgowan several weeks ago they said a good first step would be to just get a PoC fetch working, so that's what this is. |
@runcom both :) first step of a more comprehensive |
figured that thx! we've been trying the same in distribution/distribution#1252 also, feel free to add a Close to close that proposal as well.. |
e6475ad
to
66791b5
Compare
cli/command/manifest/util.go
Outdated
// Added linux/s390x as we know System z support already exists | ||
|
||
var validOSArch = map[string]bool{ | ||
"darwin/386": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this microformat defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @stevvooe ! Anything ugly in this PR is probably a copy over from my external "manifest-tool" project :) In this case, I was trying to validate the input against a known universe of Golang os+arch pairs just to sanitize what people might claim as valid pairs. The format itself is just a slash-separated copy of the Golang list of accepted pairs from golang.org. Of course this in itself is also broad because the Docker engine does not exist for every pair in that list.
cli/command/manifest/util.go
Outdated
|
||
func isValidOSArch(os string, arch string) bool { | ||
osarch := fmt.Sprintf("%s/%s", os, arch) | ||
_, ok := validOSArch[osarch] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can define the map with a struct key to avoid having to format a string here.
66791b5
to
672df82
Compare
@clnperez Thanks for the contribution! It will be great to get this going! @estesp While the demo is informative, do we have a way of doing this without introducing a yaml file format? Note that #24739 contains a lot of "modern" requirements. It would be good to get a clear design of what this tool actually does. I think a short list would be:
To accomplish this, we need to define what a manifest is as an object in docker. Up to this point, it is largely hidden from the user. @runcom I am not sure if this fully covers distribution/distribution#1252. Inspecting the registry contents and the push/pull artifacts on the engine-side may be fundamentally different operations in practice. |
@stevvooe eventually I'm sure it will |
672df82
to
67a8d03
Compare
@stevvooe The idea of not using the yaml file occurred to me as well, so if we can find a way not to pull in that dependency to the docker codebase for just that I think it would be nice. But I personally couldn't think of a simpler way (maybe json instead but it's just not as user-friendly). I updated the comment with the current short list so hopefully that's helpful. I agree that we need to figure out a way to frame what this all means for a docker user. IIUC, this PR has come about for two [main] reasons:
So keeping those in mind when defining the concept to users might be beneficial. (Have I missed one?) A point of confusion I can see for the "shallow pull" case is if a user gets back a list but was only expecting a single group of size, digest, platform information. Maybe a default would be only to display the manifest that would run on their platform, and add a flag for --list, which would show all the images. A question I have is whether we need to store this. I would say no (though not in an authoritative way by any means). |
@clnperez An completely contrived example of a yaml-less approach would be something like this:
|
@stevvooe agree that the YAML parsing/input adds a lot of potentially unnecessary code to the PR, and agree that we can probably find a "YAML-less" approach that works (similar to the flow in your comment). It made for a simple demonstration of the assembly steps, so was reasonable for my PoC. We do need to figure out the input method for the |
@estesp I agree.
Not sure if you saw this line in my example:
|
Doh. Speed reading getting me in trouble again :) that seems fine Sent from my iPhone
|
@stevvooe I think it's good to be good to be able to create the list without first having to pull down the manifests, too. |
@stevvooe The way this PR (and @estesp's tool) works currently is, using what's in the yaml file, it does all the pulls for the user under the covers (which is what I meant, in case you thought I meant not to ever pull them down at all):
Edit: Adding the Digest line that didn't get pasted. One thing that Phil had mentioned as a todo that would be nice would be to do some introspection and create the fat manifest based on the architectures stored in the manifests that step pulls. So if you start with the yaml that I used for the above example:
You could cut out the platform bits, and maybe move all that into a single command. There could be a "dry-run" flag, too, so you can dump what the list would look like. Then the entire flow could go something like:
So the same example as above would be:
The good thing about having the file (yaml or otherwise) is that if you want to just change one digest in the manifest list you can source-control it, etc., the commands are a lot shorter, and you don't have to re-type the whole command every time. But maybe you can do that with a file of commands anyway. :) @stevvooe WDYT about this command flow? |
@clnperez YAML (or any other file format) isn't needed here at all. This kind of information should be part of the upstream's image config. I'd recommend avoiding trying to intersperse flags that apply to arg values. It is fragile and hard to use. I'm not sure the |
@stevvooe Yah, I didn't much like the interspersed flags there either. Just trying to find a way to cut this down into fewer commands. As for the file, I understand it's not needed, but was just pointing out how it could be useful for users who are going to be pushing these often. |
cf6fed5
to
90f1c0b
Compare
It seems that the transfer of options of bearer token and insecure registries need to be fixed. |
@clnperez Could you please post a TL;DR; of the current patch? Sum up what the UX will look like, what the commands will do on a high level and so forth? Reviewing this line-by-line is hard until you know what this is all about on a high level. I know exactly how estesp/manifest-tool works as I've read all the code there, but IIUC this PR has evolved using that only as a starting point. Thanks, I want to see this mainlined as soon as possible and want to help to review it! |
@xiekeyang In order to use insecure registries with this command (since under the covers it has no association whatsoever with the engine), I added a new option to the local (user-scoped) config file. See https://github.com/clnperez/docker/blob/8d2a1422ce906d2a47fe20d3ee42eec6e31881b9/cli/config/configfile/file.go#L43. In
|
@StefanScherer That node constraints vs image arch discrepancy is unfortunate. On my system the |
After building the binaries of this PR I found a problem when I just run
Then I tried to understand how to use the manifest commands with an image I already have built with the manifest-tool. So here is what I understand with the usage and the commit message given. With the manifest-tool I have these two commands to create the "1.7.1" tag and the "latest" tag (both at Docker Hub stefanscherer/winspector/tags)
So as a Docker Captain I tried to do steer my ship into unexplored waters. 😄 Is the
Seems like this does not work and a "latest" will be used instead. So can we create "tags" other than latest? So for the further test I created a new repo at Docker Hub to push into a safe place.
Next try to annotate more:
|
@StefanScherer, a gigantic thanks for steering your ship off into these uncharted waters. :D I'll try to address all the items from your comment...
Had you done a Let me know if I missed or misunderstood anything here, and thanks again for testing. |
@tiborvass Should I bite the bullet and move this over to the new cli repo? Though a little sad to lose the history, it's maybe good to refocus the discussion (maybe with Stefan's questions) since I believe the command design is settled at this point. |
@StefanScherer I fixed the tags bug, and also added a |
Thanks @clnperez. I've fetched the PR again and now cross-compiled an OSX client binary.
So I'll test with a Windows client, but the same here:
Changing So I tried it again with a single repository:
That looks good and is pretty simple to use as well. 👍 Only thing I still have an issue pushing it to another repository
The docker client is logged in and has the rights to pull from and push into both repositories used here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Marked the two places to use homedir.Get()
to make it work on non-Linux platforms.
cli/command/manifest/util.go
Outdated
if err := ensureHomeIfIAmStatic(); err != nil { | ||
return "", err | ||
} | ||
userHome, err := homedir.GetStatic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to userHome := homedir.Get()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if the user's homedir isn't set? I don't think we can use this just in case it isn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I take that back. Let me take another look at an alternative. Can you try compiling a dynamic binary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or not cross-compiling... https://github.com/moby/moby/blob/master/pkg/homedir/homedir_others.go#L9-L10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the bajillionth comment on this but @StefanScherer, is the home dir always going to be set on windows? I didn't realize that there was a different user
package in runc/libcontainer
that gets us around the homedir segfault problem with static binaries. But it does return an empty string on windows.
cli/command/manifest/push.go
Outdated
insecureRegistries := []string{} | ||
// Check $HOME/.docker/config.json. There may be mismatches between what the user has in their | ||
// local config and what the daemon they're talking to allows, but we can be okay with that. | ||
userHome, err := homedir.GetStatic() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to userHome := homedir.Get()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same function is used to read the config.json
for Docker Hub credentials, so I think this is fine. On Windows I do not have HOME set and it worked.
Definitely confused on @StefanScherer's log regarding cross-repo mount. My So now I'm not sure why manifest-tool doesn't error out for the same reason? |
@estesp I never tried cross-repository pushes with manifest-tool. I only did that to test this PR when it only supported creating a "latest" and I didn't want to overwrite it. Using the same cross-repo push with manifest-tool shows the same error:
|
@StefanScherer would love to get to the bottom of this; seems easy to just change the code to accept either success code (201 or 202), but I'd like to understand why :) I just tested what I think is a very similar setup inside my own DockerHub repo and I can't recreate the error. Would you mind running your same command with |
@estesp The private repo used in the integration tests does the cross-repo blob mounts, and those pass. 🤔 |
I use a Windows Docker image in that example and it is the foreign Windows base layer that causes the problem:
The layer with sha256:bce2fbc256ea437a87dadac2f69aabd25bed4f56255549090056c1131fad0277 is the microsoft/nanoserver:10.0.14393.447 base layer, there also is a second sha256:4a8c367fd46d2e2da2a8b0fa02158540e13b3a9015daf9f17d1af354a591492f which is the Windows update layer. |
Thanks for the extra debug @StefanScherer! So, @clnperez we need to allow I'm going to make the same modification to |
The workflow will be: `docker manifest create new-list-ref-name manifest [manifests...]` `docker manifest annotate new-list-ref-name manifest --os linux --arch arm` `docker manifest push new-list-ref-name` - or - `docker manifest push -f annotated-manifests.yaml` There is also a `manifest inspect` command to allow for a *shallow pull* of an image's manifest: `docker manifest inspect manifest-or-manifest_list`. These by default show a ManifestDescriptor in the case of a single manifest, or a DeserialedManifestList. To be more in line with the existing external manifest tool, there is also a `-v` option for inspect that will show information depending on what the reference maps to (list or single manifest). Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com> Signed-off-by: Christopher Jones <tophj@linux.vnet.ibm.com>
I pushed fixes for both issues. |
Note that the CLI has moved repos so this should now go into github.com/docker/cli. |
@clnperez ping us when you have the new PR up 👏 |
Thanks, going to go ahead and close this since it looks like this is all CLI bound code. |
- What I did
Porting the manifest tool code from https://github.com/estesp/manifest-tool
Version One workflow:
manifest inspect:
docker manifest inspect busybox:latest
manifest create list:
docker manifest create busybox-manifest.yaml
Version
TwoThree Workflow:variants, os features, cpu features, an os kind, and/or an archicture
The inspect, fetch, and annotate commands are optional if all a user
wants to do is create a multi-arch manifest list based on existing
images.
Things left to do:
Probably move the manifest storage out from ~/.docker/manifests (andfigure out how to handle multiple users possibly doing simultaneous
annotations of the same image manifest).
(probably fixed by moving manifests outside a user dir).- How to verify it
make binary
Create a new unaltered fat manifest:
./bundles/latest/dynbinary-client/docker manifest create --name reponame/busybox busybox ppc64le/busybox
Annotate a manifest inside of the new list context:
./bundles/latest/dynbinary-client/docker manifest annotate reponame/busybox ppc64le/busybox --os linux --arch ppc64le
./bundles/latest/dynbinary-client/docker manifest annotate reponame/busybox busybox --osFeatures osf1,osf2
Push the manifest list to a registry:
./bundles/latest/dynbinary-client/docker manifest push reponame/busybox
Display [committed] manifest or manifest list details:
./bundles/latest/dynbinary-client/docker manifest inspect ppc64le/busybox
./bundles/latest/dynbinary-client/docker manifest inspect reponame/busybox
Pull (to verify)
docker run --rm reponame/busybox uname -m
Will pull the correct image down for your platform, and print your system's hw name.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)