copy: add InstancePlatforms field for platform-based filtering#656
copy: add InstancePlatforms field for platform-based filtering#656aguidirh wants to merge 2 commits intocontainers:mainfrom
Conversation
Add the ability to select images by platform name instead of requiring digest hashes. This implements the functionality originally proposed in containers/image#1938. When copying specific images from a multi-architecture manifest list, users currently must specify exact digest hashes. This is cumbersome and error-prone, as users must manually look up digests and can easily confuse which digest corresponds to which platform. This commit adds a new InstancePlatforms field that allows users to specify platforms by human-readable names like "linux/amd64" or "linux/arm64". The field works alongside the existing Instances field, allowing users to combine both methods. The implementation includes: 1. New InstancePlatforms []imgspecv1.Platform field in Options 2. determineSpecificImages() function to resolve platforms to digests and combine with digest-based selection 3. Efficient Set-based filtering (O(1) lookup vs O(n) with slices) 4. Size() method added to internal/set for Set length queries 5. Table-driven tests covering all selection scenarios Based on original work by @nalind in containers/image#1938, adapted for the container-libs monorepo structure. Relates to containers#227 Signed-off-by: Alex Guidi <aguidi@redhat.com>
|
Packit jobs failed. @containers/packit-build please check. |
1 similar comment
|
Packit jobs failed. @containers/packit-build please check. |
mtrmac
left a comment
There was a problem hiding this comment.
Thanks!
An extremely brief look for now.
| ArchitectureChoice: platform.Architecture, | ||
| VariantChoice: platform.Variant, | ||
| } | ||
| instanceDigest, err := updatedList.ChooseInstanceByCompression(&platformContext, options.PreferGzipInstances) |
There was a problem hiding this comment.
Hum… we have multi-compression images (where there is a gzip instance and a zstd instance for the same platform). This would only copy one of the instances.
OTOH a trivial “does the instance match the required Platform value” check might copy too much, because a v1 variant requirement would match a v1,v2,v3 instances.
There was a problem hiding this comment.
Thank you for the feedback! You're right that using ChooseInstanceByCompression() only copies one instance per platform, potentially missing multi-compression variants.
Proposed Solution
Default behavior: Copy ALL compression variants for platforms specified in InstancePlatforms. This preserves all available data for selected platforms, maintains the original digest for each instance, and avoids implicit filtering.
For compression-specific selection: Introduce a wrapper struct that allows optional compression filtering:
type PlatformSelection struct {
Platform imgspecv1.Platform
Compressions []compression.Algorithm // nil or empty = copy all compressions
}
type Options struct {
// ... existing fields ...
InstancePlatforms []PlatformSelection
}Usage Examples
Copy all compressions (default, most common):
InstancePlatforms: []PlatformSelection{
{Platform: {OS: "linux", Architecture: "amd64"}}, // Compressions nil = all
}Filter to specific compressions:
InstancePlatforms: []PlatformSelection{
{
Platform: {OS: "linux", Architecture: "amd64"},
Compressions: []compression.Algorithm{compression.Gzip},
},
}Different compressions per platform:
InstancePlatforms: []PlatformSelection{
{
Platform: {OS: "linux", Architecture: "amd64"},
Compressions: []compression.Algorithm{compression.Gzip},
},
{
Platform: {OS: "linux", Architecture: "arm64"},
Compressions: nil, // all compressions
},
}Rationale
-
Better UX: Platform-based selection should be inclusive by default. Users shouldn't lose compression variants unless explicitly requested.
-
Avoids digest lookup: The whole point of
InstancePlatformsis convenience. While users could use theInstancesfield with exact digests for compression-specific control, this would require manually looking up digests for each compression variant—a poor user experience that defeats the purpose of platform-based selection. -
Clear semantics:
InstancePlatforms= broad selection (by platform)Instances= precise selection (by digest)Compressionsfield = optional filter
-
Future extensibility: Using a wrapper struct that we control (rather than directly exposing
imgspecv1.Platform) makes it easier to extend in the future. We can add new fields for additional filtering or options without being constrained by the external OCI spec types. -
Not a breaking change: Since
InstancePlatformsis new in this PR, we can get the design right before it becomes public API.
Does this approach address your concerns? I'm happy to implement it if you agree with the direction.
image/copy/multiple_test.go
Outdated
| } | ||
|
|
||
| require.NoError(t, err) | ||
| assert.Equal(t, tt.expectedSize, specificImages.Size()) |
There was a problem hiding this comment.
This can probably use assert.ElementsMatch, for a better error output on failure and perhaps smaller test code.
There was a problem hiding this comment.
Thanks for the feedback, you're right, the test became smaller.
I just pushed a commit with your suggestion.
|
I've approved this and restarted a couple of failng tests. I'd be interested in seeing your reply to @mtrmac 's comment on the multi-compression images. Overall, a nice change, thanks! |
Address code review feedback from @mtrmac: - Use assert.ElementsMatch() instead of manual assertion loop - Remove unnecessary expectedSize field from test struct Signed-off-by: Alex Guidi <aguidi@redhat.com>
Add platform-based filtering for copying specific images
This PR adds the ability to select images by platform name (e.g.,
linux/amd64) instead of requiring digest hashes, implementing the functionality proposed in containers/image#1938.Motivation
Relates to #227
When copying specific images from a multi-architecture manifest list, the current
CopySpecificImagesmode requires users to specify exact digest hashes in theInstancesfield. This is cumbersome because:This PR adds a new
InstancePlatformsfield that allows users to specify platforms by human-readable names.Changes
1. New
InstancePlatformsfield inimage/copyInstancePlatforms []imgspecv1.Platformfield toOptionsstruct{OS: "linux", Architecture: "amd64"}Instancesfield (both can be used simultaneously)2. Platform resolution in
determineSpecificImages()(image/copy/multiple.go)ChooseInstanceByCompression()to find best match for each platform3. Efficient Set-based filtering
slices.Contains()to Set-based lookupSize()method tointernal/set/set.go4. Comprehensive test coverage
TestDetermineSpecificImageswith table-driven testsUser Experience Improvement
Before (digest-based only):
After (platform-based):
Combined (both methods):
Testing
All existing tests pass. New tests verify:
Compatibility
Instancesfield continues to work unchangedCredits
This implementation is based on the original work by @nalind in containers/image#1938, adapted for the container-libs monorepo structure with the following changes: