Skip to content
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
0409455
test(MTV-5663): add plan archive PVC cleanup regression test
myakove Aug 3, 2026
a0f48bb
fix: address Qodo review — vm_namespace and idempotent unregister
myakove Aug 3, 2026
ec7328a
fix: address CodeRabbit review findings
myakove Aug 3, 2026
50764fa
fix: use consistent vm_namespace in baseline + add test execution con…
myakove Aug 3, 2026
8d78e55
fix: address Qodo cycle 2 — parallel safety caveat + scoped baseline …
myakove Aug 3, 2026
4857030
fix: scope baseline PVC/DV check by session_uuid
myakove Aug 3, 2026
951ce42
fix: move orphan helper to utilities + align parallel safety docs
myakove Aug 3, 2026
9dcc5f8
fix: scope final orphan check by session_uuid
myakove Aug 3, 2026
ba9b286
chore: trigger Qodo re-scan for stale sticky finding
myakove Aug 3, 2026
63d732a
fix: add partial_name filter to get_orphan_resource_names
myakove Aug 3, 2026
5dad3f4
fix: handle timeout boundary + update docstring scope
myakove Aug 3, 2026
debde7d
fix: inline orphan helper back into test file
myakove Aug 3, 2026
7c1b543
fix: remove wrong session_uuid PVC name filter
myakove Aug 3, 2026
b9fb3a1
fix: prefix orphan resource names with kind (PVC/DV)
myakove Aug 3, 2026
f7822a6
chore: pre-commit autoupdate (#625)
pre-commit-ci[bot] Aug 7, 2026
ce8ddd4
test: add AAP hook integration test (MTV-6031) (#594)
AmenB Aug 7, 2026
b061c8e
ci(deps): lock file maintenance (#630)
renovate[bot] Aug 10, 2026
3e634de
chore: trigger Qodo re-scan for stale sticky finding
myakove Aug 3, 2026
b75c051
Assert VM cleanup success in test_verify_pvc_cleanup
myakove Aug 10, 2026
c1dc10a
Add filelock serialization to aap_mtv_settings and justify Any usage
myakove Aug 10, 2026
806ec50
Refactor AWX instance creation to use Resource kind_dict
myakove Aug 10, 2026
57694bf
Fix AWX instance leak in awx_deployment fixture
myakove Aug 10, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 26 additions & 5 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -598,12 +598,17 @@ with ResourceEditor(node) as editor:

## Critical Constraints

### Test Execution Prohibition
### Test Execution Requirements (MUST)

AI must NEVER run tests directly (`pytest`, `uv run pytest`). Tests require live clusters, provider connections, and credentials.
Tests interact with live OpenShift clusters and source providers. Before running tests:

AI can: Read/analyze/write/fix tests, suggest improvements, review structure
AI cannot: Execute tests, validate by running
- **Cluster access:** A dedicated test cluster with `kubeadmin` or equivalent credentials is required
- **Provider credentials:** A valid `.providers.json` with source provider connection details
- **Isolation:** Tests create unique namespaces per session (`session_uuid`) to prevent OCP resource collisions between parallel runs
- **Parallel safety:** Namespace isolation does not prevent source-provider VM conflicts when multiple
runs share the same source VM names — use separate provider configurations or VM cloning for true
parallel safety
- **Cleanup:** Tests clean up resources via `fixture_store` teardown — use `--skip-teardown` to preserve resources for debugging
Comment thread
coderabbitai[bot] marked this conversation as resolved.

### No Module-Level Provider Loading (MUST)

Expand Down Expand Up @@ -708,6 +713,20 @@ namespace = Namespace(client=ocp_admin_client, name="my-namespace")
namespace.deploy()
```

When intentionally deleting a resource mid-test (e.g., testing plan archive+delete), unregister it
from teardown to prevent session cleanup from operating on a missing resource:

```python
# Assumes Plan, Migration, archive_plan, get_migration_for_plan are already imported
from utilities.resources import unregister_teardown_resource

migration_name = get_migration_for_plan(plan).name
archive_plan(plan=self.plan_resource)
self.plan_resource.clean_up(wait=True)
unregister_teardown_resource(fixture_store=fixture_store, kind=Plan.kind, name=self.plan_resource.name)
unregister_teardown_resource(fixture_store=fixture_store, kind=Migration.kind, name=migration_name)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
```

## Test Structure Pattern

All tests follow a class-based structure with 5 base test methods:
Expand Down Expand Up @@ -1070,12 +1089,14 @@ When multiple issues exist, address them in this order:

## Parallel Execution (pytest-xdist)

Tests are parallel-safe because:
Tests are parallel-safe for OpenShift resources because:

- Unique namespaces per session via `session_uuid`
- Each worker has isolated `fixture_store`
- `create_and_store_resource()` generates unique names

**Note:** Namespace isolation does not prevent source-provider VM conflicts — see Test Execution Requirements above.

Rules:

- Always use fixtures for namespaces (never hardcode)
Expand Down
Empty file.
336 changes: 336 additions & 0 deletions tests/plan_lifecycle/test_plan_archive_pvc_cleanup.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,336 @@
"""MTV-5663: Verify PVC cleanup after archiving and deleting a failed migration plan.

Regression test for MTV-5564: archiving and deleting a failed plan left
orphan PVCs (both regular and prime PVCs) in the target namespace.

This test induces failure via a post-hook (not mid-transfer like the original
bug) to create PVCs and then fail the migration. Both paths exercise the same
Forklift plan archive+delete cleanup mechanism.
"""

from __future__ import annotations

from typing import TYPE_CHECKING, Any

import pytest
from ocp_resources.datavolume import DataVolume
from ocp_resources.migration import Migration
from ocp_resources.network_map import NetworkMap
from ocp_resources.persistent_volume_claim import PersistentVolumeClaim
from ocp_resources.plan import Plan
from ocp_resources.storage_map import StorageMap
from ocp_resources.virtual_machine import VirtualMachine
from pytest_testconfig import config as py_config
from timeout_sampler import TimeoutExpiredError, TimeoutSampler

from exceptions.exceptions import MigrationPlanExecError
from utilities.hooks import validate_hook_failure_and_check_vms
from utilities.migration_utils import archive_plan
from utilities.mtv_migration import (
create_plan_resource,
execute_migration,
get_migration_for_plan,
get_network_migration_map,
get_storage_migration_map,
)
from utilities.naming import resolve_destination_vm_name
from utilities.resources import unregister_teardown_resource
from utilities.utils import populate_vm_ids

if TYPE_CHECKING:
from kubernetes.dynamic import DynamicClient

from libs.base_provider import BaseProvider
from libs.forklift_inventory import ForkliftInventory
from libs.providers.openshift import OCPProvider


_ORPHAN_RESOURCE_WAIT_TIMEOUT = 120 # Seconds to wait for async DV/PVC garbage collection
_ORPHAN_RESOURCE_POLL_INTERVAL = 5 # Seconds between polls


def _get_orphan_resource_names(client: DynamicClient, namespace: str) -> list[str]:
"""List remaining DV and PVC names in a namespace.

The target namespace is unique per session (named after session_uuid),
so all PVCs/DVs in it belong to this test run.

Args:
client (DynamicClient): OpenShift admin client.
namespace (str): Namespace to check.

Returns:
list[str]: Prefixed names (PVC/name, DV/name) of remaining resources, empty if none.
"""
remaining_pvcs = list(PersistentVolumeClaim.get(client=client, namespace=namespace))
remaining_dvs = list(DataVolume.get(client=client, namespace=namespace))
return [f"PVC/{pvc.name}" for pvc in remaining_pvcs] + [f"DV/{dv.name}" for dv in remaining_dvs]


@pytest.mark.vsphere
@pytest.mark.rhv
@pytest.mark.openstack
@pytest.mark.openshift
@pytest.mark.tier1
@pytest.mark.incremental
@pytest.mark.parametrize(
"class_plan_config",
[pytest.param(py_config["tests_params"]["test_plan_archive_pvc_cleanup"])],
indirect=True,
ids=["MTV-5663-plan-archive-pvc-cleanup"],
)
@pytest.mark.usefixtures("cleanup_migrated_vms")
class TestPlanArchivePvcCleanup:
"""MTV-5663: Verify PVC cleanup after archiving and deleting a failed migration plan.

Regression test for MTV-5564: archiving and deleting a failed plan left
orphan PVCs (both regular and prime PVCs) in the target namespace.

Test steps:
1. Create StorageMap resource.
2. Create NetworkMap resource.
3. Create Plan with a post-hook configured to fail.
4. Execute migration — migration runs far enough to create PVCs,
then fails due to post-hook failure (MigrationPlanExecError).
5. Archive the failed plan, then delete it.
6. Delete retained destination VMs, then verify all DVs and PVCs
from this test session in the effective VM target namespace are
cleaned up — no matching orphan resources remain.
"""

storage_map: StorageMap
network_map: NetworkMap
plan_resource: Plan

def test_create_storagemap(
self,
prepared_plan: dict[str, Any],
fixture_store: dict[str, Any],
ocp_admin_client: DynamicClient,
source_provider: BaseProvider,
destination_provider: OCPProvider,
source_provider_inventory: ForkliftInventory,
target_namespace: str,
) -> None:
"""Create StorageMap resource.

Args:
prepared_plan (dict[str, Any]): The prepared migration plan.
fixture_store (dict[str, Any]): Fixture store for resource tracking.
ocp_admin_client (DynamicClient): OpenShift admin client.
source_provider (BaseProvider): Source provider instance.
destination_provider (OCPProvider): Destination provider instance.
source_provider_inventory (ForkliftInventory): Source provider inventory.
target_namespace (str): Target namespace for migration.

Raises:
AssertionError: If StorageMap creation fails.
"""
vms = [vm["name"] for vm in prepared_plan["virtual_machines"]]
self.__class__.storage_map = get_storage_migration_map(
fixture_store=fixture_store,
source_provider=source_provider,
destination_provider=destination_provider,
source_provider_inventory=source_provider_inventory,
ocp_admin_client=ocp_admin_client,
target_namespace=target_namespace,
vms=vms,
)
assert self.storage_map, "StorageMap creation failed"

def test_create_networkmap(
self,
prepared_plan: dict[str, Any],
fixture_store: dict[str, Any],
ocp_admin_client: DynamicClient,
source_provider: BaseProvider,
destination_provider: OCPProvider,
source_provider_inventory: ForkliftInventory,
target_namespace: str,
multus_network_name: dict[str, str],
) -> None:
"""Create NetworkMap resource.

Args:
prepared_plan (dict[str, Any]): The prepared migration plan.
fixture_store (dict[str, Any]): Fixture store for resource tracking.
ocp_admin_client (DynamicClient): OpenShift admin client.
source_provider (BaseProvider): Source provider instance.
destination_provider (OCPProvider): Destination provider instance.
source_provider_inventory (ForkliftInventory): Source provider inventory.
target_namespace (str): Target namespace for migration.
multus_network_name (dict[str, str]): Name of the multus network.

Raises:
AssertionError: If NetworkMap creation fails.
"""
vms = [vm["name"] for vm in prepared_plan["virtual_machines"]]
self.__class__.network_map = get_network_migration_map(
fixture_store=fixture_store,
source_provider=source_provider,
destination_provider=destination_provider,
source_provider_inventory=source_provider_inventory,
ocp_admin_client=ocp_admin_client,
target_namespace=target_namespace,
multus_network_name=multus_network_name,
vms=vms,
)
assert self.network_map, "NetworkMap creation failed"

def test_create_plan(
self,
prepared_plan: dict[str, Any],
fixture_store: dict[str, Any],
ocp_admin_client: DynamicClient,
source_provider: BaseProvider,
destination_provider: OCPProvider,
target_namespace: str,
source_provider_inventory: ForkliftInventory,
) -> None:
"""Create MTV Plan CR with a post-hook configured to fail.

Args:
prepared_plan (dict[str, Any]): The prepared migration plan.
fixture_store (dict[str, Any]): Fixture store for resource tracking.
ocp_admin_client (DynamicClient): OpenShift admin client.
source_provider (BaseProvider): Source provider instance.
destination_provider (OCPProvider): Destination provider instance.
target_namespace (str): Target namespace for migration.
source_provider_inventory (ForkliftInventory): Source provider inventory.

Raises:
AssertionError: If Plan creation fails.
"""
populate_vm_ids(prepared_plan, source_provider_inventory)

self.__class__.plan_resource = create_plan_resource(
ocp_admin_client=ocp_admin_client,
fixture_store=fixture_store,
source_provider=source_provider,
destination_provider=destination_provider,
storage_map=self.storage_map,
network_map=self.network_map,
virtual_machines_list=prepared_plan["virtual_machines"],
target_namespace=target_namespace,
warm_migration=prepared_plan.get("warm_migration", False),
target_power_state=prepared_plan["target_power_state"],
after_hook_name=prepared_plan["_post_hook_name"],
after_hook_namespace=prepared_plan["_post_hook_namespace"],
)
assert self.plan_resource, "Plan creation failed"

def test_migrate_vms(
self,
prepared_plan: dict[str, Any],
fixture_store: dict[str, Any],
ocp_admin_client: DynamicClient,
target_namespace: str,
) -> None:
"""Execute migration — expected to fail due to post-hook failure.

The migration runs far enough to create PVCs for the VM disks, then
the post-hook triggers a failure. This leaves PVCs in the target
namespace that should be cleaned up when the plan is archived and deleted.

Args:
prepared_plan (dict[str, Any]): The prepared migration plan.
fixture_store (dict[str, Any]): Fixture store for resource tracking.
ocp_admin_client (DynamicClient): OpenShift admin client.
target_namespace (str): Target namespace for migration.

Raises:
AssertionError: If migration does not fail at PostHook as expected.
"""
with pytest.raises(MigrationPlanExecError):
execute_migration(
ocp_admin_client=ocp_admin_client,
fixture_store=fixture_store,
plan=self.plan_resource,
target_namespace=target_namespace,
)

validate_hook_failure_and_check_vms(self.plan_resource, prepared_plan)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

# Verify migration created resources before we archive+delete.
# The target namespace is unique per session (named after session_uuid),
# so all PVCs/DVs in it belong to this test run. Forklift creates PVCs
# using source disk UUIDs (not session_uuid), so name filtering is wrong.
vm_namespace = prepared_plan.get("_vm_target_namespace", target_namespace)
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"
)
Comment on lines +259 to +264

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


def test_archive_and_delete_plan(self, fixture_store: dict[str, Any]) -> None:
"""Archive and delete the failed migration plan.

Args:
fixture_store (dict[str, Any]): Fixture store for resource tracking.

Raises:
AssertionError: If plan is not archived or deletion fails.
"""
plan = self.plan_resource
migration_name = get_migration_for_plan(plan).name

archive_plan(plan=plan)
conditions = plan.instance.status.conditions or []
assert any(
condition["type"] == plan.Condition.ARCHIVED and condition["status"] == plan.Condition.Status.TRUE
for condition in conditions
), f"Plan '{plan.name}' did not reach Archived condition"

assert plan.clean_up(wait=True), f"Failed to delete plan '{plan.name}' after archiving"

# Plan was deleted intentionally; unregister so session_teardown does not
# call archive_plan() on a missing Plan and abort the rest of cleanup.
unregister_teardown_resource(fixture_store=fixture_store, kind=Plan.kind, name=plan.name)
unregister_teardown_resource(fixture_store=fixture_store, kind=Migration.kind, name=migration_name)

def test_verify_pvc_cleanup(
self,
prepared_plan: dict[str, Any],
ocp_admin_client: DynamicClient,
target_namespace: str,
) -> None:
"""Verify all PVCs are cleaned up after plan archive and deletion.

Destination VMs may still exist if post-hook failure retained them.
Any remaining VMs are deleted so the orphan DV/PVC check below is not
masked by VM-owned resources.
Polls for up to 120s because DV/PVC garbage collection is async.

Args:
prepared_plan (dict[str, Any]): The prepared migration plan.
ocp_admin_client (DynamicClient): OpenShift admin client.
target_namespace (str): Target namespace for migration.

Raises:
AssertionError: If orphan resources remain after 120s timeout.
"""
vm_namespace = prepared_plan.get("_vm_target_namespace", target_namespace)
for vm in prepared_plan["virtual_machines"]:
vm_name = resolve_destination_vm_name(vm)
vm_obj = VirtualMachine(client=ocp_admin_client, name=vm_name, namespace=vm_namespace)
if vm_obj.exists:
vm_obj.clean_up(wait=True)
Comment thread
qodo-code-review[bot] marked this conversation as resolved.
Outdated

try:
for sample in TimeoutSampler(
wait_timeout=_ORPHAN_RESOURCE_WAIT_TIMEOUT,
sleep=_ORPHAN_RESOURCE_POLL_INTERVAL,
func=_get_orphan_resource_names,
client=ocp_admin_client,
namespace=vm_namespace,
):
if not sample:
return
except TimeoutExpiredError:
orphan_names = _get_orphan_resource_names(client=ocp_admin_client, namespace=vm_namespace)
if not orphan_names:
return
raise AssertionError(
f"Orphan resources remain in namespace '{vm_namespace}' after plan archive+delete: {orphan_names}"
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Loading