test/system: Add a test case for automount with multi images #23301

ypu · 2024-07-17T08:06:45Z

This test check the automount feature with multi images for following item:

multi images can be auotmounted with yaml file.
if there are same path exist in the images, the last one should trumps.
the volume is mounted readonly in the container.
the volumes are only mounted in the specific container, but not the whole pods.

Does this PR introduce a user-facing change?

NONE

Luap99

Can you merge the test with the other one above and reuse that image there. Building is slow and we are actively working on increasing the performance of system test so every second matters.

Luap99 · 2024-07-17T11:01:32Z

test/system/700-play.bats

@@ -1035,3 +1035,72 @@ spec:
    run_podman rm -f -t 0 -a
    run_podman rmi automount_test
 }
+
+@test "podman ply with multi automount volume" {


Luap99 · 2024-07-17T11:03:51Z

test/system/700-play.bats

+RUN mkdir /test1 /test_same
+RUN touch /test1/a /test1/b /test1/c
+RUN echo "I am from test1 image" > /test_same/hello_world


please chain them via && \ instead of desperate RUN instructions

OK. Will update this.

Luap99 · 2024-07-17T11:03:58Z

test/system/700-play.bats

+RUN mkdir /test2 /test_same
+RUN touch /test2/asdf /test2/ejgre /test2/lteghe
+RUN echo "I am from test2 image" > /test_same/hello_world


Luap99 · 2024-07-17T11:06:27Z

test/system/700-play.bats

+metadata:
+  labels:
+    app: test
+  name: test_pod


Do not hardcore the names please, see
#23280 and #23275

I know this is the case today but we must stop doing that.

700-play is next on my list for refactoring. @ypu if you wait maybe a week or so, until my (not-yet-filed) PR merges, you will have a much better base to work with.

Thanks @Luap99 and @edsantiago . Will wait for Ed’s PR for refactoring the script.

ypu · 2024-07-17T15:21:47Z

Thanks @Luap99 and @edsantiago for your comments. And will work with the test case after Ed’s refactor for the script get done.

edsantiago

Please rebase, and take a look at the new 700-play.bats. In particular, the way there are no longer any hardcoded names. I've added a few suggestions inline. Thank you!

edsantiago · 2024-07-19T01:48:48Z

test/system/700-play.bats

+    run_podman build -t test1 -f $PODMAN_TMPDIR/Containerfile1
+    run_podman build -t test2 -f $PODMAN_TMPDIR/Containerfile2


img1="i1-$(safename)" img2="i2-$(safename)" build -t $img1 ...

test/system/700-play.bats

edsantiago · 2024-07-19T01:51:11Z

test/system/700-play.bats

+    run_podman kube play $fname
+
+    run_podman run --rm test1 ls /test1
+    run_out_test1="$output"


Please check this for validity. Comparing the ls output is not useful if both show mistaken results.

edsantiago · 2024-07-19T01:51:30Z

test/system/700-play.bats

+    run_podman exec test_pod-testctr cat /test_same/hello_world
+    assert "I am from test2 image" "test_same should from image2"
+
+    run_podman 1 exec test_pod-testctr touch /test1/readonly


Error message check please

ypu · 2024-07-25T06:52:23Z

/release-note-none

ypu · 2024-07-25T07:38:39Z

Hi @edsantiago and @Luap99 pullreq updated. Can you help to review it again? Thanks a lot!

edsantiago

First pass review. I'm still having trouble understanding everything that is happening here; there may be other issues I've missed. Please address these, rebase, and I'll take another look.

edsantiago · 2024-07-25T11:25:45Z

test/system/700-play.bats

+    run_podman run --rm $imgname1 ls -x /
+    run_podman run --rm $imgname2 ls -x /


Are these debugging leftovers? If so please remove.

edsantiago · 2024-07-25T11:30:02Z

test/system/700-play.bats

+    run_podman exec "$podname_file-$ctrname_file" ls -x /test1
+    assert "a  b  c" "ls /test1 on image"


No, this is now wrong. "on image" means "this is a test being run on the image itself, via podman run". You changed it to "podman exec", which is on the container. It is necessary to do both.

edsantiago · 2024-07-25T11:39:28Z

test/system/700-play.bats

+    assert "$output" =~ "Read-only file system" "image mounted as readonly"
+
+    run_podman exec "$podname_file-$ctrname_file_not_mounted" ls /
+    assert "test" !~ "$output" "No volume should be mounted in $podname_file-$ctrname_file_not_mounted"


Wrong order, it should be assert "$output" !~ "test".

Suggestion: just "No volume should be mounted in the no-mount container". There's no reason to include the entire actual container name, that just makes for a hard-to-read error message.

edsantiago · 2024-07-25T11:40:54Z

test/system/700-play.bats

+    imgname1="automount-test-$(safename)1"
+    imgname2="automount-test-$(safename)2"


Suggestion: automount-test1-$(safename) and -test2-. The way they are now, 1 and 2 appear at the end of a random string, making it impossible for a human to see the difference between them.

edsantiago · 2024-07-25T11:42:59Z

test/system/700-play.bats

+    podname_file="p-$(safename)-file"
+    ctrname_file="c-$(safename)-file"


When I see a variable of the form xxx_file, my brain thinks "this references a file path". Even after staring at this code for half an hour, I still can't get used to the fact that this means "pod and container that have something to do with a file". Could you think of variable names that better reflect the nature of the test being run?

edsantiago · 2024-07-25T11:44:57Z

test/system/700-play.bats

+
+    run_podman run --rm $imgname1 cat /test_same/hello_world
+    assert "$output" = "I am from test1 image"  "cat /test_same/hello_world on image"
+    run_out_test2="$output"


eek, no, bad variable name. This needs to be test1

These test steps check the automount feature with multi images for following item: 1. multi images can be auotmounted with yaml file. 2. if there are same path exist in the images, the last one should trumps. 3. the volume is mounted readonly in the container. 4. the volumes are only mounted in the specific container, but not the whole pods. Signed-off-by: Yiqiao Pu <ypu@redhat.com>

ypu · 2024-07-26T08:11:02Z

Hi @edsantiago sorry to not consider the variable names and assert messages when combine two tests. Now I just remove the debug line, update the variables and assert messages. Can you help to check it again? Thanks a lot!

edsantiago · 2024-07-29T12:38:51Z

LGTM, thank you!

Luap99

/lgtm

openshift-ci · 2024-07-29T14:19:57Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jul 17, 2024

ypu force-pushed the automount-volume branch from fdd89d8 to 9c3bd2f Compare July 17, 2024 08:22

Luap99 reviewed Jul 17, 2024

View reviewed changes

edsantiago requested changes Jul 19, 2024

View reviewed changes

ypu force-pushed the automount-volume branch from 9c3bd2f to 73d47f6 Compare July 25, 2024 04:45

openshift-ci bot added release-note-none and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jul 25, 2024

edsantiago requested changes Jul 25, 2024

View reviewed changes

ypu force-pushed the automount-volume branch from 73d47f6 to a18bd3e Compare July 26, 2024 07:56

edsantiago approved these changes Jul 29, 2024

View reviewed changes

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

Luap99 approved these changes Jul 29, 2024

View reviewed changes

openshift-ci bot assigned Luap99 Jul 29, 2024

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

openshift-merge-bot bot merged commit c7f00b6 into containers:main Jul 29, 2024
57 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test/system: Add a test case for automount with multi images #23301

test/system: Add a test case for automount with multi images #23301

ypu commented Jul 17, 2024 •

edited

Loading

Luap99 left a comment

Luap99 Jul 17, 2024

ypu Jul 17, 2024

Luap99 Jul 17, 2024

ypu Jul 17, 2024

Luap99 Jul 17, 2024

Luap99 Jul 17, 2024

edsantiago Jul 17, 2024

ypu Jul 17, 2024

ypu commented Jul 17, 2024

edsantiago left a comment

edsantiago Jul 19, 2024

edsantiago Jul 19, 2024

edsantiago Jul 19, 2024

ypu Jul 25, 2024

ypu commented Jul 25, 2024

ypu commented Jul 25, 2024

edsantiago left a comment

edsantiago Jul 25, 2024

edsantiago Jul 25, 2024

edsantiago Jul 25, 2024

edsantiago Jul 25, 2024

edsantiago Jul 25, 2024

edsantiago Jul 25, 2024

ypu commented Jul 26, 2024

edsantiago commented Jul 29, 2024

Luap99 left a comment

openshift-ci bot commented Jul 29, 2024

		run_podman build -t test1 -f $PODMAN_TMPDIR/Containerfile1
		run_podman build -t test2 -f $PODMAN_TMPDIR/Containerfile2

		run_podman run --rm $imgname1 ls -x /
		run_podman run --rm $imgname2 ls -x /

		run_podman exec "$podname_file-$ctrname_file" ls -x /test1
		assert "a b c" "ls /test1 on image"

		imgname1="automount-test-$(safename)1"
		imgname2="automount-test-$(safename)2"

		podname_file="p-$(safename)-file"
		ctrname_file="c-$(safename)-file"

test/system: Add a test case for automount with multi images #23301

test/system: Add a test case for automount with multi images #23301

Conversation

ypu commented Jul 17, 2024 • edited Loading

Does this PR introduce a user-facing change?

Luap99 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypu commented Jul 17, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ypu commented Jul 25, 2024

ypu commented Jul 25, 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

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

ypu commented Jul 26, 2024

edsantiago commented Jul 29, 2024

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 29, 2024

ypu commented Jul 17, 2024 •

edited

Loading