Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support copy multi arch instance #2612

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
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 copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,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
ImageListPlatforms []manifest.Schema2PlatformSpec // if ImageListSelection is CopySpecificImages, copy only these target platforms
Instances []digest.Digest // if ImageListSelection is CopySpecificImages, copy only these instances and the list itself, this is auto generated by ImageListPlatforms
// 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 Expand Up @@ -325,6 +326,7 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef,
if !supportsMultipleImages(c.dest) {
return nil, fmt.Errorf("copying multiple images: destination transport %q does not support copying multiple images as a group", destRef.Transport().Name())
}

// Copy some or all of the images.
switch c.options.ImageListSelection {
case CopyAllImages:
Expand Down
86 changes: 81 additions & 5 deletions copy/multiple.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,9 @@ func platformV1ToPlatformComparable(platform *imgspecv1.Platform) platformCompar
}
osFeatures := slices.Clone(platform.OSFeatures)
sort.Strings(osFeatures)
return platformComparable{architecture: platform.Architecture,
os: platform.OS,
return platformComparable{
architecture: platform.Architecture,
os: platform.OS,
// This is strictly speaking ambiguous, fields of OSFeatures can contain a ','. Probably good enough for now.
osFeatures: strings.Join(osFeatures, ","),
osVersion: platform.OSVersion,
Expand Down Expand Up @@ -98,8 +99,65 @@ func validateCompressionVariantExists(input []OptionCompressionVariant) error {
return nil
}

func isPlatformSupported(list internalManifest.List, platform manifest.Schema2PlatformSpec) (digest.Digest, bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It’s unexpected for a predicate-named function is… to return a non-boolean value.

for _, instanceDigest := range list.Instances() {
instance, err := list.Instance(instanceDigest)
if err != nil {
return "", false
}

if instance.ReadOnly.Platform == nil {
continue
}

if instance.ReadOnly.Platform.OS == platform.OS &&
instance.ReadOnly.Platform.Architecture == platform.Architecture {
return instanceDigest, true
}
}

return "", false
}

func filterInstancesByPlatforms(list internalManifest.List, platforms []manifest.Schema2PlatformSpec) ([]digest.Digest, error) {
missingPlatforms := []manifest.Schema2PlatformSpec{}
supportedInstance := []digest.Digest{}
// Check each requested platform
for _, platform := range platforms {
if digest, ok := isPlatformSupported(list, platform); !ok {
missingPlatforms = append(missingPlatforms, platform)
} else {
supportedInstance = append(supportedInstance, digest)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is insufficient; there can be multiple instances for the same platform, and in that case, by default, all of them should be copied. (Compare EnsureCompressionVariantsExist).

Copy link
Author

Choose a reason for hiding this comment

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

I am simulating Docker buildx with this parameter, and the examples passed in are linux/amd64,linux/arm64。If user need to copy all instance, they need to add such as linux/arm64,linux/arm/v6

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t think that’s applicable. The manifest can, legitimately, contain two instances with exactly the same platform specification.

}
}

if len(missingPlatforms) > 0 {
var platformStrings []string
for _, p := range missingPlatforms {
platformStr := fmt.Sprintf("%s/%s", p.OS, p.Architecture)
if p.Variant != "" {
platformStr += "/" + p.Variant
}
platformStrings = append(platformStrings, platformStr)
}
return nil, fmt.Errorf("requested platforms not found in image: %s", strings.Join(platformStrings, ", "))
}

if len(platforms) == 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

(If this were a special case, I’d expect it to be handled at the top, before starting the other logic.)

supportedInstance = list.Instances()
}

return supportedInstance, nil
}

// prepareInstanceCopies prepares a list of instances which needs to copied to the manifest list.
func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.Digest, options *Options) ([]instanceCopy, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

prepareInstanceCopies exists, to a large extent, to be a strictly isolated ~functional piece of code that can be unit tested; the option handling, especially the way various options can interact, should have unit tests.

filteredInstanceDigests, err := filterInstancesByPlatforms(list, options.ImageListPlatforms)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The CopySpecificImages logic exists at the start of the existing range instanceDigest loop; splitting that into two separate locations is unexpected.

if err != nil {
return nil, err
}
options.Instances = filteredInstanceDigests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Outright overwriting and ignoring previously-defined options is not acceptable.


res := []instanceCopy{}
if options.ImageListSelection == CopySpecificImages && len(options.EnsureCompressionVariantsExist) > 0 {
// List can already contain compressed instance for a compression selected in `EnsureCompressionVariantsExist`
Expand All @@ -109,7 +167,8 @@ func prepareInstanceCopies(list internalManifest.List, instanceDigests []digest.
// We might define the semantics and implement this in the future.
return res, fmt.Errorf("EnsureCompressionVariantsExist is not implemented for CopySpecificImages")
}
err := validateCompressionVariantExists(options.EnsureCompressionVariantsExist)

err = validateCompressionVariantExists(options.EnsureCompressionVariantsExist)
if err != nil {
return res, err
}
Expand Down Expand Up @@ -252,15 +311,17 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
UpdateDigest: updated.manifestDigest,
UpdateSize: int64(len(updated.manifest)),
UpdateCompressionAlgorithms: updated.compressionAlgorithms,
UpdateMediaType: updated.manifestMIMEType})
UpdateMediaType: updated.manifestMIMEType,
})
case instanceCopyClone:
logrus.Debugf("Replicating instance %s (%d/%d)", instance.sourceDigest, i+1, len(instanceCopyList))
c.Printf("Replicating image %s (%d/%d)\n", instance.sourceDigest, i+1, len(instanceCopyList))
unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceCopyList[i].sourceDigest)
updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceCopyList[i].sourceDigest, copySingleImageOptions{
requireCompressionFormatMatch: true,
compressionFormat: &instance.cloneCompressionVariant.Algorithm,
compressionLevel: instance.cloneCompressionVariant.Level})
compressionLevel: instance.cloneCompressionVariant.Level,
})
if err != nil {
return nil, fmt.Errorf("replicating image %d/%d from manifest list: %w", i+1, len(instanceCopyList), err)
}
Expand All @@ -285,6 +346,21 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte,
return nil, fmt.Errorf("updating manifest list: %w", err)
}

if len(c.options.Instances) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I’d expect this logic to be driven by prepareInstanceCopies, therefore unit-testable, and unit-tested.

deletedInstanceDigests := []internalManifest.ListEdit{}
for _, instanceDigest := range updatedList.Instances() {
if !slices.Contains(c.options.Instances, instanceDigest) {
deletedInstanceDigests = append(deletedInstanceDigests, internalManifest.ListEdit{
ListOperation: internalManifest.ListOpRemove,
UpdateOldDigest: instanceDigest,
})
}
}
if err = updatedList.EditInstances(deletedInstanceDigests); err != nil {
return nil, fmt.Errorf("updating manifest list: %w", err)
}
}

Comment on lines +349 to +363
Copy link
Collaborator

Choose a reason for hiding this comment

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

We outright must not change the semantics of the existing Instances option to trigger manifest edits. c/image keeps a stable API, if at all possible.

// Iterate through supported list types, preferred format first.
c.Printf("Writing manifest list to image destination\n")
var errs []string
Expand Down
11 changes: 10 additions & 1 deletion internal/manifest/docker_schema2_list.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,8 @@ func (index *Schema2ListPublic) UpdateInstances(updates []ListUpdate) error {
UpdateDigest: instance.Digest,
UpdateSize: instance.Size,
UpdateMediaType: instance.MediaType,
ListOperation: ListOpUpdate})
ListOperation: ListOpUpdate,
})
}
return index.editInstances(editInstances)
}
Expand Down Expand Up @@ -128,6 +129,14 @@ func (index *Schema2ListPublic) editInstances(editInstances []ListEdit) error {
},
schema2PlatformSpecFromOCIPlatform(*editInstance.AddPlatform),
})
case ListOpRemove:
targetIndex := slices.IndexFunc(index.Manifests, func(m Schema2ManifestDescriptor) bool {
return m.Digest == editInstance.UpdateOldDigest
})
if targetIndex == -1 {
return fmt.Errorf("Schema2List.EditInstances: digest %s not found", editInstance.UpdateOldDigest)
}
index.Manifests = append(index.Manifests[:targetIndex], index.Manifests[targetIndex+1:]...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use some form of slices.Delete*. And have a test.

default:
return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation)
}
Expand Down
1 change: 1 addition & 0 deletions internal/manifest/list.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ const (
listOpInvalid ListOp = iota
ListOpAdd
ListOpUpdate
ListOpRemove
)

// ListEdit includes the fields which a List's EditInstances() method will modify.
Expand Down
12 changes: 11 additions & 1 deletion internal/manifest/oci_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ func (index *OCI1IndexPublic) UpdateInstances(updates []ListUpdate) error {
UpdateDigest: instance.Digest,
UpdateSize: instance.Size,
UpdateMediaType: instance.MediaType,
ListOperation: ListOpUpdate})
ListOperation: ListOpUpdate,
})
}
return index.editInstances(editInstances)
}
Expand Down Expand Up @@ -166,6 +167,15 @@ func (index *OCI1IndexPublic) editInstances(editInstances []ListEdit) error {
Platform: editInstance.AddPlatform,
Annotations: annotations,
})
case ListOpRemove:
targetIndex := slices.IndexFunc(index.Manifests, func(m imgspecv1.Descriptor) bool {
return m.Digest == editInstance.UpdateOldDigest
})

if targetIndex == -1 {
return fmt.Errorf("OCI1Index.EditInstances: digest %s not found", editInstance.UpdateOldDigest)
}
index.Manifests = append(index.Manifests[:targetIndex], index.Manifests[targetIndex+1:]...)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use some form of slices.Delete*. And have a test.

default:
return fmt.Errorf("internal error: invalid operation: %d", editInstance.ListOperation)
}
Expand Down