push/manifest-push: add support for `--force-compression` to prevent reusing other blobs #19640

flouthoc · 2023-08-16T08:01:58Z

Adds support for --force-compression which allows end-users to force
push blobs with the selected compression in --compression option, in
order to make sure that blobs of other compression on registry are not
reused.

Is equivalent to: force-compression here: https://docs.docker.com/build/exporters/#compression
Closes: #18660
Similar to Buildah's: containers/buildah#4973

push/manifest-push: add support for `--force-compression` to prevent reusing other blobs

flouthoc · 2023-08-16T08:15:40Z

@containers/podman-maintainers PTAL

vrothberg · 2023-08-16T08:53:21Z

Validate fails because of my pkg/config changes. Feel free to rebase on top of #19567 for now.

cmd/podman/images/push.go

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

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

@@ -42,6 +42,10 @@ the list or index itself. (Default true)

 @@option digestfile

+#### **--force-compression**
+
+Use the specified compression algorithm even if the destination contains a differently-compressed variant already.


I think we need to make this less abstract. Why do I need that instead of --compression? What's the use case?

Same applies to other locations where the functionality is described.

Sure, lets decide a text then I will amend it. How about if i add a text like this ?

Use the --force-compression option to force re-compressing layers imported from a previous image, if the requested compression algorithm is different from the previous compression algorithm

Text from: https://docs.docker.com/build/exporters/#compression

But shouldn't --compression just imply the force part? If I say --compression zstd it should do that and not reuse gzip blobs IMO. I cannot think of anyone actually wanting the current behaviour, to me this is a bug and does not need yet another compression option!

@Luap99 This is one of the behavior which is already there in docker and afaik c/image follows it. --add-compression does it internally but I think changing behavior of --compression is breaking. I think following behavior can be changed but I am not aware of all the use-case which could break because of changing behaviour of --compression , @mtrmac can provide better explanation about this question.

@Luap99 @vrothberg WDYT about: #19640

I don't understand the question. Can you clarify where you're pointing at?

@vrothberg I am trying to find a good text for man-page in order to resolve your request here: #19640

So before I amend the doc, my intention was to finalize the text here:

Use the --force-compression option to force re-compressing layers imported from a previous image which was pushed using a default compression format or via --compression, if the requested compression algorithm is different from the previous compression algorithm

@flouthoc can you do a PS in today's meeting on the issue of --compression-format maybe including --force-compression?

@rhatdan @giuseppe PTAL
I think we can change that later on as well and don't need to block the PR on it.

So before I amend the doc, my intention was to finalize the text here:

@flouthoc, I think we need to use very simple words. In the end it breaks down to the fact that --compression may not compress if a matching layer exists on the registry. --force... will force compression.`

we can argue about backward compatibility, but ignoring the value passed to --compression-format seems like a bug to me and not intuitive. If a user expects the layer to be compressed and ignores the compression format, it is already possible to use --compress. I don't see the value in having three different knobs just to customize the compression format to use.

@giuseppe Yes two issues I see are backward compatibility and docker compat, other than these two there are no strong reasons which prevents us to automatically include --force-compression on every --compression-format.

test/e2e/push_test.go

cmd/podman/manifest/push.go

flouthoc · 2023-08-16T11:40:56Z

Validate fails because of my pkg/config changes. Feel free to rebase on top of #19567 for now.

I will rebase after this no-rush.

Luap99 · 2023-08-21T16:06:49Z

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

@@ -42,6 +42,12 @@ the list or index itself. (Default true)

 @@option digestfile

+#### **--force-compression**


Note please do not duplicate the same text in two man pages, put it in markdown/options and use the @@option import syntax like the other flags for push and manifest push.

rhatdan · 2023-08-22T13:01:31Z

I still think we are over thinking and making the UI way to complex. My head explodes when I think of how a user will build one of these and the confusion between --compression and --force-compression.

I think we should dedicate a meeting to discuss what is the UI for a user creating these optimized images, and discuss why we are making this so complex.

flouthoc · 2023-08-22T14:46:56Z

I still think we are over thinking and making the UI way to complex. My head explodes when I think of how a user will build one of these and the confusion between --compression and --force-compression.

I think we should dedicate a meeting to discuss what is the UI for a user creating these optimized images, and discuss why we are making this so complex.

@rhatdan We already discussed in above threads and decided to merge --compression-format and --force-compression so there is no new --force-compression anymore I will be hidden underneath existing --compression-format. I'll amend the PR

Adds support for --force-compression which allows end-users to force push blobs with the selected compresison in --compression option, in order to make sure that blobs of other compression on registry are not reused. Is equivalent to: force-compression here: https://docs.docker.com/build/exporters/#compression Closes: containers#18660 Signed-off-by: Aditya R <arajan@redhat.com>

Adds support for --force-compression which allows end-users to force push blobs with the selected compresison in --compression option, in order to make sure that blobs of other compression on registry are not reused. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc · 2023-08-24T08:07:51Z

@containers/podman-maintainers @mtrmac PTAL now --force-compression is already gonna be true if --compression-format is set unless user manually sets --force-compression=false with --compression-format. And it is false in any other case.

Practically now this flag is never gonna be used manually by users unless they want to fallback to old behavior for compat reason.

vrothberg · 2023-08-24T08:27:37Z

pkg/api/server/register_images.go

@@ -726,6 +726,11 @@ func (s *APIServer) registerImagesHandlers(r *mux.Router) error {
 	//    type: string
 	//    description: Allows for pushing the image to a different destination than the image refers to.
 	//  - in: query
+	//    name: forceCompressionFormat
+	//    description: Use the specified compression algorithm if the destination contains a differently-compressed variant already.


I still have issues reading this sentence. I can be interpreted that we'll only compress "if the destination contains a differently-compressed variant already". So we won't compress if there's no variant at all on the registry?

I suggest "Enforce compressing the layers with the specified --compression and do not reuse differently compressed blobs on the registry." I find that easier to read/understand.

vrothberg · 2023-08-24T08:34:28Z

docs/source/markdown/options/force-compression.md

+If set uses the specified compression algorithm even if the destination contains a differently-compressed variant already, use this in order
+to make sure that podman does not reuses the blobs of differently-compressed variant present on the remote registry.
+
+Note: This is by default set to `true` if `push` is being performed with `--compression-format` in order to make sure that
+podman does not reuses the blobs of differently-compressed variant present on remote registry, manually set this to `false`
+in order to opt-out of this behaviour.


Suggested change

If set uses the specified compression algorithm even if the destination contains a differently-compressed variant already, use this in order

to make sure that podman does not reuses the blobs of differently-compressed variant present on the remote registry.

Note: This is by default set to `true` if `push` is being performed with `--compression-format` in order to make sure that

podman does not reuses the blobs of differently-compressed variant present on remote registry, manually set this to `false`

in order to opt-out of this behaviour.

If set uses the specified compression algorithm even if the destination contains a differently-compressed variant already.

Use this in order to make sure that podman does not reuse the blobs of a differently-compressed variant present on the remote registry.

Note: This is by default set to `true` if `push` is being performed with `--compression-format`. Manually set it to `false` in order to opt-out of this behavior.

mtrmac

(Don’t let me review UIs and comments…)

cmd/podman/images/push.go

docs/source/markdown/options/force-compression.md

mtrmac · 2023-08-24T14:06:50Z

pkg/api/handlers/libpod/images_push.go

+		Username:               username,
+	}
+
+	if _, found := r.URL.Query()["compressionFormat"]; found {


Why does the API layer need to make this conditional as well? Doing that at just one layer seems clearer to me, in particular it rules out questions whether the common/config.EngineConfig.CompressionFormat value applies (but it seems that it doesn’t, so we are actually fine.)

Also applies to the other copy.

Podman REST API can be used without CLI layer directly as well, hence its needed.

flouthoc · 2023-08-26T10:19:35Z

Addressed all the comments, expect the last one.

flouthoc · 2023-08-28T09:34:44Z

@containers/podman-maintainers PTAL

vrothberg · 2023-08-28T10:00:24Z

cmd/podman/images/push.go

@@ -102,6 +102,8 @@ func pushFlags(cmd *cobra.Command) {
 	flags.StringVar(&pushOptions.DigestFile, digestfileFlagName, "", "Write the digest of the pushed image to the specified file")
 	_ = cmd.RegisterFlagCompletionFunc(digestfileFlagName, completion.AutocompleteDefault)

+	flags.BoolVar(&pushOptions.ForceCompressionFormat, "force-compression", false, "Use the specified compression algorithm if the destination contains a differently-compressed variant already")


Suggested change

flags.BoolVar(&pushOptions.ForceCompressionFormat, "force-compression", false, "Use the specified compression algorithm if the destination contains a differently-compressed variant already")

flags.BoolVar(&pushOptions.ForceCompressionFormat, "force-compression", false, "Use the specified compression algorithm even if the destination contains a differently-compressed variant already")

The if would otherwise turn it into a pre-condition.

Same for all other occurrences of this sentence

…ion-format Value of `--force-compression` should be already `true` is `--compression-format` is selected otherwise let users decide. Signed-off-by: Aditya R <arajan@redhat.com>

mtrmac

LGTM; I’ll let Podman maintainers decide on whether the API should make that kind of decision, or whether it should be CLI-only.

vrothberg

/lgtm

openshift-ci · 2023-08-28T14:45:14Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

With discussion from here: containers/podman#19640, it was decided that `--force-compression` must be automatically `true` in case when `--compression-format` is set. Signed-off-by: Aditya R <arajan@redhat.com>

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

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

flouthoc mentioned this pull request Aug 16, 2023

compression can't be done when specify option --compression-format in push command. #18660

Closed

vrothberg reviewed Aug 16, 2023

View reviewed changes

flouthoc force-pushed the force-compression branch from 6da759a to 07a3ac0 Compare August 16, 2023 10:31

flouthoc requested a review from vrothberg August 16, 2023 10:36

flouthoc force-pushed the force-compression branch from 07a3ac0 to bf87748 Compare August 16, 2023 10:52

openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 16, 2023

flouthoc force-pushed the force-compression branch 2 times, most recently from 7c462e4 to b674370 Compare August 17, 2023 07:16

flouthoc requested a review from Luap99 August 17, 2023 07:17

flouthoc force-pushed the force-compression branch 2 times, most recently from bd4dc1b to b8f4e5b Compare August 21, 2023 06:42

Luap99 reviewed Aug 21, 2023

View reviewed changes

flouthoc force-pushed the force-compression branch from b8f4e5b to 231dec4 Compare August 24, 2023 08:05

flouthoc added 2 commits August 24, 2023 13:35

flouthoc force-pushed the force-compression branch from 231dec4 to 387e99e Compare August 24, 2023 08:06

vrothberg reviewed Aug 24, 2023

View reviewed changes

flouthoc force-pushed the force-compression branch from 387e99e to c96c923 Compare August 24, 2023 08:39

flouthoc requested a review from vrothberg August 24, 2023 08:39

mtrmac reviewed Aug 24, 2023

View reviewed changes

flouthoc force-pushed the force-compression branch from c96c923 to a42431a Compare August 26, 2023 10:19

flouthoc requested a review from mtrmac August 26, 2023 10:19

vrothberg reviewed Aug 28, 2023

View reviewed changes

push, manifest-push: --force-compression must be true with --compress…

0938ee1

…ion-format Value of `--force-compression` should be already `true` is `--compression-format` is selected otherwise let users decide. Signed-off-by: Aditya R <arajan@redhat.com>

flouthoc force-pushed the force-compression branch from a42431a to 0938ee1 Compare August 28, 2023 11:11

mtrmac reviewed Aug 28, 2023

View reviewed changes

vrothberg approved these changes Aug 28, 2023

View reviewed changes

openshift-ci bot assigned vrothberg Aug 28, 2023

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

openshift-merge-robot merged commit dd2ec7c into containers:main Aug 28, 2023

flouthoc mentioned this pull request Aug 30, 2023

push: --force-compression should be true with --compression-format containers/buildah#5013

Merged

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 27, 2023

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

		@@ -42,6 +42,12 @@ the list or index itself. (Default true)

		@@option digestfile

		#### --force-compression

	flags.BoolVar(&pushOptions.ForceCompressionFormat, "force-compression", false, "Use the specified compression algorithm if the destination contains a differently-compressed variant already")
	flags.BoolVar(&pushOptions.ForceCompressionFormat, "force-compression", false, "Use the specified compression algorithm even if the destination contains a differently-compressed variant already")

push/manifest-push: add support for --force-compression to prevent reusing other blobs #19640

push/manifest-push: add support for --force-compression to prevent reusing other blobs #19640

Uh oh!

Conversation

flouthoc commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flouthoc commented Aug 16, 2023

Uh oh!

vrothberg commented Aug 16, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flouthoc Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

giuseppe Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

flouthoc commented Aug 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Luap99 Aug 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rhatdan commented Aug 22, 2023

Uh oh!

flouthoc commented Aug 22, 2023

Uh oh!

flouthoc commented Aug 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flouthoc commented Aug 26, 2023

Uh oh!

flouthoc commented Aug 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtrmac left a comment

Choose a reason for hiding this comment

Uh oh!

vrothberg left a comment

push/manifest-push: add support for `--force-compression` to prevent reusing other blobs #19640

push/manifest-push: add support for `--force-compression` to prevent reusing other blobs #19640

flouthoc commented Aug 16, 2023 •

edited

Loading

flouthoc Aug 16, 2023 •

edited

Loading

giuseppe Aug 21, 2023 •

edited

Loading

flouthoc commented Aug 16, 2023 •

edited

Loading

Luap99 Aug 21, 2023 •

edited

Loading

flouthoc commented Aug 24, 2023 •

edited

Loading