copy: add option to strip sparse manifest lists #655
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,27 +21,32 @@ import ( | |
| "go.podman.io/image/v5/pkg/compression" | ||
| ) | ||
|
|
||
| type instanceOpKind int | ||
|
|
||
| const ( | ||
| instanceOpCopy instanceOpKind = iota | ||
| instanceOpClone | ||
| instanceOpDelete | ||
| ) | ||
|
|
||
| type instanceOp struct { | ||
| op instanceOpKind | ||
| sourceDigest digest.Digest | ||
|
|
||
| // Fields which can be used by callers when operation | ||
| // is `instanceOpCopy` | ||
| copyForceCompressionFormat bool | ||
|
|
||
| // Fields which can be used by callers when operation | ||
| // is `instanceOpClone` | ||
| cloneArtifactType string | ||
| cloneCompressionVariant OptionCompressionVariant | ||
| clonePlatform *imgspecv1.Platform | ||
| cloneAnnotations map[string]string | ||
|
|
||
| // Fields which can be used by callers when operation | ||
| // is `instanceOpDelete` | ||
| deleteIndex int | ||
| } | ||
|
|
||
| // internal type only to make imgspecv1.Platform comparable | ||
|
|
@@ -99,72 +104,95 @@ func validateCompressionVariantExists(input []OptionCompressionVariant) error { | |
| return nil | ||
| } | ||
|
|
||
| // prepareInstanceOps prepares a list of operations to perform on instances (copy, clone, or delete). | ||
| // It returns a unified list of all operations and the count of copy/clone operations (excluding deletes). | ||
| func prepareInstanceOps(list internalManifest.List, instanceDigests []digest.Digest, options *Options, cannotModifyManifestListReason string) ([]instanceOp, int, error) { | ||
| res := []instanceOp{} | ||
| deleteOps := []instanceOp{} | ||
| if options.ImageListSelection == CopySpecificImages && len(options.EnsureCompressionVariantsExist) > 0 { | ||
| // List can already contain compressed instance for a compression selected in `EnsureCompressionVariantsExist` | ||
| // 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. | ||
| return res, -1, fmt.Errorf("EnsureCompressionVariantsExist is not implemented for CopySpecificImages") | ||
| } | ||
| err := validateCompressionVariantExists(options.EnsureCompressionVariantsExist) | ||
| if err != nil { | ||
| return res, -1, err | ||
| } | ||
| compressionsByPlatform, err := platformCompressionMap(list, instanceDigests) | ||
| if err != nil { | ||
| return nil, -1, 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, -1, err | ||
| } | ||
| } | ||
|
|
||
| for i, instanceDigest := range instanceDigests { | ||
| if options.ImageListSelection == CopySpecificImages && | ||
| !specificImages.Contains(instanceDigest) { | ||
| if options.SparseManifestListAction == StripSparseManifestList { | ||
| if cannotModifyManifestListReason != "" { | ||
| return nil, -1, fmt.Errorf("we should delete instance %s from manifest list, but we cannot: %s", instanceDigest, cannotModifyManifestListReason) | ||
| } | ||
| logrus.Debugf("deleting instance %s from destination’s manifest (%d/%d)", instanceDigest, i+1, len(instanceDigests)) | ||
| deleteOps = append(deleteOps, instanceOp{ | ||
| op: instanceOpDelete, | ||
| deleteIndex: i, | ||
| }) | ||
| } else { | ||
| logrus.Debugf("skipping instance %s (%d/%d)", instanceDigest, i+1, len(instanceDigests)) | ||
| } | ||
| continue | ||
| } | ||
| instanceDetails, err := list.Instance(instanceDigest) | ||
| if err != nil { | ||
| return res, -1, fmt.Errorf("getting details for instance %s: %w", instanceDigest, err) | ||
| } | ||
| forceCompressionFormat, err := shouldRequireCompressionFormatMatch(options) | ||
| if err != nil { | ||
| return nil, -1, err | ||
| } | ||
| res = append(res, instanceOp{ | ||
| op: instanceOpCopy, | ||
| sourceDigest: instanceDigest, | ||
| copyForceCompressionFormat: forceCompressionFormat, | ||
| }) | ||
| platform := platformV1ToPlatformComparable(instanceDetails.ReadOnly.Platform) | ||
| compressionList := compressionsByPlatform[platform] | ||
| for _, compressionVariant := range options.EnsureCompressionVariantsExist { | ||
| if !compressionList.Contains(compressionVariant.Algorithm.Name()) { | ||
| res = append(res, instanceOp{ | ||
| op: instanceOpClone, | ||
| sourceDigest: instanceDigest, | ||
| cloneArtifactType: instanceDetails.ReadOnly.ArtifactType, | ||
| cloneCompressionVariant: compressionVariant, | ||
| clonePlatform: instanceDetails.ReadOnly.Platform, | ||
| cloneAnnotations: maps.Clone(instanceDetails.ReadOnly.Annotations), | ||
| }) | ||
| compressionList.Add(compressionVariant.Algorithm.Name()) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Add delete operations in reverse order (highest to lowest index) to avoid shifting | ||
| slices.Reverse(deleteOps) | ||
| copyLen := len(res) // Count copy/clone operations before appending deletes | ||
|
|
||
| if copyLen == 0 && len(deleteOps) == len(instanceDigests) { | ||
| return nil, -1, fmt.Errorf("requested operation filtered out all platforms and would create an empty image") | ||
| } | ||
|
|
||
|
Comment on lines
+189
to
+192
Member
There was a problem hiding this comment. Why was this added? Pushing an empty manifest used to work and is done by several podman tests as such this breaks podman CI now. podman-container-tools/podman#28339 I have not looked at the larger picture here if this limitation should just be removed. If this needs more work then I would propose to revert this PR to unblock others people vendor work.
Contributor
Author
There was a problem hiding this comment. This was added only to avoid pushing empty manifests. Why empty manifests are a valid scenario? I can create another PR to remove those lines above if empty manifests are a valid scenario. What do you think @mtrmac ?
Contributor
There was a problem hiding this comment. I think copying empty images should work, why not; but stripping a non-empty manifest to empty is almost certainly a mistake. I’ll try to fix that today. |
||
| res = append(res, deleteOps...) | ||
|
mtrmac marked this conversation as resolved.
|
||
|
|
||
| return res, copyLen, nil | ||
| } | ||
|
|
||
| // determineSpecificImages returns a set of images to copy based on the | ||
|
|
@@ -222,7 +250,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, | |
|
|
||
| sigs, err := c.sourceSignatures(ctx, c.unparsedToplevel, | ||
| "Getting image list signatures", | ||
| "Checking if image list destination supports signatures", true) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -248,7 +276,7 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, | |
| // Compare, and perhaps keep in sync with, the version in copySingleImage. | ||
| cannotModifyManifestListReason := "" | ||
| if len(sigs) > 0 { | ||
| cannotModifyManifestListReason = "Would invalidate signatures; consider removing them from the multi-platform list" | ||
| } | ||
| if destIsDigestedReference { | ||
| cannotModifyManifestListReason = "Destination specifies a digest" | ||
|
|
@@ -272,29 +300,31 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, | |
| } | ||
| if selectedListType != originalList.MIMEType() { | ||
| if cannotModifyManifestListReason != "" { | ||
| return nil, fmt.Errorf("Manifest list must be converted to type %q to be written to destination, but we cannot modify it: %s", selectedListType, cannotModifyManifestListReason) | ||
| } | ||
| } | ||
|
|
||
| // Copy each image, or just the ones we want to copy, in turn. | ||
| instanceDigests := updatedList.Instances() | ||
| instanceEdits := []internalManifest.ListEdit{} | ||
| instanceOpList, copyLen, err := prepareInstanceOps(updatedList, instanceDigests, c.options, cannotModifyManifestListReason) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("preparing instances for copy: %w", err) | ||
| } | ||
| c.Printf("Copying %d images generated from %d images in list\n", copyLen, len(instanceDigests)) | ||
| copyCount := 0 // Track copy/clone operations separately from delete operations | ||
| for i, instance := range instanceOpList { | ||
| // Update instances to be edited by their `ListOperation` and | ||
| // populate necessary fields. | ||
| switch instance.op { | ||
| case instanceOpCopy: | ||
| copyCount++ | ||
| logrus.Debugf("Copying instance %s (%d/%d)", instance.sourceDigest, copyCount, copyLen) | ||
| c.Printf("Copying image %s (%d/%d)\n", instance.sourceDigest, copyCount, copyLen) | ||
| unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceOpList[i].sourceDigest) | ||
| updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceOpList[i].sourceDigest, copySingleImageOptions{requireCompressionFormatMatch: instance.copyForceCompressionFormat}) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("copying image %d/%d from manifest list: %w", copyCount, copyLen, err) | ||
| } | ||
| // Record the result of a possible conversion here. | ||
| instanceEdits = append(instanceEdits, internalManifest.ListEdit{ | ||
|
|
@@ -305,17 +335,18 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, | |
| UpdateCompressionAlgorithms: updated.compressionAlgorithms, | ||
| UpdateMediaType: updated.manifestMIMEType, | ||
| }) | ||
| case instanceOpClone: | ||
| copyCount++ | ||
| logrus.Debugf("Replicating instance %s (%d/%d)", instance.sourceDigest, copyCount, copyLen) | ||
| c.Printf("Replicating image %s (%d/%d)\n", instance.sourceDigest, copyCount, copyLen) | ||
| unparsedInstance := image.UnparsedInstance(c.rawSource, &instanceOpList[i].sourceDigest) | ||
| updated, err := c.copySingleImage(ctx, unparsedInstance, &instanceOpList[i].sourceDigest, copySingleImageOptions{ | ||
| requireCompressionFormatMatch: true, | ||
| compressionFormat: &instance.cloneCompressionVariant.Algorithm, | ||
| compressionLevel: instance.cloneCompressionVariant.Level, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("replicating image %d/%d from manifest list: %w", copyCount, copyLen, err) | ||
| } | ||
| // Record the result of a possible conversion here. | ||
| instanceEdits = append(instanceEdits, internalManifest.ListEdit{ | ||
|
|
@@ -328,12 +359,17 @@ func (c *copier) copyMultipleImages(ctx context.Context) (copiedManifest []byte, | |
| AddAnnotations: instance.cloneAnnotations, | ||
| AddCompressionAlgorithms: updated.compressionAlgorithms, | ||
| }) | ||
| case instanceOpDelete: | ||
| instanceEdits = append(instanceEdits, internalManifest.ListEdit{ | ||
| ListOperation: internalManifest.ListOpDelete, | ||
| DeleteIndex: instance.deleteIndex, | ||
| }) | ||
| default: | ||
| return nil, fmt.Errorf("copying image: invalid copy operation %d", instance.op) | ||
| } | ||
| } | ||
|
|
||
| // Now reset the digest/size/types of the manifests in the list and remove deleted instances. | ||
| if err = updatedList.EditInstances(instanceEdits, cannotModifyManifestListReason != ""); err != nil { | ||
| return nil, fmt.Errorf("updating manifest list: %w", err) | ||
| } | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.