Update system tests to handle zstd:chunked images #24286

mtrmac · 2024-10-15T21:42:16Z

Compare image configs, not IDs, when checking image identity from different way of pulling them
Try to make the additional image store test a bit more robust
Remove an image identity assumption from the “load by URL” test.

Working on test failures as discussed in containers/storage#2130 .

~~At this point absolutely untested, and~~ I don’t really know what I’m doing.

Cc: @edsantiago — perhaps read this only later after this is at least minimally shown to be working.

Does this PR introduce a user-facing change?

None

openshift-ci · 2024-10-15T21:42:22Z

[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

Needs approval from an approver in each of these files:

~~OWNERS~~ [mtrmac]

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

packit-as-a-service · 2024-10-15T21:45:42Z

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

mtrmac

Yeah, this is nowhere close to working, ignore for now.

Also [255] [email protected] template with rollback

#|     FAIL: global auto-update policy gets applied
   #| expected: '.*podman-kube@-tmp-podman_bats.roXH3U-test.yaml.service,.* (test_pod-a),quay.io/libpod/testimage:20241010,false,registry.*' (using expr)
   #|   actual: 'podman-kube@-tmp-podman_bats.roXH3U-test.yaml.service,c104ff383fd7 (test_pod-a),quay.io/libpod/testimage:20241010,pending,registry'
   #|         > 'podman-kube@-tmp-podman_bats.roXH3U-test.yaml.service,aa204347c263 (test_pod-b),localhost/image:Kl8NYi3jxL,false,local'

test/system/010-images.bats

test/system/120-load.bats

test/system/150-login.bats

mtrmac · 2024-10-17T21:24:37Z

@edsantiago With this, make localsystem is passing for me in #24287 , and it should continue to work with non-chunked images.

Could you take a preliminary look to make sure I’m not missing/breaking the point of these tests, please?

See individual commit messages for details.

edsantiago · 2024-10-17T21:57:16Z

@mtrmac I've been peeking periodically and have seen no major cause for alarm. I will not be able to give this a thorough review until Monday. Apologies.

test/system/helpers.bash

mtrmac · 2024-10-17T21:59:58Z

@edsantiago Thanks! The peeks are reassuring already, and I need to make this pass first. There’s no rush at all.

edsantiago

I think the 120 tests need a complete revamp: my initial decision to use $IMAGE has backfired. Next week I will look into a different safer approach.

mtrmac · 2024-10-18T22:58:16Z

Ready for review:
System tests are passing both here, with current images, and in #24287 , with zstd:chunked images. (Integration tests are failing with zstd:chunked images, I’ll work on that in a separate PR.)

edsantiago

One suggestion, one question-and-suggestion, none blockers.

The use of cd for config digest does weird things to my brain: I can't help but see it as change directory. That's indelibly hardwired and I'm actually a little amused by it. I can't propose a better alternative (except the unwieldy $config_digest, which I see is what skopeo uses in its own system tests) so I won't block on it. Just pointing out in case you're able to think of any better name.

Nice work. Thanks for the explanatory comments.

edsantiago · 2024-10-21T12:17:41Z

test/system/010-images.bats


    CONTAINERS_STORAGE_CONF=$sconf run_podman pull -q $IMAGE
-    is "$output" "$id" "pull -q $IMAGE, using storage.conf"
+    # This is originally a regression test, (podman pull) used to output multiple image IDs. Ensure it only prints one.
+    [[ $(printf '%s' "$output" | wc -l) -le 1 ]]


Suggestion:

assert "${#lines[*]}" -le 1 "Number of output lines from podman pull"

Reason: the Bats [[ etc ]] tests generate error diagnostics that are ... sub-ideal. This is an assertion that must never fail, but even those (especially those?) deserve a way to help someone debug.

Thanks, I completely forgot assert exists despite using it elsewhere.

Fixed as proposed.

edsantiago · 2024-10-21T12:21:47Z

test/system/helpers.bash

+# with zstd:chunked, the same image might have different IDs depending on whether
+# creating layers happened based on the TOC (and per-file operations) or the full layer tarball
+function image_config_digest() {
+    echo "image_config_digest $1" >&2


Did you intend to leave these debug statements?

The printf | sha256sum below is subtle and potentially distracting; if you remove the debug statements, you could collapse everything down to

local sha_output; sha_output=$(skopeo inspect --raw --config "containers-storage:$1" | sha256sum) # strip filename ("-") from sha output # echo "${sha_output%% *}"

I'm fine with leaving the debug statements in place for a few months, but if so please add a comment explaining when it might be safe to remove them, and explaining the reason for printf (preserving exact output without the additional newline that <<<"$config" would introduce)

After the local mystery of #24286 (comment) , the most useful case of detecting broken inputs should be handled naturally; so I have dropped the logging.

edsantiago · 2024-10-21T12:34:24Z

test/system/010-images.bats

+    # Removing the additional store underneath can result in dangling layer references.
+    # Try to fix that up.
+    CONTAINERS_STORAGE_CONF=$sconf run_podman rmi $IMAGE
+    _prefetch $IMAGE


This makes me barf a little, but it's my fault for making bad assumptions in this test, and I pledge to clean it in a followup.

mtrmac · 2024-10-21T15:04:40Z

The use of cd for config digest does weird things to my brain: I can't help but see it as change directory. That's indelibly hardwired and I'm actually a little amused by it. I can't propose a better alternative (except the unwieldy $config_digest,

I don’t mind long and verbose names, if they avoid confusion. So I’ll rename to config_digest if that’s OK.

The test got the stores RW status backwards. Before zstd:chunked, both image IDs should be the same, so this used to make no difference. Signed-off-by: Miloslav Trmač <[email protected]>

When looking up the current-store image ID, do that from the same output where we verify that the ID is from the current store, instead of listing images twice. Signed-off-by: Miloslav Trmač <[email protected]>

… methods Historically, non-schema1 images had a deterministic image ID == config digest. With zstd:chunked, we don't want to deduplicate layers pulled by consuming the full tarball and layers partially pulled based on TOC, because we can't cheaply ensure equivalence; so, image IDs for images where a TOC was used differ. To accommodate that, compare images using their configs digests, not using image IDs. Signed-off-by: Miloslav Trmač <[email protected]>

The additional image store feature assumes that images / layers in the additional store never go away, while we do remove it after this test. Try to repair the store. Signed-off-by: Miloslav Trmač <[email protected]>

Don't assume that the loaded image will be deduplicated with the server image. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac · 2024-10-22T18:31:17Z

Thanks for the review, updated (except for rearchitecting the “additional image store” test).

edsantiago · 2024-10-22T18:34:47Z

...and I see that tests passed in #24287 as well. Thank you. Will try to re-review this afternoon after getting to a breaking point in other work.

edsantiago · 2024-10-22T19:36:29Z

/lgtm

Nice work, thank you

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2024

mtrmac commented Oct 15, 2024

View reviewed changes

test/system/010-images.bats Outdated Show resolved Hide resolved

test/system/120-load.bats Show resolved Hide resolved

test/system/150-login.bats Outdated Show resolved Hide resolved

test/system/150-login.bats Outdated Show resolved Hide resolved

mtrmac force-pushed the compare-image-configs branch 8 times, most recently from 51edef6 to a644fe8 Compare October 17, 2024 21:21

mtrmac changed the title ~~FIXME Various zstd:chunked test changes~~ Update system tests to handle zstd:chunked images Oct 17, 2024

mtrmac commented Oct 17, 2024

View reviewed changes

test/system/helpers.bash Outdated Show resolved Hide resolved

test/system/helpers.bash Outdated Show resolved Hide resolved

edsantiago reviewed Oct 18, 2024

View reviewed changes

mtrmac force-pushed the compare-image-configs branch 2 times, most recently from 2832b1b to 4db012e Compare October 18, 2024 22:17

mtrmac marked this pull request as ready for review October 18, 2024 22:58

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 18, 2024

edsantiago reviewed Oct 21, 2024

View reviewed changes

mtrmac added 3 commits October 22, 2024 19:15

Fix the store choice in "podman pull image with additional store"

3bc6072

The test got the stores RW status backwards. Before zstd:chunked, both image IDs should be the same, so this used to make no difference. Signed-off-by: Miloslav Trmač <[email protected]>

Simplify the additional store test

bf8f2b5

When looking up the current-store image ID, do that from the same output where we verify that the ID is from the current store, instead of listing images twice. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac added 2 commits October 22, 2024 19:36

Try to repair c/storage after removing an additional image store

77ef28c

The additional image store feature assumes that images / layers in the additional store never go away, while we do remove it after this test. Try to repair the store. Signed-off-by: Miloslav Trmač <[email protected]>

Improve "podman load - from URL"

6fd0e22

Don't assume that the loaded image will be deduplicated with the server image. Signed-off-by: Miloslav Trmač <[email protected]>

mtrmac force-pushed the compare-image-configs branch from 4db012e to 6fd0e22 Compare October 22, 2024 17:36

openshift-ci bot assigned edsantiago Oct 22, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 22, 2024

openshift-merge-bot bot merged commit 410f796 into containers:main Oct 22, 2024
57 checks passed

mtrmac deleted the compare-image-configs branch October 22, 2024 19:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update system tests to handle zstd:chunked images #24286

Update system tests to handle zstd:chunked images #24286

mtrmac commented Oct 15, 2024 •

edited

Loading

openshift-ci bot commented Oct 15, 2024

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

mtrmac left a comment

mtrmac commented Oct 17, 2024

edsantiago commented Oct 17, 2024

mtrmac commented Oct 17, 2024

edsantiago left a comment

mtrmac commented Oct 18, 2024

edsantiago left a comment

edsantiago Oct 21, 2024

mtrmac Oct 22, 2024

edsantiago Oct 21, 2024

mtrmac Oct 22, 2024

edsantiago Oct 21, 2024

mtrmac commented Oct 21, 2024

mtrmac commented Oct 22, 2024

edsantiago commented Oct 22, 2024

edsantiago commented Oct 22, 2024

Update system tests to handle zstd:chunked images #24286

Update system tests to handle zstd:chunked images #24286

Conversation

mtrmac commented Oct 15, 2024 • edited Loading

Does this PR introduce a user-facing change?

openshift-ci bot commented Oct 15, 2024

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

mtrmac left a comment

Choose a reason for hiding this comment

mtrmac commented Oct 17, 2024

edsantiago commented Oct 17, 2024

mtrmac commented Oct 17, 2024

edsantiago left a comment

Choose a reason for hiding this comment

mtrmac commented Oct 18, 2024

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago Oct 21, 2024

Choose a reason for hiding this comment

mtrmac Oct 22, 2024

Choose a reason for hiding this comment

edsantiago Oct 21, 2024

Choose a reason for hiding this comment

mtrmac Oct 22, 2024

Choose a reason for hiding this comment

edsantiago Oct 21, 2024

Choose a reason for hiding this comment

mtrmac commented Oct 21, 2024

mtrmac commented Oct 22, 2024

edsantiago commented Oct 22, 2024

edsantiago commented Oct 22, 2024

mtrmac commented Oct 15, 2024 •

edited

Loading