Add names-history support #422

saschagrunert · 2019-08-28T08:53:36Z

This commit adds a new NamesHistory field to the images.json, which
is basically a deduped list of names the image had in the past. The
first entry of the list is the latest history entry.

The main use case for this feature is to tell the end-user which
names/tags an image had in the past if it does not contain any names
any more.

Detailed use case:

Pulling image:v1 into the local registry: names: [ "image:v1" ]
Pushing a new image as image:v1 into the remote registry
Pulling image:v1 again into the local registry:
- first image: names: [ "image:v1" ]
- previous v1 image: names: [], names-history: [ "image:v1" ]

An consumer of the storage API can now process the image name and
still display image as REPOSITORY, like:
Before:

> podman images
REPOSITORY      TAG      IMAGE ID       CREATED          SIZE
image           v1       25b62d1b654a   13 seconds ago   2.07 kB
<none>          <none>   b134eff7b955   17 seconds ago   2.07 kB

After:

> podman images
REPOSITORY      TAG      IMAGE ID       CREATED          SIZE
image           v1       25b62d1b654a   13 seconds ago   2.07 kB
image           <none>   b134eff7b955   17 seconds ago   2.07 kB

Since the NamesHistory is a slice we would be able to tell the
end-user which names an image ID had before.

The change should be backwards compatible with previous versions of
containers/storage.

/cc @mtrmac

saschagrunert · 2019-08-28T08:54:37Z

@rhatdan @vrothberg @nalind @mtrmac WDYT? It is mainly another approach to improve the information density about images to end users without breaking past versions.

rhatdan · 2019-08-28T09:01:21Z

I like it. What happens if I tag the image? If I can tag it, can I still push it? Can I remove it via tag name?
Why is this a list, how could I have multiple former image names?

vrothberg

Sounds like a nice thing to have 👍

vrothberg · 2019-08-28T09:12:33Z

images.go

@@ -492,6 +504,7 @@ func (r *imageStore) SetNames(id string, names []string) error {
 		}
 		for _, name := range names {
 			if otherImage, ok := r.byname[name]; ok {
+				r.addNameToHistory(otherImage, name)


I suggest calling addNameToHistory(...) from removeName(...). This way, we're sure to not miss an execution path.

Yes, I changed it and bound the new method to the *Image type, which should be more clear. 👍

saschagrunert · 2019-08-28T09:38:03Z

What happens if I tag the image?

So from my local experiments, if I do:

Create a base image without names:

> ./bin/podman build -f Dockerfile-empty -t img:v1

Now the images.json contains the name correctly but no history:

[
  {
    "id": "a13e0f4e4ce6ce32820739c1b892a2c24f694dfc676216a9e5f19d0596da6968",
    "digest": "sha256:108f68845a70455485fb8677c3db83f596dc05ec257ddb4626254ccbd9a0c686",
    "names": [
      "localhost/img:v1"
    ],
    "layer": "5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef",
    ...
  }
]

Tag multiple other names:

> ./bin/podman tag img:v1 img:v2 img:v3

We still have it as names, because they are all distinct:

[
  {
    "id": "a13e0f4e4ce6ce32820739c1b892a2c24f694dfc676216a9e5f19d0596da6968",
    "digest": "sha256:108f68845a70455485fb8677c3db83f596dc05ec257ddb4626254ccbd9a0c686",
    "names": [
      "localhost/img:v1",
      "localhost/img:v2",
      "localhost/img:v3"
    ],
    "layer": "5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef",
    ...
  }
]

> ./podman images
REPOSITORY      TAG   IMAGE ID       CREATED              SIZE
localhost/img   v1    a13e0f4e4ce6   About a minute ago   2.07 kB
localhost/img   v2    a13e0f4e4ce6   About a minute ago   2.07 kB
localhost/img   v3    a13e0f4e4ce6   About a minute ago   2.07 kB

Now we steal the img:v1 tag by creating a new empty image:

> ./bin/podman build -f Dockerfile-empty -t img:v1

[
  {
    "id": "a13e0f4e4ce6ce32820739c1b892a2c24f694dfc676216a9e5f19d0596da6968",
    "digest": "sha256:108f68845a70455485fb8677c3db83f596dc05ec257ddb4626254ccbd9a0c686",
    "names": [
      "localhost/img:v2",
      "localhost/img:v3"
    ],
    "names-history": [
      "localhost/img:v1"
    ],
    ...
  },
  {
    "id": "12d9c90a46f52a4d6abaf2605fd0bf9a89fe8763e3f13993fb894b739b6c1111",
    "digest": "sha256:df4879cb75d87edc08639e0c7252cb70945cd304f6eceba43a2349c93f7777c3",
    "names": [
      "localhost/img:v1"
    ],
    "layer": "5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef",
    ...
  }
]

Take another tag from the first image:

> ./bin/podman tag img:v1 img:v2

[
  {
    "id": "a13e0f4e4ce6ce32820739c1b892a2c24f694dfc676216a9e5f19d0596da6968",
    "digest": "sha256:108f68845a70455485fb8677c3db83f596dc05ec257ddb4626254ccbd9a0c686",
    "names": [
      "localhost/img:v3"
    ],
    "names-history": [
      "localhost/img:v2",
      "localhost/img:v1"
    ],
    ...
  },
  {
    "id": "12d9c90a46f52a4d6abaf2605fd0bf9a89fe8763e3f13993fb894b739b6c1111",
    "digest": "sha256:df4879cb75d87edc08639e0c7252cb70945cd304f6eceba43a2349c93f7777c3",
    "names": [
      "localhost/img:v1",
      "localhost/img:v2"
    ],
    "layer": "5f70bf18a086007016e948b04aed3b82103a36bea41755b6cddfaf10ace3c6ef",
    ...
  }
]

If I can tag it, can I still push it? Can I remove it via tag name?

I think we could add dedicated command line options to buildah/podman to interact with old images, for example something like podman rmi --cleanup.

Why is this a list, how could I have multiple former image names?

See my example above, I also thought about adding something like a timestamp to indicate when it has been retagged, but this would clash with the current de-duplication approach.

rhatdan · 2019-08-28T13:01:26Z

I think we need @nalind and @mtrmac opinion on this. Also give @mrunalp and @vbatts a heads up.

mtrmac · 2019-08-28T14:17:34Z

Can we first explicitly clarify whether the goal of the PR is intended to mirror existing docker images, or whether it is a new thing being designed?

For the former, I suppose we would get more similar behavior by implementing RepoDigests the way Docker does (notably docker tag does not allow creating a “name:” entry locally I think.

For the latter, it just seems weird to me to see a $name entry in any form in the output after an explicit rm/untag of $name . (That’s an UI concern not directly impacting the design of the c/storage data, but still.)

mtrmac · 2019-08-28T14:18:42Z

For the former, I suppose we would get more similar behavior by implementing RepoDigests the way Docker does (notably docker tag does not allow creating a “name:” entry locally I think.

… and we would still need to implement RepoDigests correctly even after this PR and using the history data in docker images output would mask some of the impacts of not recording RepoDigests.

saschagrunert · 2019-08-29T07:33:04Z

Can we first explicitly clarify whether the goal of the PR is intended to mirror existing docker images, or whether it is a new thing being designed?

I think the major use case is: Displaying the image name to the user if an image has no Names any more due to re-tag, point 4

In CRI-O, we could add these values to the ListImages RPC, whereas in podman we could either add an additional colum to podman images or stripping of the tag and showing it as REPOSITORY value.

mtrmac

The implementation LGTM.

I’m not quite convinced by the proposed UI, but that’s really not something to be solved here and now.

mtrmac · 2019-08-29T17:28:12Z

images.go

+	// NamesHistory is an optional set of Names the image had in the past. The
+	// contained names are free from any duplicates, whereas the newest entry
+	// is the first one.
+	NamesHistory []string `json:"names-history,omitempty"`


Currently (and especially with the changes what I expect to happen for RepoDigests), Names can contain both name:tag and name@digest values. So, NamesHistory would currently contain both as well. Is that OK?

(Ideally, I’d like be to insist on strongly typed values like reference.NamedTagged, encoding the semantics in the type system, but with Names []string that doesn’t seem reasonable right now.)

Currently (and especially with the changes what I expect to happen for RepoDigests), Names can contain both name:tag and name@digest values. So, NamesHistory would currently contain both as well. Is that OK?

I think so.

(Ideally, I’d like be to insist on strongly typed values like reference.NamedTagged, encoding the semantics in the type system, but with Names []string that doesn’t seem reasonable right now.)

Yeah, we could change both Names and NamesHistory to be forcibly valid references, but I wanted to add the change as non-breaking as possible.

mtrmac · 2019-08-29T17:38:28Z

I also thought about adding something like a timestamp to indicate when it has been retagged, but this would clash with the current de-duplication approach.

Well, the deduplication is really not all that much code; if we need to make it a bit more complex to get the data structure we want, we should just do that.

On the one hand, if nothing needs a timestamp now, it seems rather pointless to add them now.

On the other hand, if we do end up adding timestamps in the future, it can’t be done within the current NamesHistory array and we would have to either break the ABI by changing the field, or maintain two independent fields with redundant values forever. So, getting this correct right now is worth a bit of thought before committing to the API.

(One possible way to defer that decision: make the field private or documented-to-be-private, and add a public getter method; then we can enrich the recorded data without breaking the API.)

Up to you, @nalind and others, I really understand neither the UI ambitions nor the API design of c/storage sufficiently.

rhatdan · 2019-11-05T18:26:00Z

@saschagrunert You still interested in this?

saschagrunert · 2019-11-06T08:51:23Z

Yes, I would stick to the current implementation approach and rebased on top of the latest master branch.

@mtrmac Do you see any blocker for this getting merged (if the CI passes)?
@nalind May I ask you to please take a look at this one?

mtrmac

Overall I’m not too enthusiastic about the importance of use case (and the partial overlap with missing RepoDigests functionality feels vaguely suspect), but I’m completely fine with this existing — up to @nalind.

I got another look at the implementation, though, and the code does not match the documentation AFAICS.

mtrmac · 2019-11-06T17:29:48Z

images.go

@@ -478,9 +484,14 @@ func (r *imageStore) SetMetadata(id, metadata string) error {
 }

 func (r *imageStore) removeName(image *Image, name string) {
+	image.addNameToHistory(name)


removeName is, AFAICS, only called on a name conflict (e.g. if SetNames adds another name to an image, and that name has been used by a different image); it is not called on untag (SetNames removing an existing name).

That’s good enough as far as the original motivation goes:

I think the major use case is: Displaying the image name to the user if an image has no Names any more due to re-tag, point 4

but not consistent with how NamesHistory is documented:

NamesHistory is an optional set of Names the image had in the past.

Alright, I updated the documentation as suggested.

I was somewhat expecting an implementation change instead of documenting the behavior, but, sure, this is consistent. Up to @nalind.

Yeah I can also change the implementation to match the previous documentation, this was not clear for me in the first place.

Changed the implementation and added tests on top.

This commit adds a new `NamesHistory` field to the `images.json`, which is basically a deduped list of names the image had in the past. The first entry of the list is the latest history entry. The main use case for this feature is to tell the end-user which names/tags an image had in the past if it does not contain any `names` any more. Detailed use case: 1. Pulling `image:v1` into the local registry: `names: [ "image:v1" ]` 2. Pushing a new image as `image:v1` into the remote registry 3. Pulling `image:v1` again into the local registry: - first image: `names: [ "image:v1" ]` - previous v1 image: `names: [], names-history: [ "image:v1" ]` 4. An consumer of the storage API can now process the image name and still display `image` as REPOSITORY, like: * Before: ``` > podman images REPOSITORY TAG IMAGE ID CREATED SIZE image v1 25b62d1b654a 13 seconds ago 2.07 kB <none> <none> b134eff7b955 17 seconds ago 2.07 kB ``` * After: ``` > podman images REPOSITORY TAG IMAGE ID CREATED SIZE image v1 25b62d1b654a 13 seconds ago 2.07 kB image <none> b134eff7b955 17 seconds ago 2.07 kB ``` 5. Since the `NamesHistory` is a slice we would be able to tell the end-user which names an image ID had before. The change should be backwards compatible with previous versions of containers/storage. Signed-off-by: Sascha Grunert <[email protected]>

rhatdan · 2019-11-22T06:00:44Z

LGTM

saschagrunert · 2019-11-26T08:28:30Z

Hey, may I ask regarding a new release that I can bring this feature into CRI-O and libpod?

vrothberg · 2019-11-26T08:36:19Z

The last release was just 7 days ago but I think Dan wants throw out some of the recent changes soon. @rhatdan, is anything blocking us from cutting another release?

rhatdan · 2019-11-26T11:12:48Z

I will cut a release.

rhatdan · 2019-11-26T11:16:27Z

#468

vrothberg reviewed Aug 28, 2019

View reviewed changes

saschagrunert changed the title ~~WIP/RFC: Add names-history support~~ Add names-history support Aug 28, 2019

mtrmac reviewed Aug 29, 2019

View reviewed changes

mtrmac reviewed Nov 6, 2019

View reviewed changes

rhatdan merged commit 8aac4ac into containers:master Nov 22, 2019

saschagrunert deleted the history-names branch November 22, 2019 21:24

saschagrunert mentioned this pull request Nov 26, 2019

[Discussion] Names history support for images containers/podman#4566

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add names-history support #422

Add names-history support #422

saschagrunert commented Aug 28, 2019

saschagrunert commented Aug 28, 2019

rhatdan commented Aug 28, 2019

vrothberg left a comment

vrothberg Aug 28, 2019

saschagrunert Aug 28, 2019

saschagrunert commented Aug 28, 2019

rhatdan commented Aug 28, 2019

mtrmac commented Aug 28, 2019

mtrmac commented Aug 28, 2019

saschagrunert commented Aug 29, 2019

mtrmac left a comment

mtrmac Aug 29, 2019

saschagrunert Aug 30, 2019

mtrmac Aug 30, 2019

mtrmac commented Aug 29, 2019

rhatdan commented Nov 5, 2019

saschagrunert commented Nov 6, 2019

mtrmac left a comment

mtrmac Nov 6, 2019

saschagrunert Nov 7, 2019

mtrmac Nov 7, 2019

saschagrunert Nov 8, 2019

saschagrunert Nov 11, 2019

rhatdan commented Nov 22, 2019

saschagrunert commented Nov 26, 2019

vrothberg commented Nov 26, 2019

rhatdan commented Nov 26, 2019

rhatdan commented Nov 26, 2019

Add names-history support #422

Add names-history support #422

Conversation

saschagrunert commented Aug 28, 2019

saschagrunert commented Aug 28, 2019

rhatdan commented Aug 28, 2019

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saschagrunert commented Aug 28, 2019

rhatdan commented Aug 28, 2019

mtrmac commented Aug 28, 2019

mtrmac commented Aug 28, 2019

saschagrunert commented Aug 29, 2019

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

mtrmac commented Aug 29, 2019

rhatdan commented Nov 5, 2019

saschagrunert commented Nov 6, 2019

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

rhatdan commented Nov 22, 2019

saschagrunert commented Nov 26, 2019

vrothberg commented Nov 26, 2019

rhatdan commented Nov 26, 2019

rhatdan commented Nov 26, 2019