Skip to content

DO NOT MERGE: Run tests with Zstd compression default#21571

Closed
mtrmac wants to merge 5 commits intocontainers:mainfrom
mtrmac:try-zstd-default
Closed

DO NOT MERGE: Run tests with Zstd compression default#21571
mtrmac wants to merge 5 commits intocontainers:mainfrom
mtrmac:try-zstd-default

Conversation

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Feb 8, 2024

Inspired by #20633

Does this PR introduce a user-facing change?

do not merge this

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Feb 8, 2024
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 8, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mtrmac

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2024
@mtrmac mtrmac mentioned this pull request Aug 27, 2025
Open
37 tasks
@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 8, 2024

Legitimate problems:

  • podman … push --format=v2s2 … fails with Error: compression using zstd required together with format application/vnd.docker.distribution.manifest.v2+json, which does not support it. Arguably an explicit option on the CLI should override config-file compression defaults, the intent here is clear.
  • podman … push … docker-archive:… fails with compression using zstd required but the destination only supports MIME types [application/vnd.docker.distribution.manifest.v2+json], none of which support it. Same as above, but there isn’t even an explicit CLI option.

@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 8, 2024

@giuseppe FYI. I’ll continue investigating this, just noting the Podman-side UI concerns above for now.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2024
@packit-as-a-service
Copy link

packit-as-a-service bot commented Feb 12, 2024

Ephemeral COPR build failed. @containers/packit-build please check.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2024
@packit-as-a-service
Copy link

packit-as-a-service bot commented Feb 16, 2024

Cockpit tests failed for commit 9ce62ea83c221979aff1bd91afe0c002cc73f270. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service
Copy link

packit-as-a-service bot commented Feb 19, 2024

Cockpit tests failed for commit 1a9566f043cf084167e5ef3e8e90a1690dc7bead. @martinpitt, @jelly, @mvollmer please check.

@giuseppe
Copy link
Member

giuseppe commented Feb 21, 2024

CI is green!

rhatdan
rhatdan reviewed Feb 21, 2024
View reviewed changes
test/e2e/push_test.go Outdated
Copy link
Member

@rhatdan rhatdan Feb 21, 2024

Choose a reason for hiding this comment

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

Does this require a change in podman or contianers/image?

Copy link
Contributor Author

@mtrmac mtrmac Feb 22, 2024

Choose a reason for hiding this comment

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

This is specific to containers.conf. So not c/image; Podman or maybe c/common/libimage.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 22, 2024
@mtrmac mtrmac mentioned this pull request Feb 22, 2024
Merged
@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 22, 2024

🎉 green!

#21793 now contains the test updates we do want to make; the other changes from this PR should not be merged, and instead we should change Podman’s behavior.

@mtrmac mtrmac force-pushed the try-zstd-default branch 3 times, most recently from 2529e6e to 9312529 Compare November 28, 2024 20:56
@mtrmac mtrmac force-pushed the try-zstd-default branch 3 times, most recently from 1810e21 to 386ff32 Compare December 14, 2024 00:19
@mtrmac mtrmac force-pushed the try-zstd-default branch 3 times, most recently from 7f4402b to 7b77eb4 Compare January 7, 2025 18:39
@mtrmac mtrmac mentioned this pull request Jan 9, 2025
Closed
@mtrmac mtrmac force-pushed the try-zstd-default branch 2 times, most recently from 9b8d906 to ded524f Compare January 14, 2025 21:13
@packit-as-a-service
Copy link

packit-as-a-service bot commented Jan 14, 2025

Ephemeral COPR build failed. @containers/packit-build please check.

@mtrmac mtrmac force-pushed the try-zstd-default branch 4 times, most recently from b6a45d8 to fb48d62 Compare January 21, 2025 20:53
mtrmac added 5 commits January 23, 2025 22:53
@mtrmac
DO NOT MERGE: Nerf make-and-check-size
e4157d9 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
HACK: Don't test on Fedora 40
18d14a2 
The Go image is too old, at least in the CI images

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Test with current c/storage and c/image
31a5d09 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
HACK: switch the compression to zstd
791a510 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Update the expected default compression algorithm
0d61b64 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 6, 2025 •
edited
Loading

We now (e.g. with #25007 ) always run at least some tests with Zstd, and there are no immediate plans to switch to it as default.

@mtrmac mtrmac closed this Feb 6, 2025
@mtrmac mtrmac deleted the try-zstd-default branch February 6, 2025 21:55
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 8, 2025
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@rhatdan rhatdan rhatdan left review comments

+1 more reviewer

@edsantiago edsantiago edsantiago left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@mtrmac @giuseppe @rhatdan @edsantiago @openshift-merge-robot