Add independent persistent disk migration test #249
Conversation
WalkthroughAdds a new test Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/test_copyoffload_migration.py:
- Around line 747-751: The test uses .get() for copyoffload keys which hides
missing config errors; change the assignments to use direct indexing on
copyoffload_config_data (e.g., replace
copyoffload_config_data.get("storage_vendor_product") and
copyoffload_config_data.get("datastore_id") with
copyoffload_config_data["storage_vendor_product"] and
copyoffload_config_data["datastore_id"]) so the test fails fast with a KeyError
like the other tests; ensure variable names storage_vendor_product and
datastore_id remain the same.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/test_copyoffload_migration.pytests/tests_config/config.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use built-in Python typing with dict, list, tuple, etc. - avoid custom type definitions
Useuvfor all dependency and project management operations
ALL OpenShift and Kubernetes resource interactions MUST useopenshift-python-wrapperpackage - NEVER use direct kubernetes package imports at runtime
CORRECT imports: Usefrom ocp_resources.*for resource classes andfrom ocp_utilities.infra import get_clientfor DynamicClient - FORBIDDEN: direct imports from kubernetes package, kubernetes.client, kubernetes.config, or kubernetes.dynamic at runtime
Importing kubernetes.dynamic.DynamicClient is permitted ONLY for type annotations using TYPE_CHECKING block or string literals - NEVER instantiate DynamicClient directly, always use get_client()
Every OpenShift resource must be created usingcreate_and_store_resource()function from utilities/resources.py - NEVER instantiate resources directly or use Resource.deploy()
Large functions MUST be refactored into smaller, focused functions - Maximum recommended size ~50 lines per function, each function should have single responsibility
Keep code simple and readable, avoid over-engineering
Provider configuration MUST be read from .providers.json in project root for provider connection details and authentication
Files:
tests/tests_config/config.pytests/test_copyoffload_migration.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests run against real clusters and real providers - use real environments for VMware vSphere, RHV, OpenStack, OVA, OpenShift
Support both cold and warm migration testing with resource management including comprehensive fixture-based cleanup and teardown
Support parallel execution with pytest-xdist concurrent testing using unique namespaces per session and UUID-based resource names
Files:
tests/tests_config/config.pytests/test_copyoffload_migration.py
tests/tests_config/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
All test configurations must be centralized in tests/tests_config/config.py with type annotations for global settings and descriptive test parameter names matching test function names
Files:
tests/tests_config/config.py
tests/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/test_*.py: Test parametrization MUST use indirect=True to pass parameters to plan fixture first
CRITICAL: No defaults or fallbacks for configurations WE CONTROL (py_config, plan, tests_params) - use direct key access [] to fail fast with KeyError if missing. Use .get() with validation only for external/provider configurations
Use pytest markers tier0, warm, remote, copyoffload for test categorization - defined in pytest.ini, apply as optional markers to test functions
Apply @pytest.mark.parametrize with indirect=True for test parameters, use @pytest.mark.tier0 for core functionality tests, @pytest.mark.warm for warm migrations, @pytest.mark.remote for cross-cluster tests, @pytest.mark.copyoffload for copy-offload tests
Files:
tests/test_copyoffload_migration.py
tests/test_*migration*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/test_*migration*.py: New tests MUST follow standard pattern: import pytest fixtures (request, fixture_store, ocp_admin_client, target_namespace, destination_provider, plan, source_provider, source_provider_data, multus_network_name, source_provider_inventory, source_vms_namespace), Step 1 create migration maps using create_storagemap_and_networkmap, Step 2 execute migration using migrate_vms utility
Test file naming MUST follow pattern test__migration.py for migration tests
Files:
tests/test_copyoffload_migration.py
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RedHatQE/mtv-api-tests PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T09:01:42.020Z
Learning: Applies to tests/test_*migration*.py : New tests MUST follow standard pattern: import pytest fixtures (request, fixture_store, ocp_admin_client, target_namespace, destination_provider, plan, source_provider, source_provider_data, multus_network_name, source_provider_inventory, source_vms_namespace), Step 1 create migration maps using create_storagemap_and_networkmap, Step 2 execute migration using migrate_vms utility
📚 Learning: 2026-01-12T09:01:42.020Z
Learnt from: CR
Repo: RedHatQE/mtv-api-tests PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T09:01:42.020Z
Learning: Applies to tests/test_*migration*.py : New tests MUST follow standard pattern: import pytest fixtures (request, fixture_store, ocp_admin_client, target_namespace, destination_provider, plan, source_provider, source_provider_data, multus_network_name, source_provider_inventory, source_vms_namespace), Step 1 create migration maps using create_storagemap_and_networkmap, Step 2 execute migration using migrate_vms utility
Applied to files:
tests/tests_config/config.pytests/test_copyoffload_migration.py
🧬 Code graph analysis (1)
tests/test_copyoffload_migration.py (1)
utilities/mtv_migration.py (4)
get_network_migration_map(367-400)get_storage_migration_map(269-364)migrate_vms(32-98)verify_vm_disk_count(437-462)
🪛 Ruff (0.14.10)
tests/test_copyoffload_migration.py
713-713: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
735-735: Unused function argument: copyoffload_config
(ARG001)
🔇 Additional comments (5)
tests/tests_config/config.py (1)
137-156: LGTM!The new test configuration entry follows the established pattern for copy-offload tests. The structure correctly defines the VM with
clone: True, setsdisk_type: "thin"for the base disk, and adds an independent persistent disk viaadd_disks. Thewarm_migration: Falsesetting is appropriate since independent disks cannot be snapshotted.tests/test_copyoffload_migration.py (4)
711-722: Parametrize decorator and fixture pattern look correct.The static analysis hint about using a tuple instead of a string for the first argument (PT006) can be safely ignored here — all other tests in this file use the same string format for consistency.
Regarding the unused
copyoffload_configfixture (ARG001): this is intentional. The fixture performs validation side effects before the test runs. All other copy-offload tests follow this same pattern.
753-764: LGTM!Network migration map creation follows the established pattern and passes all required parameters correctly.
766-789: LGTM!The offload plugin configuration and storage migration map creation follow the established pattern, correctly including all copy-offload specific parameters (
datastore_id,offload_plugin_config,access_mode,volume_mode).
791-808: LGTM!The migration execution and disk count verification follow the standard pattern. The test correctly:
- Executes migration via
migrate_vmswith all required parameters- Verifies the migrated VM has the expected disk count (1 base + 1 independent persistent = 2 disks)
This aligns with the PR objective of verifying the disk count on the migrated VM.
prabinovRedhat
force-pushed
the
add_independent_persistent_disk_test
branch
from
e589c8e to
1b47062
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @tests/test_copyoffload_migration.py:
- Around line 711-722: The pytest.mark.parametrize decorator here uses a single
string "plan,multus_network_name" which triggers Ruff PT006; change it to a
tuple ("plan", "multus_network_name") in this decorator (the
pytest.mark.parametrize call that supplies plan and multus_network_name for the
copyoffload test) and, if you want to fully satisfy PT006, apply the same
tuple-style change to all other pytest.mark.parametrize occurrences in this file
so the formatting is consistent across tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
tests/test_copyoffload_migration.pytests/tests_config/config.py
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Always use built-in Python typing with dict, list, tuple, etc. - avoid custom type definitions
Useuvfor all dependency and project management operations
ALL OpenShift and Kubernetes resource interactions MUST useopenshift-python-wrapperpackage - NEVER use direct kubernetes package imports at runtime
CORRECT imports: Usefrom ocp_resources.*for resource classes andfrom ocp_utilities.infra import get_clientfor DynamicClient - FORBIDDEN: direct imports from kubernetes package, kubernetes.client, kubernetes.config, or kubernetes.dynamic at runtime
Importing kubernetes.dynamic.DynamicClient is permitted ONLY for type annotations using TYPE_CHECKING block or string literals - NEVER instantiate DynamicClient directly, always use get_client()
Every OpenShift resource must be created usingcreate_and_store_resource()function from utilities/resources.py - NEVER instantiate resources directly or use Resource.deploy()
Large functions MUST be refactored into smaller, focused functions - Maximum recommended size ~50 lines per function, each function should have single responsibility
Keep code simple and readable, avoid over-engineering
Provider configuration MUST be read from .providers.json in project root for provider connection details and authentication
Files:
tests/test_copyoffload_migration.pytests/tests_config/config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Tests run against real clusters and real providers - use real environments for VMware vSphere, RHV, OpenStack, OVA, OpenShift
Support both cold and warm migration testing with resource management including comprehensive fixture-based cleanup and teardown
Support parallel execution with pytest-xdist concurrent testing using unique namespaces per session and UUID-based resource names
Files:
tests/test_copyoffload_migration.pytests/tests_config/config.py
tests/test_*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/test_*.py: Test parametrization MUST use indirect=True to pass parameters to plan fixture first
CRITICAL: No defaults or fallbacks for configurations WE CONTROL (py_config, plan, tests_params) - use direct key access [] to fail fast with KeyError if missing. Use .get() with validation only for external/provider configurations
Use pytest markers tier0, warm, remote, copyoffload for test categorization - defined in pytest.ini, apply as optional markers to test functions
Apply @pytest.mark.parametrize with indirect=True for test parameters, use @pytest.mark.tier0 for core functionality tests, @pytest.mark.warm for warm migrations, @pytest.mark.remote for cross-cluster tests, @pytest.mark.copyoffload for copy-offload tests
Files:
tests/test_copyoffload_migration.py
tests/test_*migration*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/test_*migration*.py: New tests MUST follow standard pattern: import pytest fixtures (request, fixture_store, ocp_admin_client, target_namespace, destination_provider, plan, source_provider, source_provider_data, multus_network_name, source_provider_inventory, source_vms_namespace), Step 1 create migration maps using create_storagemap_and_networkmap, Step 2 execute migration using migrate_vms utility
Test file naming MUST follow pattern test__migration.py for migration tests
Files:
tests/test_copyoffload_migration.py
tests/tests_config/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
All test configurations must be centralized in tests/tests_config/config.py with type annotations for global settings and descriptive test parameter names matching test function names
Files:
tests/tests_config/config.py
🧠 Learnings (2)
📚 Learning: 2026-01-12T09:01:42.020Z
Learnt from: CR
Repo: RedHatQE/mtv-api-tests PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T09:01:42.020Z
Learning: Applies to tests/test_*migration*.py : New tests MUST follow standard pattern: import pytest fixtures (request, fixture_store, ocp_admin_client, target_namespace, destination_provider, plan, source_provider, source_provider_data, multus_network_name, source_provider_inventory, source_vms_namespace), Step 1 create migration maps using create_storagemap_and_networkmap, Step 2 execute migration using migrate_vms utility
Applied to files:
tests/test_copyoffload_migration.pytests/tests_config/config.py
📚 Learning: 2026-01-12T09:01:42.020Z
Learnt from: CR
Repo: RedHatQE/mtv-api-tests PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-12T09:01:42.020Z
Learning: Applies to tests/test_*.py : CRITICAL: No defaults or fallbacks for configurations WE CONTROL (py_config, plan, tests_params) - use direct key access [] to fail fast with KeyError if missing. Use .get() with validation only for external/provider configurations
Applied to files:
tests/test_copyoffload_migration.py
🪛 Ruff (0.14.10)
tests/test_copyoffload_migration.py
713-713: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
735-735: Unused function argument: copyoffload_config
(ARG001)
🔇 Additional comments (3)
tests/tests_config/config.py (1)
137-156: LGTM! Configuration follows established patterns.The new test configuration entry correctly mirrors the structure of existing copyoffload test configs. The combination of
disk_typefor the base disk andadd_disksfor the independent persistent disk is appropriate for this test scenario.tests/test_copyoffload_migration.py (2)
723-738:copyoffload_configfixture is correctly includedRuff ARG001 flags this as unused, but this is a false positive. The
copyoffload_configfixture performs configuration validation as a side-effect when pytest invokes it. All other copyoffload tests in this file follow the same pattern (e.g., lines 51, 197, 347, 502, 630).
739-817: LGTM! Test implementation follows established patterns.The implementation correctly:
- Extracts copy-offload configuration using direct key access (per coding guidelines).
- Creates network and storage migration maps with copy-offload parameters.
- Executes the migration via
migrate_vms.- Verifies disk count on the migrated VM.
The docstring clearly documents the unique aspect of independent persistent disks (excluded from snapshots, requiring cold migration).
| @pytest.mark.copyoffload | ||
| @pytest.mark.parametrize( | ||
| "plan,multus_network_name", | ||
| [ | ||
| pytest.param( | ||
| py_config["tests_params"]["test_copyoffload_independent_persistent_disk_migration"], | ||
| py_config["tests_params"]["test_copyoffload_independent_persistent_disk_migration"], | ||
| ) | ||
| ], | ||
| indirect=True, | ||
| ids=["copyoffload-independent-persistent"], | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LOW: Parametrize uses string format (consistent with file)
Ruff PT006 flags this, but all other tests in this file use the same string format for pytest.mark.parametrize (lines 29, 175, 325, 480, 608). While the tuple format ("plan", "multus_network_name") is technically preferred, changing just this instance would create inconsistency.
If you decide to address PT006, apply it file-wide for consistency.
Optional: File-wide tuple format
@pytest.mark.parametrize(
- "plan,multus_network_name",
+ ("plan", "multus_network_name"),
[
pytest.param(📝 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.
| @pytest.mark.copyoffload | |
| @pytest.mark.parametrize( | |
| "plan,multus_network_name", | |
| [ | |
| pytest.param( | |
| py_config["tests_params"]["test_copyoffload_independent_persistent_disk_migration"], | |
| py_config["tests_params"]["test_copyoffload_independent_persistent_disk_migration"], | |
| ) | |
| ], | |
| indirect=True, | |
| ids=["copyoffload-independent-persistent"], | |
| ) | |
| @pytest.mark.copyoffload | |
| @pytest.mark.parametrize( | |
| ("plan", "multus_network_name"), | |
| [ | |
| pytest.param( | |
| py_config["tests_params"]["test_copyoffload_independent_persistent_disk_migration"], | |
| py_config["tests_params"]["test_copyoffload_independent_persistent_disk_migration"], | |
| ) | |
| ], | |
| indirect=True, | |
| ids=["copyoffload-independent-persistent"], | |
| ) |
🧰 Tools
🪛 Ruff (0.14.10)
713-713: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
🤖 Prompt for AI Agents
In @tests/test_copyoffload_migration.py around lines 711 - 722, The
pytest.mark.parametrize decorator here uses a single string
"plan,multus_network_name" which triggers Ruff PT006; change it to a tuple
("plan", "multus_network_name") in this decorator (the pytest.mark.parametrize
call that supplies plan and multus_network_name for the copyoffload test) and,
if you want to fully satisfy PT006, apply the same tuple-style change to all
other pytest.mark.parametrize occurrences in this file so the formatting is
consistent across tests.
4 participants
Add Independent Persistent Disk Migration Test
Summary
Adds a new test
test_copyoffload_independent_persistent_disk_migrationto verify copy-offload migration of a VM with an independent persistent disk.This test validates that MTV (Migration Toolkit for Virtualization) can successfully handle VMs with "Independent - Persistent" disks, which are excluded from snapshots and require specific handling during migration (especially cold migration).
Configuration
Changes
Implementation
tests/test_copyoffload_migration.py: Addedtest_copyoffload_independent_persistent_disk_migration.tests/tests_config/config.py: Added configuration for the new test scenario.Technical Details
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.