test(MTV-5663): add plan archive PVC cleanup regression test - #624
Open
myakove wants to merge 14 commits into
Open
test(MTV-5663): add plan archive PVC cleanup regression test#624myakove wants to merge 14 commits into
myakove wants to merge 14 commits into
Conversation
|
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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
Contributor
WalkthroughThe PR adds a regression test for PVC and DataVolume cleanup after a failed migration Plan is archived and deleted. It also adds teardown deregistration support, test configuration, and live-cluster execution guidance. ChangesPlan archive cleanup
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TestPlanArchivePvcCleanup
participant MigrationPlan
participant PostHook
participant TargetNamespace
TestPlanArchivePvcCleanup->>MigrationPlan: execute migration
MigrationPlan->>PostHook: run post-hook
PostHook-->>MigrationPlan: return failure
TestPlanArchivePvcCleanup->>MigrationPlan: archive and delete plan
TestPlanArchivePvcCleanup->>TargetNamespace: remove retained destination VMs
TestPlanArchivePvcCleanup->>TargetNamespace: poll PVC and DataVolume removal
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
PR Summary by QodoAdd regression test for plan archive PVC cleanup (MTV-5663)
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
![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: 3
🤖 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 `@AGENTS.md`:
- Around line 708-714: Make the example block self-contained by adding imports
for get_migration_for_plan, archive_plan, Plan, and Migration alongside
unregister_teardown_resource, or explicitly state that these symbols must
already be available before the example is copied.
In `@tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py`:
- Around line 237-245: Extend the test after validate_hook_failure_and_check_vms
and before any archive or deletion calls to assert that the failed migration
created the expected normal PVC, prime-* PVC, and DataVolume resources. Use the
existing resource-listing or validation helpers and
self.plan_resource/prepared_plan identifiers, preserving the final cleanup
assertions while ensuring the regression baseline is verified before archiving.
In `@utilities/resources.py`:
- Around line 87-90: Validate that kind is registered in
fixture_store["teardown"] before indexing it in the resource teardown function,
and raise a clear ValueError when absent. Preserve the existing ValueError
behavior for a registered kind whose resource name is not found.
🪄 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 Plus
Run ID: 20a9f82c-8c2c-4a58-913f-969c807f4ca4
📒 Files selected for processing (5)
AGENTS.mdtests/plan_lifecycle/__init__.pytests/plan_lifecycle/test_plan_archive_pvc_cleanup.pytests/tests_config/config.pyutilities/resources.py
Code Review by Qodo
1. VM cleanup not asserted
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Collaborator
Author
|
/qodo The following review comments were reviewed and a decision was made:
|
Both findings appear addressed:
No further action is needed for these review comments. |
rh-bot-1
added
commented-myakove
and removed
commented-qodo-code-review[bot]
changes-requested-coderabbitai[bot]
labels
|
Code review by qodo was updated up to the latest commit c7cba81 |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py (2)
302-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMEDIUM — Name the orphan-cleanup timing values.
The literals
120and5control timeout and polling behavior. Define module-level constants with units so future changes do not obscure the cleanup contract.Proposed refactor
+ORPHAN_RESOURCE_WAIT_TIMEOUT_SECONDS = 120 +ORPHAN_RESOURCE_POLL_INTERVAL_SECONDS = 5 + ... - wait_timeout=120, - sleep=5, + wait_timeout=ORPHAN_RESOURCE_WAIT_TIMEOUT_SECONDS, + sleep=ORPHAN_RESOURCE_POLL_INTERVAL_SECONDS,As per coding guidelines, replace unexplained numeric literals with named constants and explanatory comments.
🤖 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/plan_lifecycle/test_plan_archive_pvc_cleanup.py` around lines 302 - 304, Define module-level named constants with explicit time units for the orphan-cleanup timeout and polling interval, then replace the 120 and 5 arguments in the TimeoutSampler call within the orphan-cleanup flow with those constants. Add concise comments documenting their timing purpose and preserve the existing values and behavior.Source: Coding guidelines
208-208: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH — Preserve the optional
warm_migrationdefault.The shared
prepared_planfixture treatswarm_migrationas optional and defaults it toFalse. Direct indexing at Line 208 raisesKeyErrorwhen a cold-migration configuration omits the key, unless the fixture writes the normalized value back. Use.get("warm_migration", False)or verify that output contract.Proposed fix
- warm_migration=prepared_plan["warm_migration"], + warm_migration=prepared_plan.get("warm_migration", False),#!/bin/bash set -euo pipefail rg -n -C 6 'warm_migration|plan\["warm_migration"\]' \ conftest.py \ tests/tests_config/config.py \ tests/plan_lifecycle/test_plan_archive_pvc_cleanup.pyAs per coding guidelines and retrieved learnings,
warm_migrationis optional and must default toFalsewhen absent.🤖 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/plan_lifecycle/test_plan_archive_pvc_cleanup.py` at line 208, Update the plan setup at the warm_migration access to use the shared fixture’s optional semantics, defaulting to False when prepared_plan omits the key. Use prepared_plan.get("warm_migration", False) or ensure the fixture normalizes and writes the default back before this access, while preserving the existing behavior when the key is present.
♻️ Duplicate comments (1)
tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py (1)
237-245: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHIGH — Establish the failed-migration resource baseline.
The test does not prove that the failed migration created a regular PVC, a
prime-*PVC, and a DataVolume beforearchive_plan()runs. If migration fails too early, the later empty-namespace assertion can pass without exercising the MTV-5564/MTV-5663 regression.Add these assertions after hook validation and before archive or deletion:
Proposed baseline assertions
validate_hook_failure_and_check_vms(self.plan_resource, prepared_plan) + + failed_pvc_names = [ + pvc.name + for pvc in PersistentVolumeClaim.get( + client=ocp_admin_client, + namespace=target_namespace, + ) + ] + failed_dv_names = [ + data_volume.name + for data_volume in DataVolume.get( + client=ocp_admin_client, + namespace=target_namespace, + ) + ] + assert failed_dv_names, "Post-hook failure did not create a DataVolume" + assert any(not name.startswith("prime-") for name in failed_pvc_names) + assert any(name.startswith("prime-") for name in failed_pvc_names)🤖 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/plan_lifecycle/test_plan_archive_pvc_cleanup.py` around lines 237 - 245, After validate_hook_failure_and_check_vms(self.plan_resource, prepared_plan), establish the failed-migration baseline by asserting that the target namespace contains the expected regular PVC, a prime-* PVC, and a DataVolume before invoking archive_plan() or any cleanup/deletion. Use the existing resource helpers and identifiers from the test rather than relying only on the later empty-namespace assertion.
🤖 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.
Outside diff comments:
In `@tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py`:
- Around line 302-304: Define module-level named constants with explicit time
units for the orphan-cleanup timeout and polling interval, then replace the 120
and 5 arguments in the TimeoutSampler call within the orphan-cleanup flow with
those constants. Add concise comments documenting their timing purpose and
preserve the existing values and behavior.
- Line 208: Update the plan setup at the warm_migration access to use the shared
fixture’s optional semantics, defaulting to False when prepared_plan omits the
key. Use prepared_plan.get("warm_migration", False) or ensure the fixture
normalizes and writes the default back before this access, while preserving the
existing behavior when the key is present.
---
Duplicate comments:
In `@tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py`:
- Around line 237-245: After
validate_hook_failure_and_check_vms(self.plan_resource, prepared_plan),
establish the failed-migration baseline by asserting that the target namespace
contains the expected regular PVC, a prime-* PVC, and a DataVolume before
invoking archive_plan() or any cleanup/deletion. Use the existing resource
helpers and identifiers from the test rather than relying only on the later
empty-namespace assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5d37b117-224b-4586-8937-d3e0978ff63a
📒 Files selected for processing (2)
tests/plan_lifecycle/test_plan_archive_pvc_cleanup.pyutilities/resources.py
Collaborator
Author
|
@coderabbitai[bot] The following review comments were reviewed and a decision was made:
|
Contributor
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
redhat-qe-bot
added
commented-myakove
and removed
commented-coderabbitai[bot]
commented-myakove
labels
|
Code review by qodo was updated up to the latest commit 62fa630 |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
/retest all |
Regression test for MTV-5564: verifies that archiving and deleting a failed migration plan properly cleans up all PVCs (including prime-* PVCs) in the target namespace. Test flow: 1. Create StorageMap, NetworkMap, and Plan with failing post-hook 2. Execute migration (fails at PostHook, PVCs are created) 3. Archive and delete the failed plan 4. Delete retained VMs, verify no orphan DVs/PVCs remain Also adds: - unregister_teardown_resource() utility for mid-test resource deletion - Documents the utility in AGENTS.md Resource Creation section Jira: MTV-5663 Co-authored-by: Cursor <cursoragent@cursor.com>
- Use prepared_plan._vm_target_namespace in test_verify_pvc_cleanup for consistency with cleanup_migrated_vms fixture - Make unregister_teardown_resource safe/idempotent — warn instead of raising when kind/name not found Signed-off-by: Meni Yakove <myakove@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
- Add resource baseline assertion in test_migrate_vms to prove PVCs/DVs
exist before archive+delete (prevents vacuous pass)
- Name timeout magic numbers as module constants
- Use .get("warm_migration", False) for optional flag
- Make AGENTS.md example self-contained with import note
Signed-off-by: Meni Yakove <myakove@gmail.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
…straints - Use _vm_target_namespace in test_migrate_vms baseline PVC/DV check for consistency with the cleanup path in test_verify_pvc_cleanup - Add Test Execution Requirements section to AGENTS.md as guardrails after removing the test execution prohibition Co-authored-by: Cursor <cursoragent@cursor.com>
…docs - Clarify AGENTS.md parallel run isolation caveat (source-provider VM collisions not prevented by namespace isolation alone) - Document why unscoped PVC/DV listing is safe in baseline assertion (unique per-session namespace) and improve error message Signed-off-by: Meni Yakove <myakove@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Filter baseline PVC/DV assertion in test_migrate_vms by fixture_store["session_uuid"] to avoid false positives when vm_target_namespace is shared across test runs. Co-authored-by: Cursor <cursoragent@cursor.com>
- Move _get_orphan_resource_names to utilities/migration_utils.py as public get_orphan_resource_names() per AGENTS.md placement rules - Align parallel execution section with source-provider caveat Signed-off-by: Meni Yakove <myakove@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Filter get_orphan_resource_names() results by session_uuid in test_verify_pvc_cleanup to avoid false failures when vm_target_namespace contains unrelated resources. Signed-off-by: Meni Yakove <myakove@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: Meni Yakove <myakove@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Move session_uuid scoping into the utility function via a partial_name parameter, making the filtering visible at the call site. Resolves Qodo sticky finding about unscoped orphan check. Co-authored-by: Cursor <cursoragent@cursor.com>
- Return success if final orphan query is empty on TimeoutExpiredError (cleanup can complete between last poll and final check) - Update class docstring step 6 to reflect session-scoped filtering Co-authored-by: Cursor <cursoragent@cursor.com>
Move _get_orphan_resource_names back to test file as a private helper. The function has a single call site — per AGENTS.md "Don't create abstractions for single-use code", keeping it inline is correct. Signed-off-by: Meni Yakove <myakove@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Forklift creates PVCs using source disk UUIDs (e.g. 42544ee2-...), not session_uuid. The target namespace itself is unique per session (named after session_uuid), so all PVCs in it belong to this test run. Remove the partial_name/session_uuid filter that caused false baseline assertion failures. Signed-off-by: Meni Yakove <myakove@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Add PVC/ and DV/ prefix to _get_orphan_resource_names output so assertion failures clearly show which resource type is leaking. Co-authored-by: Cursor <cursoragent@cursor.com>
myakove
force-pushed
the
feat/MTV-5663-plan-archive-pvc-cleanup
branch
from
August 6, 2026 12:01
c309a5a to
b9fb3a1
Compare
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/plan_lifecycle/test_plan_archive_pvc_cleanup.py`:
- Around line 259-264: Replace the combined migration_pvcs or migration_dvs
assertion before test_archive_and_delete_plan with separate baseline assertions
confirming a regular PVC, a prime-* PVC, and a DataVolume exist. Filter or
identify each resource type explicitly, and retain clear failure messages so
missing prime PVC creation cannot make the cleanup assertion vacuous.
🪄 Autofix
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 Plus
Run ID: 2b71b42f-8f4c-4f7a-a2b9-50f435842ace
📒 Files selected for processing (5)
AGENTS.mdtests/plan_lifecycle/__init__.pytests/plan_lifecycle/test_plan_archive_pvc_cleanup.pytests/tests_config/config.pyutilities/resources.py
Comment on lines
+259
to
+264
| migration_pvcs = list(PersistentVolumeClaim.get(client=ocp_admin_client, namespace=vm_namespace)) | ||
| migration_dvs = list(DataVolume.get(client=ocp_admin_client, namespace=vm_namespace)) | ||
| assert migration_pvcs or migration_dvs, ( | ||
| f"No PVCs or DataVolumes found in namespace '{vm_namespace}' after " | ||
| "post-hook failure — the archive+delete cleanup assertion would be vacuous" | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
HIGH — Assert each failed-migration resource type before archive.
Line 261 accepts one PVC or one DataVolume. It does not prove that the failed migration created a regular PVC, a prime-* PVC, and a DataVolume. If prime-* PVC creation regresses, the final empty-state check still passes because no prime-* PVC exists to clean up.
Assert all three baseline conditions before test_archive_and_delete_plan.
Proposed fix
migration_pvcs = list(PersistentVolumeClaim.get(client=ocp_admin_client, namespace=vm_namespace))
migration_dvs = list(DataVolume.get(client=ocp_admin_client, namespace=vm_namespace))
- assert migration_pvcs or migration_dvs, (
- f"No PVCs or DataVolumes found in namespace '{vm_namespace}' after "
- "post-hook failure — the archive+delete cleanup assertion would be vacuous"
- )
+ pvc_names = [pvc.name for pvc in migration_pvcs]
+ assert migration_dvs, f"No DataVolumes found in namespace '{vm_namespace}' after post-hook failure"
+ assert any(not name.startswith("prime-") for name in pvc_names), (
+ f"No regular PVC found in namespace '{vm_namespace}' after post-hook failure"
+ )
+ assert any(name.startswith("prime-") for name in pvc_names), (
+ f"No prime PVC found in namespace '{vm_namespace}' after post-hook failure"
+ )🤖 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/plan_lifecycle/test_plan_archive_pvc_cleanup.py` around lines 259 -
264, Replace the combined migration_pvcs or migration_dvs assertion before
test_archive_and_delete_plan with separate baseline assertions confirming a
regular PVC, a prime-* PVC, and a DataVolume exist. Filter or identify each
resource type explicitly, and retain clear failure messages so missing prime PVC
creation cannot make the cleanup assertion vacuous.
Comment on lines
+316
to
+318
| vm_obj = VirtualMachine(client=ocp_admin_client, name=vm_name, namespace=vm_namespace) | ||
| if vm_obj.exists: | ||
| vm_obj.clean_up(wait=True) |
There was a problem hiding this comment.
1. Vm cleanup not asserted 🐞 Bug ☼ Reliability
test_verify_pvc_cleanup deletes retained destination VMs via vm_obj.clean_up(wait=True) but does not verify the deletion succeeded. If VM deletion fails (or returns a failure indicator), the subsequent orphan DV/PVC assertion can fail for VM-owned volumes, misattributing the failure to plan archive+delete cleanup.
Agent Prompt
### Issue description
`test_verify_pvc_cleanup()` attempts to delete any retained destination VMs before checking for orphan DVs/PVCs, but it ignores whether VM deletion actually succeeded. This can make the orphan-resource assertion noisy and misleading if VM cleanup fails.
### Issue Context
In the same test file, plan deletion is explicitly checked using `assert plan.clean_up(wait=True)`, implying `clean_up()` is expected to either return a success indicator (or raise). VM deletion should be handled similarly so failures are attributed correctly.
### Fix Focus Areas
- tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py[313-319]
### Proposed change
Update the VM deletion loop to validate deletion success and surface a VM-specific error, e.g.:
```python
if vm_obj.exists:
assert vm_obj.clean_up(wait=True), (
f"Failed to delete destination VM '{vm_name}' in namespace '{vm_namespace}'"
)
```
(If `clean_up()` raises on failure in your wrapper, this still improves diagnosability when it returns a falsy value.)
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit b9fb3a1 |
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.
Summary
Regression test for MTV-5564: verifies that archiving and deleting a failed migration plan properly cleans up all PVCs (including
prime-*PVCs) in the target namespace.Test Flow (6-step)
MigrationPlanExecError)Changes
tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py— 6-step test classtests/plan_lifecycle/__init__.py— package inittests/tests_config/config.py— addedtest_plan_archive_pvc_cleanupconfig entryutilities/resources.py— addedunregister_teardown_resource()for mid-test resource deletionAGENTS.md— documentedunregister_teardown_resource(), removed test execution prohibitionTest Results
Related
Made with Cursor
Summary by CodeRabbit
Bug Fixes
Documentation
Tests