[WIP] Manifest cmd #27455

Open
wants to merge 1 commit into
from

Projects

None yet

9 participants

@clnperez
Contributor
clnperez commented Oct 17, 2016 edited

- What I did
Porting the manifest tool code from https://github.com/estesp/manifest-tool

Version One workflow:

  • manifest fetch:
    docker manifest fetch busybox:latest

  • manifest create list:

  1. Create a yaml file, e.g.
image: docker.io/clnperez/hello-world:latest
manifests:
  -
    image: docker.io/ppc64le/hello-world:latest
    platform:
      architecture: ppc64le
      os: linux
  -
    image: docker.io/hello-world:latest
    platform:
      architecture: amd64
      features:
        - sse
      variant: v_a
      os: linux

  1. docker manifest create busybox-manifest.yaml

Version Two Workflow:

  • inspect: will pull down and print out a manifest or manifest list
  • fetch: same as inspect but will overwrite any locally-made
    annotations, but won't print the list
  • annotate: will modify the locally-stored manifest list by adding
    variants, os features, cpu features, an os kind, and/or an archicture
  • create: will assemble and push a new manifest list. This also creates
    cross-repo blob mounts for any layers not existing on the new manifest
    lists's target repo.

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:

  • Add support for plain http registries.
  • Probably move the manifest storage out from ~/.docker/manifests (and
    figure out how to handle multiple users possibly doing simultaneous
    annotations of the same image manifest).
  • Lots of cleanup.
  • Lots more tests (and make existing ones much more thorough).
  • Fix a the user lookup issue when running a static binary (probably fixed by moving manifests outside a user dir).
  • Move borrowed/duplicated code from docker/docker#29478 into a sharable place (in a separate PR)

- How to verify it

  • make binary

  • Create a new unaltered fat manifest:
    ./bundles/latest/dynbinary-client/docker manifest create reponame/busybox busybox ppc64le/busybox

  • Annotate a manifest, then create a new fat manifest:
    ./bundles/latest/dynbinary-client/docker manifest annotate ppc64le/busybox --os linux --arch ppc64le
    ./bundles/latest/dynbinary-client/docker manifest annotate busybox --osFeatures osf1,osf2
    ./bundles/latest/dynbinary-client/docker manifest create reponame/busybox busybox ppc64le/busybox

  • Overwrite your local manifest and start over:
    ./bundles/latest/dynbinary-client/docker manifest fetch ppc64le/busybox

  • Display manifest or manifest list details:
    ./bundles/latest/dynbinary-client/docker manifest inspect ppc64le/busybox

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

manatee pulling baby manatee

@clnperez
Contributor

@tophj-ibm is working on tests as well.

@runcom
Member
runcom commented Oct 17, 2016

is this just a new command for building manifest lists or?

@clnperez
Contributor

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

@estesp
Member
estesp commented Oct 17, 2016

@runcom both :) first step of a more comprehensive manifest subcommand in response to this comment from @stevvooe : #24739 (comment)

@runcom
Member
runcom commented Oct 17, 2016

@runcom both :) first step of a more comprehensive manifest subcommand in response to this comment from @stevvooe : #24739 (comment)

figured that thx! we've been trying the same in docker/distribution#1252 also, feel free to add a Close to close that proposal as well..

cli/command/manifest/util.go
+// Added linux/s390x as we know System z support already exists
+
+var validOSArch = map[string]bool{
+ "darwin/386": true,
@stevvooe
stevvooe Oct 17, 2016 Contributor

Where is this microformat defined?

@estesp
estesp Oct 17, 2016 Member

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
+
+func isValidOSArch(os string, arch string) bool {
+ osarch := fmt.Sprintf("%s/%s", os, arch)
+ _, ok := validOSArch[osarch]
@stevvooe
stevvooe Oct 17, 2016 Contributor

You can define the map with a struct key to avoid having to format a string here.

@stevvooe
Contributor

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

  • inspect current manifests
  • shallow pull remote manifests
  • take manifests that are partially pulled into full images
  • assemble multi-arch manifests for images or other sub-manifests

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 docker/distribution#1252. Inspecting the registry contents and the push/pull artifacts on the engine-side may be fundamentally different operations in practice.

@runcom
Member
runcom commented Oct 17, 2016

@stevvooe eventually I'm sure it will

@clnperez
Contributor

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

  1. Users wanting to be able to do something like a 'shallow pull' or an image inspect but without pulling the image. So we can fetch the manifest and display it.
  2. The need to stop tagging arch-specific images as arch-specific (the assemble), and also to be able to find out which images support your architecture (the fetch).

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

@stevvooe
Contributor

@clnperez An completely contrived example of a yaml-less approach would be something like this:

docker manifest pull mybuildregistry:5000/redis-arm 
docker manifest pull mybuildregistry:5000/redis-s390x
docker manifest pull mybuildregistry:5000/redis-x86
docker manifest annotate --platform.features sse4 mybuildregistry:5000/redis-x86
docker manifest assemble mybuildregistry:5000/redis mybuildregistry:5000/redis-arm mybuildregistry:5000/redis-s390x mybuildregistry:5000/redis-x86
docker manifest push mybuildregistry:5000/redis
@estesp
Member
estesp commented Oct 18, 2016

@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 features and variant information that the platform object is capable to hold, which will not be part of the image layer/manifest content, at least not at the moment.

@stevvooe
Contributor

@estesp I agree.

We do need to figure out the input method for the features and variant information that the platform object is capable to hold, which will not be part of the image layer/manifest content, at least not at the moment.

Not sure if you saw this line in my example:

docker manifest annotate --platform.features sse4 mybuildregistry:5000/redis-x86
@estesp
Member
estesp commented Oct 18, 2016

Doh. Speed reading getting me in trouble again :) that seems fine

Sent from my iPhone

On Oct 18, 2016, at 7:02 PM, Stephen Day notifications@github.com wrote:

@estesp I agree.

We do need to figure out the input method for the features and variant information that the platform object is capable to hold, which will not be part of the image layer/manifest content, at least not at the moment.

Not sure if you saw this line in my example:

docker manifest annotate --platform.features sse4 mybuildregistry:5000/redis-x86

You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@clnperez
Contributor

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

@clnperez

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

How would this work? Where do you get the manifests from initially?

@clnperez
Contributor
clnperez commented Oct 19, 2016 edited

@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):

> docker manifest create hello-world-manifest.yaml                                                                                
INFO[0000] Retrieving digests of images...              
INFO[0003] Image "docker.io/ppc64le/hello-world:latest" is digest sha256:4145a08320bcfbc55c4005ddae4e4048af16f3abc754c721801ab88b53710033; size: 503 
INFO[0006] Image "docker.io/hello-world:latest" is digest sha256:0256e8a36e2070f7bf2d0b0763dbabdd67798512411de4cdcf9431a1feb60fd9; size: 502
Digest: sha256:0fb0350d85ea3cef108f5494b95314d7df66e2d89ba23c734ebd27d2a64fe677

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:

image: docker.io/clnperez/hello-world:latest
manifests:
  -
    image: docker.io/ppc64le/hello-world:latest
    platform:
      architecture: ppc64le
      os: linux
  -
    image: docker.io/hello-world:latest
    platform:
      architecture: amd64
      features:
        - sse
      os: linux

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:

docker login
docker manifest create [--dry-run] digest1 [-f d1feature1,d1feature2...] [digest2[-f...]|digest3[-f...]|...]

So the same example as above would be:

docker manifest create docker.io/clnperez/hello-world:latest  ppc64le/hello-world:latest hello-world:latest -f sse

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?

@stevvooe
Contributor

@clnperez docker manifest assemble mybuildregistry:5000/redis mybuildregistry:5000/redis-arm mybuildregistry:5000/redis-s390x mybuildregistry:5000/redis-x86 d could always do the pull.

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 annotate command is quite right, but if we could pull out a handle to manipulate, that would allow the user to insert the right data. Really, what is happening here, is several descriptors are being staged for assembly. One needs to be able to create a set of descriptors based on the upstream content, annotate the descriptors, then assemble that set of descriptors.

@clnperez
Contributor
clnperez commented Oct 20, 2016 edited

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

@clnperez
Contributor
clnperez commented Dec 1, 2016

@stevvooe It took a month, but it's way different now, and and uses the commands you suggested. It's far from perfect, but how does this sound? (I updated the description to reflect the new commands.)

@clnperez
Contributor
clnperez commented Dec 1, 2016

My biggest issue with the way this works now (under the covers) is that it does a fetch of a manifest for everything (every inspect, every fetch, annotate, and create) even though the manifests are locally stored (named as their digest hash). I'm sure there's a way around that but that's how it works at present.

@clnperez
Contributor
clnperez commented Dec 5, 2016

Pinging a few other people I didn't before for general input. @duglin @tianon

Feel free to pull in anyone else for usage +1's or -1s. 😃

@tianon
Member
tianon commented Dec 5, 2016

In discussing this sort of functionality with users, I've heard many express interest in the ability to construct a manifest list somewhat ad-hoc from a group of machines. I think this comment from @StefanScherer sums up the expectations around multi-arch/manifest list support I've seen while talking to folks.

Essentially, a way for a given build machine to build an image for the current architecture and "push" that image into the appropriate architecture slot in an existing manifest list (or create a new one) in a reasonably safe way would cover a lot of the most simple use cases right out of the box. For example, if we're building golang:latest for both Linux on amd64 and Nano Server, then having both architectures build/push elsewhere, then a third process which builds the manifest list is a bit hairy, and it'd be neat if it were possible to instead simply push directly to golang:latest from both places (with a new subcommand or flag) and have it add to an existing manifest.

It looks like something like this could be implemented by using inspect, etc, but that does still leave the problem of needing to push an image explicitly before it can be added to a manifest list, right? (hoping I've missed something 😇)

@stevvooe
Contributor
stevvooe commented Dec 5, 2016

@clnperez Thanks for taking the feedback into account! Sorry for not being more responsive.

Do you have a doc with a API flow example in it? To be honest, it is hard to just read the code and infer if it is right or not.

It might be a good idea to get agreement there and then review against that.

@clnperez
Contributor
clnperez commented Dec 6, 2016

@stevvooe I updated the initial comment with a couple of flow examples. Is that helpful?

Honestly at this point I'd rather have a little more discussion around that flow before really having people digging into the code. Things there still need some work. I just wanted a working prototype for people to play with for now.

@tianon and @StefanScherer -- Can you also try this out and/or take a look at the description to get a general idea of how creating a manifest list will work? This is that "hairy" "third process which builds the manifest list," and while I do like the idea of pushing directly to an image tag by the builder with a new subcommand or flag, I'm not sure if that would be logistically feasible (please discuss!). Image builders would need permission to push to the repo the manifest lives in. Will we have a giant multi-arch repo that all fat manifests live in, or are all project (e.g. mongodb, golang, busybox) owners going to be responsible for collecting image refs for each multi-arch image and creating their own fat manifests?

@stevvooe
Contributor
stevvooe commented Dec 8, 2016

@clnperez That looks great! Thank you so much for accommodating!

I think the missing aspect is that we need to represent the changes to the manifest in way that doesn't assume manifests can be modified in place. For example, annotating a manifest would changes its digest, creating a new manifest. With the cli flow presented here, that would entail changing the manifest for busybox with each change, which isn't ideal on a multi-user system. (Note that my suggestion contained this drawback).

There needs to be someway to represent the flow:

  1. I am creating a manifest list.
  2. I want to add these images, with these annotations.
  3. Now compile this manifest list.

Once that user flow is represented, I think we'll be a good spot.

Does that help? If this is still confusing, I can take another stab at the user flow.

@clnperez
Contributor
clnperez commented Dec 8, 2016

@stevvooe I do get what you're saying. I had the same concern when I started out, which is why my first (current) design was to put the annotated manifests into the user's ~/.docker/ in a new dir call manifests. However, that's not going to work for a couple of reasons (one of which is clear by the current jenkins failures). I'll work on an alternate solution.

@stevvooe
Contributor

@clnperez Great!

It might be interesting to look into a model where the user downloads the manifest locally to a cache, makes the modifications, then posts it back.

I'll be interested in hearing what you come up with!

@clnperez
Contributor

@stevvooe That's kind of what I was going for with putting the manifests in ~/.docker/manifests, but it looks like it's not always going to exist, like in the case of root. I could just have it create the ~/.docker dir if it doesn't exist. WDYT?

@dquintela dquintela referenced this pull request in dquintela/docker-softether Jan 6, 2017
Open

Create multi-arch image #6

@stevvooe
Contributor
stevvooe commented Jan 7, 2017

@clnperez That sounds fine!

@clnperez
Contributor
clnperez commented Jan 9, 2017

@stevvooe Awesome, thanks! I changed mkdir to mkdirAll and also fixed up a lot of newly-missing registry and digest stuff with a rebase. So now the tests pass, and I'll work on some of the "todos." Do you think we could shoot for getting this into 1.14?

@thaJeztah
Member

Windows failure looks related;

18:54:07 ----------------------------------------------------------------------
18:54:07 FAIL: docker_cli_manifest_test.go:54: DockerManifestSuite.SetUpTest
18:54:07 
18:54:07 docker_cli_manifest_test.go:57:
18:54:07     s.regV2 = setupRegistry(c, false, "", privateRegistryURLV2)
18:54:07 docker_utils_test.go:1018:
18:54:07     c.Assert(err, check.IsNil)
18:54:07 ... value *exec.Error = &exec.Error{Name:"registry-v2", Err:(*errors.errorString)(0xc0420090f0)} ("exec: \"registry-v2\": executable file not found in %PATH%")
18:54:07 
18:54:07 
18:54:07 ----------------------------------------------------------------------
18:54:07 PANIC: docker_cli_manifest_test.go:111: DockerManifestSuite.TestManifestAnnotate
18:54:07 
18:54:07 ... Panic: Fixture has panicked (see related PANIC)
18:54:07 
18:54:07 ----------------------------------------------------------------------
@clnperez
Contributor

@thaJeztah Yah, I saw that. I'm planning on looking into differences for Windows (e.g. does the registry just not run on windows?).

@clnperez clnperez WIP: Add Manifest Command
A re-work of my previous port of estesp's manifest tool,
which used a yaml file to annotate and push a new manifest
list. This approach uses a series of commands:

- inspect: will pull down and print out a manifest or manifest list
- fetch: same as inspect but will overwrite any locally-made
  annotations, but won't print the list
- annotate: will modify the locally-stored manifest list by adding
  variants, os features, cpu features, an os kind, and/or an archicture
- create: will assemble and push a new manifest list. This also creates
  cross-repo blob mounts for any layers not existing on the new manifest
lists's target repo.

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:

- Add support for plain http registries.
- Lots of cleanup.
- Lost more tests (and make existing ones much more thorough).
- Add some indicator that a manifest inspect shows a not-yet-commited
manifest.

Signed-off-by: Christy Perez <christy@linux.vnet.ibm.com>
4bc444e
@clnperez
Contributor

@luxas, I just saw your PR to the original manifest tool in github.com/estesp/manifest-tool/pull/16. Will you be able to review this when it's closer to done? Since you're familiar with the original code an also using manifests already, your input would be really valuable.

@clnperez
Contributor

Insecure registry support added.

+ flags := cmd.Flags()
+
+ // @TODO: Should we do any validation? We can't have an exhaustive list
+ flags.StringVar(&opts.os, "os", "", "Add ios info to a manifest before pushing it.")
@tophj-ibm
tophj-ibm Jan 19, 2017 Contributor

I'm guessing you mean os and not ios / IOs 😄

@luxas
luxas commented Jan 19, 2017

@clnperez Sure, I can review this when it's done. I've been watching this thread for quite a long time; waiting for a sign to review it when it's considered ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment