tests: replace hardcoded test pool names with unique checksummed names#995
tests: replace hardcoded test pool names with unique checksummed names#995tasleson wants to merge 1 commit into
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughThis pull request introduces deterministic, collision-resistant test naming utilities by adding checksum-based name generation and validation functions. Hardcoded test pool and volume group identifiers across the test suite are replaced with generated names using shared constants, and the emergency cleanup script is updated to validate and target these dynamically-named resources. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/bin/snapm-test-name (1)
15-24: Reject invalidsubsystemandcontextvalues at generation time.This CLI currently accepts arbitrary strings, so it can emit names with spaces, slashes, or overlong segments that LVM/Stratis cannot actually create. Failing fast here keeps the new manual workflow reliable.
Suggested hardening
_generate() { local subsystem="$1" local context="$2" + [[ "$subsystem" =~ ^(lm|st)$ ]] || { + printf 'Invalid subsystem: %s\n' "$subsystem" >&2 + return 2 + } + [[ "$context" =~ ^[a-z0-9]+$ ]] || { + printf 'Invalid context: %s\n' "$context" >&2 + return 2 + } local random_hex random_hex=$(od -An -tx1 -N4 /dev/urandom | tr -d ' \n')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/bin/snapm-test-name` around lines 15 - 24, In _generate(), validate the incoming subsystem and context variables before building the name: ensure they are non-empty, match a safe charset (e.g., allow only [A-Za-z0-9._-] and reject spaces/slashes/other punctuation), and enforce a maximum segment length (pick a sane limit like 32 characters) to prevent overly long LVM/Stratis names; if validation fails, print a clear error to stderr and exit or return a non-zero status instead of emitting a name. Use the existing symbols subsystem, context, and the _generate function to locate where to add these checks and the error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/bin/cleanup.sh`:
- Around line 25-39: The _validate_snapm_name() function currently only checks
the trailing separator and 4-char checksum; tighten it to validate the full
advertised format by matching the name against the pattern
snapm_<subsystem>_<context>_<8hex>_<4hex> (validate subsystem/context token
structure and that the two hex fields are the correct lengths and hex
characters), then compute/verify the trailing 4-char checksum as now; update the
validators in tests/_util.py and tests/bin/snapm-test-name to use the same
strict regex and checksum logic so the ownership check used before
vgremove/stratis destroy only accepts properly formatted test-owned names.
- Around line 41-49: The cleanup loop currently sets LVM_SYSTEM_DIR from the
caller's $PWD which breaks when the script is run outside the repo; instead
determine the repo root relative to the script and set LVM_SYSTEM_DIR from that.
In tests/bin/cleanup.sh compute the script directory using ${BASH_SOURCE[0]}
(e.g., resolve dirname "${BASH_SOURCE[0]}" and cd to its parent to get the repo
root) and export LVM_SYSTEM_DIR="$REPO_ROOT/tests/lvm" before calling vgremove
inside the for-loop (symbols to update: the LVM_SYSTEM_DIR export in the
vgremove block, and ensure this resolution happens prior to using
_validate_snapm_name / vgremove).
---
Nitpick comments:
In `@tests/bin/snapm-test-name`:
- Around line 15-24: In _generate(), validate the incoming subsystem and context
variables before building the name: ensure they are non-empty, match a safe
charset (e.g., allow only [A-Za-z0-9._-] and reject spaces/slashes/other
punctuation), and enforce a maximum segment length (pick a sane limit like 32
characters) to prevent overly long LVM/Stratis names; if validation fails, print
a clear error to stderr and exit or return a non-zero status instead of emitting
a name. Use the existing symbols subsystem, context, and the _generate function
to locate where to add these checks and the error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fb3ff91-40ed-4964-8cd6-6cfa9b3d0bb3
📒 Files selected for processing (8)
tests/_util.pytests/bin/cleanup.shtests/bin/snapm-test-nametests/test_boot.pytests/test_lvm2.pytests/test_manager.pytests/test_mounts.pytests/test_stratis.py
| _validate_snapm_name() { | ||
| local name="$1" | ||
| if [ ${#name} -lt 6 ]; then | ||
| return 1 | ||
| fi | ||
| local sep="${name: -5:1}" | ||
| if [ "$sep" != "_" ]; then | ||
| return 1 | ||
| fi | ||
| local prefix="${name:0:${#name}-5}" | ||
| local embedded="${name: -4}" | ||
| local computed | ||
| computed=$(printf '%s' "$prefix" | sha256sum | cut -c1-4) | ||
| [ "$embedded" = "$computed" ] | ||
| } |
There was a problem hiding this comment.
Make the ownership check match the advertised name format.
_validate_snapm_name() only verifies the trailing separator and checksum. Any snapm_lm_* / snapm_st_* name with a matching 4-character suffix is therefore treated as test-owned, even if it does not follow snapm_<subsystem>_<context>_<8hex>_<4hex>. That is too loose for the destructive vgremove / stratis ... destroy calls below.
Suggested tightening
_validate_snapm_name() {
local name="$1"
- if [ ${`#name`} -lt 6 ]; then
- return 1
- fi
- local sep="${name: -5:1}"
- if [ "$sep" != "_" ]; then
- return 1
- fi
+ [[ "$name" =~ ^snapm_[a-z0-9]+_[a-z0-9]+_[0-9a-f]{8}_[0-9a-f]{4}$ ]] || return 1
local prefix="${name:0:${`#name`}-5}"
local embedded="${name: -4}"
local computed
computed=$(printf '%s' "$prefix" | sha256sum | cut -c1-4)
[ "$embedded" = "$computed" ]
}Please keep the equivalent validators in tests/_util.py and tests/bin/snapm-test-name aligned with this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/bin/cleanup.sh` around lines 25 - 39, The _validate_snapm_name()
function currently only checks the trailing separator and 4-char checksum;
tighten it to validate the full advertised format by matching the name against
the pattern snapm_<subsystem>_<context>_<8hex>_<4hex> (validate
subsystem/context token structure and that the two hex fields are the correct
lengths and hex characters), then compute/verify the trailing 4-char checksum as
now; update the validators in tests/_util.py and tests/bin/snapm-test-name to
use the same strict regex and checksum logic so the ownership check used before
vgremove/stratis destroy only accepts properly formatted test-owned names.
There was a problem hiding this comment.
What we're trying to avoid is destroying something that the test didn't create. A human would have to create a name, then take the shasum of the same, grab the last 4 characters of the shasum and append it onto the end. I don't think any human would normally go through such a thing.
There was a problem hiding this comment.
@tasleson, that's a fair point — the checksum is already a strong enough ownership signal. The probability of a coincidentally matching 4-character SHA-256 suffix on a non-test name is ~1 in 65 536, and as you note, deliberately constructing such a name would require intentionally running sha256sum on the prefix and appending the result — not something that happens by accident. The existing validation is sufficient for its purpose of guarding against accidental destruction of non-test resources. I'll withdraw my suggestion.
( ˶ˆ꒳ˆ˵ )🐇
🧠 Learnings used
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-26T19:07:42.290Z
Learning: In the snapm virt_tests module, verify_lvm_setup() is intended as a lightweight sanity check for essential volumes, not comprehensive validation for every storage layout. The responsibility for testing new layout kickstart fragments lies with the developer submitting them. Adding extensive validation for each layout would be redundant and create clutter.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-27T21:01:32.324Z
Learning: In the snapm project, the design philosophy is to fail fast when expected files (like systemd units or tmpfiles.d configuration) don't exist during installation steps. This is intentional behavior - if these files are missing, it indicates a fundamental issue with the build/test setup that should abort the process immediately rather than continue with an incomplete installation.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T19:32:44.315Z
Learning: In systemd/tmpfiles.d configuration for snapm: use lowercase 'z' (non-recursive SELinux context restoration) instead of uppercase 'Z' (recursive) for /run/snapm and its subdirectories. Recursive context application is hazardous because /run/snapm/mounts contains mounted snapshot filesystems that have their own SELinux contexts which should not be modified. If a user runs systemd-tmpfiles --create, recursive relabeling would incorrectly change contexts on mounted content.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-10-22T16:43:42.894Z
Learning: In snapm/manager/_manager.py _check_snapm_dir: the correct order for directory validation is: 1) check if path exists and validate it's a directory (not a symlink), 2) os.makedirs() to create if needed, 3) os.chmod() to fix permissions if directory existed with wrong perms, 4) validate final mode matches expected. Do not attempt chmod after makedirs for a newly created directory since makedirs(mode=...) already sets permissions. Do not log errors when chmod fails but the mode is already correct.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 759
File: systemd/tmpfiles.d/snapm.conf:9-9
Timestamp: 2025-12-17T16:23:50.413Z
Learning: In systemd/tmpfiles.d configuration for snapm: use uppercase 'Z' (recursive SELinux context restoration) for /var/cache/snapm because it is a persistent cache directory tree (including subdirectories like /var/cache/snapm/diffcache) that should have proper labeling applied recursively. Unlike /run/snapm/mounts which contains mounted snapshot filesystems, /var/cache/snapm stores cache files and needs recursive context application.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-08-25T17:19:19.563Z
Learning: The man page changes mentioned in the AI summary for PR `#383` were rebasing artifacts and should be ignored when reviewing this branch - there are no actual man page changes on the bmr-vm-tests branch.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2026-01-23T20:10:43.866Z
Learning: PR `#954` in snapshotmanager/snapm removes a redundant `dest_path in used_dests` check at line 1236 of snapm/fsdiff/engine.py. This check became dead code after commit 77f1a3d2 which filters candidates in `used_dests` before the check runs. This is purely code cleanup/optimization with no functional changes, not a bug fix or enhancement.
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 0
File: :0-0
Timestamp: 2025-09-14T02:14:30.316Z
Learning: PR `#478` in snapshotmanager/snapm fixes a regression in the LVM2 plugin where `can_snapshot()` method throws `SnapmCalloutError` when probing non-device-mapper devices. The fix adds device major number validation to prevent calling dmsetup on non-dm devices, allowing proper "no provider found" error messages instead of confusing dmsetup errors.
Test resources previously used generic names (test_vg0, pool0, pool1) that could theoretically collide with real resources and made it impossible to verify ownership before cleanup. Replace with dynamically generated names in the format: snapm_<subsystem>_<context>_<8 random hex>_<4 hex SHA-256 checksum> The embedded checksum allows the cleanup script to validate that a resource was created by the test suite before destroying it. A standalone bash CLI tool (snapm-test-name) is provided for developers to manually generate and validate names. Resolves: snapshotmanager#978 Assisted-by: Claude <noreply@anthropic.com>
tasleson
force-pushed
the
test_asset_names
branch
from
789c9a1 to
d9ad1c2
Compare
1 participant
Test resources previously used generic names (test_vg0, pool0, pool1)
that could theoretically collide with real resources and made it
impossible to verify ownership before cleanup.
Replace with dynamically generated names in the format:
snapm_<8 random hex>_<4 hex SHA-256 checksum>
The embedded checksum allows the cleanup script to validate that a
resource was created by the test suite before destroying it. A
standalone bash CLI tool (snapm-test-name) is provided for developers
to manually generate and validate names.
Resolves: #978
Note: This may all seem like overkill, but this is one of those "Low-probability, high-consequence events"
Summary by CodeRabbit