Repository navigation
fix(performance): pin the headroom rejection message numerically - #1049
Merged
Merged
Conversation
isMountPointSuitable rejects an unsuitable mount entry at mount_linux.go:37,
before it reaches the free-space branch at mount_linux.go:50. The headroom test
called it with t.TempDir() and asserted the reason contains "insufficient
space", so on a CI runner whose temporary directory is backed by tmpfs the
tmpfs rejection returned first and the assertion failed:
"filesystem type is tmpfs (device not mounted as a real filesystem)"
does not contain "insufficient space"
The outcome depended on which runner the job landed on rather than on the code
under test, which made Test Backend fail intermittently on unrelated pull
requests.
Probe the directory at a trivial required size first and skip when it is not a
benchmark target, matching requireBenchmarkableDir in resolve_target_test.go.
The headroom assertion still runs, and still fails, on a runner with a real
filesystem.
Test-only change. The collector's tmpfs rejection is deliberate and unchanged.
Fixes #1038
The headroom test called isMountPointSuitable with t.TempDir() and asserted the rejection contains "insufficient space". The mount entry checks reject a tmpfs at mount_linux.go:37, before the space check at mount_linux.go:50, and CI runners put the temporary directory on tmpfs, so the assertion failed on the runners that do and passed on the ones that do not. Skipping on an unsuitable directory stops the failure but leaves nothing covering the rule: ubuntu-latest has /tmp on tmpfs, so the assertion would not execute in CI at all, and "insufficient space" and "headroom" appear nowhere else under collector/. Extract the rule into spaceRejection(availableBytes, requiredBytes), which isMountPointSuitable now calls. The behavior is unchanged, including the message text, and the rule is testable without a filesystem of any particular type, so TestSpaceRejection runs on every runner. It pins the boundary at exactly enough and one byte short, and asserts the required figure numerically: the message has to name the test file plus the headroom, not the test file alone, or it implies the free space is the whole budget. The integration test stays as a check that isMountPointSuitable still consults the rule, and skips only when the directory is rejected for a reason other than space. An "insufficient space" rejection means the space check ran, so it is no longer a reason to skip: on a nearly-full real filesystem the probe at one byte fails against the 64 MiB headroom, and the earlier version skipped there even though the assertion would have held. Fixes #1038
…test-tmpfs # Conflicts: # collector/pkg/performance/mount_linux_test.go
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.
Summary
Pins the
spaceRejectionmessage numerically, so the required figure has to be the test file plus the headroom rather than the test file alone.Type of Change
Related Issues
Follow-up to #1040, which merged at
54fdbf7ewhile this change was still under review atb107b38c. Related to #1038.Reason
A review of #1040 found that
TestSpaceRejectionasserted onlyContains "insufficient space",Contains "headroom", andContains "1024 bytes available". That leaves one mutation alive: change the second argument atcollector/pkg/performance/mount_linux.go:69fromneededBytesback torequiredBytes, or the third fromuint64(spaceHeadroomBytes)to anything else, and every assertion still passes.The consequence is the defect the test exists to prevent. An operator reading
1152921504606846976 bytes requiredwhen the real requirement is that plus 64 MiB concludes the free space is the whole budget, which is exactly why the message names the headroom separately.Changes Made
collector/pkg/performance/mount_linux_test.go: assert the required figure asrequested+headroomand the headroom asplus %d bytes headroom, both computed rather than matched as loose substrings.No change to
mount_linux.go;spaceRejectionand its message are untouched.Testing
mount_linux_test.gois Linux-only by filename and this host is macOS with no container runtime, so the tests could not be executed locally. CI is the execution check.TestSpaceRejectiontouches no filesystem, so it runs rather than skipping.Checklist
Additional Notes
Review: not required (test-only change; no runtime code, no security or access boundary, trivially revertible).
Known and deliberately out of scope:
requiredBytes + spaceHeadroomBytesis uint64 arithmetic with no overflow guard, so aperformance.temp_file_sizeat or nearMaxUint64wraps andspaceRejectionreturns no rejection for a file the mount point cannot hold. That predates #1040 and the extraction preserved it exactly; it deserves its own change rather than being folded into a test fix.