make client in migration tests mandatory#4994
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (55)
📝 WalkthroughWalkthroughThis PR makes the ChangesVM Migration Client Requirement Propagation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Rationale: This is a high-volume but low-complexity refactoring with a highly repetitive pattern across ~50 test files: (1) add Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4994 +/- ##
==========================================
- Coverage 98.67% 98.65% -0.02%
==========================================
Files 25 25
Lines 2487 2461 -26
==========================================
- Hits 2454 2428 -26
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
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
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
added
size/L
needs-rebase
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
requested review from
Anatw,
SamAlber,
azhivovk,
dalia-frank,
dshchedr,
geetikakay,
hmeir,
josemacassan,
kshvaika,
mijankow,
orelmisan,
rlobillo,
rnetser and
servolkov
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/virt/node/log_verbosity/test_log_virt_launcher.py`:
- Around line 99-103: The migrated_vm_with_policy fixture relies on
`@pytest.mark.usefixtures` which does not guarantee execution order, risking
migration starting before the source pod is captured. Add
source_pod_log_verbosity_test as an explicit parameter to the
migrated_vm_with_policy function signature to create a hard dependency that
ensures the pod is captured before migration begins. Apply the same fix to the
other affected fixture location mentioned in the comment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 43ce4a43-ae16-49bb-a3a6-985eea34a064
📒 Files selected for processing (55)
tests/infrastructure/instance_types/supported_os/test_rhel_os.pytests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.pytests/infrastructure/numa/conftest.pytests/network/bgp/evpn/test_evpn_connectivity.pytests/network/bgp/test_bgp_connectivity.pytests/network/flat_overlay/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.pytests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.pytests/network/libs/stuntime.pytests/network/localnet/conftest.pytests/network/localnet/test_default_bridge.pytests/network/localnet/test_ovs_bridge.pytests/network/migration/test_masquerade_connectivity_after_migration.pytests/network/migration/test_migration.pytests/network/service_mesh/test_service_mesh.pytests/network/sriov/test_sriov.pytests/network/user_defined_network/ip_specification/test_ip_specification.pytests/network/user_defined_network/test_user_defined_network.pytests/network/user_defined_network/test_user_defined_network_passt.pytests/storage/cross_cluster_live_migration/test_cclm.pytests/storage/cross_cluster_live_migration/utils.pytests/storage/online_resize/test_online_resize.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/test_hotplug.pytests/storage/upgrade/test_upgrade_storage.pytests/virt/cluster/aaq/conftest.pytests/virt/cluster/aaq/test_aaq_allocation_methods.pytests/virt/cluster/aaq/test_arq.pytests/virt/cluster/common_templates/centos/test_centos_os_support.pytests/virt/cluster/common_templates/fedora/test_fedora_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.pytests/virt/cluster/common_templates/windows/test_windows_os_support.pytests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.pytests/virt/cluster/longevity_tests/utils.pytests/virt/cluster/migration_and_maintenance/test_migration_policy.pytests/virt/cluster/sysprep/test_sysprep.pytests/virt/cluster/vm_cloning/test_vm_cloning.pytests/virt/cluster/vm_lifecycle/test_vm_run_strategy.pytests/virt/node/general/test_machinetype.pytests/virt/node/general/test_windows_vtpm_bitlocker.pytests/virt/node/general/test_wsl2.pytests/virt/node/high_performance_vm/test_isolate_emulator_thread.pytests/virt/node/hotplug/test_cpu_memory_hotplug.pytests/virt/node/log_verbosity/test_log_virt_launcher.pytests/virt/node/migration_and_maintenance/test_odf_vm_migration.pytests/virt/node/migration_and_maintenance/test_post_copy_migration.pytests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.pytests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.pytests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.pytests/virt/node/workload_density/test_kernel_samepage_merging.pytests/virt/node/workload_density/test_swap.pytests/virt/upgrade/test_upgrade_virt.pytests/virt/utils.pyutilities/virt.py
| _L2_ENDPOINT_IPV4: str = f"{random_ipv4_address(net_seed=EVPN_CUDN_NET_SEED, host_address=249)}/24" | ||
| _L2_ENDPOINT_IPV6: str = f"{random_ipv6_address(net_seed=EVPN_CUDN_NET_SEED, host_address=249)}/64" |
There was a problem hiding this comment.
i did during merge conflict resolution. should it stay before pytestmark block?
|
/retest security-committer-identity |
1 similar comment
|
/retest security-committer-identity |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4873. Overlapping filesutilities/virt.py |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/network/bgp/evpn/test_evpn_connectivity.py`:
- Line 54: There is an unresolved review thread anchored on the line containing
the _L2_ENDPOINT_IPV6 variable definition that is blocking the merge. Locate
this unresolved comment thread in the pull request review interface, read the
specific concern or question that was raised in the conversation, provide a
substantive and thoughtful response that directly addresses the reviewer's
concern (whether by explaining the code choice, acknowledging the issue and
fixing it, or documenting why the current approach is appropriate), and then
mark the thread as resolved after your response has been posted.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ac949053-49a6-4e8d-8c25-6307e7f901bd
📒 Files selected for processing (55)
tests/infrastructure/instance_types/supported_os/test_rhel_os.pytests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.pytests/infrastructure/numa/conftest.pytests/network/bgp/evpn/test_evpn_connectivity.pytests/network/bgp/test_bgp_connectivity.pytests/network/flat_overlay/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.pytests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.pytests/network/libs/stuntime.pytests/network/localnet/conftest.pytests/network/localnet/test_default_bridge.pytests/network/localnet/test_ovs_bridge.pytests/network/migration/test_masquerade_connectivity_after_migration.pytests/network/migration/test_migration.pytests/network/service_mesh/test_service_mesh.pytests/network/sriov/test_sriov.pytests/network/user_defined_network/ip_specification/test_ip_specification.pytests/network/user_defined_network/test_user_defined_network.pytests/network/user_defined_network/test_user_defined_network_passt.pytests/storage/cross_cluster_live_migration/test_cclm.pytests/storage/cross_cluster_live_migration/utils.pytests/storage/online_resize/test_online_resize.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/test_hotplug.pytests/storage/upgrade/test_upgrade_storage.pytests/virt/cluster/aaq/conftest.pytests/virt/cluster/aaq/test_aaq_allocation_methods.pytests/virt/cluster/aaq/test_arq.pytests/virt/cluster/common_templates/centos/test_centos_os_support.pytests/virt/cluster/common_templates/fedora/test_fedora_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.pytests/virt/cluster/common_templates/windows/test_windows_os_support.pytests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.pytests/virt/cluster/longevity_tests/utils.pytests/virt/cluster/migration_and_maintenance/test_migration_policy.pytests/virt/cluster/sysprep/test_sysprep.pytests/virt/cluster/vm_cloning/test_vm_cloning.pytests/virt/cluster/vm_lifecycle/test_vm_run_strategy.pytests/virt/node/general/test_machinetype.pytests/virt/node/general/test_windows_vtpm_bitlocker.pytests/virt/node/general/test_wsl2.pytests/virt/node/high_performance_vm/test_isolate_emulator_thread.pytests/virt/node/hotplug/test_cpu_memory_hotplug.pytests/virt/node/log_verbosity/test_log_virt_launcher.pytests/virt/node/migration_and_maintenance/test_odf_vm_migration.pytests/virt/node/migration_and_maintenance/test_post_copy_migration.pytests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.pytests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.pytests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.pytests/virt/node/workload_density/test_kernel_samepage_merging.pytests/virt/node/workload_density/test_swap.pytests/virt/upgrade/test_upgrade_virt.pytests/virt/utils.pyutilities/virt.py
|
|
||
|
|
||
| _L2_ENDPOINT_IPV4: str = f"{random_ipv4_address(net_seed=EVPN_CUDN_NET_SEED, host_address=249)}/24" | ||
| _L2_ENDPOINT_IPV6: str = f"{random_ipv6_address(net_seed=EVPN_CUDN_NET_SEED, host_address=249)}/64" |
There was a problem hiding this comment.
HIGH: Resolve the unresolved thread on this line to unblock merge.
can-be-merged is currently failing because a conversation anchored here is still unresolved. Please post a substantive in-thread response and re-resolve it.
As per coding guidelines, "NEVER merge a PR with known unresolved issues — fix or document them first."
🧰 Tools
🪛 GitHub Check: can-be-merged
[error] 54-54: PR has an unresolved review conversation at tests/network/bgp/evpn/test_evpn_connectivity.py:54
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/network/bgp/evpn/test_evpn_connectivity.py` at line 54, There is an
unresolved review thread anchored on the line containing the _L2_ENDPOINT_IPV6
variable definition that is blocking the merge. Locate this unresolved comment
thread in the pull request review interface, read the specific concern or
question that was raised in the conversation, provide a substantive and
thoughtful response that directly addresses the reviewer's concern (whether by
explaining the code choice, acknowledging the issue and fixing it, or
documenting why the current approach is appropriate), and then mark the thread
as resolved after your response has been posted.
Sources: Coding guidelines, Pipeline failures
|
/verified |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get current head SHA and first changed file
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4994 \
--jq '{head_sha: .head.sha}'
echo ""
echo "=== FIRST FILE AND FIRST CHANGED LINE ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4994/files \
--jq '.[0] | {filename: .filename, patch: .patch}' | head -30
echo ""
echo "=== SMOKE MARKED FILES ==="
rg -l '`@pytest.mark.smoke`' tests/ --type py 2>/dev/null | head -20
echo ""
echo "=== GATING MARKED FILES THAT CALL migrate_vm_and_verify ==="
rg -l 'migrate_vm_and_verify' tests/ --type py | while read f; do
if rg -q '`@pytest.mark.gating`' "$f" 2>/dev/null; then
echo "$f"
fi
doneLength of output: 3259 🏁 Script executed: #!/bin/bash
BODY='## Test Execution Plan
**Run smoke tests: True**
Dependency path:
- `tests/infrastructure/instance_types/supported_os/test_rhel_os.py::TestVMMigrationAndState::test_migrate_vm` (smoke+gating) → calls `migrate_vm_and_verify(vm=..., client=admin_client, ...)` → `utilities/virt.py::migrate_vm_and_verify` (signature changed to mandatory `client: DynamicClient`)
- `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py::TestCommonTemplatesRhel::test_migrate_vm` (smoke) → same call chain
**Run gating tests: True**
Dependency path: 11 gating-marked files directly call `migrate_vm_and_verify` with the now-mandatory `client` argument.
**Affected tests to run**
*(smoke)*
- `tests/infrastructure/instance_types/supported_os/test_rhel_os.py::TestVMMigrationAndState::test_migrate_vm` (smoke+gating)
- `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py::TestCommonTemplatesRhel::test_migrate_vm` (smoke)
*(gating)*
- `tests/virt/upgrade/test_upgrade_virt.py` (gating)
- `tests/virt/node/general/test_machinetype.py` (gating)
- `tests/storage/test_hotplug.py` (gating)
- `tests/storage/online_resize/test_online_resize.py` (gating)
- `tests/virt/cluster/vm_cloning/test_vm_cloning.py` (gating)
- `tests/virt/cluster/vm_lifecycle/test_vm_run_strategy.py` (gating)
- `tests/network/localnet/test_default_bridge.py` (gating)
- `tests/virt/cluster/migration_and_maintenance/test_migration_policy.py` (gating)
- `tests/network/user_defined_network/test_user_defined_network.py` (gating)
- `tests/network/migration/test_masquerade_connectivity_after_migration.py` (gating)
*(other modified — no smoke/gating marker but directly affected)*
- `tests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.py`
- `tests/infrastructure/numa/conftest.py`
- `tests/network/bgp/evpn/test_evpn_connectivity.py`
- `tests/network/bgp/test_bgp_connectivity.py`
- `tests/network/flat_overlay/conftest.py`
- `tests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.py`
- `tests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.py`
- `tests/network/localnet/test_ovs_bridge.py`
- `tests/network/migration/test_migration.py`
- `tests/network/service_mesh/test_service_mesh.py`
- `tests/network/sriov/test_sriov.py`
- `tests/network/user_defined_network/ip_specification/test_ip_specification.py`
- `tests/network/user_defined_network/test_user_defined_network_passt.py`
- `tests/storage/cross_cluster_live_migration/test_cclm.py`
- `tests/storage/storage_migration/test_storage_class_migration.py`
- `tests/storage/upgrade/test_upgrade_storage.py`
- `tests/virt/cluster/aaq/test_aaq_allocation_methods.py`
- `tests/virt/cluster/aaq/test_arq.py`
- `tests/virt/cluster/common_templates/centos/test_centos_os_support.py`
- `tests/virt/cluster/common_templates/fedora/test_fedora_os_support.py`
- `tests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.py`
- `tests/virt/cluster/common_templates/windows/test_windows_os_support.py`
- `tests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.py`
- `tests/virt/cluster/sysprep/test_sysprep.py`
- `tests/virt/node/general/test_windows_vtpm_bitlocker.py`
- `tests/virt/node/general/test_wsl2.py`
- `tests/virt/node/high_performance_vm/test_isolate_emulator_thread.py`
- `tests/virt/node/hotplug/test_cpu_memory_hotplug.py`
- `tests/virt/node/log_verbosity/test_log_virt_launcher.py`
- `tests/virt/node/migration_and_maintenance/test_odf_vm_migration.py`
- `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py`
- `tests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.py`
- `tests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.py`
- `tests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.py`
- `tests/virt/node/workload_density/test_kernel_samepage_merging.py`
- `tests/virt/node/workload_density/test_swap.py`'
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4994/reviews \
--method POST \
--field commit_id="b49268ab4825c3a1dc6f2e9a8fce64754349762f" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=tests/infrastructure/instance_types/supported_os/test_rhel_os.py" \
--field "comments[][position]=1" \
--field "comments[][body]=$BODY"Length of output: 6065 |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5219. Overlapping filesutilities/virt.py |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5167. Overlapping filestests/virt/upgrade/test_upgrade_virt.py |
|
/retest tox |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4810. Overlapping filestests/network/user_defined_network/ip_specification/test_ip_specification.py |
Require an explicit DynamicClient for VM migration so callers must choose the correct client (admin vs unprivileged). Update all fixtures, helpers, and tests to pass client through to migrate_vm_and_verify. Assisted-by: Cursor Agent (Composer 2.5) Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Move setup-only fixtures from test signatures to @pytest.mark.usefixtures where the fixture return value is not used by the test body. Keeps test parameters limited to resources the test actually inspects. Assisted-by: Cursor Agent (Composer 2.5) Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Align modified fixtures with project rules to always return or yield a value even when used only for setup. Assisted-by: Cursor Agent (Composer 2.5) Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Annotate tests, fixtures, and helpers updated for mandatory client with TYPE_CHECKING imports and parameter return types. Assisted-by: Cursor Agent (Composer 2.5) Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Add from __future__ import annotations to modules that annotate signatures with TYPE_CHECKING-only imports, preventing NameError when annotations are evaluated at runtime. Assisted-by: Cursor Agent (Composer 2.5) Signed-off-by: vsibirsk <vsibirsk@redhat.com>
|
/verified |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
Signed-off-by: vsibirsk <vsibirsk@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/network/migration/test_migration.py (1)
294-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Ensure cleanup runs when setup fails before
yieldAt Line 294, migration is created with
wait_for_migration_success=False, so cleanup is manual. If the wait loop fails before reachingyield, the cleanup at Line 303 is skipped, leaking migration resources. Usetry/finallyaround wait+yield.Suggested change
migrated_vmi = migrate_vm_and_verify(vm=running_vmb, client=admin_client, wait_for_migration_success=False) - for sample in TimeoutSampler( - wait_timeout=TIMEOUT_1MIN, - sleep=1, - func=lambda: vmb_ip_before_migration != running_vmb.vmi.interfaces[0]["ipAddress"], - ): - if sample: - break - yield - migrated_vmi.clean_up() + try: + for sample in TimeoutSampler( + wait_timeout=TIMEOUT_1MIN, + sleep=1, + func=lambda: vmb_ip_before_migration != running_vmb.vmi.interfaces[0]["ipAddress"], + ): + if sample: + break + yield + finally: + migrated_vmi.clean_up()As per coding guidelines: fixture cleanup must be reliable; guarding teardown with
finallyprevents resource leaks on setup exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/network/migration/test_migration.py` around lines 294 - 303, The fixture does not guarantee cleanup of the migrated_vmi resource if an exception occurs during the wait loop before reaching the yield statement. Wrap the TimeoutSampler loop and the yield statement in a try block, then place the migrated_vmi.clean_up() call in a finally block to ensure cleanup always executes regardless of whether exceptions occur during the wait loop or the yield phase. This prevents resource leaks when the migration validation fails before the fixture yields control to the test.Source: Coding guidelines
tests/network/migration/test_masquerade_connectivity_after_migration.py (1)
68-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Make migration teardown exception-safe in fixture setup
At Line 68, migration starts with manual cleanup responsibility. If the IP-change wait fails before
yield, the teardown at Line 84 never runs, leaving migration resources behind and making subsequent tests flaky. Wrap wait+yield intry/finally.Suggested change
migrated_vmi = migrate_vm_and_verify( vm=running_vm_for_migration, client=admin_client, wait_for_migration_success=False ) - - for sample in TimeoutSampler( - wait_timeout=60, - sleep=1, - func=lambda: ( - ip_addresses_before - != lookup_iface_status(vm=running_vm_for_migration, iface_name=iface_name)["ipAddresses"] - ), - ): - if sample: - break - - yield - migrated_vmi.clean_up() + try: + for sample in TimeoutSampler( + wait_timeout=60, + sleep=1, + func=lambda: ( + ip_addresses_before + != lookup_iface_status(vm=running_vm_for_migration, iface_name=iface_name)["ipAddresses"] + ), + ): + if sample: + break + + yield + finally: + migrated_vmi.clean_up()As per coding guidelines: fixtures with cleanup must reliably execute cleanup code after
yield; usingfinallypreserves that behavior even on setup exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/network/migration/test_masquerade_connectivity_after_migration.py` around lines 68 - 84, The fixture lacks exception-safe teardown because if the TimeoutSampler wait loop fails before yield, the migrated_vmi.clean_up() call at the end never executes, leaving migration resources behind. Wrap the migrate_vm_and_verify call and the TimeoutSampler loop in a try block, then move the migrated_vmi.clean_up() call into a finally block to guarantee cleanup always runs regardless of whether the wait succeeds or raises an exception.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/network/migration/test_masquerade_connectivity_after_migration.py`:
- Around line 66-67: Replace the dynamic interface lookup at line 66 where
iface_name is assigned from running_vm_for_migration.vmi.interfaces[0].name with
the hardcoded string 'default'. This ensures that the lookup_iface_status
function on line 67 consistently validates the masquerade interface regardless
of how the interfaces array gets ordered, making the test deterministic and
preventing false failures due to interface ordering changes.
In
`@tests/network/user_defined_network/ip_specification/test_ip_specification.py`:
- Around line 85-89: The assertions for guest IP specifications lack descriptive
failure messages, making it difficult to diagnose test failures in CI. Add a
descriptive message parameter to both assert statements (the one comparing
guest_ipv4 == ip_to_request around line 88-89 and the similar one around lines
170-174) that explicitly includes the expected value (ip_to_request) and actual
value (guest_ipv4) to provide clear context when the assertions fail.
In `@tests/virt/cluster/longevity_tests/utils.py`:
- Line 53: The type annotation for `vms_with_pids` on line 53 declares the inner
dictionary as `dict[str, VirtualMachineForTests]`, but the actual data structure
includes a "pid" key that holds a non-VM value (accessed at line 181 with
`["pid"]`). Update the type annotation to accurately reflect the actual payload
shape by modifying the inner dictionary type to account for both
VirtualMachineForTests objects and the pid value (such as using Union or a
TypedDict if available). Ensure the type hint matches how the structure is
actually used throughout the function to maintain static type checking
integrity.
In `@tests/virt/node/migration_and_maintenance/test_post_copy_migration.py`:
- Around line 93-101: The fixture drained_node_with_hotplugged_vm currently
performs setup work but returns None implicitly. Modify the fixture to yield the
hotplugged_vm resource after the drain_node context manager block, so that
dependent tests can access the prepared VM and fixture composition remains
explicit. This ensures the cleanup code in the context manager still executes
properly after the test completes.
In `@utilities/virt.py`:
- Around line 2216-2221: The pkill command with the pattern "oc adm drain" is
too broad and can kill unrelated drain operations from other tests running in
parallel. Instead of using the generic pattern-based pkill in the finally block,
capture the process ID (PID) when starting the nohup drain command and store it,
then use that specific PID with the kill command to terminate only the drain
process started for this particular node. This ensures that only the drain
process associated with this specific test cleanup is terminated, preventing
interference with concurrent tests.
---
Outside diff comments:
In `@tests/network/migration/test_masquerade_connectivity_after_migration.py`:
- Around line 68-84: The fixture lacks exception-safe teardown because if the
TimeoutSampler wait loop fails before yield, the migrated_vmi.clean_up() call at
the end never executes, leaving migration resources behind. Wrap the
migrate_vm_and_verify call and the TimeoutSampler loop in a try block, then move
the migrated_vmi.clean_up() call into a finally block to guarantee cleanup
always runs regardless of whether the wait succeeds or raises an exception.
In `@tests/network/migration/test_migration.py`:
- Around line 294-303: The fixture does not guarantee cleanup of the
migrated_vmi resource if an exception occurs during the wait loop before
reaching the yield statement. Wrap the TimeoutSampler loop and the yield
statement in a try block, then place the migrated_vmi.clean_up() call in a
finally block to ensure cleanup always executes regardless of whether exceptions
occur during the wait loop or the yield phase. This prevents resource leaks when
the migration validation fails before the fixture yields control to the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f70f56c-5d3f-4174-b6f4-d41f26a255db
📒 Files selected for processing (55)
tests/infrastructure/instance_types/supported_os/test_rhel_os.pytests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.pytests/infrastructure/numa/conftest.pytests/network/bgp/evpn/test_evpn_connectivity.pytests/network/bgp/test_bgp_connectivity.pytests/network/flat_overlay/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.pytests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.pytests/network/libs/stuntime.pytests/network/localnet/conftest.pytests/network/localnet/test_default_bridge.pytests/network/localnet/test_ovs_bridge.pytests/network/migration/test_masquerade_connectivity_after_migration.pytests/network/migration/test_migration.pytests/network/service_mesh/test_service_mesh.pytests/network/sriov/test_sriov.pytests/network/user_defined_network/ip_specification/test_ip_specification.pytests/network/user_defined_network/test_user_defined_network.pytests/network/user_defined_network/test_user_defined_network_passt.pytests/storage/cross_cluster_live_migration/test_cclm.pytests/storage/cross_cluster_live_migration/utils.pytests/storage/online_resize/test_online_resize.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/test_hotplug.pytests/storage/upgrade/test_upgrade_storage.pytests/virt/cluster/aaq/conftest.pytests/virt/cluster/aaq/test_aaq_allocation_methods.pytests/virt/cluster/aaq/test_arq.pytests/virt/cluster/common_templates/centos/test_centos_os_support.pytests/virt/cluster/common_templates/fedora/test_fedora_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.pytests/virt/cluster/common_templates/windows/test_windows_os_support.pytests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.pytests/virt/cluster/longevity_tests/utils.pytests/virt/cluster/migration_and_maintenance/test_migration_policy.pytests/virt/cluster/sysprep/test_sysprep.pytests/virt/cluster/vm_cloning/test_vm_cloning.pytests/virt/cluster/vm_lifecycle/test_vm_run_strategy.pytests/virt/node/general/test_machinetype.pytests/virt/node/general/test_windows_vtpm_bitlocker.pytests/virt/node/general/test_wsl2.pytests/virt/node/high_performance_vm/test_isolate_emulator_thread.pytests/virt/node/hotplug/test_cpu_memory_hotplug.pytests/virt/node/log_verbosity/test_log_virt_launcher.pytests/virt/node/migration_and_maintenance/test_odf_vm_migration.pytests/virt/node/migration_and_maintenance/test_post_copy_migration.pytests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.pytests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.pytests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.pytests/virt/node/workload_density/test_kernel_samepage_merging.pytests/virt/node/workload_density/test_swap.pytests/virt/upgrade/test_upgrade_virt.pytests/virt/utils.pyutilities/virt.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/network/migration/test_migration.py (1)
294-303:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Ensure cleanup runs when setup fails before
yieldAt Line 294, migration is created with
wait_for_migration_success=False, so cleanup is manual. If the wait loop fails before reachingyield, the cleanup at Line 303 is skipped, leaking migration resources. Usetry/finallyaround wait+yield.Suggested change
migrated_vmi = migrate_vm_and_verify(vm=running_vmb, client=admin_client, wait_for_migration_success=False) - for sample in TimeoutSampler( - wait_timeout=TIMEOUT_1MIN, - sleep=1, - func=lambda: vmb_ip_before_migration != running_vmb.vmi.interfaces[0]["ipAddress"], - ): - if sample: - break - yield - migrated_vmi.clean_up() + try: + for sample in TimeoutSampler( + wait_timeout=TIMEOUT_1MIN, + sleep=1, + func=lambda: vmb_ip_before_migration != running_vmb.vmi.interfaces[0]["ipAddress"], + ): + if sample: + break + yield + finally: + migrated_vmi.clean_up()As per coding guidelines: fixture cleanup must be reliable; guarding teardown with
finallyprevents resource leaks on setup exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/network/migration/test_migration.py` around lines 294 - 303, The fixture does not guarantee cleanup of the migrated_vmi resource if an exception occurs during the wait loop before reaching the yield statement. Wrap the TimeoutSampler loop and the yield statement in a try block, then place the migrated_vmi.clean_up() call in a finally block to ensure cleanup always executes regardless of whether exceptions occur during the wait loop or the yield phase. This prevents resource leaks when the migration validation fails before the fixture yields control to the test.Source: Coding guidelines
tests/network/migration/test_masquerade_connectivity_after_migration.py (1)
68-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Make migration teardown exception-safe in fixture setup
At Line 68, migration starts with manual cleanup responsibility. If the IP-change wait fails before
yield, the teardown at Line 84 never runs, leaving migration resources behind and making subsequent tests flaky. Wrap wait+yield intry/finally.Suggested change
migrated_vmi = migrate_vm_and_verify( vm=running_vm_for_migration, client=admin_client, wait_for_migration_success=False ) - - for sample in TimeoutSampler( - wait_timeout=60, - sleep=1, - func=lambda: ( - ip_addresses_before - != lookup_iface_status(vm=running_vm_for_migration, iface_name=iface_name)["ipAddresses"] - ), - ): - if sample: - break - - yield - migrated_vmi.clean_up() + try: + for sample in TimeoutSampler( + wait_timeout=60, + sleep=1, + func=lambda: ( + ip_addresses_before + != lookup_iface_status(vm=running_vm_for_migration, iface_name=iface_name)["ipAddresses"] + ), + ): + if sample: + break + + yield + finally: + migrated_vmi.clean_up()As per coding guidelines: fixtures with cleanup must reliably execute cleanup code after
yield; usingfinallypreserves that behavior even on setup exceptions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/network/migration/test_masquerade_connectivity_after_migration.py` around lines 68 - 84, The fixture lacks exception-safe teardown because if the TimeoutSampler wait loop fails before yield, the migrated_vmi.clean_up() call at the end never executes, leaving migration resources behind. Wrap the migrate_vm_and_verify call and the TimeoutSampler loop in a try block, then move the migrated_vmi.clean_up() call into a finally block to guarantee cleanup always runs regardless of whether the wait succeeds or raises an exception.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/network/migration/test_masquerade_connectivity_after_migration.py`:
- Around line 66-67: Replace the dynamic interface lookup at line 66 where
iface_name is assigned from running_vm_for_migration.vmi.interfaces[0].name with
the hardcoded string 'default'. This ensures that the lookup_iface_status
function on line 67 consistently validates the masquerade interface regardless
of how the interfaces array gets ordered, making the test deterministic and
preventing false failures due to interface ordering changes.
In
`@tests/network/user_defined_network/ip_specification/test_ip_specification.py`:
- Around line 85-89: The assertions for guest IP specifications lack descriptive
failure messages, making it difficult to diagnose test failures in CI. Add a
descriptive message parameter to both assert statements (the one comparing
guest_ipv4 == ip_to_request around line 88-89 and the similar one around lines
170-174) that explicitly includes the expected value (ip_to_request) and actual
value (guest_ipv4) to provide clear context when the assertions fail.
In `@tests/virt/cluster/longevity_tests/utils.py`:
- Line 53: The type annotation for `vms_with_pids` on line 53 declares the inner
dictionary as `dict[str, VirtualMachineForTests]`, but the actual data structure
includes a "pid" key that holds a non-VM value (accessed at line 181 with
`["pid"]`). Update the type annotation to accurately reflect the actual payload
shape by modifying the inner dictionary type to account for both
VirtualMachineForTests objects and the pid value (such as using Union or a
TypedDict if available). Ensure the type hint matches how the structure is
actually used throughout the function to maintain static type checking
integrity.
In `@tests/virt/node/migration_and_maintenance/test_post_copy_migration.py`:
- Around line 93-101: The fixture drained_node_with_hotplugged_vm currently
performs setup work but returns None implicitly. Modify the fixture to yield the
hotplugged_vm resource after the drain_node context manager block, so that
dependent tests can access the prepared VM and fixture composition remains
explicit. This ensures the cleanup code in the context manager still executes
properly after the test completes.
In `@utilities/virt.py`:
- Around line 2216-2221: The pkill command with the pattern "oc adm drain" is
too broad and can kill unrelated drain operations from other tests running in
parallel. Instead of using the generic pattern-based pkill in the finally block,
capture the process ID (PID) when starting the nohup drain command and store it,
then use that specific PID with the kill command to terminate only the drain
process started for this particular node. This ensures that only the drain
process associated with this specific test cleanup is terminated, preventing
interference with concurrent tests.
---
Outside diff comments:
In `@tests/network/migration/test_masquerade_connectivity_after_migration.py`:
- Around line 68-84: The fixture lacks exception-safe teardown because if the
TimeoutSampler wait loop fails before yield, the migrated_vmi.clean_up() call at
the end never executes, leaving migration resources behind. Wrap the
migrate_vm_and_verify call and the TimeoutSampler loop in a try block, then move
the migrated_vmi.clean_up() call into a finally block to guarantee cleanup
always runs regardless of whether the wait succeeds or raises an exception.
In `@tests/network/migration/test_migration.py`:
- Around line 294-303: The fixture does not guarantee cleanup of the
migrated_vmi resource if an exception occurs during the wait loop before
reaching the yield statement. Wrap the TimeoutSampler loop and the yield
statement in a try block, then place the migrated_vmi.clean_up() call in a
finally block to ensure cleanup always executes regardless of whether exceptions
occur during the wait loop or the yield phase. This prevents resource leaks when
the migration validation fails before the fixture yields control to the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4f70f56c-5d3f-4174-b6f4-d41f26a255db
📒 Files selected for processing (55)
tests/infrastructure/instance_types/supported_os/test_rhel_os.pytests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.pytests/infrastructure/numa/conftest.pytests/network/bgp/evpn/test_evpn_connectivity.pytests/network/bgp/test_bgp_connectivity.pytests/network/flat_overlay/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.pytests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.pytests/network/libs/stuntime.pytests/network/localnet/conftest.pytests/network/localnet/test_default_bridge.pytests/network/localnet/test_ovs_bridge.pytests/network/migration/test_masquerade_connectivity_after_migration.pytests/network/migration/test_migration.pytests/network/service_mesh/test_service_mesh.pytests/network/sriov/test_sriov.pytests/network/user_defined_network/ip_specification/test_ip_specification.pytests/network/user_defined_network/test_user_defined_network.pytests/network/user_defined_network/test_user_defined_network_passt.pytests/storage/cross_cluster_live_migration/test_cclm.pytests/storage/cross_cluster_live_migration/utils.pytests/storage/online_resize/test_online_resize.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/test_hotplug.pytests/storage/upgrade/test_upgrade_storage.pytests/virt/cluster/aaq/conftest.pytests/virt/cluster/aaq/test_aaq_allocation_methods.pytests/virt/cluster/aaq/test_arq.pytests/virt/cluster/common_templates/centos/test_centos_os_support.pytests/virt/cluster/common_templates/fedora/test_fedora_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.pytests/virt/cluster/common_templates/windows/test_windows_os_support.pytests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.pytests/virt/cluster/longevity_tests/utils.pytests/virt/cluster/migration_and_maintenance/test_migration_policy.pytests/virt/cluster/sysprep/test_sysprep.pytests/virt/cluster/vm_cloning/test_vm_cloning.pytests/virt/cluster/vm_lifecycle/test_vm_run_strategy.pytests/virt/node/general/test_machinetype.pytests/virt/node/general/test_windows_vtpm_bitlocker.pytests/virt/node/general/test_wsl2.pytests/virt/node/high_performance_vm/test_isolate_emulator_thread.pytests/virt/node/hotplug/test_cpu_memory_hotplug.pytests/virt/node/log_verbosity/test_log_virt_launcher.pytests/virt/node/migration_and_maintenance/test_odf_vm_migration.pytests/virt/node/migration_and_maintenance/test_post_copy_migration.pytests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.pytests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.pytests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.pytests/virt/node/workload_density/test_kernel_samepage_merging.pytests/virt/node/workload_density/test_swap.pytests/virt/upgrade/test_upgrade_virt.pytests/virt/utils.pyutilities/virt.py
📜 Review details
🔇 Additional comments (57)
tests/virt/cluster/aaq/conftest.py (1)
1-4: LGTM!Also applies to: 47-49, 162-164
tests/virt/cluster/aaq/test_aaq_allocation_methods.py (1)
1-15: LGTM!Also applies to: 51-54
tests/virt/cluster/aaq/test_arq.py (1)
1-4: LGTM!Also applies to: 33-37, 124-127
tests/virt/cluster/common_templates/centos/test_centos_os_support.py (1)
5-8: LGTM!Also applies to: 33-37, 139-141
tests/virt/cluster/common_templates/fedora/test_fedora_os_support.py (1)
5-8: LGTM!Also applies to: 35-38, 215-218
tests/virt/cluster/vm_lifecycle/test_vm_run_strategy.py (1)
1-7: LGTM!Also applies to: 22-25, 192-196, 314-322
tests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.py (1)
1-4: LGTM!Also applies to: 20-24, 40-48, 62-70, 84-93
tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py (1)
5-8: LGTM!Also applies to: 36-40, 197-201
tests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.py (1)
7-12: LGTM!Also applies to: 24-26, 123-126
tests/virt/cluster/common_templates/windows/test_windows_os_support.py (1)
5-8: LGTM!Also applies to: 31-35, 141-144
tests/virt/cluster/migration_and_maintenance/test_migration_policy.py (1)
1-4: LGTM!Also applies to: 18-20, 108-127
tests/virt/cluster/sysprep/test_sysprep.py (1)
1-7: LGTM!Also applies to: 26-27, 209-211
tests/virt/cluster/vm_cloning/test_vm_cloning.py (1)
1-4: LGTM!Also applies to: 34-35, 304-305
tests/storage/cross_cluster_live_migration/test_cclm.py (1)
7-10: LGTM!Also applies to: 25-29, 103-106, 156-159, 236-239
tests/storage/storage_migration/test_storage_class_migration.py (1)
1-4: LGTM!Also applies to: 25-29, 87-94, 216-227, 327-339
tests/storage/online_resize/test_online_resize.py (1)
5-8: LGTM!Also applies to: 26-30, 174-177
tests/storage/test_hotplug.py (1)
5-9: LGTM!Also applies to: 35-37, 211-222, 256-263, 299-307
tests/storage/upgrade/test_upgrade_storage.py (1)
1-4: LGTM!Also applies to: 27-32, 167-173, 182-182
tests/network/bgp/evpn/test_evpn_connectivity.py (1)
19-22: LGTM!Also applies to: 38-47, 101-104, 118-118, 145-148, 162-162
tests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.py (1)
11-14: LGTM!Also applies to: 23-27, 43-46, 65-65, 75-78, 97-97
tests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.py (1)
1-3: LGTM!Also applies to: 14-17, 31-37
tests/network/localnet/conftest.py (1)
401-406: LGTM!tests/network/localnet/test_default_bridge.py (1)
1-5: LGTM!Also applies to: 19-23, 25-37
tests/network/localnet/test_ovs_bridge.py (1)
1-5: LGTM!Also applies to: 19-23, 25-35
tests/network/migration/test_masquerade_connectivity_after_migration.py (1)
1-5: LGTM!Also applies to: 20-21
tests/network/migration/test_migration.py (1)
5-11: LGTM!Also applies to: 45-47, 268-272
tests/network/bgp/test_bgp_connectivity.py (1)
1-14: LGTM!Also applies to: 28-33
tests/network/flat_overlay/conftest.py (1)
1-5: LGTM!Also applies to: 33-37, 297-301
tests/network/service_mesh/test_service_mesh.py (1)
1-4: LGTM!Also applies to: 15-19, 86-90
tests/network/sriov/test_sriov.py (1)
1-4: LGTM!Also applies to: 15-20, 112-119
tests/network/user_defined_network/ip_specification/test_ip_specification.py (1)
10-29: LGTM!Also applies to: 67-68, 105-105, 118-139
tests/network/user_defined_network/test_user_defined_network.py (1)
1-4: LGTM!Also applies to: 17-21, 88-95, 121-124, 133-136
tests/network/user_defined_network/test_user_defined_network_passt.py (1)
1-4: LGTM!Also applies to: 20-24, 91-114
utilities/virt.py (1)
1911-1955: LGTM!Also applies to: 2677-2681
tests/virt/utils.py (1)
132-140: LGTM!tests/storage/cross_cluster_live_migration/utils.py (1)
77-94: LGTM!tests/virt/cluster/longevity_tests/utils.py (1)
1-39: LGTM!tests/network/libs/stuntime.py (1)
31-31: LGTM!tests/infrastructure/instance_types/supported_os/test_rhel_os.py (1)
1-4: LGTM!Also applies to: 31-35, 145-152
tests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.py (1)
1-4: LGTM!Also applies to: 34-36, 142-146
tests/infrastructure/numa/conftest.py (1)
1-4: LGTM!Also applies to: 16-17, 77-80
tests/virt/node/general/test_machinetype.py (1)
1-4: LGTM!Also applies to: 29-31, 129-138
tests/virt/node/general/test_windows_vtpm_bitlocker.py (1)
1-5: LGTM!Also applies to: 24-26, 130-134
tests/virt/node/general/test_wsl2.py (1)
6-11: LGTM!Also applies to: 28-30, 122-124
tests/virt/node/high_performance_vm/test_isolate_emulator_thread.py (1)
5-8: LGTM!Also applies to: 19-23, 92-95
tests/virt/node/hotplug/test_cpu_memory_hotplug.py (2)
1-4: LGTM!Also applies to: 31-35
89-91: LGTM!Also applies to: 146-148
tests/virt/node/log_verbosity/test_log_virt_launcher.py (1)
1-5: LGTM!Also applies to: 21-25, 100-114, 138-144
tests/virt/node/migration_and_maintenance/test_odf_vm_migration.py (1)
1-16: LGTM!Also applies to: 63-66
tests/virt/node/migration_and_maintenance/test_post_copy_migration.py (1)
1-38: LGTM!Also applies to: 80-89
tests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.py (1)
1-20: LGTM!Also applies to: 103-106
tests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.py (1)
1-4: LGTM!Also applies to: 19-23, 48-53
tests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.py (1)
1-4: LGTM!Also applies to: 19-20, 109-114
tests/virt/node/workload_density/test_kernel_samepage_merging.py (1)
1-4: LGTM!Also applies to: 17-21, 205-209
tests/virt/node/workload_density/test_swap.py (1)
1-5: LGTM!Also applies to: 19-21, 195-202
tests/virt/upgrade/test_upgrade_virt.py (2)
1-4: LGTM!Also applies to: 29-33, 142-149, 158-163, 278-283, 358-362
69-69: No action required. The fixturevirt_launcher_images_from_csv_before_upgradeis correctly defined attests/virt/upgrade/conftest.py:378with session scope and is properly referenced throughout the codebase. No stale references exist.
🛑 Comments failed to post (5)
tests/network/migration/test_masquerade_connectivity_after_migration.py (1)
66-67:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLOW: Pin the masquerade interface lookup to
defaultAt Line 66, selecting
interfaces[0]can drift when interface ordering changes, and then Line 67 may validate the wrong interface. Use the stable masquerade name directly to keep this test deterministic.Suggested change
- iface_name = running_vm_for_migration.vmi.interfaces[0].name + iface_name = "default"Based on learnings: in tests under
tests/network, prefer hardcoded interface name'default'overvm.vmi.interfaces[0].namefor masquerade migration checks.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/network/migration/test_masquerade_connectivity_after_migration.py` around lines 66 - 67, Replace the dynamic interface lookup at line 66 where iface_name is assigned from running_vm_for_migration.vmi.interfaces[0].name with the hardcoded string 'default'. This ensures that the lookup_iface_status function on line 67 consistently validates the masquerade interface regardless of how the interfaces array gets ordered, making the test deterministic and preventing false failures due to interface ordering changes.Source: Learnings
tests/network/user_defined_network/ip_specification/test_ip_specification.py (1)
85-89:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLOW: Add descriptive failure messages to the new guest-IP assertions.
These two new assertions make triage harder when they fail in CI because expected/actual values are not explicit in the assertion message.
Suggested patch
- assert guest_ipv4 == ip_to_request + assert guest_ipv4 == ip_to_request, f"Guest IP mismatch: expected {ip_to_request}, got {guest_ipv4}" ... - assert guest_ipv4 == ip_to_request + assert guest_ipv4 == ip_to_request, f"Guest IP mismatch: expected {ip_to_request}, got {guest_ipv4}"As per coding guidelines, tests should include descriptive assertion failure messages; based on learnings, this is expected for integration tests under
tests/.Also applies to: 170-174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/network/user_defined_network/ip_specification/test_ip_specification.py` around lines 85 - 89, The assertions for guest IP specifications lack descriptive failure messages, making it difficult to diagnose test failures in CI. Add a descriptive message parameter to both assert statements (the one comparing guest_ipv4 == ip_to_request around line 88-89 and the similar one around lines 170-174) that explicitly includes the expected value (ip_to_request) and actual value (guest_ipv4) to provide clear context when the assertions fail.Sources: Coding guidelines, Learnings
tests/virt/cluster/longevity_tests/utils.py (1)
53-53:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH:
vms_with_pidstype annotation does not match actual payload shapeLine 53 declares
dict[str, dict[str, VirtualMachineForTests]], but the same structure is read with a non-VM value at Line 181 (["pid"]). This weakens static checks and can mask contract regressions between migration and PID-verification paths.Proposed fix
from typing import TYPE_CHECKING +from typing import TypedDict if TYPE_CHECKING: from kubernetes.dynamic import DynamicClient +class VmMigrationEntry(TypedDict): + vm: VirtualMachineForTests + pid: int | str + def run_migration_loop( client: DynamicClient, iterations: int, - vms_with_pids: dict[str, dict[str, VirtualMachineForTests]], + vms_with_pids: dict[str, VmMigrationEntry], os_type: str, wsl2_guest: bool = False, ) -> None:As per coding guidelines, "Type hints are MANDATORY."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/virt/cluster/longevity_tests/utils.py` at line 53, The type annotation for `vms_with_pids` on line 53 declares the inner dictionary as `dict[str, VirtualMachineForTests]`, but the actual data structure includes a "pid" key that holds a non-VM value (accessed at line 181 with `["pid"]`). Update the type annotation to accurately reflect the actual payload shape by modifying the inner dictionary type to account for both VirtualMachineForTests objects and the pid value (such as using Union or a TypedDict if available). Ensure the type hint matches how the structure is actually used throughout the function to maintain static type checking integrity.Source: Coding guidelines
tests/virt/node/migration_and_maintenance/test_post_copy_migration.py (1)
93-101:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMEDIUM: Fixture should return or yield the provided resource
Line 93 defines a fixture that performs setup work but returns
None. Returning (or yielding) the resource keeps fixture composition explicit and lets dependent tests inspect the prepared VM when needed.Suggested fix
`@pytest.fixture`() def drained_node_with_hotplugged_vm(admin_client, hco_namespace, compact_cluster, hotplugged_vm): with drain_node( admin_client=admin_client, node=hotplugged_vm.vmi.get_node(privileged_client=admin_client), hco_namespace=hco_namespace, compact_cluster=compact_cluster, ): check_migration_process_after_node_drain(client=admin_client, vm=hotplugged_vm, admin_client=admin_client) clean_up_migration_jobs(client=admin_client, vm=hotplugged_vm) + return hotplugged_vmAs per coding guidelines, “Fixtures should return/yield the resource even if it’s a setup fixture.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/virt/node/migration_and_maintenance/test_post_copy_migration.py` around lines 93 - 101, The fixture drained_node_with_hotplugged_vm currently performs setup work but returns None implicitly. Modify the fixture to yield the hotplugged_vm resource after the drain_node context manager block, so that dependent tests can access the prepared VM and fixture composition remains explicit. This ensures the cleanup code in the context manager still executes properly after the test completes.Source: Coding guidelines
utilities/virt.py (1)
2216-2221:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Scope drain-process termination to avoid cross-test interference
Line 2221 uses
pkill -f "oc adm drain", which can kill unrelated drain operations running in parallel and destabilize other tests. Cleanup should target only the drain process started for this node.Suggested fix
- LOGGER.info("Terminate drain process") - run_command(command=shlex.split('pkill -f "oc adm drain"'), check=False, verify_stderr=False) + LOGGER.info(f"Terminate drain process for node {node.name}") + run_command( + command=shlex.split(f'pkill -f "oc adm drain {node.name}"'), + check=False, + verify_stderr=False, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@utilities/virt.py` around lines 2216 - 2221, The pkill command with the pattern "oc adm drain" is too broad and can kill unrelated drain operations from other tests running in parallel. Instead of using the generic pattern-based pkill in the finally block, capture the process ID (PID) when starting the nohup drain command and store it, then use that specific PID with the kill command to terminate only the drain process started for this particular node. This ensures that only the drain process associated with this specific test cleanup is terminated, preventing interference with concurrent tests.
|
/verified |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL — Verification rules for smoke and gating decisions: |
| @@ -1,3 +1,7 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
Test Execution Plan
Run smoke tests: True
Dependency paths:
tests/infrastructure/instance_types/supported_os/test_rhel_os.py::TestVMMigrationAndState::test_migrate_vm(smoke+gating) → callsmigrate_vm_and_verify(vm=..., client=admin_client, ...)→utilities/virt.py::migrate_vm_and_verify(signature changed to mandatoryclient: DynamicClient)tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py::TestCommonTemplatesRhel::test_migrate_vm(smoke) → same call chain
Run gating tests: True
Dependency path: 10 gating-marked files directly call migrate_vm_and_verify with the now-mandatory client argument.
Affected tests to run
(smoke)
tests/infrastructure/instance_types/supported_os/test_rhel_os.py::TestVMMigrationAndState::test_migrate_vm(smoke+gating)tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py::TestCommonTemplatesRhel::test_migrate_vm(smoke)
(gating)
tests/virt/upgrade/test_upgrade_virt.py(gating)tests/virt/node/general/test_machinetype.py(gating)tests/storage/test_hotplug.py(gating)tests/storage/online_resize/test_online_resize.py(gating)tests/virt/cluster/vm_cloning/test_vm_cloning.py(gating)tests/virt/cluster/vm_lifecycle/test_vm_run_strategy.py(gating)tests/network/localnet/test_default_bridge.py(gating)tests/virt/cluster/migration_and_maintenance/test_migration_policy.py(gating)tests/network/user_defined_network/test_user_defined_network.py(gating)tests/network/migration/test_masquerade_connectivity_after_migration.py(gating)
(other modified — no smoke/gating marker but directly affected)
tests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.pytests/infrastructure/numa/conftest.pytests/network/bgp/evpn/test_evpn_connectivity.pytests/network/bgp/test_bgp_connectivity.pytests/network/flat_overlay/conftest.pytests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.pytests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.pytests/network/localnet/test_ovs_bridge.pytests/network/migration/test_migration.pytests/network/service_mesh/test_service_mesh.pytests/network/sriov/test_sriov.pytests/network/user_defined_network/ip_specification/test_ip_specification.pytests/network/user_defined_network/test_user_defined_network_passt.pytests/storage/cross_cluster_live_migration/test_cclm.pytests/storage/storage_migration/test_storage_class_migration.pytests/storage/upgrade/test_upgrade_storage.pytests/virt/cluster/aaq/test_aaq_allocation_methods.pytests/virt/cluster/aaq/test_arq.pytests/virt/cluster/common_templates/centos/test_centos_os_support.pytests/virt/cluster/common_templates/fedora/test_fedora_os_support.pytests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.pytests/virt/cluster/common_templates/windows/test_windows_os_support.pytests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.pytests/virt/cluster/sysprep/test_sysprep.pytests/virt/node/general/test_windows_vtpm_bitlocker.pytests/virt/node/general/test_wsl2.pytests/virt/node/high_performance_vm/test_isolate_emulator_thread.pytests/virt/node/hotplug/test_cpu_memory_hotplug.pytests/virt/node/log_verbosity/test_log_virt_launcher.pytests/virt/node/migration_and_maintenance/test_odf_vm_migration.pytests/virt/node/migration_and_maintenance/test_post_copy_migration.pytests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.pytests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.pytests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.pytests/virt/node/workload_density/test_kernel_samepage_merging.pytests/virt/node/workload_density/test_swap.py
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get current head SHA
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4994 \
--jq '{head_sha: .head.sha, state: .state}'
echo ""
echo "=== FIRST CHANGED FILE ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4994/files \
--jq '.[0] | {filename: .filename, patch: (.patch // "no patch"), sha: .sha}' | head -40
echo ""
echo "=== VERIFY SMOKE/GATING FILES STILL CALL migrate_vm_and_verify ==="
rg -l 'migrate_vm_and_verify' tests/ --type py | while read f; do
if rg -q '`@pytest.mark.smoke`' "$f" 2>/dev/null; then
echo "SMOKE: $f"
fi
if rg -q '`@pytest.mark.gating`' "$f" 2>/dev/null; then
echo "GATING: $f"
fi
doneLength of output: 3195 🏁 Script executed: #!/bin/bash
BODY='## Test Execution Plan
**Run smoke tests: True**
Dependency paths:
- `tests/infrastructure/instance_types/supported_os/test_rhel_os.py::TestVMMigrationAndState::test_migrate_vm` (smoke+gating) → calls `migrate_vm_and_verify(vm=..., client=admin_client, ...)` → `utilities/virt.py::migrate_vm_and_verify` (signature changed to mandatory `client: DynamicClient`)
- `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py::TestCommonTemplatesRhel::test_migrate_vm` (smoke) → same call chain
**Run gating tests: True**
Dependency path: 10 gating-marked files directly call `migrate_vm_and_verify` with the now-mandatory `client` argument.
**Affected tests to run**
*(smoke)*
- `tests/infrastructure/instance_types/supported_os/test_rhel_os.py::TestVMMigrationAndState::test_migrate_vm` (smoke+gating)
- `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py::TestCommonTemplatesRhel::test_migrate_vm` (smoke)
*(gating)*
- `tests/virt/upgrade/test_upgrade_virt.py` (gating)
- `tests/virt/node/general/test_machinetype.py` (gating)
- `tests/storage/test_hotplug.py` (gating)
- `tests/storage/online_resize/test_online_resize.py` (gating)
- `tests/virt/cluster/vm_cloning/test_vm_cloning.py` (gating)
- `tests/virt/cluster/vm_lifecycle/test_vm_run_strategy.py` (gating)
- `tests/network/localnet/test_default_bridge.py` (gating)
- `tests/virt/cluster/migration_and_maintenance/test_migration_policy.py` (gating)
- `tests/network/user_defined_network/test_user_defined_network.py` (gating)
- `tests/network/migration/test_masquerade_connectivity_after_migration.py` (gating)
*(other modified — no smoke/gating marker but directly affected)*
- `tests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.py`
- `tests/infrastructure/numa/conftest.py`
- `tests/network/bgp/evpn/test_evpn_connectivity.py`
- `tests/network/bgp/test_bgp_connectivity.py`
- `tests/network/flat_overlay/conftest.py`
- `tests/network/l2_bridge/rhel9_rhel10_cluster/test_connectivity.py`
- `tests/network/l2_bridge/vmi_interfaces_stability/test_interfaces_stability.py`
- `tests/network/localnet/test_ovs_bridge.py`
- `tests/network/migration/test_migration.py`
- `tests/network/service_mesh/test_service_mesh.py`
- `tests/network/sriov/test_sriov.py`
- `tests/network/user_defined_network/ip_specification/test_ip_specification.py`
- `tests/network/user_defined_network/test_user_defined_network_passt.py`
- `tests/storage/cross_cluster_live_migration/test_cclm.py`
- `tests/storage/storage_migration/test_storage_class_migration.py`
- `tests/storage/upgrade/test_upgrade_storage.py`
- `tests/virt/cluster/aaq/test_aaq_allocation_methods.py`
- `tests/virt/cluster/aaq/test_arq.py`
- `tests/virt/cluster/common_templates/centos/test_centos_os_support.py`
- `tests/virt/cluster/common_templates/fedora/test_fedora_os_support.py`
- `tests/virt/cluster/common_templates/rhel/test_rhel_tablet_device.py`
- `tests/virt/cluster/common_templates/windows/test_windows_os_support.py`
- `tests/virt/cluster/longevity_tests/test_multi_vm_multi_migration.py`
- `tests/virt/cluster/sysprep/test_sysprep.py`
- `tests/virt/node/general/test_windows_vtpm_bitlocker.py`
- `tests/virt/node/general/test_wsl2.py`
- `tests/virt/node/high_performance_vm/test_isolate_emulator_thread.py`
- `tests/virt/node/hotplug/test_cpu_memory_hotplug.py`
- `tests/virt/node/log_verbosity/test_log_virt_launcher.py`
- `tests/virt/node/migration_and_maintenance/test_odf_vm_migration.py`
- `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py`
- `tests/virt/node/migration_and_maintenance/test_vm_disk_load_with_migration.py`
- `tests/virt/node/migration_and_maintenance/test_vm_memory_load_with_migration.py`
- `tests/virt/node/readonly_disk/test_vm_with_windows_guest_tools.py`
- `tests/virt/node/workload_density/test_kernel_samepage_merging.py`
- `tests/virt/node/workload_density/test_swap.py`'
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4994/reviews \
--method POST \
--field commit_id="88bfe5bd32c5ece7702e4a5446a34ab49b6d67cd" \
--field event="COMMENT" \
--field body="" \
--field "comments[][path]=tests/infrastructure/instance_types/supported_os/test_rhel_os.py" \
--field "comments[][position]=1" \
--field "comments[][body]=$BODY"Length of output: 6066 |
|
/approve |
11 participants
What this PR does / why we need it:
modified
migrate_vm_and_verifyfunc client arg to be mandatoryand update relevant calls in functions and tests
Assisted-by: Cursor Agent (Composer 2.5)
Which issue(s) this PR fixes:
Aligns migration tests with default client deprecation in python-wrapper repo
https://redhat.atlassian.net/browse/CNV-68519
Special notes for reviewer:
jira-ticket:
https://redhat.atlassian.net/browse/CNV-88319
Summary by CodeRabbit
Tests
usefixtureswhere applicable).Refactor / Documentation