tests: add basic zstd:chunked system test #24413

giuseppe · 2024-10-29T23:09:11Z

Does this PR introduce a user-facing change?

None

edsantiago

Thank you! I'm OOTO tomorrow all day so this is a very cursory first pass with some suggestions

edsantiago · 2024-10-29T23:17:34Z

test/system/150-login.bats

@@ -398,6 +398,84 @@ EOF
    assert "$output" =~ "--retry-delay .*pull failures \(default \"5s\"\)"
 }

+@test "push and pull zstd chunked image" {
+    iid=localhost:${PODMAN_LOGIN_REGISTRY_PORT}/$(random_string 10 | tr A-Z a-z)


Could you rename this please? To me, iid refers to a sha.

And, there's a new helper in town, safename. The preferred way to do that now would be

.../img-$(safename)

edsantiago · 2024-10-29T23:21:08Z

test/system/150-login.bats

+    iid2=localhost:${PODMAN_LOGIN_REGISTRY_PORT}/$(random_string 10 | tr A-Z a-z)
+    iid3=localhost:${PODMAN_LOGIN_REGISTRY_PORT}/$(random_string 10 | tr A-Z a-z)


when using safename multiple times, I've found it best to differentiate in the prefix:

.../img2-$(safename) .../img3-$(safename)

edsantiago · 2024-10-29T23:26:50Z

test/system/150-login.bats

+               $iid3
+
+    run_podman diff $iid3 $iid2
+    sorted_output=$(echo $output | sort)


I didn't expect this to work, and it doesn't, and for many reasons. First and most important, echo is weird in ways that one has no choice but to shrug and accept:

echo $multilinestring <<<---- collapses to just one line! echo "$multilinestring" <<<---- with quotes, it preserves newlines

so anyhow, the sort there is a NOP. I'm not sure what it is you're trying to check, and it's late in my day, so I'm sorry not to be able to offer a suggestion for how to solve this.

edsantiago · 2024-10-29T23:27:21Z

test/system/150-login.bats

+               $iid3
+
+    run_podman diff $iid3 $iid2
+    sorted_output=$(echo $output | sort)


Same as above re: echo and newlines and sort

test/system/150-login.bats

edsantiago · 2024-10-29T23:30:44Z

test/system/150-login.bats

+               --cert-dir ${PODMAN_LOGIN_WORKDIR}/trusted-registry-cert-dir \
+               --creds ${PODMAN_LOGIN_USER}:${PODMAN_LOGIN_PASS} \


These two lines are a great candidate for refactoring into $pushpullargs or somesuch

giuseppe · 2024-10-30T16:38:27Z

@edsantiago thanks for the review.

I changed the original tests, the test is just a starting point

edsantiago

Minor suggestions inline, and one major request: please move this out of 150-login.bats. None of this has anything to do with credential testing. Maybe a new zstd-chunked file? Or a more generic compression? I think we're going to need to add more tests to handle more subtle cases.

Many, many thanks for this. It would never have occurred to me that podman diff would be useful in some way. Your in-depth expertise is exactly what we need to write tests and develop confidence in this new feature.

edsantiago · 2024-10-31T13:55:19Z

test/system/150-login.bats

+        run_podman push $image dir:$push_dir
+
+        run_podman inspect $image
+        for layer in $(jq '.[].RootFS.Layers.[] | gsub("^sha256:"; "")' <<< $output | tr -d \"); do


jq -r will emit output without doublequotes

edsantiago · 2024-10-31T13:56:21Z

test/system/150-login.bats

+            # the checksum for the layer is already validated, but for the sake
+            # of the test let's check it again
+            run -0 sha256sum < $layer_file
+            assert "$output" = "$layer  -" "digest mismatch for layer $layer"


suggestion for error message: include $image in the string.

test/system/150-login.bats

edsantiago

Test LGTM with one big question. I'll defer to others on the vendoring.

edsantiago · 2024-10-31T17:50:21Z

test/system/155-partial-pull.bats

+        # replace the image with a "podman load" from what was stored
+        run_podman rmi $image
+        run_podman load < $PODMAN_TMPDIR/image.tar


wait, doesn't this completely change what is being tested?

giuseppe · 2024-11-04T14:59:31Z

@edsantiago I've added a new test to make sure the image content doesn't change after a push/pull

mheon · 2024-11-04T18:51:56Z

LGTM

rhatdan · 2024-11-04T18:57:55Z

/lgtm

edsantiago · 2024-11-04T19:00:09Z

Tests LGTM. @Luap99 heads-up this will probably conflict with #24447
Thanks @giuseppe

edsantiago · 2024-11-04T19:41:52Z

int remote rawhide root has timed out three times. I'm not liking this.

/hold

Luap99 · 2024-11-04T20:00:42Z

int remote rawhide root has timed out three times. I'm not liking this.

also got a timeout in #24447 but didn't think to much of it as I did not try to rerun it yet as I knew it was broken anyway
https://cirrus-ci.com/task/6327338403627008

edsantiago · 2024-11-04T20:05:47Z

Thank you! That's good news. In the other log, it looks like all healthcheck commands time out. But since the entire test run times out, there's no (sane) way to see what other tests time out. In your log, I see a wide variety of tests timing out. And the cumulative effect of many random tests hanging for 90s is a test failure.

Something is definitely broken with remote root. ~~(remote rootless is passing just fine)~~(EDIT: int tests do not run remote rootless. My mistake). So far, no way to reproduce locally.

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe · 2024-11-08T11:39:49Z

rebased

giuseppe · 2024-11-08T14:28:33Z

tests are green now

/hold cancel

Luap99

/lgtm

openshift-ci · 2024-11-08T14:32:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, giuseppe, Luap99

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~~ [Luap99,edsantiago,giuseppe]

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

mtrmac · 2024-11-22T20:25:56Z

test/system/155-partial-pull.bats

+@test "push and pull zstd chunked image" {
+    image1=localhost:${PODMAN_LOGIN_REGISTRY_PORT}/img1-$(safename)
+
+    globalargs="--pull-option enable_partial_pulls=true"


@giuseppe I can’t see any consumer of this option (set twice in this PR). I know about enable_partial_images .

Is this a typo, or am I missing something?

Filed #24686 to fix that.

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. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 29, 2024

edsantiago suggested changes Oct 29, 2024

View reviewed changes

giuseppe force-pushed the add-test-zstd-chunked branch 5 times, most recently from f69b757 to c69fda7 Compare October 30, 2024 10:15

giuseppe changed the title ~~WIP: add basic zstd:chunked system test~~ tests: add basic zstd:chunked system test Oct 30, 2024

giuseppe marked this pull request as ready for review October 30, 2024 16:36

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 30, 2024

giuseppe force-pushed the add-test-zstd-chunked branch 2 times, most recently from d9b1465 to b660ac5 Compare October 31, 2024 12:46

edsantiago suggested changes Oct 31, 2024

View reviewed changes

giuseppe force-pushed the add-test-zstd-chunked branch from b660ac5 to e53f0f8 Compare October 31, 2024 14:55

edsantiago reviewed Oct 31, 2024

View reviewed changes

edsantiago approved these changes Oct 31, 2024

View reviewed changes

giuseppe marked this pull request as draft November 4, 2024 12:23

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

giuseppe force-pushed the add-test-zstd-chunked branch 2 times, most recently from 62de9bf to df71e86 Compare November 4, 2024 12:33

giuseppe marked this pull request as ready for review November 4, 2024 13:12

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 Nov 4, 2024

openshift-ci bot assigned rhatdan Nov 4, 2024

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

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 4, 2024

edsantiago mentioned this pull request Nov 4, 2024

Vendoring: timeout in root remote rawhide int tests #24463

Closed

test: add zstd:chunked system tests

30a82ca

Signed-off-by: Giuseppe Scrivano <[email protected]>

giuseppe force-pushed the add-test-zstd-chunked branch from df71e86 to 30a82ca Compare November 8, 2024 11:39

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 8, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 8, 2024

Luap99 approved these changes Nov 8, 2024

View reviewed changes

openshift-ci bot assigned Luap99 Nov 8, 2024

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

openshift-merge-bot bot merged commit ee5b8de into containers:main Nov 8, 2024
52 of 53 checks passed

mtrmac reviewed Nov 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: add basic zstd:chunked system test #24413

tests: add basic zstd:chunked system test #24413

giuseppe commented Oct 29, 2024

edsantiago left a comment

edsantiago Oct 29, 2024

edsantiago Oct 29, 2024

edsantiago Oct 29, 2024

edsantiago Oct 29, 2024

edsantiago Oct 29, 2024

giuseppe commented Oct 30, 2024

edsantiago left a comment

edsantiago Oct 31, 2024

edsantiago Oct 31, 2024

edsantiago left a comment

edsantiago Oct 31, 2024

giuseppe commented Nov 4, 2024

mheon commented Nov 4, 2024

rhatdan commented Nov 4, 2024

edsantiago commented Nov 4, 2024

edsantiago commented Nov 4, 2024

Luap99 commented Nov 4, 2024

edsantiago commented Nov 4, 2024 •

edited

Loading

giuseppe commented Nov 8, 2024

giuseppe commented Nov 8, 2024

Luap99 left a comment

openshift-ci bot commented Nov 8, 2024

mtrmac Nov 22, 2024

mtrmac Nov 26, 2024

		iid2=localhost:${PODMAN_LOGIN_REGISTRY_PORT}/$(random_string 10 \| tr A-Z a-z)
		iid3=localhost:${PODMAN_LOGIN_REGISTRY_PORT}/$(random_string 10 \| tr A-Z a-z)

		--cert-dir ${PODMAN_LOGIN_WORKDIR}/trusted-registry-cert-dir \
		--creds ${PODMAN_LOGIN_USER}:${PODMAN_LOGIN_PASS} \

tests: add basic zstd:chunked system test #24413

tests: add basic zstd:chunked system test #24413

Conversation

giuseppe commented Oct 29, 2024

Does this PR introduce a user-facing change?

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Oct 30, 2024

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giuseppe commented Nov 4, 2024

mheon commented Nov 4, 2024

rhatdan commented Nov 4, 2024

edsantiago commented Nov 4, 2024

edsantiago commented Nov 4, 2024

Luap99 commented Nov 4, 2024

edsantiago commented Nov 4, 2024 • edited Loading

giuseppe commented Nov 8, 2024

giuseppe commented Nov 8, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edsantiago commented Nov 4, 2024 •

edited

Loading