Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Try zstd:chunked in local cache registry #388

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Try zstd:chunked in local cache registry #388

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion IMG_SFX
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20241016t144444z-f40f39d13
20241022t120000z-f40f39d13
6 changes: 5 additions & 1 deletion cache_images/local-cache-registry
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ declare -a IMAGELIST=(
testimage:00000000
testimage:00000004
testimage:20221018
testimage:20241010
testimage:20241011
Copy link
Member

Choose a reason for hiding this comment

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

Do we need some form of process to delete the old ones? Otherwise we just keep adding new ones. I guess for testimage not a big concern as it is small enough for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have a find-unused-images script but I don't quite trust it yet. Am planning to fine-tune it then submit it as part of the repo one day. It's just a oneliner:

for i in $(sed -ne '/IMAGELIST=/,/^[^ ]/p' <cache_images/local-cache-registry | sed -ne 's/^  *//p');do grep -q -R $i ../libpod/test ../buildah/tests || echo "unused $i";done

...and its current output is:

unused fedora/python-311:latest
unused podman_python:latest
unused testimage:20240123       <<< yay nice catch
unused testimage:multiimage     <<< wrong and hard to test for and maybe I can find a way to not need it
unused podman/stable:v4.3.1     <<< obviously wrong, but hard to check for
unused podman/stable:v4.8.0     <<< ibid

The python ones I think are used in libpod compose tests, but have never taken the time to investigate why

testimage:multiimage
testimage@sha256:1385ce282f3a959d0d6baf45636efe686c1e14c3e7240eb31907436f7bc531fa
Expand Down Expand Up @@ -218,7 +219,10 @@ function cache_image() {

for retry in 1 2 3 0;do
skopeo --registries-conf /dev/null \
copy --all --dest-tls-verify=false \
copy --all \
--dest-tls-verify=false \
--dest-compress-format zstd:chunked \
Copy link
Collaborator

Choose a reason for hiding this comment

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

This changes image digests, which are hard-coded in tests (e.g. ALPINELISTDIGEST).

That’s fine in principle — and I’ll update the test digests in the integration PR — I’m just noting that it’s expected that some tests will start failing with this.

Outside of the current zstd:chunked testing effort, consider using --preserve-digests in the command to make any accidental digest changes (e.g. if an unusual image weren’t supported by the cache registry and triggered a format conversion) an error, to make sure this creates a bit-for-bit-identical copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for this. The new images built just now use --preserve-digests. Would you care to try with c20241022t120000z-f40f39d13 ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I’m sorry, with --preserve-digest + --dest-compress-format, --dest-compress-format is, AFAICS, ignored (because that requires a digest change). I’ll inspect the images to confirm.

Arguably it’s a bug that this option combination is not rejected.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ack, thanks, I misunderstood.

So, back to your initial comment: "This changes image digests". Are these changes reliably consistent? When we make new VM images next week/year, different version of skopeo, different kernel/filesystem/tools, different phase of the moon, will the image digests be the same?

If so, then it might be okay to change the digests in the tests, but it's important to remember that e2e tests need to be able to run using local cache and also run using quay. Tests can determine the environment via UsingCacheRegistry() (returns bool). This is ugly but it's a potential way to get tests to pass.

If digests are not consistently reproducible, all bets are off and the tests will need to be completely reimagined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these changes reliably consistent?

No; there is no guarantee that compression produces consistent results even with one version, and the output can (and, from time to time, does) change when the implementation changes.

For the purpose of the current experiment, I’ll hard-code the digests as they exist in the local cache. But, other than that, we’ll indeed need to specifically maintain zstd:chunked images on quay.io — or, alternatively, not hard-code the expected digests in tests, and obtain them at runtime, or maybe at the cache build time, using something like Skopeo.

Copy link
Member

Choose a reason for hiding this comment

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

I think the digest always depends on the compression, so as long as the compression is reproducible it should produce the same digest?

In any case is there any need to hard code the digests? Skopeo inspect can give us that

skopeo inspect --no-tags  --format {{.Digest}} docker://quay.io/libpod/testimage:20241011

(The command seems quite slow for some reason, so it isn't something I would like to run in every test but only once and cache it)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Compression isn’t reproducible.

Using skopeo inspect is definitely an option — noting that it decreases the test coverage a bit: if c/image were broken, we’d see consistent incorrect data from both Skopeo and the Podman binary being tested.

Copy link
Member

Choose a reason for hiding this comment

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

Using skopeo inspect is definitely an option — noting that it decreases the test coverage a bit: if c/image were broken, we’d see consistent incorrect data from both Skopeo and the Podman binary being tested.

Fair but one thing to consider is we would be using skopeo from the rpm build and podman from main so if there is a breaking change only podman on main should be different until we have a new skopeo + c/image build in our VMs so it seem less likely that we hit a corner case there?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure — and the risk is not that high.

Maybe, first, we need some thought about the final test plan: do we actually want to add a gzip / zstd:chunked dimension to the testing matrix, to make this a variable? Maybe the necessary testing coverage can be achieved by a few dedicated tests/images. That’s very unclear to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly. This is one of the questions on my radar, that I don't yet know enough to answer. My hope is that we can do an integration, mixed set of images, but as of this writing my recollection is that some images have worked (with zstd:chunked) and some have not, and that makes me uncomfortable. It's possible that the reason for this is that some tests use different images and that's what triggers the failures, but I haven't spent the time to investigate. Thank you for bringing this up.

--preserve-digests \
docker://$registry/$img \
docker://127.0.0.1:${REGISTRY_PORT}/$img \
&& return
Expand Down