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.

Sign up for GitHub

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

Jump to bottom

[release-5.26] backport API changes copy and list components from upstream. #2071

Closed
wants to merge 49 commits into from
Closed

[release-5.26] backport API changes copy and list components from upstream. #2071

wants to merge 49 commits into from

Conversation

flouthoc
Copy link
Contributor

@flouthoc flouthoc commented Aug 4, 2023

flouthoc and others added 30 commits August 4, 2023 16:27
@flouthoc
list: ListUpdate add Platform field
29d86b6 
`c/image` uses `Instance(` API to get the required details of an
instance while performing replication `Platform` of instance is needed
hence `ListUpdate` must include platform.

Needed by: containers#1987

Signed-off-by: Aditya R <[email protected]>
@flouthoc
blob: TryReusingBlobWithOptions consider requiredCompression if set
85a92ce 
TryReusingBlob now contains a new option `RequiredCompression` which
filters the blob by checking against a compression of the blob which is
considerd to be resued, in case `RequiredCompression` is set and `info`
of the blob being reused does not matches, no blob is returned.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
helpers_test,cleanup: correct argument order
76f7a38 
Fixes: containers#2023 (comment).

`assert.Equal` expects `assert.Equal(t, expected, actual)` instead of `assert.Equal(t, actual, expected)`.

Ref:https://pkg.go.dev/github.com/stretchr/testify/assert#Assertions.Equal

Signed-off-by: Aditya R <[email protected]>
@mtrmac @flouthoc
Fix TestOCI1IndexChooseInstanceByCompression on non-amd64
45c50cf 
Setting SystemContext.ArchitectureChoice to "" does not mean "match any/the first platform";
it's the default behavior of SystemContext, and it means "choose for the current runtime
architecture". (Originally discussed in
containers#1789 (comment) )

I.e. on amd64 these two test cases are redundant with the first two instances above,
and on other architectures (notably ARM) they cause failures.

So just drop them.

Signed-off-by: Miloslav Trmač <[email protected]>
@flouthoc
listupdate,oci: instance show read-only annotations and CompressionAl…
668ff9b 
…gorithmNames

There is a need to read annotations of a particular instance to get its
compression. Expose `Annotations` as a read-only field.

Needed By: containers#1987

Signed-off-by: Aditya R <[email protected]>
@mtrmac @flouthoc
Remove a comment
27e00a8 
- Generally I discourage unsing named return values, it's easy
  to miss that it wasn't set
- I have no idea what the "in the middle of a multi-streamed copy"
  paragraph refers to.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Add *copy.Options to copier
3ec1d30 
... so that we don't have to carry it around in extra
parameters. Migrating individual functions will follow.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Use copier.options in setupSigners
906e173 
Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Use copier.options in sourceSignatures
71fabfb 
Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Make compareImageDestinationManifestEqual a method on imageCopier
426ed94 
... so that we can eliminate three parameters.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Use copier.options in copySingleImage
d720db5 
Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Use copier.options in copyMultipleImages
1e0dafd 
Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Consistently use c.options in copy.Image after it is set
34bb3d7 
... so that the code looks the same here and in possible callees.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Eliminate copier.progress and progressInterval
1f912a3 
Use options.Progress and ProgressInterval directly.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Eliminate copier.ociDecryptConfig
b82a12f 
Use c.options.OciDecryptConfig directly.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Eliminate copier.ociEncryptConfig
9f5ec36 
Use c.options.OciEncryptConfig directly.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Eliminate copier.downloadForeignLayers
5d09be4 
Use c.options.DownloadForeignLayers directly.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Eliminate imageCopier.ociEncryptLayers
9bdb796 
Use ic.c.options.OciEncryptLayers directly.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Introduce copier.unparsedToplevel
2f9d323 
So that we don't need to pass it around in a parameter.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Use copier.unparsedToplevel in copySingleImage
5c88283 
Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Use c.unparsedToplevel in copyMultipleImages
41b8377 
Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Eliminate the unparsedToplevel variable in copy.Image
3abf692 
Use copier.unparsedToplevel now that it exists.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Add policyContext to copier
96446da 
... so that we don't need to carry it around in parameters.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Use copier.policyContext in copySingleImage
e3482e5 
Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Remove the policyContext parameter from copyMultipleImages
76b14a9 
It is no longer used.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Return a new struct copySingleImageResult from copySingleImage
1154668 
... so that we don't have so many unnamed return values, and
we can manage the return values as a batch.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Have compareImageDestinationManifestEqual return a *copySingleImageRe…
171a085 
…sult on a match

... so that the caller doesn't have to assemble it. Using a pointer-or-nil eliminates
a separate boolean.

Signed-off-by: Miloslav Trmač <[email protected]>
@mtrmac @flouthoc
Track the manifest upload data in a wipResult variable
6d08bb4 
... instead of three separate ones.

Signed-off-by: Miloslav Trmač <[email protected]>
@flouthoc
oci_index: init Annotations if nil and required
c459834 
If `UpdateCompressionAlgorithms` is set then `Annotations` map must be
initialized if its not set.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/single: set requiredCompression if compressionFormat is changed
924b778 
Modifies `copy/single` to correctly use the feature from
containers#2023, that is if compression is
changed blob must be resued only if it matches required compression.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/single: wrap arguments in copySingleImageOptions
63deaa7 
Create a wrapper around arguments of `copySingleImage`

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/single: add custom compressionFormat, compressionLevel fields
6272732 
After containers#2048 there is no room for copy/multiple to pass custom compressionFormat to copySingleImage
while processing each instance, following PR introduces that functionality again and wraps options
to simpler struct.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/single: honor c.options.DestCtx for regular copy
cb28b83 
If its a regular copy callers might not set `compressionFormat` and
`compressionLevel` in such case still honor compression which is set in
DestinationCtx from copier.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy: add EnsureCompressionVariantExist for instanceCopyClone
09ac80a 
* copy.Options now contains a new field `EnsureCompressionVariantExists
  map[string]int` which allows users to specify if they want to clone
images with specified `compression`.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
internal/set: implement AddSlice for easier syntax
4536074 
Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple: implement instanceCopyClone
ed1cb97 
Implement `instanceCopyClone` for multiple compression.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple: instanceCopyClone set custom compression
e2551af 
While performing copy, set a custom compression passed generated from
prepareInstanceCopies.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple: error on EnsureCompressionVariant and CopySpecificImages
d392ed7 
Following option does not provides a way to detect and exclude
compression if it already exists, this feature may be implemented in
future.

See: containers#1987 (comment)

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple: report more meaningful copy msg
0aaab65 
After implementing `instanceCopyClone` now instance to be copied can
exceed the original number of instances in the source, so amend report
message to make it more meaningful.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple_test: retrofit tests for instanceCopyCopy
3b5d09e 
Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple_test: implement test for instanceCopyClone with helpers
3214de2 
Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple_test: multiple copy requests of same compression
e4749bd 
* test multiple copy requests of same compression

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple_test: clone must happen once for duplicate entries
4c97ed8 
Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/copy: fail copySingleImage cases on EnsureCompressionVariantsExist
ca3c953 
EnsureCompressionVariantsExist is only valid when working with a
manifest list.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
copy/multiple: priority of instanceCopyCopy must be higher
d932df1 
When copying multiple images i.e `instanceCopyClone` and no image
exists in registry in such case if clones are prepared and
copied first then original copies will reuse blobs from the clone which
is unexpected since argument `requireCompressionFormatMatch` is by
default false for `instanceCopyCopy` case.

Problem described in following PR is easily reproduceable when working
with tools such as buildah, because without this PR buildah will push
`clones` first instead of originals and later on `originals` will reuse
blobs from `clones`.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 4, 2023 •
edited
Loading

@mtrmac PTAL, I will create a bump version PR after this.

@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 4, 2023 •
edited
Loading

Huh skopeo tests are failing I wonder why ? is it because I missed to backport some commit ?

Edit: Looks like skopeo test on this branch is using skopeo from upstream, which has updated deps. Not sure if we can update those deps in this backport. @mtrmac Any suggestions ?

@flouthoc flouthoc mentioned this pull request Aug 4, 2023
Closed
@mtrmac
Copy link
Collaborator

mtrmac commented Aug 4, 2023

@flouthoc On a stable branch, set SKOPEO_CI_TAG to a similarly frozen Skopeo version (a branch if there is one, or just a commit ID).

Ideally we should have done that at time of branching … I’m afraid that rarely happens.

@mtrmac mtrmac added the kind/feature A request for, or a PR adding, new functionality label Aug 4, 2023
@mtrmac
Copy link
Collaborator

mtrmac commented Aug 4, 2023

Code LGTM… but per the other conversation, this should be a 5.27.0. Let me work on that…

@mtrmac mtrmac mentioned this pull request Aug 4, 2023
Merged
@mtrmac
Copy link
Collaborator

mtrmac commented Aug 4, 2023

@flouthoc I have filed #2074 for what I think is necessary to make the tests pass, and #2075 as a proof of concept — note that that’s against the wrong branch.

@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 5, 2023

Okay should I close this PR and create similar PR against release-5.27 then

@mtrmac
Copy link
Collaborator

mtrmac commented Aug 5, 2023

@flouthoc Now that CI on the 5.27 branch is running, yes, let’s do a 5.27.0.

I’ll update #2075 just so that it is ready, but you did all the backporting work so you deserve the full credit — if you want to rebase+re-file this, by all means go ahead.

mtrmac
mtrmac reviewed Aug 5, 2023
View reviewed changes
.cirrus.yml
@@ -6,9 +6,9 @@ env:
#### Global variables used for all tasks
####
# Name of the ultimate destination branch for this CI run
DEST_BRANCH: "main"
DEST_BRANCH: "release-5.26"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Great catch!

.cirrus.yml Show resolved Hide resolved
@flouthoc
Copy link
Contributor Author

flouthoc commented Aug 5, 2023

@mtrmac it seems just pointing #2075 to newer branch will do the trick, it will be easier for you to point changes #2075 to release-5.27. Thanks.

@flouthoc flouthoc closed this Aug 5, 2023
@mtrmac mtrmac mentioned this pull request Aug 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cevich cevich cevich left review comments

@mtrmac mtrmac mtrmac left review comments

Assignees
No one assigned
Labels
kind/feature A request for, or a PR adding, new functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@flouthoc @mtrmac @cevich