libimage: add support for `ForceCompressionFormat` for image `copier` and `manifest` #1602

flouthoc · 2023-08-08T06:27:36Z

Bump c/image to v5.26.1-0.20230807184415-3fb422379cfa
libimage/copier: wire ForceCompressionFormat for image copy
manifest: add support ForceCompressionFormat

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.

See: containers/image#2068 for more details.

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc · 2023-08-08T06:28:33Z

@mtrmac @vrothberg @giuseppe @rhatdan PTAL

vrothberg · 2023-08-08T07:04:12Z

libimage/copier.go

@@ -49,6 +49,10 @@ type CopyOptions struct {
 	CompressionFormat *compression.Algorithm
 	// CompressionLevel specifies what compression level is used
 	CompressionLevel *int
+	// ForceCompressionFormat ensures that the compression algorithm set in
+	// DestinationCtx.CompressionFormat is used exclusively, and blobs of other


I don't think it's clear what DestinationCtx.CompressionFormat is in libimage.

I think it's important to highlight why setting CompressionFormat may not suffice.

It should be CompressionFormat I have amended this.

vrothberg · 2023-08-08T07:05:54Z

libimage/push_test.go

+
+	pushOptions := &PushOptions{}
+	pushOptions.CompressionFormat = &compression.Gzip
+	pushOptions.ForceCompressionFormat = true


How could we verify that layers are getting compressed correctly? Should we load/pull the archive and inspect?

Easier and actual integration test would be against a registry, where we can pre-upload a gzip and while pushing again with ForceCompressionFormat we can check push logs that no reuse candidates are there, it can be easily verified by inspecting push logs. Its easier to add this test on Podman/Buildah side.

could we push to a local "oci:" directory and inspect the tarball there?

let me check that, I'll amend test if that works.

I don't think the problem which ForceCompressionFormat is solving persists with other transports which works locally, c/image force pushes blobs for oci: and dir: transports when same destination is used even ending up creating invalid OCI images. ( I have added a test and it passes even when ForceCompressionFormat = false , I see similar behavior when directly working with buildah )

I am only able to reproduce the problem which ForceCompressionFormat solves with registry as transport where candidates are actually re-evaluated if they can reused before actually Copying content. @mtrmac could you confirm this because afaics local transports and not attempting to re-use dest blobs as registry:// is doing.

dir: can’t be used to test reuse because every write to dir: deletes the previous contents.

Local transports are also trying to reuse blobs, but they don’t look for candidates in the blob info cache.

In effect, that means that reuse happens between compressed and uncompressed versions, only if the digest from the source matches an existing blob on the destination. In particular, copying an image from an uncompressed source (most commonly c/storage) to a destination uncompressed, and then copying the same image from the same uncompressed source with compression enabled triggers a reuse.

Demo:

# Create an uncompressed source $ skopeo copy --override-os linux --dest-decompress docker://quay.io/libpod/alpine dir:uncompressed0 # Normal behavior of copies to OCI: compress $ skopeo copy --override-os linux dir:uncompressed0 oci:direct $ ls -la direct/blobs/sha256 … 2765584 … d8761bb9b4751d85618813b8f32d545dfb8dabc79ec8c2a59429851e8e5ee6f5 # Copy uncompressed first $ skopeo copy --override-os linux --dest-oci-accept-uncompressed-layers dir:uncompressed0 oci:reuse $ ls -la reuse/blobs/sha256 … 5589504 … 5e0d8111135538b8a86ce5fc969849efce16c455fd016bb3dc53131bcedc4da5 # … And then a compressed copy triggers reuse: $ skopeo copy --override-os linux dir:uncompressed0 oci:reuse … Copying blob 5e0d81111355 skipped: already exists … # With a compressed layer not added $ ls -la reuse/blobs/sha256 … 5589504 … 5e0d8111135538b8a86ce5fc969849efce16c455fd016bb3dc53131bcedc4da5

@mtrmac Thanks :) this worked for me in the test.

mtrmac

This looks plausible at a very quick glance, I didn’t read the test now.

(An alternative would be to add an ExclusiveCompressionFormat option, which would set the algorithm + the flag in one, but that’s not orthogonal, e.g. we would need to also support setting compression level. To an extent this seems to what we anticipate the CLI design to be … and I think --force-compression-format or something like that is reasonable, and IIRC there’s precedent for an option with some similar name in Docker.)

flouthoc · 2023-08-08T11:27:58Z

--force-compression-format SGTM for cli end.

mtrmac · 2023-08-10T17:05:32Z

--force-compression-format SGTM for cli end.

Found the original reference: containers/buildah#4613 (comment) .

mtrmac · 2023-08-10T17:06:27Z

… which is not using the --force-compression-format spelling, because it is not a top-level CLI option at all. Still, a sort of a precedent.

Implement containers/image#2068 for libimage/copier. Signed-off-by: Aditya R <arajan@redhat.com>

Implement containers/image#2068 for libimage/manifest.Push Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc · 2023-08-11T08:59:13Z

@vrothberg @giuseppe @mtrmac PTAL

giuseppe

LGTM

vrothberg

/lgtm

openshift-ci · 2023-08-11T09:26:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, giuseppe, vrothberg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giuseppe,vrothberg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mtrmac · 2023-08-11T20:17:17Z

libimage/push_test.go

+	// Push newly pulled alpine to directory with uncompressed blobs
+	pushOptions := &PushOptions{}
+	pushOptions.SystemContext = &types.SystemContext{}
+	pushOptions.SystemContext.DirForceDecompress = true
+	pushOptions.Writer = os.Stdout
+	dirDest := t.TempDir()
+	_, err = runtime.Push(ctx, "docker.io/library/alpine:latest", "dir:"+dirDest, pushOptions)
+	require.NoError(t, err)
+
+	// Pull uncompressed alpine from `dir:dirDest` as source.
+	pullOptions = &PullOptions{}
+	pullOptions.Writer = os.Stdout
+	pullOptions.Architecture = "arm64"
+	pulledImages, err = runtime.Pull(ctx, "dir:"+dirDest, config.PullPolicyAlways, pullOptions)
+	require.NoError(t, err)
+	require.Len(t, pulledImages, 1)


FWIW I didn’t test this now but I don’t think the dir: step is necessary if the test is structured primarily around c/storage.

c/storage itself is a source of always-uncompressed data.

(This works well enough as is, no need to go back and spend time changing it.)

vendor: bump c/image to v5.26.1-0.20230807184415-3fb422379cfa

16ed6ef

Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the implement-forcecompressionformat branch 2 times, most recently from d94715a to 170f138 Compare August 8, 2023 07:01

vrothberg reviewed Aug 8, 2023

View reviewed changes

mtrmac reviewed Aug 8, 2023

View reviewed changes

flouthoc force-pushed the implement-forcecompressionformat branch 2 times, most recently from 6cf031c to a045159 Compare August 9, 2023 08:13

flouthoc force-pushed the implement-forcecompressionformat branch from a045159 to 58ce959 Compare August 11, 2023 08:46

flouthoc added 2 commits August 11, 2023 14:22

libimage/copier: wire ForceCompressionFormat for image copy

acd9733

Implement containers/image#2068 for libimage/copier. Signed-off-by: Aditya R <arajan@redhat.com>

manifest: add support ForceCompressionFormat

248cc73

Implement containers/image#2068 for libimage/manifest.Push Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the implement-forcecompressionformat branch from 58ce959 to 248cc73 Compare August 11, 2023 08:58

flouthoc requested review from giuseppe, vrothberg and mtrmac August 11, 2023 08:59

giuseppe approved these changes Aug 11, 2023

View reviewed changes

openshift-ci bot added the approved label Aug 11, 2023

vrothberg approved these changes Aug 11, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Aug 11, 2023

openshift-ci bot added the lgtm label Aug 11, 2023

openshift-merge-robot merged commit 524b4d5 into containers:main Aug 11, 2023
7 checks passed

mtrmac reviewed Aug 11, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

libimage: add support for `ForceCompressionFormat` for image `copier` and `manifest` #1602

libimage: add support for `ForceCompressionFormat` for image `copier` and `manifest` #1602

flouthoc commented Aug 8, 2023

flouthoc commented Aug 8, 2023

vrothberg Aug 8, 2023

flouthoc Aug 9, 2023

vrothberg Aug 8, 2023

flouthoc Aug 8, 2023 •

edited

giuseppe Aug 8, 2023

flouthoc Aug 8, 2023

flouthoc Aug 9, 2023 •

edited

mtrmac Aug 10, 2023

flouthoc Aug 11, 2023

mtrmac left a comment

flouthoc commented Aug 8, 2023 •

edited

mtrmac commented Aug 10, 2023

mtrmac commented Aug 10, 2023

flouthoc commented Aug 11, 2023

giuseppe left a comment

vrothberg left a comment

openshift-ci bot commented Aug 11, 2023

mtrmac Aug 11, 2023

libimage: add support for ForceCompressionFormat for image copier and manifest #1602

libimage: add support for ForceCompressionFormat for image copier and manifest #1602

Conversation

flouthoc commented Aug 8, 2023

flouthoc commented Aug 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc Aug 8, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc Aug 9, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac left a comment

Choose a reason for hiding this comment

flouthoc commented Aug 8, 2023 • edited

mtrmac commented Aug 10, 2023

mtrmac commented Aug 10, 2023

flouthoc commented Aug 11, 2023

giuseppe left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 11, 2023

Choose a reason for hiding this comment

libimage: add support for `ForceCompressionFormat` for image `copier` and `manifest` #1602

libimage: add support for `ForceCompressionFormat` for image `copier` and `manifest` #1602

flouthoc Aug 8, 2023 •

edited

flouthoc Aug 9, 2023 •

edited

flouthoc commented Aug 8, 2023 •

edited