test: remove /dev/loopX in TestRwLoop #9308

ZhangShuaiyi · 2023-10-30T12:23:40Z

After run TestRwLoop, there will leave a /dev/loopX not removed.

	file, err := setupLoop(backingFile, LoopParams{Autoclear: false})

Autoclear is false in TestRwLoop, so /dev/loopX will not be cleared automatically.

[root@containerddev containerd]# losetup
[root@containerddev containerd]# go test -v github.com/containerd/containerd/mount -test.root -run TestRwLoop
=== RUN   TestRwLoop
--- PASS: TestRwLoop (0.02s)
PASS
ok      github.com/containerd/containerd/mount  0.026s
[root@containerddev containerd]# losetup
NAME       SIZELIMIT OFFSET AUTOCLEAR RO BACK-FILE                        DIO LOG-SEC
/dev/loop0         0      0         0  0 /tmp/losetup1667762043 (deleted)   0     512
[root@containerddev containerd]# go test -v github.com/containerd/containerd/mount -test.root -run TestRwLoop
=== RUN   TestRwLoop
--- PASS: TestRwLoop (0.00s)
PASS
ok      github.com/containerd/containerd/mount  0.012s
[root@containerddev containerd]# losetup
NAME       SIZELIMIT OFFSET AUTOCLEAR RO BACK-FILE                        DIO LOG-SEC
/dev/loop1         0      0         0  0 /tmp/losetup473965374 (deleted)    0     512
/dev/loop0         0      0         0  0 /tmp/losetup1667762043 (deleted)   0     512
[root@containerddev containerd]# losetup -d /dev/loop0
[root@containerddev containerd]# losetup -d /dev/loop1
[root@containerddev containerd]# losetup
[root@containerddev containerd]#

Defer call removeLoop to remove loop device at end.

[root@containerddev containerd]# go test -v github.com/containerd/containerd/mount -test.root -run TestRwLoop
=== RUN   TestRwLoop
--- PASS: TestRwLoop (0.00s)
PASS
ok      github.com/containerd/containerd/mount  0.013s
[root@containerddev containerd]# losetup
[root@containerddev containerd]#

k8s-ci-robot · 2023-10-30T12:23:51Z

Hi @ZhangShuaiyi. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

estesp · 2023-10-30T14:24:47Z

This looks like a good cleanup; can you please sign your commit according to our guidance here: https://github.com/containerd/project/blob/main/CONTRIBUTING.md#sign-your-work

austinvazquez

Nice find. For project checks, you would need to add a DCO. See https://github.com/containerd/project/blob/main/CONTRIBUTING.md#sign-your-work for help on this.

austinvazquez · 2023-10-30T14:30:04Z

mount/losetup_linux_test.go

+	defer func() {
+		dev := file.Name()
+		file.Close()
+		if err := removeLoop(dev); err != nil {
+			t.Fatal(err)
+		}
+	}()


Alternatively, for conciseness should the solution be to just use Autoclear: true similar toTestRoLoop?

@austinvazquez How about using Autoclear: ture in TestRwLoop, and add a new test case for Autoclear: false?

+1, yeah it was not clear before that TestRwLoop was intended to test Autoclear: false. The tests you have added are much more intentional. Thanks.

ZhangShuaiyi · 2023-10-30T14:47:13Z

@estesp Thanks, I will add a DCO later

henry118 · 2023-10-30T16:57:26Z

/ok-to-test

k8s-ci-robot · 2023-10-30T16:57:42Z

@henry118: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

Details

In response to this:

/ok-to-test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ZhangShuaiyi

Run 10 times TestAutoclearTrueLoop in a 1C4G vm. TestAutoclearTrueLoop not always pass.

root@ubuntu2004:~/containerd# uname -a
Linux ubuntu2004 5.4.0-42-generic #46-Ubuntu SMP Fri Jul 10 00:24:02 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
root@ubuntu2004:~/containerd# nproc
1
root@ubuntu2004:~/containerd# go test -count 10 github.com/containerd/containerd/mount -test.root -run TestAutoclearTrueLoop
--- FAIL: TestAutoclearTrueLoop (0.01s)
    losetup_linux_test.go:136: removeLoop should fail if Autoclear is true
--- FAIL: TestAutoclearTrueLoop (0.03s)
    losetup_linux_test.go:136: removeLoop should fail if Autoclear is true
--- FAIL: TestAutoclearTrueLoop (0.01s)
    losetup_linux_test.go:136: removeLoop should fail if Autoclear is true
--- FAIL: TestAutoclearTrueLoop (0.02s)
    losetup_linux_test.go:136: removeLoop should fail if Autoclear is true
FAIL
FAIL    github.com/containerd/containerd/mount  0.254s
FAIL

ZhangShuaiyi · 2023-10-31T03:41:13Z

Loop deivce's autoclear is asynchronous. __loop_clr_fd was called in lo_release when Autoclear: true.
After add a 100ms sleep in TestAutoclearTrueLoop to wait loop device autoclear, the TestAutoclearTrueLoop can always be passed.

root@ubuntu2004:~/containerd# go test -count 100 github.com/containerd/containerd/mount -test.root -run TestAutoclearTrueLoop
ok      github.com/containerd/containerd/mount  11.218s

henry118 · 2023-10-31T20:20:47Z

@ZhangShuaiyi mind squashing your commits? the changes lgtm

Signed-off-by: Shuaiyi Zhang <[email protected]>

ZhangShuaiyi · 2023-11-01T03:51:41Z

@henry118 Squashed, thanks!

ZhangShuaiyi · 2023-11-01T11:38:52Z

/retest

Update fork-external main with upstream main @ 452ec25 Related work items: containerd#5890, containerd#7647, containerd#9218, containerd#9233, containerd#9258, containerd#9270, containerd#9274, containerd#9279, containerd#9283, containerd#9286, containerd#9289, containerd#9290, containerd#9294, containerd#9295, containerd#9297, containerd#9305, containerd#9306, containerd#9308, containerd#9316, containerd#9317, containerd#9319, containerd#9320, containerd#9321

k8s-ci-robot added the needs-ok-to-test label Oct 30, 2023

austinvazquez reviewed Oct 30, 2023

View reviewed changes

ZhangShuaiyi force-pushed the fix/TestRwLoop branch 3 times, most recently from a3a25cb to f111733 Compare October 30, 2023 16:24

henry118 removed the needs-ok-to-test label Oct 30, 2023

henry118 added the ok-to-test label Oct 30, 2023

ZhangShuaiyi commented Oct 31, 2023

View reviewed changes

ZhangShuaiyi force-pushed the fix/TestRwLoop branch 2 times, most recently from 21a4d44 to c4a447b Compare October 31, 2023 04:52

test: use 'Autoclear: ture' in TestRwLoop and add Autoclear test

b6adf43

Signed-off-by: Shuaiyi Zhang <[email protected]>

ZhangShuaiyi force-pushed the fix/TestRwLoop branch from c4a447b to b6adf43 Compare November 1, 2023 03:50

estesp approved these changes Nov 1, 2023

View reviewed changes

austinvazquez approved these changes Nov 1, 2023

View reviewed changes

henry118 approved these changes Nov 1, 2023

View reviewed changes

estesp added this pull request to the merge queue Nov 1, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2023

estesp added this pull request to the merge queue Nov 1, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 1, 2023

estesp added this pull request to the merge queue Nov 2, 2023

Merged via the queue into containerd:main with commit 3d6c5ea Nov 2, 2023

test: remove /dev/loopX in TestRwLoop #9308

test: remove /dev/loopX in TestRwLoop #9308

Uh oh!

Conversation

ZhangShuaiyi commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Oct 30, 2023

Uh oh!

estesp commented Oct 30, 2023

Uh oh!

austinvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

austinvazquez Oct 30, 2023

Choose a reason for hiding this comment

Uh oh!

ZhangShuaiyi Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austinvazquez Nov 1, 2023

Choose a reason for hiding this comment

Uh oh!

ZhangShuaiyi commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henry118 commented Oct 30, 2023

Uh oh!

k8s-ci-robot commented Oct 30, 2023

Uh oh!

ZhangShuaiyi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZhangShuaiyi commented Oct 31, 2023

Uh oh!

henry118 commented Oct 31, 2023

Uh oh!

ZhangShuaiyi commented Nov 1, 2023

Uh oh!

ZhangShuaiyi commented Nov 1, 2023

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ZhangShuaiyi commented Oct 30, 2023 •

edited

Loading

ZhangShuaiyi Oct 30, 2023 •

edited

Loading

ZhangShuaiyi commented Oct 30, 2023 •

edited

Loading

ZhangShuaiyi left a comment •

edited

Loading