copy.Options: add an InstancePlatforms field #1938
Conversation
nalind
force-pushed
the
instance-platforms
branch
2 times, most recently
from
19cbc8b
to
bec4f3b
Compare
There was a problem hiding this comment.
Thanks!
The implementation looks good overall.
WRT the API:
Looking at the example in containers/skopeo#1935 , and the code in https://github.com/osbuild/osbuild/blob/b92168bc16d8251256ec34df938d616bca99cd22/sources/org.osbuild.skopeo#L105 , it seems that the primary use case revolves around the CopySystemImage choice, what containers/skopeo#1935 calls --multi-arch=sparse:[system]. (Cc: @achilleas-k to keep me honest.)
How is that going to be represented? I’m a bit torn.
- One approach is to model it in this API explicitly (and therefore inherit the way
platform.WantedPlatformsconsumestypes.SystemContext): seems more consistent, OTOH the API could be more complex, either with the members ofInstancePlatformssome kind of sum type (enum{ case system; case platform(platform) }), or with an extraInstanceForSystemPlatformfield. Not quite elegant. - Another is to do this just in the Skopeo CLI. Probably simpler all around, OTOH I think it’s valuable to be fairly strict about Skopeo features all being available to other callers of c/image.
Ideally I think some kind of c/image API this would be better, assuming we can design a practical one that doesn’t grow to an absurd size.
This PR doesn’t say, so I assume it plans to use the Skopeo approach? Or a separate c/image PR?
(For the record, and mostly because I wrote this for the initial implementation which was destructive. Not a concern for the code as is.
- There are two kinds of users in this area
- Some want to keep the original manifest list, and create a sparse copy (with some instances missing), so that the digest references are not broken — that’s what RUN-1803 wants, some registries reject such lists.
- Some want to strip the modify the manifest list, so that the copy is accepted by all registries. That’s e.g. Add copy option to strip sparse manifests #1707 .
The API to choose instances should (for the future where the “strip” variant exists) allow both, with a single orthogonal switch between the two behaviors. That’s probably going to happen “by default” in many implementations (the API of EditInstances kind of forces that on us :) ), just something to keep an eye on.)
Cc: @flouthoc for the interaction with the Zstd work — I think this should be mostly orthogonal, just a heads-up about potential merge conflicts.
|
The VM img.keepalive test had failed, it look like a google instance issue. I've restarted. |
containers/skopeo#1987 implements that in Skopeo. |
|
Note to self: the |
|
Oh… The decision on what to filter on is actually non-obvious. Do we want platform triplets, or separately OSes and architectures? All three? containers/skopeo#1987 (comment) for a starting point, to continue tomorrow… |
nalind
force-pushed
the
instance-platforms
branch
2 times, most recently
from
474b858
to
d2f3505
Compare
nalind
force-pushed
the
instance-platforms
branch
4 times, most recently
from
60d1598
to
f275417
Compare
internal/set/set.go
Outdated
| @@ -44,3 +44,7 @@ func (s *Set[E]) Empty() bool { | |||
| func (s *Set[E]) Values() []E { | |||
| return maps.Keys(s.m) | |||
| } | |||
|
|
|||
| func (s Set[E]) Size() int { | |||
There was a problem hiding this comment.
| func (s Set[E]) Size() int { | |
| func (s *Set[E]) Size() int { |
please.
The current version works just fine, but I managed to inattentively screw up the Add/Delete methods (#1951 ), so using a pointer receiver everywhere would hopefully make it harder for me to make the same mistake again.
nalind
force-pushed
the
instance-platforms
branch
from
48e0897
to
b60035d
Compare
Add an InstancePlatforms field to the copy.Options structure. When asked to copy only a specific subset of instances, combine the Instances list with the instances which match the platforms given in the InstancePlatforms list, returning an error if we're unable to find an instance which matches any of the InstancePlatforms. Signed-off-by: Nalin Dahyabhai <nalin@redhat.com>
nalind
force-pushed
the
instance-platforms
branch
from
b60035d
to
76bbbbe
Compare
Add an
InstancePlatformsfield to thecopy.Optionsstructure. When asked to copy only a specific subset of instances, combine theInstanceslist with the instances which match the platforms given in theInstancePlatformslist, returning an error if we're unable to find an instance which matches any of theInstancePlatforms. I think this is a necessary step in implementing RUN-1803 in skopeo cleanly.