blob: `TryReusingBlobWithOptions` consider `RequiredCompression` if set #2023

flouthoc · 2023-07-02T07:25:35Z

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.

flouthoc · 2023-07-02T07:26:42Z

I tested the docker blob's in combination with a variant of code from #1987 and it is working as expected. I made similar changes to other transport as well where change was applicable.

flouthoc · 2023-07-02T15:37:35Z

@mtrmac PTAL

directory/directory_dest.go

docker/docker_image_dest.go

internal/private/private.go

storage/storage_dest.go

internal/private/private.go

mtrmac · 2023-07-07T00:14:13Z

I think it would be easier if the new option only applied in the CanSubstitute case; then these simple implementations that never substitute blobs wouldn’t need to add extra conditions.

On second thought, I’m afraid that doesn’t work well. For converting from gzip to zstd, we ideally want an operation to express “I have a gzip blob, but I want zstd; so check if there is a zstd variant, but don’t use a gzip one even if it is an exact match”. In that case CanSubstitute will be set, but even an exact match needs to consider the RequiredCompression condition.

So every single transport needs to deal with this, sadly, and your original approach was correct.

The options.RequiredCompression == nil || (options.RequiredCompression.Name() == info.CompressionAlgorithm.Name()) condition will, then, be copy&pasted so many times that it seems useful to share it as a helper function in the internal/imagedestination/impl package.

Note that the info.CompressionAlgorithm field is not, in general, expected to be provided to TryReusingBlob (really adding that field has been a mistake.) We do set it in the copy pipeline, but really it is only documented as an input to .UpdateLayerInfos. A minimal fix would be to document that if RequiredCompression is set, info.CompressionAlgorithm must be set as well; I think it would be cleaner instead, to provide the algorithm specifically as an input for TryReusingBlobWithOptions, as a new field TryReusingBlobOptions.OriginalCompression or something like that. And then document that it must be set along with RequiredCompression.

flouthoc · 2023-07-08T00:12:22Z

@mtrmac PTAL

mtrmac

I’m sorry, for some reason 3 comments were left pending.

docker/docker_image_dest.go

internal/private/private.go

internal/imagedestination/impl/compat.go

mtrmac

blobCacheDestination.TryReusingBlobWithOptions also needs to implement this
AFAICS the docker transport can crash
blob: TryReusingBlobWithOptions consider RequiredCompression if set #2023 (comment) : I’d prefer not to add the new semantics of carrying the original blob’s algorithm in BlobInfo; one more field in TryReusingBlobOptions seems more explicit to me

internal/imagedestination/impl/helpers.go

docker/docker_image_dest.go

mtrmac · 2023-07-10T19:23:45Z

docker/docker_image_dest.go

+			continue
+		}
+		if !impl.BlobMatchesRequiredCompression(options, compressionAlgorithm) {
+			logrus.Debugf("Ignoring candidate blob %s as reuse candidate since compression %s was requested and blob has compression %s", candidate.Digest.String(), options.RequiredCompression.Name(), compressionAlgorithm.Name())


This also needs to not crash on compressionAlgorithm == nil.

(Also compare the “Trying to reuse” code; including candidateRepo.Name() seems useful — especially if the code is moved so that the “trying” logs aren’t emitted at all.)

(Absolutely non-blocking: making this a bit shorter would help on narrow terminals / log viewers: “Not using …, compression mismatch (%s vs. %s)”. The text should make sense but it can probably be assumed the reader can refer to the related source code, so it doesn’t need to be quite self-documenting.)

docker/docker_image_dest.go

flouthoc · 2023-07-11T14:42:04Z

@mtrmac Could you PTAL again.

mtrmac

Thanks, one last cleanup round please.

docker/docker_image_dest.go

internal/imagedestination/impl/helpers.go

internal/private/private.go

mtrmac

Also one outstanding item from #2023 (review):

blobCacheDestination.TryReusingBlobWithOptions also needs to implement this

ostree/ostree_dest.go

internal/imagedestination/impl/helpers.go

mtrmac

Thanks!

internal/imagedestination/impl/helpers_test.go

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]>

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]>

ForceCompressionFormat allows end users to force selected compression format (set in DestinationCtx.CompressionFormat) which ensures that while copying blobs of other compression algorithms are not reused. Following flag is a frontend wrapper for: containers#2023 Will help in: * containers/buildah#4613 * containers/podman#18660 Signed-off-by: Aditya R <[email protected]>

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]>

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]>

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]>

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]>

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]>

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 force-pushed the tryreusingblob-compression branch from 65b7f41 to bb04be5 Compare July 2, 2023 15:36

flouthoc mentioned this pull request Jul 2, 2023

copy: implement instanceCopyClone for zstd compression #1987

Merged

mtrmac reviewed Jul 3, 2023

View reviewed changes

flouthoc force-pushed the tryreusingblob-compression branch 2 times, most recently from e9089cc to 1bf8943 Compare July 3, 2023 17:33

flouthoc requested a review from mtrmac July 3, 2023 17:35

flouthoc mentioned this pull request Jul 6, 2023

manifest, push: implement --add-compression to push with compressed variants. containers/buildah#4912

Merged

flouthoc force-pushed the tryreusingblob-compression branch from 1bf8943 to d3d5df7 Compare July 7, 2023 05:33

mtrmac reviewed Jul 8, 2023

View reviewed changes

flouthoc force-pushed the tryreusingblob-compression branch from d3d5df7 to 7304098 Compare July 8, 2023 01:56

flouthoc requested a review from mtrmac July 8, 2023 02:01

flouthoc force-pushed the tryreusingblob-compression branch 2 times, most recently from 1779182 to 77b9752 Compare July 10, 2023 15:33

mtrmac reviewed Jul 10, 2023

View reviewed changes

flouthoc force-pushed the tryreusingblob-compression branch from 77b9752 to cdd22f1 Compare July 11, 2023 06:14

flouthoc requested a review from mtrmac July 11, 2023 14:42

mtrmac reviewed Jul 11, 2023

View reviewed changes

docker/docker_image_dest.go Outdated Show resolved Hide resolved

internal/imagedestination/impl/helpers.go Outdated Show resolved Hide resolved

internal/private/private.go Outdated Show resolved Hide resolved

internal/private/private.go Outdated Show resolved Hide resolved

flouthoc force-pushed the tryreusingblob-compression branch 4 times, most recently from 3938c90 to c637ac7 Compare July 12, 2023 08:14

flouthoc requested a review from mtrmac July 12, 2023 16:32

mtrmac reviewed Jul 12, 2023

View reviewed changes

ostree/ostree_dest.go Outdated Show resolved Hide resolved

internal/imagedestination/impl/helpers.go Outdated Show resolved Hide resolved

mtrmac reviewed Jul 12, 2023

View reviewed changes

internal/imagedestination/impl/helpers.go Outdated Show resolved Hide resolved

mtrmac reviewed Jul 12, 2023

View reviewed changes

internal/imagedestination/impl/helpers.go Outdated Show resolved Hide resolved

flouthoc force-pushed the tryreusingblob-compression branch from 3d230ea to 4f8c039 Compare July 12, 2023 18:28

mtrmac reviewed Jul 12, 2023

View reviewed changes

internal/imagedestination/impl/helpers_test.go Show resolved Hide resolved

mtrmac approved these changes Jul 12, 2023

View reviewed changes

mtrmac merged commit fa37116 into containers:main Jul 12, 2023

flouthoc mentioned this pull request Jul 13, 2023

helpers_test,cleanup: correct argument order #2039

Merged

flouthoc mentioned this pull request Jul 18, 2023

copy/multiple: instanceCopyCopy honor UpdateCompressionAlgorithms #2047

Merged

flouthoc mentioned this pull request Aug 1, 2023

manifest, push: implement --add-compression to push with compressed variants. containers/podman#19468

Merged

flouthoc mentioned this pull request Aug 2, 2023

copy: add support for ForceCompressionFormat #2068

Merged

flouthoc mentioned this pull request Aug 4, 2023

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

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blob: `TryReusingBlobWithOptions` consider `RequiredCompression` if set #2023

blob: `TryReusingBlobWithOptions` consider `RequiredCompression` if set #2023

flouthoc commented Jul 2, 2023

flouthoc commented Jul 2, 2023

flouthoc commented Jul 2, 2023

mtrmac commented Jul 7, 2023 •

edited

Loading

flouthoc commented Jul 8, 2023

mtrmac left a comment

mtrmac left a comment

mtrmac Jul 10, 2023

flouthoc commented Jul 11, 2023

mtrmac left a comment

mtrmac left a comment

mtrmac left a comment •

edited

Loading

blob: TryReusingBlobWithOptions consider RequiredCompression if set #2023

blob: TryReusingBlobWithOptions consider RequiredCompression if set #2023

Conversation

flouthoc commented Jul 2, 2023

flouthoc commented Jul 2, 2023

flouthoc commented Jul 2, 2023

mtrmac commented Jul 7, 2023 • edited Loading

flouthoc commented Jul 8, 2023

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac Jul 10, 2023

Choose a reason for hiding this comment

flouthoc commented Jul 11, 2023

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac left a comment • edited Loading

Choose a reason for hiding this comment

blob: `TryReusingBlobWithOptions` consider `RequiredCompression` if set #2023

blob: `TryReusingBlobWithOptions` consider `RequiredCompression` if set #2023

mtrmac commented Jul 7, 2023 •

edited

Loading

mtrmac left a comment •

edited

Loading