WIP: MTV-2628: Specify firstboot scripts for migration#309
Conversation
WalkthroughAdds an end-to-end MTV warm-migration test using virt-customize firstboot scripts: creates a ConfigMap, StorageMap, NetworkMap, and Plan CR, runs the migration (including cutover retrieval) and validates migrated VMs; also adds a test case entry with the ConfigMap payload. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Runner
participant OCP as OCP API
participant MigCtrl as Migration Controller
participant Src as Source Provider
participant Dst as Destination Provider
Test->>OCP: create ConfigMap (virt-customize scripts)
OCP-->>Test: ConfigMap created
Test->>OCP: create StorageMap & NetworkMap
OCP-->>Test: Maps created
Test->>OCP: create MTV Plan CR (populate VM IDs)
OCP-->>Test: Plan created
Test->>MigCtrl: trigger migration (execute_migration)
MigCtrl->>Src: quiesce / pre-copy VMs
MigCtrl->>Dst: provision target resources
MigCtrl-->>Test: migration started (cutover value available)
Test->>MigCtrl: request cutover value
MigCtrl-->>Test: return cutover value
Test->>MigCtrl: perform cutover
MigCtrl->>Dst: finalize VM boot
Test->>Dst: verify VMs (SSH/connectivity, run scripts)
Dst-->>Test: verification results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 8ebf6f5
Previous suggestionsSuggestions up to commit 8f492e4
Suggestions up to commit 015b1fa
|
||||||||||||||||||||||||||||||||||||||
|
This one is still in progress, no need for review, thank you |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_mtv_warm_migration_feature.py`:
- Around line 43-56: Add the pytest incremental marker to the
TestWarmVirtCustomizeFirstboot test class so downstream tests stop if an earlier
dependent step fails: annotate the class (TestWarmVirtCustomizeFirstboot) with
`@pytest.mark.incremental` alongside the existing markers so that sequential
methods (configmap → storagemap → networkmap → plan → migrate → check) will
fail-fast and not access uninitialized class attributes like self.storage_map.
- Around line 1-8: The module uses the typing symbols TYPE_CHECKING and Any but
never imports them; add an import for these symbols from the typing module
(e.g., include "from typing import TYPE_CHECKING, Any") at the top of the file
so references to TYPE_CHECKING and Any used in annotations and conditional
imports resolve correctly (look for usages of TYPE_CHECKING and Any in the file
to place the import).
- Around line 83-88: The test currently calls pytest.fail() when the fixture
test_configmap is falsy; remove that conditional and let the fixture
test_configmap perform validation or raise an exception (or mark the test with
`@pytest.mark.skipif` if the ConfigMap is optional). Update the test method that
sets self.__class__.configmap = test_configmap and the subsequent assertions
(self.configmap.exists and self.configmap.name checks) to assume a valid fixture
and simply assert the properties; ensure any creation/validation logic is moved
into the test_configmap fixture (or adjust the fixture to raise ValueError with
a clear message when creation fails).
- Around line 242-278: Remove the unused target_namespace parameter from the
test_check_vms signature: update the function definition of test_check_vms to
drop the target_namespace argument (it is never referenced) while keeping
prepared_plan, source_provider, destination_provider, source_provider_data,
source_vms_namespace, source_provider_inventory, and vm_ssh_connections; note
that check_vms reads the target namespace from prepared_plan via
plan["_vm_target_namespace"], so no other changes to the call site or to
check_vms are required.
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. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_mtv_warm_migration_feature.py`:
- Around line 252-266: The docstring for the "Validate migrated VMs" function
documents a non-existent parameter target_namespace; open the function that
lists prepared_plan, source_provider, destination_provider,
source_provider_data, source_vms_namespace, source_provider_inventory and
vm_ssh_connections in its signature (the validate/migration validation function
in tests/test_mtv_warm_migration_feature.py) and remove the "target_namespace:
Target namespace for migration" entry from the docstring (or, if the parameter
was intended, add it to the function signature consistently). Also ensure the
Args/Returns formatting remains consistent after removing the line.
- Around line 74-84: Remove the unnecessary "Returns:" section from the
docstring of the test that creates the virt-customize ConfigMap (the docstring
starting "Create virt-customize ConfigMap resource") and from the other test
methods listed (test_create_storagemap, test_create_networkmap,
test_create_plan, test_migrate_vms, test_check_vms); keep the rest of the
docstring (Args and Raises) intact so the docstrings describe purpose, args, and
raises but omit any Returns: block for these void test functions.
| """Create virt-customize ConfigMap resource. | ||
|
|
||
| Args: | ||
| test_configmap: ConfigMap fixture that creates the virt-customize ConfigMap | ||
|
|
||
| Returns: | ||
| None | ||
|
|
||
| Raises: | ||
| AssertionError: If ConfigMap creation fails or doesn't exist | ||
| """ |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LOW: Omit Returns: section for void functions.
WHY: Per repository convention, docstrings for functions returning None should not include a Returns: section. It adds noise without providing value.
🔧 Suggested fix
"""Create virt-customize ConfigMap resource.
Args:
test_configmap: ConfigMap fixture that creates the virt-customize ConfigMap
- Returns:
- None
-
Raises:
AssertionError: If ConfigMap creation fails or doesn't exist
"""This same pattern applies to all other test methods in this class (test_create_storagemap, test_create_networkmap, test_create_plan, test_migrate_vms, test_check_vms).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mtv_warm_migration_feature.py` around lines 74 - 84, Remove the
unnecessary "Returns:" section from the docstring of the test that creates the
virt-customize ConfigMap (the docstring starting "Create virt-customize
ConfigMap resource") and from the other test methods listed
(test_create_storagemap, test_create_networkmap, test_create_plan,
test_migrate_vms, test_check_vms); keep the rest of the docstring (Args and
Raises) intact so the docstrings describe purpose, args, and raises but omit any
Returns: block for these void test functions.
Signed-off-by: Chenli Hu <chhu@redhat.com>
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/test_mtv_warm_migration_feature.py (1)
113-115: 🧹 Nitpick | 🔵 TrivialLOW: Remove
Returns: Nonesection from void function docstring.WHY: Per repository convention documented in past reviews, docstrings for functions returning
Noneshould not include aReturns:section—it adds noise without value.This applies to this method and also to
test_create_networkmap(lines 154-155),test_create_plan(lines 194-195), andtest_migrate_vms(lines 228-229).🔧 Suggested fix for this method
Args: ... target_namespace: Target namespace for migration - Returns: - None - Raises: AssertionError: If StorageMap creation fails🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_mtv_warm_migration_feature.py` around lines 113 - 115, Remove the unnecessary "Returns: None" docstring section from the void test functions: the test method with the docstring around the first diff and the functions test_create_networkmap, test_create_plan, and test_migrate_vms; edit each function's docstring to omit the entire Returns block so the docstring only contains the description/setup lines (no code changes otherwise), preserving existing indentation and triple-quote style.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_mtv_warm_migration_feature.py`:
- Around line 28-29: The test silently masks a missing project config by calling
py_config.get("source_provider", "") when computing _SOURCE_PROVIDER_TYPE;
change this to directly read py_config["source_provider"] (or first check
py_config.get("source_provider") and set _SOURCE_PROVIDER_TYPE to None if
missing) and then call load_source_providers().get(value, {}).get("type") only
when a value is present so missing configuration fails loudly or results in an
explicit None; update the logic around _SOURCE_PROVIDER_TYPE and any skipif
checks that depend on it (referencing _SOURCE_PROVIDER_TYPE, py_config, and
load_source_providers) accordingly.
---
Duplicate comments:
In `@tests/test_mtv_warm_migration_feature.py`:
- Around line 113-115: Remove the unnecessary "Returns: None" docstring section
from the void test functions: the test method with the docstring around the
first diff and the functions test_create_networkmap, test_create_plan, and
test_migrate_vms; edit each function's docstring to omit the entire Returns
block so the docstring only contains the description/setup lines (no code
changes otherwise), preserving existing indentation and triple-quote style.
| _SOURCE_PROVIDER_TYPE = load_source_providers().get(py_config.get("source_provider", ""), {}).get("type") | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
MEDIUM: Avoid .get() with defaults for project-controlled configuration.
WHY this matters: Using py_config.get("source_provider", "") silently masks missing configuration. If source_provider isn't configured, the test will run with an empty string and the skipif logic won't work as intended—it may incorrectly allow tests to run when they shouldn't.
The guideline states project configs should use direct access so configuration errors surface early.
🔧 Proposed fix
-_SOURCE_PROVIDER_TYPE = load_source_providers().get(py_config.get("source_provider", ""), {}).get("type")
+_SOURCE_PROVIDER_TYPE = load_source_providers().get(py_config["source_provider"], {}).get("type")If source_provider is truly optional for some test runs, add explicit validation:
_source_provider = py_config.get("source_provider")
if not _source_provider:
_SOURCE_PROVIDER_TYPE = None
else:
_SOURCE_PROVIDER_TYPE = load_source_providers().get(_source_provider, {}).get("type")As per coding guidelines: "For configurations controlled by the project (py_config, plan, tests_params), never use defaults. Always use direct access."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_mtv_warm_migration_feature.py` around lines 28 - 29, The test
silently masks a missing project config by calling
py_config.get("source_provider", "") when computing _SOURCE_PROVIDER_TYPE;
change this to directly read py_config["source_provider"] (or first check
py_config.get("source_provider") and set _SOURCE_PROVIDER_TYPE to None if
missing) and then call load_source_providers().get(value, {}).get("type") only
when a value is present so missing configuration fails loudly or results in an
explicit None; update the logic around _SOURCE_PROVIDER_TYPE and any skipif
checks that depend on it (referencing _SOURCE_PROVIDER_TYPE, py_config, and
load_source_providers) accordingly.
Either convert to draft or set as WIP |
|
/wip |
redhat-qe-bot2
changed the title
6 participants
PR Type
Tests, Enhancement
Description
Add comprehensive warm migration test with virt-customize firstboot scripts
Test ConfigMap creation with firstboot and run shell scripts
Validate StorageMap, NetworkMap, and Plan resource creation
Execute warm migration with cutover and verify VM migration success
Add test configuration for warm migration with virt-customize feature
Diagram Walkthrough
File Walkthrough
test_mtv_warm_migration_feature.py
Warm migration test with virt-customize firstboot scriptstests/test_mtv_warm_migration_feature.py
TestWarmVirtCustomizeFirstbootvirt-customize
for RHV
config.py
Add warm migration virt-customize test configurationtests/tests_config/config.py
test_warm_virt_customize_firstbootto testparameters
mtv-feature-rhel9with warm migration enabled01_linux_firstboot_test.shand
01_linux_run_test.shSummary by CodeRabbit