-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 #138
Add manifest command #138
Conversation
Thanks for moving this PR over to the docker/cli repo! This PR is quite large! It would be much easier to review if it could be split up and submitted incrementally. Would it be possible to include just one or two sub commands at first? The YAML config I think could be added later on. We'd like to keep a separation between the |
@dnephin It would be a lot of work to split things back out. I realize it would be easier to review and I of course don't want to make reviewers' lives harder -- but at the same time, I would appreciate some compromise. To use your yaml suggestion as an example, I was asked to remove the yaml func, then to add it back in, so I've already been through this twice with the yaml feature alone. I'm not trying to be stubborn -- just asking for a little mercy where it can be given (without sacrificing project integrity, of course). As for including only a few of the sub-commands, none of the ones there make sense without all the others, except maybe As for your other point about splitting some of it out into another package, I think that makes sense and I can look into it. |
cli/command/manifest/annotate.go
Outdated
|
||
flags.StringVar(&opts.os, "os", "", "Add ios info to a manifest before pushing it.") | ||
flags.StringVar(&opts.arch, "arch", "", "Add arch info to a manifest before pushing it.") | ||
flags.StringSliceVar(&opts.cpuFeatures, "cpuFeatures", []string{}, "Add feature info to a manifest before pushing it.") |
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.
cpu-features
?
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.
Ah, sure.
cli/command/manifest/annotate.go
Outdated
flags.StringVar(&opts.os, "os", "", "Add ios info to a manifest before pushing it.") | ||
flags.StringVar(&opts.arch, "arch", "", "Add arch info to a manifest before pushing it.") | ||
flags.StringSliceVar(&opts.cpuFeatures, "cpuFeatures", []string{}, "Add feature info to a manifest before pushing it.") | ||
flags.StringSliceVar(&opts.osFeatures, "osFeatures", []string{}, "Add feature info to a manifest before pushing it.") |
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.
os-features
?
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.
Yep, same. Thanks.
cli/command/manifest/annotate.go
Outdated
// Make sure the manifests are pulled, find the file you need, unmarshal the json, edit the file, and done. | ||
targetRef, err := reference.ParseNormalizedNamed(opts.target) | ||
if err != nil { | ||
return fmt.Errorf("Annotate: Error parsing name for manifest list (%s): %s", opts.target, err) |
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.
Suggest using Wrap
from github.com/pkg/errors
.
cli/command/manifest/fetch.go
Outdated
service: registryService, | ||
repoInfo: repoInfo, | ||
}, nil | ||
case registry.APIVersion1: |
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.
Manifest lists don't support v1 registries, so I'm unsure what v1ManifestFetcher
is useful for. Even if v1 registries are supported in some way, I would recommend against adding support for them in a new feature.
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.
Maybe not clear in the initial comment, but, this subcommand isn't just for manifest lists. It also allows you to do docker manifest inspect [blah]
of an image name that could be living in a v1 registry. This is so that users can do the "shallow pull" of an image that several people had requested in earlier issues
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.
@clnperez How do v1 registries have manifests? Do you have an example of the output?
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 still don't think it makes sense to support v1 registries. Support for v1 push/pull is going to be removed in a few months: moby/moby#33629
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.
@stevvooe I personally did not add the image info + manifest pull code, but the image info (metadata) was being pulled to get info to populate the manifest list, IIRC. I would have to double-check, but I'll just pull the v1 bits out altogether if we're not going to support it in push/pull soon.
cli/command/manifest/fetch_v2.go
Outdated
configJSON []byte // raw serialized image config | ||
unmarshalledConfig image.Image // deserialized image config | ||
) | ||
if runtime.GOOS == "windows" { |
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.
Shouldn't need the special case for windows here. In the pull code, it's so that on Linux layers can download before the image config is receive. Since this code isn't downloading layers, there's no benefit to the concurrency.
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.
Thanks!
cli/command/manifest/inspect.go
Outdated
if err != nil { | ||
return err | ||
} | ||
fmt.Fprintf(dockerCli.Out(), "%s\n", prettyJSON.String()) |
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.
fmt.Fprintln(dockerCli.Out(), prettyJSON.String())
cli/command/manifest/inspect.go
Outdated
if err != nil { | ||
return err | ||
} | ||
fmt.Fprintf(dockerCli.Out(), "%s\n", prettyJSON.String()) |
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.
fmt.Fprintln(dockerCli.Out(), prettyJSON.String())
cli/command/manifest/inspect.go
Outdated
if err != nil { | ||
return err | ||
} | ||
fmt.Fprintf(dockerCli.Out(), "%s\n", prettyJSON.String()) |
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.
fmt.Fprintln(dockerCli.Out(), prettyJSON.String())
cli/command/manifest/inspect.go
Outdated
return err | ||
} | ||
prettyJSON.Reset() | ||
err = json.Indent(&prettyJSON, jsonBytes, "", " ") |
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.
Why the unmarshal/marshal instead of calling Indent
on img.CanonicalJSON
?
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'll double-check, and maybe I was doing something else wrong, but if I didn't do the unmarshal first, it just printed ugly bytes.
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.
Nope. You're right.
cli/command/manifest/inspect.go
Outdated
if err != nil { | ||
return err | ||
} | ||
fmt.Fprintf(dockerCli.Out(), "%s\n", prettyJSON.String()) |
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.
fmt.Fprintln(dockerCli.Out(), prettyJSON.String())
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
==========================================
+ Coverage 51.23% 51.36% +0.13%
==========================================
Files 237 244 +7
Lines 15399 15762 +363
==========================================
+ Hits 7889 8096 +207
- Misses 7008 7121 +113
- Partials 502 545 +43 |
063558f
to
dc0fc04
Compare
I just found out that I can use Use case: Moving an image from one stage to another (after successful tests on different machines) without pulling and pushing the whole image. And probably able to retag Windows images as well on a Linux machine (where you can't pull and push Windows images). PS: I tried that with manifest-tool 0.5.0, but get an error "You specified a manifest list entry from a digest that points to a current manifest list. Manifest lists do not allow recursion." So +1 👍 having the "docker manifest" command. Oh, this only works if the source manifest only has one platform, for a multiarch manifest as source the same error occurs:
|
@StefanScherer That looks like a bug. Perhaps, if one creates a manifest list from an existing manifest list, it should clone it, rather than trying to reference it. |
cli/command/manifest/annotate.go
Outdated
variant string // an architecture variant | ||
os string | ||
arch string | ||
cpuFeatures []string |
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.
CPU features are being removed from OCI and we are going to deprecate them in docker's format. Let's remove them here.
@clnperez Are there a few examples of the manifest inspect output? I ask because the output of these commands tend to become what people think of when we talk about the data structure. This can be problematic, because people will look at that and say "Just add x", when that is actually impossible, because the internal data structure is misrepresented. |
I think what @StefanScherer is looking for is more like a "manifest clone" or what might be like a Instead, using |
@stevvooe here's what I had pasted in the original PR: moby/moby#27455 (comment) Since then I changed the flag from I'm not aware of the the use-case, though. @StefanScherer, @estesp, @tianon, et al., do you think displaying layers of the image referenced by the manifest list is useful? Here's an image's manifest, and it's image data (including layers). It's just the last one printed (which happens to be the arm image's info) when I run
@stevvooe If you think having the -v is bad and will cause the scenario you described, and people think that the layers output is useful, I can remove this flag for now and we can figure out something better later after the PR is in. |
@StefanScherer can you paste what you were doing that Steven said sounds like a bug? I think I'm misunderstanding what you did b/c it sounds to me like you just made a manifest list with only one entry in it, which is fine. But I don't see why Phil's tool wouldn't allow that. |
@clnperez Do we ever print out |
@stevvooe No, we don't print that out. That's the "franken-manifest" you thought was a bad idea before. If you've reconsidered and you think it's useful I could easily put that back, and only print the manifest struct with the -v flag. Right now I print:
|
@clnperez If you think it is useful to show the "franken manifest" and make it clear that it is not the manifest but a summary of data structures, then by all means, let's have something along those lines. Either way, this is looking good. @dnephin What are the real blockers for getting this in as experimental, at least? |
@clnperez As Phil mentioned I'm more searching for a way to remote copy/clone a tag or manifest without pulling the image (or images which is eg. not possible for Windows images if running through a Linux Docker host). My steps to reproduce the situation is this. Trying to create a manifest from a manifest list which only has one platform in it works:
This also can be pushed afterwards. Trying to create a manifest from a multiarch manifest list does not work:
So a "copy" or "clone" command would describe the step better than a "create" command. |
Thanks for the clarification @StefanScherer. That's not a bug so much as an unintended use-case. ;) I feel like we should start a list of "Features to add once this PR is in." I'm seeing a lot of cool things go into @estesp's manifest tool lately (which are exciting, b/c that means we're getting closer to multi-arch usage in the wild). |
Are there plans to support the yml like file format from manifest-tool? |
Also is there any plan to be able to update manifests one at a time? For instance, I have builds for windows and Linux and they do not have any dependencies on each other so when one finishes I just want it to go create/update it's own subtag under the main tag. Each build process can be isolated in this way. |
yes, some features were removed from the initial PR to make it more manageable for review. As soon as @clnperez recovers from the feat of this PR, I think there are plans to create PRs to add those capabilities back in.
A manifest list is one entity in the registry; there is no way to only modify one part of an existing entity; however, continued (re-)pushes with "sub"/architecture builds that are complete would be one way to solve this. This means your repo name/tag reference will be changing as platform entries are added/removed, which could cause downstream image user churn. As an example, there was a similar issue last year with a popular official image that was pushed first by an ARM/ARM64 build completion without amd64 being done.. causing consternation that that main official image was not operational for several hours on amd64. |
@estesp thanks so much for your quick response. So in your 2nd comment on being able to run one at a time. I'm okay with the churn so let me ask a scenario. If I have 2 independent build pipelines that do the following. ----Build Pipeline 1----- ----Build Pipeline 2----- Is this last one wins? Where the {multi-arch-tag} has one subtag either {os-specific-tag-1} or {os-specific-tag-2}, basically the last build to run wins? Again, thanks so much. |
Just to be clear, the root issue isn't actually solved -- still crops up from time to time because we only have a hacky workaround to help it happen less often currently. 😅 (docker-library/official-images#3835) |
@AceHack I have the yaml re-add on my todo for next week hopefully. It would be nice if we could find a way to only update one part of a manifest list on a registry (maybe by pulling, copying, and repushing with original bits and updated bits). |
It would also be nice to use some sort of etag i.e. optimistic concurrency concept so in the unlikely case that both push at the same time there will be no concurrency funny business. One will win and the other will just get the etag failure and retry. Thanks. |
Here's the PR for the yaml re-add: #866 |
Docker CE Edge documentation for |
I tried to create a manifest list but failed utterly. I created two images, say I was under the impression that every image also has a manifest. If this is not true, how do I create the manifest for the image? This part is missing from the documentation. |
@agronholm would your registry happen to not be using tls? if the command can't find the manifest on a registry that meets all the default requirements, then it will just tell you one doesn't exist. if that's the case, you can use the --insecure flag on create & push. |
No, it is using TLS. Also, the documentation did not give me any indication that |
Ah, yes. It sounds like there needs to be some clarification around that. It's mentioned in the insecure registries section, but if you aren't working with an insecure registry you're not going to read that section. :) |
Alright, so since I am not using an insecure registry, how can I figure out why it tells me there is no such manifest? |
I think I may have misread your previous comment:
The language sounded like you didn't have them pushed. Do you? That is a requirement. |
I did try that but it made no difference. |
Can you try with the -D flag and see if you get any new info? |
Enable inspection (aka "shallow pull") of images' manifest info, and also the creation of manifest lists (aka "fat manifests").
The workflow for creating a manifest list will be:
docker manifest create new-list-ref-name image-ref [image-ref...]
docker manifest annotate new-list-ref-name image-ref --os linux --arch arm
docker manifest push new-list-ref-name
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
.To be more in line with the existing external manifest tool, there is
also a
-v
option for inspect that will show information depending onwhat the reference maps to (list or single manifest).
Signed-off-by: Christy Norman Perez christy@linux.vnet.ibm.com
- What I did
- How I did it
- How to verify it
- Description for the changelog
See moby/moby#27455 for history.
TODO: