Skip to content

DO NOT MERGE: Smoke-test of a costly “always compute old-style IDs” approach#24419

Closed
mtrmac wants to merge 11 commits intocontainers:mainfrom
mtrmac:mitr-zstd-costly-old-ids-integration
Closed

DO NOT MERGE: Smoke-test of a costly “always compute old-style IDs” approach#24419
mtrmac wants to merge 11 commits intocontainers:mainfrom
mtrmac:mitr-zstd-costly-old-ids-integration

Conversation

@mtrmac
Copy link
Contributor

@mtrmac mtrmac commented Oct 30, 2024

This is a variant of #24287 which reverts (some of) the system test changes to again expect old-style IDs, and includes containers/storage#2155 / containers/image#2613 .

⚠️ WARNING: all of this is a quick sketch, absolutely untested.

This exists purely to give us options. With this, zstd:chunked loses some of the performance wins, and it should revert to old-style IDs. We then have the option to reintroduce the ID change and performance improvement (perhaps even based on measurements) later.

And, separately, this enforces that layers should match against the config’s DiffIDs, resolving the signing ambiguity.

FYI @giuseppe @edsantiago @mheon @baude

Does this PR introduce a user-facing change?

None

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

openshift-ci bot commented Oct 30, 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-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 30, 2024
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 30, 2024
@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch from c179f32 to 5345c94 Compare October 30, 2024 15:19
@packit-as-a-service
Copy link

packit-as-a-service bot commented Oct 30, 2024

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

1 similar comment
@packit-as-a-service
Copy link

packit-as-a-service bot commented Oct 30, 2024

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

@mtrmac mtrmac changed the title DO NOT MERGE: Smoke-test of costly “always compute old-style IDs” approach DO NOT MERGE: Smoke-test of a costly “always compute old-style IDs” approach Oct 30, 2024
@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch 6 times, most recently from 3d26edf to 85bf387 Compare October 31, 2024 18:29
@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 4, 2024 •
edited
Loading

Note to self: If/after containers/storage#2156 merges, this PR will need to re-enable enable_partial_images.

Edit: done.

@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch from 85bf387 to 776b38d Compare November 18, 2024 16:48
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 18, 2024
@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch from 776b38d to 93405a1 Compare November 18, 2024 16:50
@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 18, 2024 •
edited
Loading

The test images with all images converted to zstd:chunked are too old. For now I’m leaving that variant in https://github.com/mtrmac/libpod/tree/mitr-zstd-costly-old-ids-integration-chunked-test-images , and reverting to test with the current images here.

⚠️ That significantly decreases the scope of testing; many of the unexpected behavior changes were happening only with most images converted to zstd:chunked. With this, we are mostly back to the test coverage of #21903 , which is known to not demonstrate many of the surprises.

@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch from 7a0271e to b14da5e Compare November 18, 2024 17:00
@edsantiago
Copy link
Member

edsantiago commented Nov 18, 2024

The test images with all images converted to zstd:chunked are too old

Would you care to follow (and test and review) my new simplified automation_images doc? You will need to import https://github.com/containers/automation_images/pull/388/files but apart from that it really should be fairly simple.

@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch from b14da5e to 4148a66 Compare November 18, 2024 17:35
@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 18, 2024

Thanks, I’ll definitely try that.

@mtrmac
Copy link
Contributor Author

mtrmac commented Nov 22, 2024

The test images with all images converted to zstd:chunked are too old.

Note to self: containers/automation_images#395 .

@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch 5 times, most recently from ac0bb3f to 9b2c391 Compare November 29, 2024 00:31
@mtrmac mtrmac mentioned this pull request Jan 6, 2025
Merged
@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch 3 times, most recently from b128c72 to 782aa77 Compare January 10, 2025 20:34
@mtrmac mtrmac mentioned this pull request Jan 13, 2025
Merged
@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch 4 times, most recently from 6828853 to 7cf1875 Compare January 16, 2025 20:16
@mtrmac
Copy link
Contributor Author

mtrmac commented Jan 16, 2025

This is now on top of #25007, and differs from it by:

  • using a CI image where all images are zstd:chunked
  • enabling partial pulls for the whole Podman test suite
  • updating the tests to work with the unexpectedly-Zstd images

@mtrmac mtrmac mentioned this pull request Jan 17, 2025
Closed
@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch 2 times, most recently from 8f28e37 to 2c318f6 Compare January 23, 2025 22:01
mtrmac added 11 commits February 5, 2025 22:52
@mtrmac
Don't use all of tempdir as podman's --tmpdir
27ccfef 
... because (podman system reset) will delete all of it,
interfering with the test storing other data in the directory.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Add tests for chunked pulls
4429093 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
Merge branch 'zstd-chunked-with-tests' into HEAD
44df41b
@mtrmac
DO NOT MERGE: Nerf make-and-check-size
861ec6e 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
DO NOT MERGE: Test with a zstd:chunked testimage
878ea11 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
DO NOT MERGE: Test with a zstd:chunked container
8599ded 
... from containers/automation_images#395 .

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
??? Work around zstd:chunked images in health checks
e8dc6b7 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
WIP: Conditional digests (BUT STANDARD IDs)
ba498f4 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
WIP: Conditional image IDs
d75e219 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
HACK: Re-enable partial pulls by default
1538b0d 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac
HACK: nerf tree_status.sh
928e459 
Signed-off-by: Miloslav Trmač <mitr@redhat.com>
@mtrmac mtrmac force-pushed the mitr-zstd-costly-old-ids-integration branch 2 times, most recently from 9e28a0b to 928e459 Compare February 5, 2025 23:11
@mtrmac
Copy link
Contributor Author

mtrmac commented Feb 6, 2025

We now (e.g. with #25007 ) always run at least some tests with Zstd; converting every image is a possibility but losing gzip coverage would not be obviously better.

@mtrmac mtrmac closed this Feb 6, 2025
@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

No reviews

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mtrmac @edsantiago @openshift-merge-robot