bug fix: handle CopySpecificImages correctly #1483
Conversation
Signed-off-by: Philipp Rosenkranz <[email protected]>
There was a problem hiding this comment.
Thanks for the PR.
For better or worse, the current implementation (essentially a sparse copy) is international (e.g. documented in skopeo copy --multi-arch=index-only). We can’t just break it.
A feature to strip the image of unwanted architectures (while still keeping it a multi-arch image, not just extracting a single architecture) does sound useful, but it would be a new feature. There’s also a more generic request to add manifest editing support (e.g. containers/skopeo#1136 ), and ideally we would want that to replace the c/common / Buildah / Podman implementations.
So that would be interesting, but a significantly larger work than this.
The need to keep the API of c/image stable is also a fairly significant constraint. One we can work with, but it requires more infrastructure, and thinking carefully about any new API additions.
| @@ -33,6 +33,10 @@ type List interface { | |||
| // must be specified. | |||
| UpdateInstances([]ListUpdate) error | |||
|
|
|||
| // UpdateInstancesAndDiscardOthers overrides the instances slice with information contained in the passed-in slice. | |||
| // All instances which do not have a matching digest in the passed-in slice will be discarded. | |||
| UpdateInstancesAndDiscardOthers([]ListUpdate) error | |||
There was a problem hiding this comment.
I’m afraid we can’t just add new methods to interfaces, that’s an API break. We are building infrastructure to allow extending things (see c/image/internal/private), but that doesn’t currently cover manifest.List.
So this would end up being a significantly larger PR.
| // the images we want to copy. | ||
| if len(copiedUpdates) < len(instanceDigests) { | ||
| updates = copiedUpdates | ||
| if err = updatedList.UpdateInstancesAndDiscardOthers(updates); err != nil { |
There was a problem hiding this comment.
I don’t see why we would pass the whole update to both UpdateInstancesAndDiscardOthers and to UpdateInstances below.
I don’t immediately know whether it makes more sense to have a single “edit + change set of instances” function, or to split that into edit/add/remove. UpdateInstancesAndDiscardOthers intuitively looks pretty far from a generic/orthogonal interface.
Note to self: Compare also the “manifest editing” implementation in Buildah, Podman, containers/common, to understand the problem space and necessary features.
| for _, u := range listUpdate { | ||
| if m.Digest == u.Digest { | ||
| existingManifest[u.Digest] = m | ||
| } | ||
| } |
There was a problem hiding this comment.
AFAICS this could unconditionally build existingManifest[m.Digest] = m; the entries to be removed would just be ignored. That turns this from O(N^2) into O(N).
Same in the other implementation.
| } | ||
| list.Manifests = []Schema2ManifestDescriptor{} | ||
| for _, u := range listUpdate { | ||
| updatedManifest, ok := existingManifest[u.Digest] |
There was a problem hiding this comment.
I don’t think this works at all in the case where the copy had to convert the format of the original manifest, so u.Digest doesn‘t match any pre-existing value in list.Manifests.
Currently the
ImageListSelectionsettingcopy.CopySpecificImagesincopy.Optionsis useless since any digest slice passed toInstancesmust have the same length as the slice of manifests in the source list.Therefore, the only way to use
copy.CopySpecificImagesis to pass intoInstancesall digests which are present in the source list of manifests. Thus,copy.CopySpecificImagesbehaves like a really clunky version ofcopy.CopyAllImages.This PR ensures that
copy.CopySpecificImagesis actually useful (again?) by removing any manifest from the list which cannot be matched with a digest listed inInstances.