tests/slurm-epilog: check the cleanup status and add pathname regressions - #1410
Open
MaxFreedomPollard wants to merge 1 commit into
Open
MaxFreedomPollard wants to merge 1 commit into
MaxFreedomPollard wants to merge 1 commit into
Conversation
Three test-only follow-ups from the review of NVIDIA#1408: - mount_case discarded the cleanup's exit status. A cleanup that exits early leaves every payload in place, which is what the assertions look for, so it passed. The status is now part of the result. - The owned-mount-root case only checked that the payload survived, which is also true of a cleanup that never ran. It now requires a sibling outside the mount to be removed as well. - Nothing covered mount points that read as find -path patterns. Add cases for [, backslash, * and ?, and for a trailing newline, each next to the sibling its mishandled name would match. They script the mount table through an awk stub, so they run without root. No production code changes. Fixes NVIDIA#1409 Signed-off-by: Max Freedom Pollard <272618364+MaxFreedomPollard@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1409. Test-only, no production code changes.
I saw in #1408 that this follow-up was offered to @100milliongold. If you already have it in progress, I am happy to close this one.
The three gaps from the issue:
mount_casenow checks the cleanup's exit status. An early exit leaves every payload in place, which is what the assertions look for, so it used to pass.[, backslash,*,?and a trailing newline, each next to the sibling its mishandled name would match (data1next todata[1],datax1next todata*1). Each asserts both halves: the payload under the mount survives and the sibling is cleaned up. They script the mount table through anawkstub that prints the mount point in mountinfo's octal-escaped form, so they run without root. They are skipped when/proc/self/mountinfois not readable, because the script checks for it before callingawk.Testing
Run in an Ubuntu 24.04 container: 17 passed as an unprivileged user (mount-boundary cases skipped), 21 passed as root with
--privileged. ShellCheck reports nothing before or after.To check that the new assertions fail when they should, I ran the old and new harness against mutated copies of the template:
pattern=$mount_point, no escaping[and backslash fail (payload gone),*and?fail (sibling spared)mount_point=$(printf '%b' "$field")exit 17afterset -exrm -frreplaced by: