WIP: [Storage] Automate testing of predictable PVC/DV names after VM restore#4780
Open
acinko-rh wants to merge 1 commit intoRedHatQE:mainfrom
Open
Conversation
Signed-off-by: Adam Cinko <acinko@redhat.com>
Contributor
📝 WalkthroughWalkthroughThis PR adds test coverage for KubeVirt restore naming behavior. A new context manager in ChangesVM Restore Naming Validation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
Contributor
|
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
Contributor
Author
|
/wip |
openshift-virtualization-qe-bot-4
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+1
to
+5
| """ | ||
| Test for predictable DataVolume and PersistentVolumeClaim names when restoring a VM. | ||
|
|
||
| Jira: https://redhat.atlassian.net/browse/CNV-80304 | ||
| """ |
Contributor
There was a problem hiding this comment.
HIGH: PR description is missing required template sections.
Per the PR template guidelines, the following required sections must be present with meaningful content:
##### What this PR does / why we need it:(must have meaningful content, not just the description field in the PR body)##### Which issue(s) this PR fixes:##### Special notes for reviewer:##### jira-ticket:
Please restore the missing PR template sections from .github/pull_request_template.md.
🤖 Prompt for 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.
In `@tests/storage/test_predictible_names.py` around lines 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.
Comment on lines
+59
to
+101
| with vm_restore_with_prefix_policy( | ||
| name=f"{restored_vm_name}-restore", | ||
| namespace=namespace.name, | ||
| vm_name=restored_vm_name, | ||
| snapshot_name=snapshot.name, | ||
| client=admin_client, | ||
| prefix_policy=VOLUME_RESTORE_POLICY, | ||
| dry_run=False, | ||
| ) as restore: | ||
| restore.wait_restore_done(timeout=TIMEOUT_10MIN) | ||
|
|
||
| restored_dvs = list( | ||
| DataVolume.get( | ||
| dyn_client=admin_client, | ||
| namespace=namespace.name, | ||
| label_selector=f"restore.kubevirt.io/source-vm-name={source_vm_name}", | ||
| ) | ||
| ) | ||
| restored_pvcs = list( | ||
| PersistentVolumeClaim.get( | ||
| dyn_client=admin_client, | ||
| namespace=namespace.name, | ||
| label_selector=f"restore.kubevirt.io/source-vm-name={source_vm_name}", | ||
| ) | ||
| ) | ||
|
|
||
| assert restored_dvs, ( | ||
| f"No DataVolumes found with label restore.kubevirt.io/source-vm-name={source_vm_name}" | ||
| ) | ||
| assert restored_pvcs, f"No PVCs found with label restore.kubevirt.io/source-vm-name={source_vm_name}" | ||
|
|
||
| restored_dv_name = restored_dvs[0].name | ||
| restored_pvc_name = restored_pvcs[0].name | ||
|
|
||
| yield { | ||
| "source_dv_name": source_dv_name, | ||
| "source_pvc_name": source_dv_name, | ||
| "source_vm_name": vm.name, | ||
| "restored_vm_name": restored_vm_name, | ||
| "restored_dv_name": restored_dv_name, | ||
| "restored_pvc_name": restored_pvc_name, | ||
| "volumeRestorePolicy": restore.instance.spec.volumeRestorePolicy, | ||
| } |
Contributor
There was a problem hiding this comment.
MAJOR: Restored VM and its DVs/PVCs are never cleaned up by the fixture.
When with vm_restore_with_prefix_policy(...) exits, only the VirtualMachineRestore object is deleted. The restore itself creates a new VM named restored_vm_name along with new DataVolumes and PVCs that are not owned by the restore object and are not deleted by any context manager in the fixture. If the namespace fixture is module- or session-scoped, these resources leak across tests and could cause quota exhaustion or name collisions.
The fixture should clean up the restored VM (which will cascade-delete its owned DVs/PVCs). For example:
with vm_restore_with_prefix_policy(...) as restore:
restore.wait_restore_done(timeout=TIMEOUT_10MIN)
# ... collect names ...
yield {
...
}
# Teardown: delete the restored VM so its DVs/PVCs are removed
restored_vm = VirtualMachineForTests(
name=restored_vm_name,
namespace=namespace.name,
client=admin_client,
)
if restored_vm.exists:
restored_vm.delete(wait=True)Alternatively, wrap the restore in a VirtualMachineForTests context for restored_vm_name if the restore is guaranteed to create that VM.
🤖 Prompt for 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.
In `@tests/storage/test_predictible_names.py` around lines 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.
Comment on lines
+113
to
+115
| @pytest.mark.polarion("CNV-80304") | ||
| @pytest.mark.usefixtures("vm_snapshot_restore_dicts_scope_function") | ||
| def test_restored_dv_and_pvc_names_have_vm_prefix_default_names(self, vm_snapshot_restore_dicts_scope_function): |
Contributor
There was a problem hiding this comment.
HIGH: Remove redundant @pytest.mark.usefixtures — fixture return value is actively used.
Per the guideline: "Always use @pytest.mark.usefixtures when fixture return value is not used by the test." The return value here is used extensively (lines 139–144). Having both the decorator and the parameter is a violation. Remove the decorator.
🔧 Proposed fix
`@pytest.mark.polarion`("CNV-80304")
- `@pytest.mark.usefixtures`("vm_snapshot_restore_dicts_scope_function")
def test_restored_dv_and_pvc_names_have_vm_prefix_default_names(self, vm_snapshot_restore_dicts_scope_function):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @pytest.mark.polarion("CNV-80304") | |
| @pytest.mark.usefixtures("vm_snapshot_restore_dicts_scope_function") | |
| def test_restored_dv_and_pvc_names_have_vm_prefix_default_names(self, vm_snapshot_restore_dicts_scope_function): | |
| `@pytest.mark.polarion`("CNV-80304") | |
| def test_restored_dv_and_pvc_names_have_vm_prefix_default_names(self, vm_snapshot_restore_dicts_scope_function): |
🤖 Prompt for 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.
In `@tests/storage/test_predictible_names.py` around lines 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.
Comment on lines
+139
to
+159
| source_dv_name = vm_snapshot_restore_dicts_scope_function["source_dv_name"] | ||
| source_pvc_name = vm_snapshot_restore_dicts_scope_function["source_pvc_name"] | ||
| restored_vm_name = vm_snapshot_restore_dicts_scope_function["restored_vm_name"] | ||
| restored_dv_name = vm_snapshot_restore_dicts_scope_function["restored_dv_name"] | ||
| restored_pvc_name = vm_snapshot_restore_dicts_scope_function["restored_pvc_name"] | ||
| volume_restore_policy = vm_snapshot_restore_dicts_scope_function["volumeRestorePolicy"] | ||
|
|
||
| expected_restored_dv_name = f"{restored_vm_name}-{source_dv_name}"[:63] | ||
| expected_restored_pvc_name = f"{restored_vm_name}-{source_pvc_name}"[:63] | ||
|
|
||
| assert volume_restore_policy == VOLUME_RESTORE_POLICY, ( | ||
| f"volumeRestorePolicy is '{volume_restore_policy}', expected '{VOLUME_RESTORE_POLICY}'" | ||
| ) | ||
|
|
||
| assert restored_dv_name == expected_restored_dv_name, ( | ||
| f"Restored DV name is '{restored_dv_name}', expected '{expected_restored_dv_name}'" | ||
| ) | ||
|
|
||
| assert restored_pvc_name == expected_restored_pvc_name, ( | ||
| f"Restored PVC name is '{restored_pvc_name}', expected '{expected_restored_pvc_name}'" | ||
| ) |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Duplicate assertion logic — extract a shared helper.
Lines 139–159 and 211–231 are identical. If the truncation logic or policy check ever changes, both blocks must be updated in sync. Extract a private helper:
♻️ Suggested refactor
`@staticmethod`
def _assert_predictable_names(restore_dicts: dict) -> None:
source_dv_name = restore_dicts["source_dv_name"]
source_pvc_name = restore_dicts["source_pvc_name"]
restored_vm_name = restore_dicts["restored_vm_name"]
restored_dv_name = restore_dicts["restored_dv_name"]
restored_pvc_name = restore_dicts["restored_pvc_name"]
volume_restore_policy = restore_dicts["volumeRestorePolicy"]
expected_restored_dv_name = f"{restored_vm_name}-{source_dv_name}"[:63]
expected_restored_pvc_name = f"{restored_vm_name}-{source_pvc_name}"[:63]
assert volume_restore_policy == VOLUME_RESTORE_POLICY, (
f"volumeRestorePolicy is '{volume_restore_policy}', expected '{VOLUME_RESTORE_POLICY}'"
)
assert restored_dv_name == expected_restored_dv_name, (
f"Restored DV name is '{restored_dv_name}', expected '{expected_restored_dv_name}'"
)
assert restored_pvc_name == expected_restored_pvc_name, (
f"Restored PVC name is '{restored_pvc_name}', expected '{expected_restored_pvc_name}'"
)Then each test becomes a single call:
def test_restored_dv_and_pvc_names_have_vm_prefix_default_names(self, vm_snapshot_restore_dicts_scope_function):
self._assert_predictable_names(restore_dicts=vm_snapshot_restore_dicts_scope_function)🤖 Prompt for 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.
In `@tests/storage/test_predictible_names.py` around lines 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).
| f"Restored PVC name is '{restored_pvc_name}', expected '{expected_restored_pvc_name}'" | ||
| ) | ||
|
|
||
| @pytest.mark.polarion("CNV-80304b") |
Contributor
There was a problem hiding this comment.
HIGH: CNV-80304b is not a valid Polarion ID.
Polarion test IDs follow the CNV-NNNNN format (all-digit suffix). CNV-80304b with a letter suffix won't map to a real Jira/Polarion ticket and will break result reporting. Either reuse the existing CNV-80304 ID (if this is a sub-test of the same scenario) or obtain a new dedicated Polarion ID for this parametrized test.
🤖 Prompt for 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.
In `@tests/storage/test_predictible_names.py` at 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.
Comment on lines
+548
to
+552
| client: DynamicClient, | ||
| prefix_policy: str, | ||
| dry_run: bool, | ||
| **kwargs, | ||
| ) -> Generator[VirtualMachineRestore]: |
Contributor
There was a problem hiding this comment.
HIGH: Fix FBT001 (boolean positional arg) and ANN003 (untyped **kwargs) — the linter must be satisfied per repo guidelines.
The coding guidelines prohibit # noqa suppressions and require fixing the code directly. Both dry_run: bool as a positional arg and the untyped **kwargs violate active Ruff rules. Additionally, Generator[VirtualMachineRestore] provides only one of the three required type parameters for collections.abc.Generator.
🔧 Proposed fix
+from typing import Any
...
`@contextmanager`
def vm_restore_with_prefix_policy(
name: str,
namespace: str,
vm_name: str,
snapshot_name: str,
client: DynamicClient,
prefix_policy: str,
- dry_run: bool,
- **kwargs,
-) -> Generator[VirtualMachineRestore]:
+ *,
+ dry_run: bool = False,
+ **kwargs: Any,
+) -> Generator[VirtualMachineRestore, None, None]:The * separator makes dry_run keyword-only (FBT001), adding = False makes it optional since False is the only value callers pass. **kwargs: Any addresses ANN003. Generator[VirtualMachineRestore, None, None] provides the required YieldType, SendType, ReturnType params.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| client: DynamicClient, | |
| prefix_policy: str, | |
| dry_run: bool, | |
| **kwargs, | |
| ) -> Generator[VirtualMachineRestore]: | |
| client: DynamicClient, | |
| prefix_policy: str, | |
| *, | |
| dry_run: bool = False, | |
| **kwargs: Any, | |
| ) -> Generator[VirtualMachineRestore, None, None]: |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 550-550: Boolean-typed positional argument in function definition
(FBT001)
[warning] 551-551: Missing type annotation for **kwargs
(ANN003)
🤖 Prompt for 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.
In `@tests/storage/utils.py` around lines 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.
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
D/S test |
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
5 participants
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.
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:
https://docs.google.com/document/d/10lFE1K9Cgpnjh0V_VqQ-ARaTEjccU9jRjhV4KMdLm2U/edit?tab=t.0
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
Co-authored: Claude Code
https://redhat.atlassian.net/browse/CNV-80304
jira-ticket:
Summary by CodeRabbit