Page MenuHomeFreeBSD

Fix geli device cleanup
ClosedPublic

Authored by ngie on Apr 9 2019, 12:56 AM.

Details

Summary

Final cleanup routines shouldn't be called from testcases; it should be called
from the testcase cleanup routine.

Furthermore, geli_test_cleanup should take care of cleaning up geli providers
and the memory disks used for the geli providers. geli_test_cleanup will always
be executed whereas the equivalent logic in geli_test_body, may not have been
executed if the test failed prior to the logic being run.

Prior to this change, the test case was trying to clean up $md twice: once in
at the end of the test case body function, and the other in the cleanup function.
The cleanup function logic was failing because there wasn't anything to clean up
in the cleanup function and the errors weren't being ignored.

This fixes FreeBSD test suite runs after r345864.

PR: 237128
MFC with: r345864

Test Plan
$ sudo kyua test -k /usr/tests/sys/geom/class/eli/Kyuafile online_resize_test:online_resize; sudo mdconfig -l
online_resize_test:online_resize  ->  passed  [17.098s]

Results file id is usr_tests_sys_geom_class_eli.20190409-005019-784972
Results saved to /root/.kyua/store/results.usr_tests_sys_geom_class_eli.20190409-005019-784972.db

1/1 passed (0 failed)
$

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ngie created this revision.Apr 9 2019, 12:56 AM
asomers accepted this revision.Apr 9 2019, 3:23 AM

I don't know why you added a blank line at 179, but otherwise LGTM.

This revision is now accepted and ready to land.Apr 9 2019, 3:23 AM
pjd accepted this revision.Apr 9 2019, 5:44 AM

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.

PS. The patch is fine.

ngie added a comment.Apr 9 2019, 6:30 AM
In D19854#426189, @pjd wrote:

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.
PS. The patch is fine.

Long story short is that it was doubling up the cleanup process.

It would allocate the device in body function, clean it up in the body function, then try to clean up the same device in the cleanup function, failing because it didn't exist in a state that could be deallocated/detached.

ngie edited the summary of this revision. (Show Details)Apr 9 2019, 6:33 AM

I don't know why you added a blank line at 179, but otherwise LGTM.

Fixed :)...

pjd added a comment.Apr 9 2019, 7:20 AM
In D19854#426191, @ngie wrote:
In D19854#426189, @pjd wrote:

Do you mind elaborating why cleaning up like that is wrong? The reason I did it was to avoid creating multiple md devices. Here we only have 8 iterations of the loop, but I could easly imagine tests were I'd have hundreds and cleaning as I go would be a good thing then, IMHO. But maybe I'm missing something obvious here, so if you could explain that would be great. Thanks.
PS. The patch is fine.

Long story short is that it was doubling up the cleanup process.
It would allocate the device in body function, clean it up in the body function, then try to clean up the same device in the cleanup function, failing because it didn't exist in a state that could be deallocated/detached.

Hmm, I thought that ignoring exit code for mdconfig and redirecting its stderr to /dev/null won't make it fail the tests. I wanted to leave it in case the tests somehow exited in the middle. Anyway, looks good with your patch.

A more important reason not to clean up in the body is because it won't necessarily get run. If the test fails, the body will exit immediately. That's why cleanup is done in a separate process.

ngie edited the summary of this revision. (Show Details)Apr 9 2019, 4:19 PM
This revision was automatically updated to reflect the committed changes.