-
Notifications
You must be signed in to change notification settings - Fork 1.9k
ZTS: fix zpool_export tests from always passing even if a failure occurs #18168
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR fixes a critical bug in the ZFS test suite where zpool_export tests would always pass even when failures occurred. The root cause was that the cleanup function used default_cleanup which calls log_pass, overwriting the test's exit code. Additionally, it fixes zpool_export_002_pos.ksh which was failing after the cleanup bug was fixed.
Changes:
- Changed
zpool_export_cleanupto usedefault_cleanup_noexitinstead ofdefault_cleanupto preserve test exit codes - Added explicit pool and filesystem setup in
zpool_export_002_pos.kshsince the pool gets destroyed in the prior test
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/zfs-tests/tests/functional/cli_root/zpool_export/zpool_export.kshlib | Fixed cleanup function to use default_cleanup_noexit to avoid overwriting test exit codes with log_pass |
| tests/zfs-tests/tests/functional/cli_root/zpool_export/zpool_export_002_pos.ksh | Added manual pool/filesystem setup since pool is destroyed by previous test, and updated test documentation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
54bd76f to
a7e1b49
Compare
|
just rebased on the master branch |
|
Rebasing this one more time on the master branch should sort out the CI infrastructure issue. |
Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: JT Pennington <[email protected]>
Sponsored-by: Klara, Inc. Sponsored-by: Wasabi Technology, Inc. Signed-off-by: JT Pennington <[email protected]>
a7e1b49 to
bc2a5a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tests/zfs-tests/tests/functional/cli_root/zpool_export/zpool_export_002_pos.ksh
Show resolved
Hide resolved
behlendorf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me like the cleanup in d7265b3 introduced this bug by destroying the pool after each test. Our usual convention for a test group is if a pool is created in setup.sh it is expected to persistent until cleanup.sh which destroys it. Individual tests that destroy a pool are also responsible for create it.
So it seems to me the bug here is zpool_export_cleanup shouldn't be destroying the pool at all on failure (or success). It simply needs to make sure the existing pool is ready for the test case. Alternately, I think it'd be fine if you updated all of the tests in this group to create/destroy a pool for use in their individual test.
Motivation and Context
Currently the export tests will always pass, you can add a log_fail and it will still pass.
log_fail calls endlog (tests/test-runner/include/logapi.shlib:366-369) with exit code of 1.
endlog (tests/test-runner/include/logapi.shlib:428-460) runs the cleanup function log_onexit zpool_export_cleanup.
zpool_export_cleanup (tests/zfs-tests/tests/functional/cli_root/zpool_export/zpool_export.kshlib:29-33) calls default_cleanup. However default_cleanup (tests/zfs-tests/include/libtest.shlib:531-536) ends with log_pass which calls _endlog with an exit code of 0. This overwrites the original exit code of 1 and results in the test passing.
default_cleanup_noexit (tests/zfs-tests/include/libtest.shlib:549-623) should be used in the zpool_export_cleanup function so the exit code is not overwritten.
After fixing zpool_export.kshlib, the failure of zpool_export_002_pos.ksh became visible. I then fixed this test by ensuring the pool gets created, since it was destroyed in the prior test.
How Has This Been Tested?
Tested on Linux and FreeBSD
Types of changes
Checklist:
Signed-off-by.