feat: Add MTV 2.10.0 target features testing #247
Conversation
Implement comprehensive testing for MTV 2.10.0 targeting features: - targetNodeSelector: Schedule VMs to labeled worker nodes - targetLabels: Apply custom labels to migrated VMs - targetAffinity: Configure pod affinity rules for migrated VMs Includes MTV version detection to skip feature tests on older clusters.
WalkthroughAdds migration controls for VM node placement, labels, and pod affinity via new plan keys ( Changes
Sequence Diagram(s)sequenceDiagram
actor Test
participant Fixture as labeled_worker_node Fixture
participant K8sAPI as Kubernetes API
participant Prometheus as Prometheus/Thanos
participant MTV as MTV Migration
participant Validator as Post-Migration Validator
Test->>Fixture: request labeled_worker_node
Fixture->>K8sAPI: list worker nodes
K8sAPI-->>Fixture: worker node list
Fixture->>Prometheus: query memory metrics
Prometheus-->>Fixture: memory metrics
Fixture->>K8sAPI: apply label to chosen node
K8sAPI-->>Fixture: label applied
Fixture-->>Test: return labeled_worker_node / labeled_vm
Test->>MTV: call migrate_vms(labeled_worker_node, labeled_vm, target_affinity)
MTV->>MTV: get_mtv_version / is_mtv_version_supported
alt MTV >= 2.10.0
MTV->>K8sAPI: build plan with node selector, labels, affinity
else
MTV->>K8sAPI: build plan without affinity
end
MTV->>MTV: execute migration
MTV-->>Test: migration complete
Test->>Validator: call check_vms(...)
Validator->>Validator: get_mtv_version / is_mtv_version_supported
alt MTV >= 2.10.0
Validator->>K8sAPI: verify VM node placement
Validator->>K8sAPI: verify VM labels
Validator->>K8sAPI: verify VM affinity
else
Validator->>K8sAPI: verify base VM state only
end
K8sAPI-->>Validator: verification results
Validator-->>Test: aggregated validation outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Key Areas for Review AttentionCRITICAL:
HIGH:
MEDIUM:
LOW:
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_copyoffload_migration.py (1)
12-12: LOW: Remove unusedimport time.The
timemodule is imported but not used anywhere in this file.♻️ Proposed fix
-import time
🤖 Fix all issues with AI agents
In @conftest.py:
- Around line 691-692: Remove the deprecated duplicate `socket.timeout` from the
exception tuple in the except clause so only `TimeoutError` is used; update the
tuple in the handler that currently reads `(URLError, TimeoutError,
socket.timeout, json.JSONDecodeError)` to `(URLError, TimeoutError,
json.JSONDecodeError)` so the `LOGGER.warning` block that logs "Prometheus query
failed for '{query}': {e}" continues to catch timeouts via `TimeoutError`
without the redundant alias.
- Around line 802-803: The code creates temporary lists to extract the first
element from target_node_selector; replace the two list(...) calls with a single
iterator-based extraction to avoid allocating lists — e.g., obtain label_key and
config_value from target_node_selector by using
next(iter(target_node_selector.items())) (or next(iter(target_node_selector))
for only the key) so that label_key and config_value are assigned directly
without creating intermediate lists.
- Around line 710-721: The loops that parse Prometheus responses assume
item.get("value", [None, "0"])[1] exists and is numeric; defensively validate
the value before converting to float to avoid IndexError/ValueError: for both
places where allocatable_result and requested_result are iterated (use the
variables allocatable_result, requested_result, worker_nodes, metrics, and node
to locate the code), check that item.get("value") is a sequence with length >= 2
and that the second element is not None/empty, fall back to "0" if missing, and
wrap the float/int conversion in a try/except (or conditional) to handle
non-numeric data so metrics.setdefault(...)/metrics[node][...] always receives a
safe int.
- Around line 675-693: The _query_prometheus function currently opens whatever
URL is passed in; before building the request, parse the provided url with
urllib.parse.urlparse and validate the scheme is "https" (reject other schemes),
logging a warning and returning None for invalid schemes to avoid SSRF. Place
this check at the start of _query_prometheus (before request_url
creation/urllib.request.urlopen) and only proceed to build the request and open
the URL when the scheme is valid; keep the existing error handling for
network/JSON errors intact.
In @libs/providers/openshift.py:
- Around line 157-163: The code accesses cnv_vm.instance.spec.template multiple
times; extract template = cnv_vm.instance.spec.template once and use
template.metadata and template.spec for the labels and affinity assignments
(i.e., set result_vm_info["labels"] from template.metadata.labels if present,
and result_vm_info["affinity"] from template.spec.affinity if present), keeping
the same fallback values; this reduces duplicate attribute access and aligns
with the prior use of template.spec.domain in the surrounding code.
In @utilities/mtv_migration.py:
- Around line 64-71: The current code in mtv_migration.py masks target_affinity
by setting it to None when MTV < 2.10.0, which lets affinity tests run and
falsely pass; instead add a defensive test fixture (mirroring the behavior of
labeled_worker_node and labeled_vm in conftest.py) that inspects the migration
plan and calls pytest.skip() if plan contains "target_affinity" but
is_mtv_version_supported(ocp_admin_client, "2.10.0") is False, so
post_migration.py’s validation (which checks plan.get("target_affinity")) is not
accidentally bypassed; alternatively, stop zeroing target_affinity in
mtv_migration.py and log then let the fixture handle skipping so tests are
consistently skipped on unsupported MTV versions.
In @utilities/post_migration.py:
- Around line 1223-1232: The MTV version check block catches RuntimeError but
writes the error into res under the inconsistent "_mtv_version" key; update the
exception handling in the try/except around
is_mtv_version_supported(destination_provider.ocp_resource.client, "2.10.0") to
keep catching RuntimeError but store the error message under the
VM/provider-consistent key "_provider" (e.g., replace
res.setdefault("_mtv_version", ...) with res.setdefault("_provider", ...)) and
leave mtv_version_supported set to False on error so behavior is unchanged.
- Around line 1243-1262: The try/except blocks around check_vm_node_placement,
check_vm_labels, and check_vm_affinity are catching broad Exception (in
functions check_vm_node_placement, check_vm_labels, check_vm_affinity) which can
hide programming errors; update each block to catch only the expected
verification error type(s) (e.g., AssertionError or a domain-specific
VerificationError) and let other exceptions propagate (or re-raise them), and
when appending the error message to res[vm_name] use an explicit string
conversion for the exception (e.g., format with {exp!s}) to satisfy explicit
conversion checks.
In @utilities/utils.py:
- Around line 126-129: The current check treats an existing but empty csvs.items
as falsy and wraps csvs incorrectly; change the branch that sets csv_list so it
uses the presence of the attribute rather than its truthiness: replace "if
hasattr(csvs, 'items') and csvs.items:" with "if hasattr(csvs, 'items'):" and
leave "csv_list = list(csvs.items)" as-is so empty item lists become an empty
csv_list; keep the else branch "csv_list = [csvs] if csvs is not None else []"
unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
README.mdconftest.pylibs/providers/openshift.pypyproject.tomltests/test_copyoffload_migration.pytests/test_mtv_cold_migration.pytests/test_mtv_warm_migration.pyutilities/migration_utils.pyutilities/mtv_migration.pyutilities/post_migration.pyutilities/utils.py
🧰 Additional context used
🧬 Code graph analysis (8)
utilities/utils.py (1)
conftest.py (1)
ocp_admin_client(393-404)
libs/providers/openshift.py (1)
libs/base_provider.py (1)
vm_dict(70-74)
tests/test_mtv_warm_migration.py (1)
conftest.py (2)
labeled_worker_node(784-812)labeled_vm(816-842)
conftest.py (1)
utilities/utils.py (1)
is_mtv_version_supported(154-162)
tests/test_mtv_cold_migration.py (1)
conftest.py (2)
labeled_worker_node(784-812)labeled_vm(816-842)
utilities/migration_utils.py (1)
conftest.py (3)
labeled_worker_node(784-812)labeled_vm(816-842)plan(846-947)
utilities/mtv_migration.py (2)
utilities/utils.py (1)
is_mtv_version_supported(154-162)conftest.py (4)
labeled_worker_node(784-812)labeled_vm(816-842)ocp_admin_client(393-404)plan(846-947)
tests/test_copyoffload_migration.py (1)
conftest.py (2)
labeled_worker_node(784-812)labeled_vm(816-842)
🪛 Ruff (0.14.10)
utilities/utils.py
145-145: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
150-150: Logging statement uses f-string
(G004)
utilities/post_migration.py
979-979: Logging statement uses f-string
(G004)
1021-1021: Logging statement uses f-string
(G004)
1055-1055: Logging statement uses f-string
(G004)
1247-1247: Do not catch blind exception: Exception
(BLE001)
1248-1248: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1254-1254: Do not catch blind exception: Exception
(BLE001)
1255-1255: Use explicit conversion flag
Replace with conversion flag
(RUF010)
1261-1261: Do not catch blind exception: Exception
(BLE001)
1262-1262: Use explicit conversion flag
Replace with conversion flag
(RUF010)
conftest.py
671-671: Logging statement uses f-string
(G004)
675-675: Boolean-typed positional argument in function definition
(FBT001)
675-675: Boolean default positional argument in function definition
(FBT002)
679-679: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
687-687: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
691-691: Replace aliased errors with TimeoutError
Replace with builtin TimeoutError
(UP041)
692-692: Logging statement uses f-string
(G004)
736-736: Avoid specifying long messages outside the exception class
(TRY003)
743-743: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
748-748: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
753-753: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
757-757: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
759-759: Logging statement uses f-string
(G004)
778-778: Logging statement uses f-string
(G004)
780-780: Logging statement uses f-string
(G004)
783-783: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
802-802: Prefer next(iter(target_node_selector.keys())) over single element slice
Replace with next(iter(target_node_selector.keys()))
(RUF015)
803-803: Prefer next(iter(target_node_selector.values())) over single element slice
Replace with next(iter(target_node_selector.values()))
(RUF015)
807-807: Logging statement uses f-string
(G004)
815-815: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
837-837: Logging statement uses f-string
(G004)
840-840: Logging statement uses f-string
(G004)
utilities/migration_utils.py
241-241: Logging statement uses f-string
(G004)
247-247: Logging statement uses f-string
(G004)
utilities/mtv_migration.py
71-71: Logging statement uses f-string
(G004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: tox
- GitHub Check: pre-commit
- GitHub Check: build-container
🔇 Additional comments (30)
libs/providers/openshift.py (2)
9-9: LGTM!Import is correctly placed and necessary for the
NotFoundErrorhandling in the node_name extraction block.
171-183: Good defensive implementation for node placement capture.LOW (observation only): The try/except with both
AttributeErrorandNotFoundErrorcombined with theifchain is appropriately defensive. WHY this matters: the VMI'snodeNamemay not be immediately available during the scheduling phase, and the resource could theoretically be deleted between checks (race condition).Note that this block is more defensive than existing code (line 185 directly iterates
cnv_vm.vmi.interfaces). Sincestart_vm(wait=True)is called before this, the VMI should exist, but the extra caution fornodeNamespecifically is justified because node assignment is an asynchronous scheduling decision.pyproject.toml (1)
39-39: LGTM! Version constraint aligns with new version parsing functionality.The
packaging>=24.0constraint is appropriate given the newis_mtv_version_supported()utility that usespackaging.version.parse()for MTV version comparisons.README.md (1)
215-237: LGTM! Clear documentation for the new MTV 2.10.0 targeting features.The documentation effectively explains:
target_node_selectorwith auto-generation optiontarget_labelswith both auto and explicit value supporttarget_affinitywith proper pod affinity structureThe note about Prometheus-based node selection with fallback behavior (lines 218-219) is a helpful addition for users who may not have cluster metrics available.
utilities/utils.py (1)
154-162: LGTM! Clean version comparison implementation.The function correctly uses
packaging.version.parse()for semantic version comparison, which properly handles pre-release versions and various version string formats.conftest.py (2)
783-812: LGTM! Well-structured fixture with proper cleanup.The
labeled_worker_nodefixture correctly:
- Returns
Nonewhen feature not configured (allows tests to run without the feature)- Skips test if MTV version < 2.10.0
- Uses Prometheus-based intelligent node selection with fallback
- Properly cleans up the node label in teardown
815-842: LGTM! Clean fixture implementation for VM label generation.The
labeled_vmfixture properly handles:
- Early return with
Nonewhen labels not configured- MTV version gating with clear skip message
- Both "auto" (session UUID) and explicit label values
tests/test_mtv_cold_migration.py (2)
37-38: LGTM! Fixtures correctly integrated into cold migration tests.The new
labeled_worker_nodeandlabeled_vmfixtures are properly added as parameters and passed through tomigrate_vms, enabling node selector and label testing for cold migrations.Also applies to: 70-71
100-101: LGTM! Consistent fixture integration in remote OCP test.Same pattern applied correctly to the remote OCP test function.
Also applies to: 128-129
tests/test_mtv_warm_migration.py (2)
55-56: LGTM! Fixtures integrated into warm migration test.The new fixtures are correctly added and passed through to
migrate_vmsfor the sanity warm migration test.Also applies to: 84-85
116-117: LGTM! Consistent fixture integration across remaining warm migration tests.Both
test_mtv_migration_warm_2disks2nicsandtest_warm_remote_ocpfollow the same correct pattern.Also applies to: 145-146, 176-177, 205-206
tests/test_copyoffload_migration.py (4)
55-56: LGTM! Fixtures and docstrings properly integrated into thin migration test.The new fixtures are correctly added as parameters, passed to
migrate_vms, and documented in the docstring. This enables node selector and label testing for copy-offload migrations.Also applies to: 112-114, 174-176
207-208: LGTM! Consistent integration in thick lazy migration test.Same correct pattern applied with updated docstrings.
Also applies to: 267-269, 330-332
363-365: LGTM! Fixtures integrated across multi-disk migration tests.Both
test_copyoffload_multi_disk_migrationandtest_copyoffload_multi_disk_different_path_migrationcorrectly integrate the new fixtures.Also applies to: 423-425, 488-490, 523-525, 558-560, 622-624
657-658: LGTM! RDM test properly updated with new fixtures and documentation.The RDM virtual disk migration test correctly integrates the new fixtures, with updated docstring documenting all parameters including the new
labeled_worker_nodeandlabeled_vmfixtures.Also applies to: 689-691, 749-751
utilities/post_migration.py (5)
29-29: LGTM! Import addition aligns with the new MTV version gating requirements.
959-980: LGTM! Well-structured helper function for node placement verification.The function correctly:
- Validates node assignment exists before comparison
- Uses
pytest.failconsistently with other check functions- Provides informative error messages with expected vs actual values
982-1022: LGTM! Comprehensive label verification with detailed error reporting.The function properly:
- Handles missing and incorrect labels separately for clear diagnostics
- Converts potential proxy objects to dict for reliable comparison
- Aggregates all mismatches before failing for complete feedback
1024-1056: LGTM! Affinity verification handles object-to-dict conversion correctly.The
to_dict()pattern handles Kubernetes resource objects that may not directly compare as dicts.
1118-1119: LGTM! Signature extension follows established patterns.The new optional parameters with
Nonedefaults maintain backward compatibility.utilities/migration_utils.py (3)
203-206: LGTM! Signature extension with optional parameters maintains backward compatibility.The type hints
dict | Noneaccurately reflect the nullable nature of these parameters.
237-248: LGTM! Guard conditions correctly require both plan config AND fixture data.The dual-condition pattern (
plan.get(...) and labeled_* is not None) prevents partial configuration issues:
- If plan specifies
target_node_selectorbut fixture didn't run →Nonepropagates- If fixture ran but plan doesn't specify → also
NoneThis defensive approach aligns with the fixture behavior in
conftest.pythat yieldsNonewhen features aren't configured.
273-275: LGTM! Return values correctly propagate the prepared configuration.The keys align with the
run_migrationsignature expectations inutilities/mtv_migration.py.utilities/mtv_migration.py (7)
28-28: LGTM! Import aligns with new MTV version gating functionality.
52-53: LGTM! Signature extension follows established patterns with optional parameters.
89-91: LGTM! Parameters correctly forwarded toprepare_migration_for_tests.
113-114: LGTM! Parameters correctly forwarded tocheck_vmsfor post-migration verification.
142-144: LGTM! Signature extension with appropriate type hints.The
dict[str, str]for selectors/labels anddictfor affinity correctly represent the expected structures.
169-171: LGTM! Docstring additions clearly document the new parameters.
201-203: LGTM! Plan kwargs correctly include all three new target feature parameters.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @conftest.py:
- Around line 674-697: The boolean parameter insecure on _query_prometheus
should be made keyword-only to avoid positional confusion; change the function
signature to require insecure be passed as a keyword (e.g., add a lone * before
insecure) and update any call sites to call insecure=<bool> rather than relying
on positional ordering, leaving the default True intact and keeping existing
validation and SSL handling logic unchanged.
In @utilities/post_migration.py:
- Around line 1223-1262: The catch blocks around check_vm_node_placement,
check_vm_labels, and check_vm_affinity currently catch AssertionError but the
helpers call pytest.fail() which raises pytest Failed (a subclass of Exception),
so change those except clauses to catch Exception (consistent with other checks
in this file) so failures are collected instead of aborting; update the except
lines for check_vm_node_placement, check_vm_labels, and check_vm_affinity to
"except Exception as exp" and keep the existing res[vm_name].append(...)
behavior.
In @utilities/utils.py:
- Line 34: The import of packaging.version is misplaced after a local import and
breaks import ordering; move "from packaging import version" up into the
third-party imports block alongside other external imports (near where other
non-stdlib/ non-local imports are declared, e.g., around the existing
third-party imports) and remove the duplicate line from after the local import
of utilities.resources so that imports follow the stdlib → third-party → local
convention.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
conftest.pylibs/providers/openshift.pyutilities/mtv_migration.pyutilities/post_migration.pyutilities/utils.py
🧰 Additional context used
🧬 Code graph analysis (5)
utilities/utils.py (1)
conftest.py (1)
ocp_admin_client(392-403)
libs/providers/openshift.py (5)
libs/providers/rhv.py (2)
start_vm(132-142)vm_dict(156-220)libs/providers/openstack.py (2)
start_vm(372-394)vm_dict(139-206)libs/providers/vmware.py (2)
start_vm(262-264)vm_dict(507-602)libs/base_provider.py (1)
vm_dict(70-74)libs/providers/ova.py (1)
vm_dict(30-35)
utilities/mtv_migration.py (2)
utilities/utils.py (1)
is_mtv_version_supported(154-162)conftest.py (4)
labeled_worker_node(802-829)labeled_vm(833-859)plan(863-964)ocp_admin_client(392-403)
conftest.py (1)
utilities/utils.py (1)
is_mtv_version_supported(154-162)
utilities/post_migration.py (2)
utilities/utils.py (2)
get_value_from_py_config(390-405)is_mtv_version_supported(154-162)conftest.py (4)
labeled_worker_node(802-829)labeled_vm(833-859)destination_provider(459-474)plan(863-964)
🪛 Ruff (0.14.10)
utilities/utils.py
145-145: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
150-150: Logging statement uses f-string
(G004)
utilities/mtv_migration.py
70-70: Logging statement uses f-string
(G004)
conftest.py
670-670: Logging statement uses f-string
(G004)
674-674: Boolean-typed positional argument in function definition
(FBT001)
674-674: Boolean default positional argument in function definition
(FBT002)
678-678: Logging statement uses f-string
(G004)
683-683: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
691-691: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
696-696: Logging statement uses f-string
(G004)
754-754: Avoid specifying long messages outside the exception class
(TRY003)
761-761: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
766-766: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
771-771: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
775-775: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
777-777: Logging statement uses f-string
(G004)
796-796: Logging statement uses f-string
(G004)
798-798: Logging statement uses f-string
(G004)
801-801: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
824-824: Logging statement uses f-string
(G004)
832-832: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
854-854: Logging statement uses f-string
(G004)
857-857: Logging statement uses f-string
(G004)
utilities/post_migration.py
979-979: Logging statement uses f-string
(G004)
1021-1021: Logging statement uses f-string
(G004)
1055-1055: Logging statement uses f-string
(G004)
🔇 Additional comments (22)
utilities/utils.py (2)
110-151: LGTM! Well-structured MTV version detection.The function has proper defensive checks for None values, uses safe attribute access with
hasattr/getattr, and provides clear error messages. The approach of searching CSVs by display name is reliable for operator detection.
154-162: LGTM! Clean version comparison implementation.Good use of
packaging.version.parse()for proper semantic version comparison. The function correctly propagatesRuntimeErrorfromget_mtv_version()as documented.libs/providers/openshift.py (3)
9-9: LGTM! Appropriate import addition.
NotFoundErrorimport is correctly added for handling cases where VMI might not exist during node name extraction.
153-166: LGTM! Clean template extraction with proper null-safety.Good optimization to extract template once and derive multiple fields. The defensive checks for
template,template_metadata, andtemplate_specensure no AttributeError surprises.
174-186: LGTM! Robust node name extraction with appropriate error handling.The extraction correctly happens after
start_vm()since VMI only exists when the VM is running. The chain of defensive checks and exception handling forAttributeError/NotFoundErroris appropriate for this scenario where the VMI might not be fully populated.utilities/post_migration.py (4)
29-29: LGTM! Import updated correctly.The
is_mtv_version_supportedimport is properly added alongside existing utilities imports.
959-979: LGTM! Clean node placement validation.The function provides clear error messages showing both expected and actual node names, making debugging straightforward.
982-1021: LGTM! Thorough label validation with detailed error reporting.Good implementation that separates missing labels from incorrect values, making it easy to diagnose exactly what's wrong. The dict conversion handles Kubernetes ResourceField objects properly.
1024-1055: Remove: Dict equality comparison concern is based on incorrect assumptions about Python dict behavior.The
!=comparison on line 1048 works correctly for affinity dictionaries. Python dicts compare by content, not key ordering—nested structures with different key ordering will compare equal. The code already normalizes the actual affinity viato_dict()when available, matching the structure of the expected affinity dict from test data. A type mismatch between actual and expected values would be caught correctly by the inequality operator. No evidence of failures or type inconsistencies exists in the codebase for this comparison pattern.utilities/mtv_migration.py (4)
6-6: LGTM! Import additions for MTV version support.The
pytestimport enablespytest.skip()for version checks, andis_mtv_version_supportedis properly imported from utilities.Also applies to: 29-29
65-70: LOW: Potentially redundant MTV version check for target_affinity.The
labeled_worker_nodeandlabeled_vmfixtures in conftest.py already callpytest.skip()if MTV version is below 2.10.0. However,target_affinitydoesn't have a fixture, so this check is correct and necessary for that feature.The current implementation is actually correct because:
targetNodeSelector→ handled bylabeled_worker_nodefixturetargetLabels→ handled bylabeled_vmfixturetargetAffinity→ requires this check since there's no corresponding fixture
141-143: LGTM! Clean parameter propagation for target features.New parameters are correctly added to both the function signature and
plan_kwargsdict. The docstrings accurately describe each parameter's purpose and behavior whenNone.Also applies to: 200-202
101-114: LGTM! Correct propagation of labeling fixtures to validation.The
labeled_worker_nodeandlabeled_vmparameters are properly passed through tocheck_vms()for post-migration validation.conftest.py (9)
8-8: LGTM! Import additions for new functionality.All imports are appropriate for the new Prometheus-based node selection and labeling features:
random,ssl,urllibfor Prometheus queryingclient,ApiExceptionfrom kubernetes for node operationsRoutefor Thanos URL discoveryis_mtv_version_supportedfor version gatingAlso applies to: 10-12, 16-16, 19-20, 29-29, 70-70, 79-79
652-656: LGTM! Clean worker node enumeration.Simple and effective filtering by the standard
node-role.kubernetes.io/workerlabel.
659-671: LGTM! Correct Prometheus URL discovery.Using the
thanos-querierroute fromopenshift-monitoringnamespace is the proper way to access cluster metrics in OpenShift.
700-745: LGTM! Robust Prometheus metrics parsing with good defensive coding.The function handles edge cases well:
- Missing allocatable metrics returns
None- Missing requested metrics defaults to 0
- Malformed values caught with try/except
- Only includes nodes that are in the worker_nodes list
The PromQL for requested memory correctly filters to only running pods.
748-779: LGTM! Smart node selection with graceful fallbacks.The function implements a sensible strategy:
- Try metrics-based selection (highest available memory)
- Fall back to random selection if metrics unavailable
The S311 static analysis warnings about
random.choiceare false positives here — this is for test scheduling, not cryptographic purposes. Random selection among worker nodes is perfectly appropriate.
782-788: LGTM! Correct node labeling implementation.Proper read-modify-patch pattern with handling for nodes that might not have any labels initially.
791-798: LGTM! Correct label cleanup with appropriate error handling.Using
{label_key: None}in a strategic merge patch is the correct Kubernetes pattern for removing a label. Catching and loggingApiExceptionrather than raising is appropriate for teardown code — cleanup failures shouldn't fail the test.
801-829: LGTM! Well-structured fixture with proper lifecycle management.The fixture correctly:
- Returns
Noneearly if feature not configured (allows tests to run without this feature)- Skips tests on unsupported MTV versions
- Selects optimal node based on memory availability
- Cleans up the label after test completion
The
scope="function"is explicit but indeed implied by default — removing it would be a minor style cleanup.
832-859: LGTM! Clean label generation fixture.The fixture correctly:
- Returns
Noneif target_labels not configured- Skips on unsupported MTV versions
- Replaces "auto" values with session UUID for uniqueness
- No teardown needed since this generates label data, not cluster resources
- Make insecure parameter keyword-only in conftest.py - Change exception handling from AssertionError to Exception in post_migration.py - Fix import ordering in utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
conftest.pyutilities/post_migration.pyutilities/utils.py
🧰 Additional context used
🧬 Code graph analysis (3)
utilities/utils.py (1)
conftest.py (1)
ocp_admin_client(392-403)
utilities/post_migration.py (2)
utilities/utils.py (2)
get_value_from_py_config(390-405)is_mtv_version_supported(154-162)conftest.py (3)
labeled_worker_node(802-829)labeled_vm(833-859)plan(863-964)
conftest.py (1)
utilities/utils.py (1)
is_mtv_version_supported(154-162)
🪛 Ruff (0.14.10)
utilities/utils.py
145-145: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
150-150: Logging statement uses f-string
(G004)
utilities/post_migration.py
979-979: Logging statement uses f-string
(G004)
1021-1021: Logging statement uses f-string
(G004)
1055-1055: Logging statement uses f-string
(G004)
1247-1247: Do not catch blind exception: Exception
(BLE001)
1254-1254: Do not catch blind exception: Exception
(BLE001)
1261-1261: Do not catch blind exception: Exception
(BLE001)
conftest.py
670-670: Logging statement uses f-string
(G004)
678-678: Logging statement uses f-string
(G004)
683-683: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
691-691: Audit URL open for permitted schemes. Allowing use of file: or custom schemes is often unexpected.
(S310)
696-696: Logging statement uses f-string
(G004)
754-754: Avoid specifying long messages outside the exception class
(TRY003)
761-761: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
766-766: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
771-771: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
775-775: Standard pseudo-random generators are not suitable for cryptographic purposes
(S311)
777-777: Logging statement uses f-string
(G004)
796-796: Logging statement uses f-string
(G004)
798-798: Logging statement uses f-string
(G004)
801-801: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
824-824: Logging statement uses f-string
(G004)
832-832: scope='function' is implied in @pytest.fixture()
Remove implied scope argument
(PT003)
854-854: Logging statement uses f-string
(G004)
857-857: Logging statement uses f-string
(G004)
🔇 Additional comments (14)
utilities/utils.py (2)
23-23: LGTM - Import addition is appropriate.The
packaging.versionimport is the standard library for semantic version comparison in Python. Good choice over rolling your own version parsing logic.
154-162: LGTM - Clean version comparison logic.The function correctly uses
packaging.version.parse()which handles semantic versioning properly, including pre-release versions. The caller controlsmin_version, so invalid input is unlikely.One edge case: if
get_mtv_versionreturns an invalid version string (e.g., "unknown"),version.parse()will raiseInvalidVersion. This would bubble up as an unexpected exception rather than the documentedRuntimeError. Consider whether this needs handling:# Optional defensive check try: return version.parse(mtv_version) >= version.parse(min_version) except InvalidVersion as e: raise RuntimeError(f"Invalid MTV version format: {mtv_version}") from eutilities/post_migration.py (4)
959-979: LGTM - Clean node placement verification.The function follows the established pattern of other
check_*functions in this module. Usingpytest.fail()is consistent with sibling functions likecheck_cpu,check_memory, etc.
982-1021: LGTM - Well-structured label verification with good error reporting.The function provides clear differentiation between missing and incorrect labels in the error message, which will make debugging test failures straightforward. Converting to
dict()on line 1000 correctly handles Kubernetes API ResourceField objects.
1024-1055: LGTM - Affinity verification with proper type handling.The function correctly handles both raw dicts and Kubernetes API objects by checking for
to_dict()method. Python's dict equality comparison is order-independent for keys, so the direct!=comparison on line 1048 will work correctly for nested structures.
1223-1262: LGTM - MTV feature checks integrated following existing patterns.The broad
Exceptioncatches (lines 1247, 1254, 1261) are flagged by static analysis (BLE001), but this pattern is intentional and consistent with the rest ofcheck_vms- the function collects all failures rather than failing fast. Each check appends errors tores[vm_name]for consolidated reporting.HIGH: Defensive null check is good practice (line 1238-1241) - Pre-validating
node_namebefore calling the check function prevents cryptic failures.conftest.py (8)
652-656: LGTM - Simple and correct worker node detection.
659-671: LGTM - Robust Prometheus URL discovery with graceful fallback.The function correctly queries the
thanos-querierroute inopenshift-monitoringnamespace. ReturningNoneon failure allows callers to implement fallback behavior (random node selection).
674-697: LGTM - Secure Prometheus querying with proper validation.The URL scheme validation (lines 676-679) addresses the S310 static analysis hint by rejecting non-HTTPS URLs. The
insecureparameter correctly uses keyword-only syntax (*, insecure: bool = True), making it explicit at call sites.Note on SSL verification: The default
insecure=Trueis appropriate for internal cluster communication where the Thanos route uses self-signed certificates. The parameter name makes the security implication clear.
700-745: LGTM - Robust memory metrics parsing with defensive value handling.The PromQL queries (lines 704-705) correctly:
- Query allocatable memory per node
- Sum requested memory only for running pods (using phase="Running" == 1 join)
The value parsing (lines 718-724, 731-738) is defensive, handling:
- Missing or malformed Prometheus response structures
- Non-numeric values with fallback to 0
- Edge cases where only allocatable data exists (requested defaults to 0)
748-779: LGTM - Smart node selection with appropriate fallbacks.LOW: S311 static analysis hints are false positives. The
random.choice()calls are for test infrastructure node selection, not cryptographic purposes. Usingsecrets.choice()would add unnecessary complexity.The selection logic is sound:
- Try to get auth token → fallback to random if unavailable
- Try to get Prometheus URL → fallback to random
- Try to get metrics → fallback to random
- Select node(s) with maximum available memory
- If tie, randomly choose among them (line 775)
This layered fallback ensures tests can run even without Prometheus access.
782-798: LGTM - Correct node labeling with proper cleanup.Line 794: Setting a label value to
Nonein a strategic merge patch correctly removes the label - this is the idiomatic Kubernetes approach.Line 797-798: Catching
ApiExceptionduring cleanup and logging a warning (rather than raising) is appropriate - cleanup failures shouldn't fail tests that have already passed.
801-829: LGTM - Well-structured fixture with proper setup/teardown.The fixture correctly:
- Short-circuits early if
target_node_selectornot configured (lines 804-807)- Gates feature on MTV 2.10.0+ (lines 809-810)
- Uses smart node selection with Prometheus fallback (line 818)
- Supports
"auto"placeholder for dynamic label values (line 821)- Cleans up the label in teardown (line 829)
LOW (PT003): The explicit
scope="function"is technically redundant but serves as documentation. You could remove it per the static analysis hint, but it's a style preference.
832-859: LGTM - Clean label generation fixture.The fixture correctly generates label values without applying them - the actual labeling is handled by the migration plan. The
"auto"placeholder pattern (line 849-850) matcheslabeled_worker_nodefor consistency.No cleanup is needed since this fixture only generates the label dictionary; it doesn't modify cluster state.
| def get_mtv_version(ocp_admin_client: DynamicClient) -> str: | ||
| """ | ||
| Get MTV operator version from the cluster. | ||
| Raises: | ||
| RuntimeError: If MTV operator not found or version cannot be determined | ||
| """ | ||
| mtv_version: str | None = None | ||
|
|
||
| try: | ||
| csv_resource = ocp_admin_client.resources.get( | ||
| api_version="operators.coreos.com/v1alpha1", kind="ClusterServiceVersion" | ||
| ) | ||
| csvs = csv_resource.get() | ||
|
|
||
| # Access the items attribute (it's a list, not a method) | ||
| if hasattr(csvs, "items"): | ||
| csv_list = list(csvs.items) | ||
| else: | ||
| csv_list = [csvs] if csvs is not None else [] | ||
|
|
||
| # Search for MTV operator by display name | ||
| for csv in csv_list: | ||
| if csv is None: | ||
| continue | ||
|
|
||
| if hasattr(csv, "spec") and csv.spec: | ||
| display_name = getattr(csv.spec, "displayName", None) | ||
| mtv_version_str = getattr(csv.spec, "version", None) | ||
|
|
||
| if display_name and display_name == "Migration Toolkit for Virtualization Operator": | ||
| mtv_version = mtv_version_str | ||
| break | ||
|
|
||
| except Exception as e: | ||
| raise RuntimeError(f"Failed to determine MTV version: {e}") from e | ||
|
|
||
| if not mtv_version: | ||
| raise RuntimeError("MTV operator version not found") | ||
|
|
||
| LOGGER.info(f"Found MTV operator version: {mtv_version}") | ||
| return mtv_version |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
MEDIUM: Consider more specific exception handling and potential edge case.
The function is well-structured, but a few observations:
-
Line 129: If
csvsexists but has noitemsattribute, wrapping a single CSV in a list may not be correct - a single CSV wouldn't be the MTV operator itself; it's more likely an API error. -
Line 144-145: Catching bare
Exceptionis broad. The Kubernetes client typically raises more specific exceptions (ApiException,ApiValueError) that could provide better error context. -
Line 140: The displayName match is exact. If Red Hat ever changes the display name (e.g., adds "for" → "For"), this silently fails.
♻️ Optional: More defensive exception handling
- except Exception as e:
- raise RuntimeError(f"Failed to determine MTV version: {e}") from e
+ except ApiException as e:
+ raise RuntimeError(f"Failed to query ClusterServiceVersion API: {e.reason}") from e
+ except Exception as e:
+ raise RuntimeError(f"Unexpected error determining MTV version: {e}") from eThis requires importing ApiException from kubernetes.client.exceptions.
🧰 Tools
🪛 Ruff (0.14.10)
145-145: Avoid specifying long messages outside the exception class
(TRY003)
148-148: Avoid specifying long messages outside the exception class
(TRY003)
150-150: Logging statement uses f-string
(G004)
myakove
left a comment
myakove
left a comment
There was a problem hiding this comment.
Partial review, please fix all comments and I will do full review
| yield OCPProvider(ocp_resource=provider, fixture_store=fixture_store) | ||
|
|
||
|
|
||
| def _get_worker_nodes(v1: client.CoreV1Api) -> list[str]: |
There was a problem hiding this comment.
Why do you use CoreV1Api directly? You should use openshift-python-wrapper for any ocp resources.
| ] | ||
|
|
||
|
|
||
| def _get_prometheus_url(ocp_admin_client: DynamicClient) -> str | None: |
There was a problem hiding this comment.
Move all functions to the proper places in utilities, in conftests we should have only fixtures.
And please add this rule to CLAUDE.md
| name="thanos-querier", | ||
| namespace="openshift-monitoring", | ||
| ) | ||
| if thanos_route.exists: |
There was a problem hiding this comment.
This is not needed, use ensure_exists=True
| return None | ||
|
|
||
|
|
||
| def _query_prometheus(url: str, query: str, token: str, *, insecure: bool = True) -> list[dict[str, Any]] | None: |
There was a problem hiding this comment.
Use Prometheus from openshift-python-utilities, we have all there
| return f"https://{thanos_route.instance.spec.host}" | ||
| except (NotFoundError, ApiException) as e: | ||
| LOGGER.warning(f"Could not get Thanos querier route: {e}") | ||
| return None |
There was a problem hiding this comment.
Why not raise if not found?
6 participants
Implement comprehensive testing for MTV 2.10.0 targeting features:
Includes MTV version detection to skip feature tests on older clusters.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.