manifest, push: implement `--add-compression` to push with compressed variants. #19468

flouthoc · 2023-08-01T10:05:57Z

This is a feature PR for containers/image feature merged in below PR:

Adds a new flag --add-compression to manifest push, which when added will push existing manifest on host while replicating the instances with required compression.

manifest, push: implement --add-compression to push with compressed variants.

Similar implementation to: containers/buildah#4912

flouthoc · 2023-08-01T10:06:19Z

Needs: containers/common#1590 (review)
/hold

flouthoc · 2023-08-01T16:37:17Z

Suspecting issues due to some change in c/common testing it here: #19472

flouthoc · 2023-08-01T18:17:04Z

@containers/podman-maintainers PTAL PR is similar to containers/buildah#4912

flouthoc · 2023-08-01T18:17:43Z

@mtrmac PTAL

mtrmac

Podman maintainers, please take a look as well — I think I have a general understanding but there are certainly aspects of the engine / remote API design that I’m missing.

mtrmac · 2023-08-01T18:26:23Z

docs/source/markdown/podman-manifest-push.1.md.in

+Makes sure that requested compression variant for each platform is added to the manifest list keeping original instance
+intact in the same manifest list. Supported values are (`gzip`, `zstd` and `zstd:chunked`)
+
+Note: This is different than `--compression` which replaces the instance with requested with specified compression


That option is named --compression-format in Podman.

Can we just extend --compression to be a comma separated list.

--compression gzip,zstd

I think that makes for a better User UI.

Right now the two have quite different semantics, as the text above says.

Not sure why you say that.
If I say --compression gzip, then it is going to push the image if it has gzip compression or change the compression to gzip if it was something else.
If I push --compression gzip,zstd then I would figure it is going to push gzip and zstd, even if the original was something different.

@rhatdan We have already used --add-compression since it was overlapping with --compression on buildah side, it will be good here for consistency.

Extending --compression changes functionality for example --compression gzip will replace instances , but --compression gzip,zstd will only replace gzip and add zstd while changing the order of input will change functionality entirely consider --compression zstd,gzip it will replace zstd and add gzip.

We had a prior discussion here: containers/common#1585

The existing --compress prefers to reuse blobs, even if they don’t use the expected algorithm. That’s a long-standing behavior and I wouldn’t be surprised if users by now relied on it.

(It’s not necessarily the option semantics we would design now as the default, if we were building the CLI from scratch with the feature set of the underlying codebase as of today, but I think the years of past practice change the calculation. @flouthoc is already working on making it possible to force using the requested algorithm independently from the cloning functionality, but that’s not this PR.)

pkg/api/handlers/libpod/manifests.go

pkg/domain/entities/images.go

pkg/domain/infra/abi/manifest.go

cmd/podman/manifest/inspect.go

mtrmac

Thanks, this looks close to done to me.

At this point I’ll need to defer to actual Podman experts — please review independently.

pkg/bindings/manifests/manifests.go

pkg/domain/entities/images.go

Signed-off-by: Aditya R <[email protected]>

flouthoc · 2023-08-02T08:33:52Z

Rebased. @containers/podman-maintainers PTAL

cmd/podman/manifest/push.go

pkg/api/handlers/libpod/manifests.go

Luap99 · 2023-08-02T10:14:29Z

test/system/012-manifest.bats

+    if ! type -p skopeo; then
+        skip "skopeo not available"
+    fi


to my knowledge skopeo is a hard requirement so this skip makes no sense
cc @edsantiago

The tests which are using skopeo have this check directly or indirectly so I have added it here as well.

Luap99 · 2023-08-02T10:17:12Z

test/system/012-manifest.bats

+    if ! type -p skopeo; then
+        skip "skopeo not available"
+    fi
+    skip_if_remote "running a local registry doesn't work with podman-remote"


why? Not having remote tests is a big issue. We need tests to cover remote client and API as well.
If we cannot set this up in system tests we should use e2e tests or the bindings and API tests.

Added a e2e test.

flouthoc · 2023-08-02T18:08:00Z

@containers/podman-maintainers PTAL

mtrmac

Absolutely non-blocking tentative suggestions — don’t let me slow down this work unnecessarily.

mtrmac · 2023-08-02T20:15:49Z

test/e2e/manifest_test.go

+		// 4 images must be pushed two for gzip and two for zstd
+		Expect(output).To(ContainSubstring("Copying 4 images generated from 2 images in list"))
+
+		session = podmanTest.Podman([]string{"run", "--rm", "--net", "host", "quay.io/skopeo/stable", "inspect", "--tls-verify=false", "--raw", "docker://localhost:5000/list:latest"})


Absolutely non-blocking: This could plausibly call c/image instead of using a subprocess.

OTOH this is easier to follow for people unfamiliar with that codebase.

This is similar to systems test for easier review and familiarity.

I think it would make more sense to directly execute skopeo on the host instead of pulling the image as pull are flaky and slow. We already depend on it in system test so I see no problem using it in e2e as well.

mtrmac · 2023-08-02T20:15:57Z

test/e2e/manifest_test.go

+		session = podmanTest.Podman([]string{"build", "--platform", "linux/amd64", "-t", "imageone", "build/basicalpine"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(Exit(0))
+
+		session = podmanTest.Podman([]string{"build", "--platform", "linux/arm64", "-t", "imagetwo", "build/basicalpine"})
+		session.WaitWithDefaultTimeout()
+		Expect(session).Should(Exit(0))


Do we have multi-arch image that could be used for this test without requiring a build, by any chance?

(I have no idea … and if this works, maybe we can spend CPU cycles instead of our attention.)

quay.io/libpod/20221018? Or even :00000004 if you want a tiny image.

Sure I'll consider it if I am going to make any further change and repush this. ( I hope this not a hard blocker since OTOH building image makes tests independent of any external image ( pulling ) which has been encouraged in past if i can recall )

IIRC the Podman e2e tests already have some infrastructure to pre-pull / cache images, so that not every pull needs to reach out to the internet. But I don’t know the details by heart.

Luap99

LGTM, tests can be improved later if needed.

vrothberg

One last nit on the docs

vrothberg · 2023-08-03T08:42:00Z

docs/source/markdown/podman-manifest-push.1.md.in

+intact in the same manifest list. Supported values are (`gzip`, `zstd` and `zstd:chunked`)
+
+Note: This is different than `--compression-format` which replaces the instance with requested with specified compression
+while `--add-compression` makes sure than each instance has it variant added to manifest list without modifying the


For some reason I cannot propose a change via the GitHub UI but the sentence above has typos:

makes sure that each instance has its variant added to manifest list without modifying the
original instance.

I still find the note hard to understand and suggest to be more explicit and less abstract:

"Note that --compression-format controls the compression format of each instance in the manifest list. --add-compression will add another variant for each instance in the list with the specified compressions. --compression-format gzip --add-compression zstd will push a manifest list with each instance being compressed with gzip plus an additional variant of each instance being compressed with zstd."

I am sure there's a better description than mine but I find it important to be more explicit.

Maybe worth mentioning that it can be specified multiple times.

Maybe worth mentioning that it can be specified multiple times.

Amended text and added above line as well.

vrothberg

/lgtm
/hold

openshift-ci · 2023-08-03T08:50:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, Luap99, 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~~ [Luap99,flouthoc,vrothberg]

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

Adds support for --add-compression which accepts multiple compression formats and when used it will add all instances in a manifest list with requested compression formats. Signed-off-by: Aditya R <[email protected]>

vrothberg · 2023-08-03T08:52:55Z

/lgtm
/hold

flouthoc · 2023-08-03T11:11:45Z

/hold cancel

flouthoc · 2023-08-03T11:34:39Z

cc @TomSweeneyRedHat

openshift-ci bot added release-note approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 1, 2023

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Aug 1, 2023

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2023

flouthoc force-pushed the manifest-add-compression branch from d738d08 to ffbf32e Compare August 1, 2023 15:18

flouthoc force-pushed the manifest-add-compression branch from ffbf32e to 7c64566 Compare August 1, 2023 16:52

mtrmac reviewed Aug 1, 2023

View reviewed changes

flouthoc force-pushed the manifest-add-compression branch 3 times, most recently from b9dc2aa to 9f64ac9 Compare August 2, 2023 02:10

mtrmac reviewed Aug 2, 2023

View reviewed changes

pkg/bindings/manifests/manifests.go Outdated Show resolved Hide resolved

pkg/domain/entities/images.go Outdated Show resolved Hide resolved

flouthoc force-pushed the manifest-add-compression branch from 9f64ac9 to 1363a56 Compare August 2, 2023 04:12

vendor: bump c/image to v5.26.1-0.20230801083106-fcf7f0e1712a

49b8b97

Signed-off-by: Aditya R <[email protected]>

flouthoc force-pushed the manifest-add-compression branch from 1363a56 to 3e2d9ca Compare August 2, 2023 08:33

Luap99 reviewed Aug 2, 2023

View reviewed changes

flouthoc force-pushed the manifest-add-compression branch 4 times, most recently from 7828753 to 22b1661 Compare August 2, 2023 15:16

flouthoc requested a review from Luap99 August 2, 2023 15:38

mtrmac reviewed Aug 2, 2023

View reviewed changes

Luap99 approved these changes Aug 3, 2023

View reviewed changes

flouthoc requested a review from vrothberg August 3, 2023 08:30

vrothberg reviewed Aug 3, 2023

View reviewed changes

flouthoc force-pushed the manifest-add-compression branch 2 times, most recently from 07b7e22 to 9509e1a Compare August 3, 2023 08:49

vrothberg approved these changes Aug 3, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Aug 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2023

manifest/push: add support for --add-compression

346f9cb

Adds support for --add-compression which accepts multiple compression formats and when used it will add all instances in a manifest list with requested compression formats. Signed-off-by: Aditya R <[email protected]>

flouthoc force-pushed the manifest-add-compression branch from 9509e1a to 346f9cb Compare August 3, 2023 08:50

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 3, 2023

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 3, 2023

openshift-merge-robot merged commit bde942e into containers:main Aug 3, 2023

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Nov 2, 2023

github-actions bot locked as resolved and limited conversation to collaborators Nov 2, 2023

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

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

Conversation

flouthoc commented Aug 1, 2023 • edited Loading

flouthoc commented Aug 1, 2023

flouthoc commented Aug 1, 2023 • edited Loading

flouthoc commented Aug 1, 2023

flouthoc commented Aug 1, 2023

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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 2, 2023 • edited Loading

Choose a reason for hiding this comment

flouthoc Aug 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flouthoc commented Aug 2, 2023

mtrmac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Luap99 left a comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrothberg left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Aug 3, 2023

vrothberg commented Aug 3, 2023

flouthoc commented Aug 3, 2023

flouthoc commented Aug 3, 2023

manifest, push: implement `--add-compression` to push with compressed variants. #19468

manifest, push: implement `--add-compression` to push with compressed variants. #19468

flouthoc commented Aug 1, 2023 •

edited

Loading

flouthoc commented Aug 1, 2023 •

edited

Loading

flouthoc commented Aug 2, 2023 •

edited

Loading

flouthoc Aug 2, 2023 •

edited

Loading