Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions image/copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (

encconfig "github.com/containers/ocicrypt/config"
digest "github.com/opencontainers/go-digest"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/sirupsen/logrus"
"go.podman.io/image/v5/docker/reference"
internalblobinfocache "go.podman.io/image/v5/internal/blobinfocache"
Expand Down Expand Up @@ -93,8 +94,9 @@ type Options struct {
PreserveDigests bool
// manifest MIME type of image set by user. "" is default and means use the autodetection to the manifest MIME type
ForceManifestMIMEType string
ImageListSelection ImageListSelection // set to either CopySystemImage (the default), CopyAllImages, or CopySpecificImages to control which instances we copy when the source reference is a list; ignored if the source reference is not a list
Instances []digest.Digest // if ImageListSelection is CopySpecificImages, copy only these instances and the list itself
ImageListSelection ImageListSelection // set to either CopySystemImage (the default), CopyAllImages, or CopySpecificImages to control which instances we copy when the source reference is a list; ignored if the source reference is not a list
Instances []digest.Digest // if ImageListSelection is CopySpecificImages, copy only these instances, instances matching the InstancePlatforms list, and the list itself
InstancePlatforms []imgspecv1.Platform // if ImageListSelection is CopySpecificImages, copy only matching instances, instances listed in the Instances list, and the list itself
// Give priority to pulling gzip images if multiple images are present when configured to OptionalBoolTrue,
// prefers the best compression if this is configured as OptionalBoolFalse. Choose automatically (and the choice may change over time)
// if this is set to OptionalBoolUndefined (which is the default behavior, and recommended for most callers).
Expand Down
44 changes: 42 additions & 2 deletions image/copy/multiple.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import (
"go.podman.io/image/v5/internal/set"
"go.podman.io/image/v5/manifest"
"go.podman.io/image/v5/pkg/compression"
"go.podman.io/image/v5/types"
)

type instanceCopyKind int
Expand Down Expand Up @@ -104,7 +105,7 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.
res := []instanceCopy{}
if options.ImageListSelection == CopySpecificImages && len(options.EnsureCompressionVariantsExist) > 0 {
// List can already contain compressed instance for a compression selected in `EnsureCompressionVariantsExist`
// Its unclear what it means when `CopySpecificImages` includes an instance in options.Instances,
// It's unclear what it means when `CopySpecificImages` includes an instance in options.Instances,
// EnsureCompressionVariantsExist asks for an instance with some compression,
// an instance with that compression already exists, but is not included in options.Instances.
// We might define the semantics and implement this in the future.
Expand All @@ -118,9 +119,19 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.
if err != nil {
return nil, err
}

// Determine which specific images to copy (combining digest-based and platform-based selection)
var specificImages *set.Set[digest.Digest]
if options.ImageListSelection == CopySpecificImages {
specificImages, err = determineSpecificImages(options, list)
if err != nil {
return nil, err
}
}

for i, instanceDigest := range instanceDigests {
if options.ImageListSelection == CopySpecificImages &&
!slices.Contains(options.Instances, instanceDigest) {
!specificImages.Contains(instanceDigest) {
logrus.Debugf("Skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests))
continue
}
Expand Down Expand Up @@ -157,6 +168,35 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.
return res, nil
}

// determineSpecificImages returns a set of images to copy based on the
// Instances and InstancePlatforms fields of the passed-in options structure
func determineSpecificImages(options *Options, updatedList internalManifest.List) (*set.Set[digest.Digest], error) {
// Start with the instances that were listed by digest.
specificImages := set.New[digest.Digest]()
for _, instanceDigest := range options.Instances {
specificImages.Add(instanceDigest)
}

if len(options.InstancePlatforms) > 0 {
// Choose the best match for each platform we were asked to
// also copy, and add it to the set of instances to copy.
for _, platform := range options.InstancePlatforms {
platformContext := types.SystemContext{
OSChoice: platform.OS,
ArchitectureChoice: platform.Architecture,
VariantChoice: platform.Variant,
}
instanceDigest, err := updatedList.ChooseInstanceByCompression(&platformContext, options.PreferGzipInstances)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. Better UX: Platform-based selection should be inclusive by default. Users shouldn't lose compression variants unless explicitly requested.

  2. Avoids digest lookup: The whole point of InstancePlatforms is convenience. While users could use the Instances field 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.

  3. Clear semantics:

    • InstancePlatforms = broad selection (by platform)
    • Instances = precise selection (by digest)
    • Compressions field = optional filter
  4. 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.

  5. Not a breaking change: Since InstancePlatforms is 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.

if err != nil {
return nil, fmt.Errorf("choosing instance for platform spec %v: %w", platform, err)
}
specificImages.Add(instanceDigest)
}
}

return specificImages, nil
}

// copyMultipleImages copies some or all of an image list's instances, using
// c.policyContext to validate source image admissibility.
func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, retErr error) {
Expand Down
73 changes: 73 additions & 0 deletions image/copy/multiple_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"testing"

digest "github.com/opencontainers/go-digest"
imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
internalManifest "go.podman.io/image/v5/internal/manifest"
Expand Down Expand Up @@ -170,3 +171,75 @@ func convertInstanceCopyToSimplerInstanceCopy(copies []instanceCopy) []simplerIn
}
return res
}

// TestDetermineSpecificImages tests the platform-based and digest-based instance selection
func TestDetermineSpecificImages(t *testing.T) {
validManifest, err := os.ReadFile(filepath.Join("..", "internal", "manifest", "testdata", "ociv1.image.index.json"))
require.NoError(t, err)
list, err := internalManifest.ListFromBlob(validManifest, internalManifest.GuessMIMEType(validManifest))
require.NoError(t, err)

// Digests from ociv1.image.index.json
ppc64leDigest := digest.Digest("sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f")
amd64Digest := digest.Digest("sha256:5b0bcabd1ed22e9fb1310cf6c2dec7cdef19f0ad69efa1f392e94a4333501270")

tests := []struct {
name string
instances []digest.Digest
instancePlatforms []imgspecv1.Platform
expectedDigests []digest.Digest
expectError bool
}{
{
name: "PlatformOnly",
instancePlatforms: []imgspecv1.Platform{
{OS: "linux", Architecture: "ppc64le"},
},
expectedDigests: []digest.Digest{ppc64leDigest},
},
{
name: "DigestOnly",
instances: []digest.Digest{amd64Digest},
expectedDigests: []digest.Digest{amd64Digest},
},
{
name: "Combined",
instances: []digest.Digest{ppc64leDigest},
instancePlatforms: []imgspecv1.Platform{
{OS: "linux", Architecture: "amd64"},
},
expectedDigests: []digest.Digest{ppc64leDigest, amd64Digest},
},
{
name: "NonExistentPlatform",
instancePlatforms: []imgspecv1.Platform{
{OS: "linux", Architecture: "arm64"},
},
expectError: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
options := &Options{
Instances: tt.instances,
InstancePlatforms: tt.instancePlatforms,
}
specificImages, err := determineSpecificImages(options, list)

if tt.expectError {
assert.Error(t, err)
assert.Contains(t, err.Error(), "choosing instance for platform")
return
}

require.NoError(t, err)
// Convert Set to slice for comparison
actualDigests := []digest.Digest{}
for d := range specificImages.All() {
actualDigests = append(actualDigests, d)
}
assert.ElementsMatch(t, tt.expectedDigests, actualDigests)
})
}
}
4 changes: 4 additions & 0 deletions image/internal/set/set.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,7 @@ func (s *Set[E]) Empty() bool {
func (s *Set[E]) All() iter.Seq[E] {
return maps.Keys(s.m)
}

func (s *Set[E]) Size() int {
return len(s.m)
}