Repository navigation
feat: Add simultaneous copyoffload migrations test - #285
Merged
Merged
Conversation
Contributor
WalkthroughAdds a utility to extract a single VM from a multi‑VM prepared plan, two class‑scoped fixtures exposing single‑VM plans, a new test class to run two copy‑offload migrations concurrently (per‑plan maps/plans and two Migration CRs), updates test configs, and treats missing Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as TestSuite
participant Store as fixture_store
participant OCP as ocp_admin_client
participant PlanCR as Plan CRs
participant MigCR as Migration CRs
TestSuite->>Store: create StorageMap (plan1)
TestSuite->>Store: create StorageMap (plan2)
TestSuite->>Store: create NetworkMap (plan1)
TestSuite->>Store: create NetworkMap (plan2)
TestSuite->>PlanCR: create Plan CR (plan1)
TestSuite->>PlanCR: create Plan CR (plan2)
TestSuite->>OCP: create Migration CR (mig1)
TestSuite->>OCP: create Migration CR (mig2)
par Parallel migrations
MigCR->>OCP: execute copy-offload for VM (plan1)
MigCR->>OCP: execute copy-offload for VM (plan2)
end
TestSuite->>OCP: wait_for_migration_complate(mig1)
TestSuite->>OCP: wait_for_migration_complate(mig2)
TestSuite->>OCP: validate migrated VM names & disk counts (plan1)
TestSuite->>OCP: validate migrated VM names & disk counts (plan2)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions ✨Latest suggestions up to 84bf948
Previous suggestionsSuggestions up to commit 84bf948
Suggestions up to commit 09241a7
✅ Suggestions up to commit e5415f2
✅ Suggestions up to commit 49caece
✅ Suggestions up to commit c5c5a45
✅ Suggestions up to commit c2d126f
✅ Suggestions up to commit ed213c5
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
![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
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 924-944: prepared_plan_2 currently assumes there are at least two
VMs and will raise an IndexError; add an explicit guard that checks
len(plan["virtual_machines"]) >= 2 and raise a ValueError with a clear message
(e.g., "prepared_plan_2 requires at least 2 virtual_machines in prepared_plan")
if the check fails, then proceed to slice to the second VM and build
source_vms_data as before; also update the prepared_plan_2 docstring to include
a Raises section documenting the ValueError so callers know the precondition and
the exception behavior (reference symbols: prepared_plan_2,
plan["virtual_machines"], vm_name, plan["source_vms_data"]).
- Around line 901-944: The fixtures prepared_plan_1 and prepared_plan_2 should
be converted from generator-style to return-style: change the return type from
Generator[...] to dict[str, Any], update the docstrings "Yields"→"Returns" and
add a "Raises" section documenting IndexError and KeyError, and replace "yield
plan" with "return plan"; add defensive checks before indexing
plan["virtual_machines"] (ensure len(...) > 0 for prepared_plan_1 and > 1 for
prepared_plan_2) and verify the vm_name exists in
prepared_plan["source_vms_data"], raising IndexError/KeyError with clear
messages if those conditions fail so the functions fail fast with informative
errors.
In `@tests/test_copyoffload_migration.py`:
- Around line 2312-2627: The test duplicates plan1/plan2 flows; collapse them
into the standard five-step tests by replacing
test_create_storagemap_plan1/plan2, test_create_networkmap_plan1/plan2,
test_create_plan1/plan2, test_migrate_vms_simultaneously, and
test_check_vms_plan1/plan2 with unified methods named test_create_storagemap,
test_create_networkmap, test_create_plan, test_migrate_vms, and test_check_vms
that iterate over both prepared_plan_1 and prepared_plan_2 (or call a shared
helper) to call get_storage_migration_map, get_network_migration_map,
create_plan_resource, create_and_store_resource (Migration), check_vms and
verify_vm_disk_count for each plan; keep using class attributes
storage_map_1/storage_map_2, network_map_1/network_map_2,
plan_resource_1/plan_resource_2 but set them inside the loop/helper to preserve
later references and assertions.
- Around line 2320-2456: The class-level `@pytest.mark.usefixtures` currently
includes multus_network_name while the same fixture is also declared as a
parameter to test_create_networkmap_plan1 and test_create_networkmap_plan2;
remove multus_network_name from the `@pytest.mark.usefixtures` decorator on the
TestSimultaneousCopyoffloadMigrations class so the fixture is only requested
explicitly via the test function parameters (leave multus_network_name in the
parameter lists of test_create_networkmap_plan1 and
test_create_networkmap_plan2), and keep usefixtures only for fixtures that are
truly side-effect-only.
- Around line 2332-2627: The new test functions (test_create_storagemap_plan1,
test_create_storagemap_plan2, test_create_networkmap_plan1,
test_create_networkmap_plan2, test_create_plan1, test_create_plan2,
test_migrate_vms_simultaneously, test_check_vms_plan1, test_check_vms_plan2)
must be annotated with explicit type hints for all parameters and return values
(e.g., fixture types, client types, namespace: str, etc.) and their docstrings
expanded into the project-standard format including Args (with types and brief
descriptions), Returns (type/meaning, usually None for tests) and Raises
(expected exceptions or test failure conditions); update each function signature
to add the annotations and replace the short triple-quoted summary with a full
docstring block listing Args/Returns/Raises consistent with other tests in the
repo so static analysis and linters pass.
In `@tests/tests_config/config.py`:
- Around line 373-388: The two VM entries in the
"test_simultaneous_copyoffload_migrations" virtual_machines list both use the
same template name "xcopy-template-test" with identical clone settings; add
explicit, distinct clone_name fields (e.g., "xcopy-template-test-clone-1" and
"xcopy-template-test-clone-2") to each VM dict so the clone behavior is clear
and the test is more readable while keeping all other settings (source_vm_power,
guest_agent, clone, disk_type) unchanged.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
shellymiron
reviewed
Feb 2, 2026
elsapassaro
force-pushed
the
simultaneous_migration_plans
branch
from
February 2, 2026 11:19
c2d126f to
c5c5a45
Compare
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@tests/test_copyoffload_migration.py`:
- Around line 3162-3176: Add a "Returns: None" section to the function docstring
that documents validation of migrated VMs (the docstring listing Args like
prepared_plan_1, source_provider, destination_provider, etc.); update the
docstring for that validation function to include a Returns: None entry
consistent with the class's docstring style so the docstring contains Args,
Returns, and Raises sections.
- Around line 3043-3149: Tests use long inline AssertionError messages when a
plan completes early; extract those messages into named constants or a helper
(e.g., EARLY_COMPLETION_MSG_PLAN1, EARLY_COMPLETION_MSG_PLAN2 or a function
format_early_completion_msg(plan_name, status, other_status)) and replace the
inline strings in the assertions inside test_migrate_vms_simultaneously with
references to those constants/helper; update the assertions that check
status_1/status_2 against Plan.Status.SUCCEEDED/Plan.Status.FAILED to call the
formatter so the logic (in _check_plan_status and the TimeoutSampler loop)
remains unchanged but readability and lint (Ruff TRY003) are satisfied.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
Contributor
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 3100-3107: The nested helper function _check_plan_status lacks a
structured docstring; add a docstring to _check_plan_status that includes an
Args section describing the plan parameter (type Plan), a Returns section
stating it returns a str ("Executing", Plan.Status.SUCCEEDED, or
Plan.Status.FAILED), and a Raises section (if applicable) noting any exceptions
that may be propagated (e.g., AttributeError or KeyError when
plan.instance.status.conditions is malformed); place this docstring immediately
above the _check_plan_status definition so repo tooling and linters recognize
the Args/Returns/Raises entries.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
Collaborator
Author
|
/verified |
myakove
requested changes
Feb 4, 2026
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 945-959: The prepared_plan_2 fixture docstring is missing a Raises
section describing ValueError propagation from extract_vm_from_plan; update the
prepared_plan_2 docstring (like prepared_plan_1) to include a "Raises" entry
stating that a ValueError is raised if the source plan contains fewer than 2 VMs
(i.e., vm_index=1 is invalid), referencing extract_vm_from_plan and the vm_index
condition so readers know when the exception can occur.
- Around line 928-942: The fixture prepared_plan_1 calls extract_vm_from_plan
which can raise ValueError for insufficient VMs or missing VM data, so update
the prepared_plan_1 docstring to include a Raises section that documents
ValueError and when it is raised (e.g., insufficient VMs in prepared_plan or
missing VM in source_vms_data), referencing extract_vm_from_plan so callers know
the exception is propagated. Ensure the Raises entry is concise and follows the
existing docstring style.
Collaborator
Author
|
/verified |
myakove
approved these changes
Feb 5, 2026
Collaborator
|
/approve |
|
Failed to build and push ghcr.io/redhatqe/mtv-api-tests:latest |
3 tasks done
This was referenced Nov 4, 2026
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
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.
User description
Summary
This PR adds a new test to validate simultaneous execution of two copyoffload migration plans with MTV. The test demonstrates the ability to run multiple migrations concurrently, which is important for validating MTV's scalability and concurrent migration handling.
Changes
Configuration (
tests/tests_config/config.py)test_simultaneous_copyoffload_migrationsconfiguration that defines two VMs to be cloned from the same template (xcopy-template-test)Test Suite (
tests/test_copyoffload_migration.py)TestSimultaneousCopyoffloadMigrationsclass with 10 test methods:Reusable Fixtures (
conftest.py)prepared_plan_1fixture: Extracts first VM from multi-VM prepared planprepared_plan_2fixture: Extracts second VM from multi-VM prepared planKey Features
Test Execution
Prerequisites
This test requires:
xcopy-template-testtemplate.providers.jsonTesting Checklist
PR Type
Tests, Enhancement
Description
Add test class for simultaneous copyoffload migration execution
Create two independent migration plans with separate storage/network maps
Execute both migrations concurrently and validate results independently
Add reusable fixtures for multi-VM simultaneous migration testing
Diagram Walkthrough
File Walkthrough
conftest.py
Add fixtures for simultaneous migration plan preparationconftest.py
prepared_plan_1fixture to extract first VM from multi-VMprepared plan
prepared_plan_2fixture to extract second VM from multi-VMprepared plan
migrations
test_copyoffload_migration.py
Add simultaneous copyoffload migration test classtests/test_copyoffload_migration.py
TestSimultaneousCopyoffloadMigrationsclass with 10 test methodsMigrationresource and utility functions for concurrentexecution
config.py
Add simultaneous copyoffload migrations test configurationtests/tests_config/config.py
test_simultaneous_copyoffload_migrationsconfiguration entryxcopy-template-testtemplateSummary by CodeRabbit
Tests
Bug Fixes
Chores
Refactor