[Storage] Automate testing of predictable PVC/DV names after VM restore#4780
[Storage] Automate testing of predictable PVC/DV names after VM restore#4780acinko-rh wants to merge 83 commits into
Conversation
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
📝 WalkthroughWalkthroughAdds fixtures and a parametrized test that create a VirtualMachineRestore with volume_restore_policy "PrefixTargetName", wait for completion, and assert the restored first entry's dataVolumeName and persistentVolumeClaim equal the deterministic name built from "<vm_name>-<source_volume_name>" truncated to 63 chars. ChangesVM Restore Naming Validation
🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: Suggested reviewers:
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot-2
requested review from
ema-aka-young,
kgoldbla and
kshvaika
|
/wip |
openshift-virtualization-qe-bot-4
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/storage/test_predictible_names.py`:
- Around line 59-101: The fixture using vm_restore_with_prefix_policy currently
yields without deleting the created restored VM (restored_vm_name), so
DataVolumes/PVCs leak; after yielding, locate the created VM (use
VirtualMachineForTests(name=restored_vm_name, namespace=namespace.name,
client=admin_client) or check via the API) and delete it (if exists, call
delete(wait=True)) so that its owned DVs/PVCs are cascade-removed; alternatively
wrap the restored VM in a VirtualMachineForTests context manager for teardown to
ensure cleanup.
- Line 161: The Polarion ID used in the pytest marker is invalid ("CNV-80304b");
update the pytest.mark.polarion decorator in
tests/storage/test_predictible_names.py (the pytest-marked test using
`@pytest.mark.polarion`) to use a valid CNV-NNNNN format—either change
"CNV-80304b" to the numeric "CNV-80304" if appropriate or replace it with a
newly issued valid Polarion ID so test reporting will map correctly.
- Around line 1-5: The PR is missing required template sections per
.github/pull_request_template.md; update the pull request description to include
the sections "##### What this PR does / why we need it:", "##### Which issue(s)
this PR fixes:", "##### Special notes for reviewer:", and "##### jira-ticket:"
with meaningful content (not just duplicating the brief description) so
reviewers can see motivation, linked issues (e.g., CNV-80304), any reviewer
instructions, and the jira-ticket; reference the test file name
tests/storage/test_predictible_names.py in the "What this PR does" section to
explain the change and why it’s needed.
- Around line 139-159: Extract the duplicated assertion block into a private
helper (e.g., a `@staticmethod` named _assert_predictable_names) that accepts the
restore dict (vm_snapshot_restore_dicts_scope_function), reads
source_dv_name/source_pvc_name/restored_vm_name/restored_dv_name/restored_pvc_name/volumeRestorePolicy,
computes expected_restored_dv_name and expected_restored_pvc_name using
f"{restored_vm_name}-{source_*}"[:63], and asserts volume_restore_policy ==
VOLUME_RESTORE_POLICY and the two restored name equality checks; then replace
the two identical inline blocks (the ones using source_dv_name, source_pvc_name,
restored_vm_name, restored_dv_name, restored_pvc_name, volumeRestorePolicy) with
calls to
self._assert_predictable_names(vm_snapshot_restore_dicts_scope_function).
- Around line 113-115: The test function
test_restored_dv_and_pvc_names_have_vm_prefix_default_names currently both
declares the fixture vm_snapshot_restore_dicts_scope_function as a function
parameter and also decorates the test with
`@pytest.mark.usefixtures`("vm_snapshot_restore_dicts_scope_function"); remove the
redundant decorator
`@pytest.mark.usefixtures`("vm_snapshot_restore_dicts_scope_function") so the
fixture's return value (accessed inside the test) is provided via the function
parameter only and the test continues to use
vm_snapshot_restore_dicts_scope_function within lines ~139–144.
In `@tests/storage/utils.py`:
- Around line 548-552: The function signature currently exposes dry_run as a
positional boolean, leaves **kwargs untyped, and uses an incomplete Generator
type; update the signature to make dry_run keyword-only and optional by adding a
'*' before it and setting dry_run: bool = False, change **kwargs to **kwargs:
Any, and update the return annotation to Generator[VirtualMachineRestore, None,
None]; also ensure Any is imported from typing (or typing_extensions) if not
already.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84bf76c2-6c1a-4b27-bd09-212c6fcd4010
📒 Files selected for processing (3)
tests/storage/constants.pytests/storage/test_predictible_names.pytests/storage/utils.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4780 published |
|
/verified |
|
Clean rebase detected — no code changes compared to previous head ( |
|
D/S test |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
REPO="RedHatQE/openshift-virtualization-tests"
PR_NUMBER=4780
echo "=== PR head SHA ==="
gh api repos/$REPO/pulls/$PR_NUMBER --jq '{head_sha: .head.sha, state: .state}'
echo ""
echo "=== Files changed ==="
gh api repos/$REPO/pulls/$PR_NUMBER/files --jq '.[].filename'
echo ""
echo "=== Patch for first changed file (to get valid position) ==="
gh api repos/$REPO/pulls/$PR_NUMBER/files --jq '.[0] | {filename: .filename, patch: .patch}'
echo ""
echo "=== Smoke markers ==="
rg -rl "`@pytest.mark.smoke`" tests/ 2>/dev/null | head -20 || echo "none"
echo ""
echo "=== Gating markers ==="
rg -rl "`@pytest.mark.gating`" tests/ 2>/dev/null | head -20 || echo "none"
echo ""
echo "=== Does tests/storage/utils.py change touch utilities? ==="
gh api repos/$REPO/pulls/$PR_NUMBER/files --jq '.[] | select(.filename == "tests/storage/utils.py") | .patch' | head -30Length of output: 5273 🏁 Script executed: #!/bin/bash
REPO="RedHatQE/openshift-virtualization-tests"
PR_NUMBER=4780
HEAD_SHA="c39867604e0f40ab9747a3edf1ed427dc3360200"
BODY="**Test Execution Plan**
- **Run smoke tests: False** — Verified: smoke-marked files are in \`tests/virt/cluster/\`, \`tests/virt/node/\`, \`tests/storage/cdi_clone/\`, \`tests/storage/cdi_upload/\`, \`tests/install_upgrade_operators/\`, and \`tests/infrastructure/instance_types/\`. None import from \`tests/storage/snapshots/\` or depend on the new fixtures.
- **Run gating tests: False** — Verified: all gating-marked tests are in \`tests/virt/\`. None import from \`tests/storage/snapshots/\` or the changed conftest.
**Affected tests to run:**
- \`tests/storage/snapshots/test_snapshots.py::TestRestoreSnapshots::test_restore_snapshot_with_predictable_names\` — new test verifying predictable DV/PVC names after VM restore using \`volumeRestorePolicy=PrefixTargetName\`"
gh api repos/$REPO/pulls/$PR_NUMBER/reviews \
--method POST \
--field commit_id="$HEAD_SHA" \
--field body="" \
--field event="COMMENT" \
--field "comments[][path]=tests/storage/snapshots/conftest.py" \
--field "comments[][position]=1" \
--field "comments[][body]=$BODY"Length of output: 2981 |
|
Clean rebase detected — no code changes compared to previous head ( |
|
D/S test |
Signed-off-by: Adam Cinko <acinko@redhat.com>
|
Clean rebase detected — no code changes compared to previous head ( |
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4780 published |
|
/approve |
|
/verified |
|
Clean rebase detected — no code changes compared to previous head ( |
|
/verified |
|
/retest tox |
|
/check-can-merge |
|
/retest cnv-smoke |
|
/lgtm |
|
No cnv-smoke/validation configured for this repository |
|
/lgtm |
20 participants
Short description:
There's a feature that let's the user chose they want to restore a VM with predictable DV/PVC name derived from the vm-name from the snapshot.
More details:
When restoring a VM Snapshot, you can set
volumeRestorePolicywithPrefixTargetNameto give the restored DV/PVC have a name based on the VM name from the snapshot.Which issue(s) this PR fixes:
https://redhat.atlassian.net/browse/CNV-80304
Special notes for reviewer:
Co-authored: Claude Code
Depends of wrapper PR: RedHatQE/openshift-python-wrapper#2726
jira-ticket:
Summary by CodeRabbit