feat(MTV-6088): add MTV-777 combined VM+populator inflight throttling test with DRS VM override - #602
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
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
Branch Management
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
Security Checks
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
/verified |
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds VMware clone placement and DRS controls, VM in-flight controller limit management, combined VM/populator migration monitoring, and a new copy-offload throttling test configuration. ChangesVM and Populator Throttling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant MigrationMonitor
participant Inventory
participant Plan
Test->>MigrationMonitor: Start monitored migration
MigrationMonitor->>Inventory: Resolve VM ESXi hosts
MigrationMonitor->>Plan: Poll migration status
Plan-->>MigrationMonitor: VM and populator activity
MigrationMonitor-->>Test: Return peak concurrency
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@conftest.py`:
- Around line 1129-1130: The guaranteed pyVmomi properties are being accessed
through unnecessary getattr-based defensive checks. In conftest.py lines
1129-1130, update the provider VM setup to access provider_vm_api.runtime.host
and runtime_host.name directly; in libs/providers/vmware.py lines 350-353,
access vm.runtime.host and host.parent directly while preserving the existing
host absence handling and error behavior.
In `@tests/copyoffload/test_copyoffload_migration.py`:
- Around line 5605-5638: Remove the unused target_namespace parameter from
test_check_vms and its docstring, leaving the check_vms invocation and all other
fixture parameters unchanged.
In `@utilities/copyoffload_migration.py`:
- Around line 1707-1806: Update the _combined_callback function to catch
ValueError from populator_tracker.poll(status), alongside ApiException, and log
it as a warning so transient migration UID lookup misses do not abort
wait_for_migration_complate. Keep the existing VM tracking and log-capture
behavior unchanged, matching the callback error-isolation pattern used for the
other monitors.
In `@utilities/forklift_controller_populator.py`:
- Around line 520-532: The shared lock helper must return the lock used by all
ForkliftController inflight-limit mutations. Update
get_forkliftcontroller_vm_populator_inflight_lock_path to use the existing
populator-inflight.lock filename, ensuring
vm_populator_inflight_forkliftcontroller and
populator_inflight_forkliftcontroller serialize through the same mutex.
🪄 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: 21d9548a-dca5-404c-bcee-e222cff355fc
📒 Files selected for processing (9)
AGENTS.mdconftest.pylibs/providers/vmware.pytests/copyoffload/conftest.pytests/copyoffload/test_copyoffload_migration.pytests/tests_config/config.pyutilities/copyoffload_constants.pyutilities/copyoffload_migration.pyutilities/forklift_controller_populator.py
redhat-qe-bot1
added
commented-prabinovRedhat
and removed
changes-requested-coderabbitai[bot]
verified
labels
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/forklift_controller_populator.py (1)
534-604: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDo not silently suppress VM-limit restoration failures.
When the test body raises and cleanup also fails, this path logs the restoration error but preserves no actionable failure for the dirty controller state. A later test can inherit
test_limit, causing cross-test throttling failures. Surface the cleanup failure while preserving the original exception, or otherwise fail the worker/session when restoration cannot be confirmed.🤖 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/forklift_controller_populator.py` around lines 534 - 604, The vm_inflight_limit cleanup in the finally block must not silently suppress restoration failures when the test body also raised. Update the pending_exc handling around _ensure_forklift_controller_vm_limit and wait_for_vm_inflight_deployment so the cleanup error is surfaced while preserving the original test exception, or terminate the worker/session if restoration cannot be confirmed; retain the current behavior of raising cleanup errors when no prior exception exists.
🤖 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 `@utilities/forklift_controller_populator.py`:
- Around line 521-526: Update the docstring for the shared-lock path helper near
get_forkliftcontroller_populator_inflight_lock_path() to state that the VM and
populator fixtures mutate controller_max_vm_inflight and
controller_max_populator_inflight, respectively, while preserving the existing
shared-mutex rationale.
---
Outside diff comments:
In `@utilities/forklift_controller_populator.py`:
- Around line 534-604: The vm_inflight_limit cleanup in the finally block must
not silently suppress restoration failures when the test body also raised.
Update the pending_exc handling around _ensure_forklift_controller_vm_limit and
wait_for_vm_inflight_deployment so the cleanup error is surfaced while
preserving the original test exception, or terminate the worker/session if
restoration cannot be confirmed; retain the current behavior of raising cleanup
errors when no prior exception exists.
🪄 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: f8d925a8-7fcc-4460-a6e8-310ee48d5de0
📒 Files selected for processing (5)
conftest.pylibs/providers/vmware.pytests/copyoffload/test_copyoffload_migration.pyutilities/copyoffload_migration.pyutilities/forklift_controller_populator.py
💤 Files with no reviewable changes (1)
- tests/copyoffload/test_copyoffload_migration.py
|
/verified |
2d1b0ed to
4de71ad
Compare
… event expectation Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
conftest.py (1)
1113-1121: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHIGH: Do not let per-VM host overrides defeat
clone_to_same_host.
setdefault()preserves an existingtarget_esxi_host, including a conflicting host orNone, so the resulting clones may not sharefirst_vm_esxi_host. That invalidates the downstream per-host throttling assertions. Reject conflicting overrides or assign the captured host unconditionally whenclone_to_same_host=True.The follow-up commits added same-host lookup and DRS controls, but this override remains an escape hatch from that contract.
🤖 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 `@conftest.py` around lines 1113 - 1121, Update the clone-options logic in the virtual-machine loop so that when plan["clone_to_same_host"] is enabled and first_vm_esxi_host is set, target_esxi_host is assigned unconditionally to first_vm_esxi_host. Do not preserve per-VM overrides in this mode, ensuring every clone uses the captured host.utilities/copyoffload_migration.py (1)
1821-1835: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winCatch
ValueErrorin the VM tracker branch of_combined_callbacktoo.
_count_active_vms_by_host()(called viavm_tracker.poll()) explicitly documentsRaises: ValueError: If an active VM cannot be resolved to a host in vm_host_map, but thevm_tracker.poll(status)branch here only catchesApiException. This is the exact same hazard that was already found and fixed for the populator branch two lines below (except (ApiException, ValueError) as err:), which exists precisely because_resolve_migration_uid()could raiseValueError. An uncaughtValueErrorhere would abortwait_for_migration_complate's entire poll loop instead of just skipping a transient VM-status/host-mapping miss.🐛 Proposed fix
try: vm_tracker.poll(status) - except ApiException as err: + except (ApiException, ValueError) as err: LOGGER.warning(f"VM concurrency tracking failed during poll: {err}")🤖 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/copyoffload_migration.py` around lines 1821 - 1835, Update the vm_tracker.poll(status) exception handler in _combined_callback to catch both ApiException and ValueError, matching the populator_tracker.poll(status) branch. Preserve the existing warning message and allow the callback to continue to log status after either expected tracking failure.
🤖 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 `@conftest.py`:
- Around line 1097-1100: Move the disable_drs_for_vms validation out of the
source_provider.type != Provider.ProviderType.OVA branch so it also runs for OVA
plans. Reject any OVA plan with disable_drs_for_vms enabled, while preserving
the existing VMware clone-provider requirement for non-OVA plans and failing
before provider-specific processing.
---
Outside diff comments:
In `@conftest.py`:
- Around line 1113-1121: Update the clone-options logic in the virtual-machine
loop so that when plan["clone_to_same_host"] is enabled and first_vm_esxi_host
is set, target_esxi_host is assigned unconditionally to first_vm_esxi_host. Do
not preserve per-VM overrides in this mode, ensuring every clone uses the
captured host.
In `@utilities/copyoffload_migration.py`:
- Around line 1821-1835: Update the vm_tracker.poll(status) exception handler in
_combined_callback to catch both ApiException and ValueError, matching the
populator_tracker.poll(status) branch. Preserve the existing warning message and
allow the callback to continue to log status after either expected tracking
failure.
🪄 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 Plus
Run ID: de06b744-946e-4a01-ac79-9156a96215dc
📒 Files selected for processing (4)
AGENTS.mdconftest.pytests/copyoffload/test_copyoffload_migration.pyutilities/copyoffload_migration.py
Raise ValueError in prepared_plan for OVA providers and document that the flag is unsupported for OVA. Co-authored-by: Cursor <cursoragent@cursor.com>
redhat-qe-bot1
added
commented-prabinovRedhat
commented-coderabbitai[bot]
and removed
changes-requested-coderabbitai[bot]
labels
|
/verified |
|
@myakove can we merge this PR? |
|
/lgtm |
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
|
/cherry-pick v2.11 |
|
Cherry-pick conflicts were resolved by AI Cherry-picked PR feat(MTV-6088): add MTV-777 combined VM+populator inflight throttling test with DRS VM override into v2.11: #613 @prabinovRedhat Manual verification is required — please review the changes and test before merging. |
6 participants
Jira / Polarion
What was added
disable_drs_for_vm()onVMWareProviderPer-VM vSphere DRS automation override using
vim.cluster.DrsVmConfigSpec+vim.cluster.ConfigSpecEx. Pins a cloned VM to its current ESXi host by settingDrsVmConfigInfo(enabled=False), preventing DRS from relocating it before migration starts.TestCopyoffloadVmPopulatorThrottlingMigration(MTV-777, 7-step)New test class covering combined VM + populator inflight throttling:
controller_max_vm_inflight=1(only 1 VM migrates at a time per ESXi host)controller_max_populator_inflight=2(only 2 populate pods active per ESXi host)verify_vm_inflight_throttling()Combined monitoring utilities
execute_migration_monitoring_vm_and_populator_inflight()— single migration run with 3 simultaneous callbacks: VM concurrency tracker + populator concurrency tracker + log captureverify_vm_inflight_throttling()— pure computation, validates peak ≤ limit AND peak ≥ min(limit, vm_count)_VmConcurrencyTracker— mirrors_PopulatorConcurrencyTrackerfor VM-level schedulingVM+populator inflight fixture
vm_populator_inflight_forkliftcontroller(class-scoped) — patches bothcontroller_max_vm_inflightandcontroller_max_populator_inflightwith a single combined file lock. Nested context managers ensure both limits are restored on teardown even after failures.disable_drs_for_vmin conftest.pyprepared_planWhen
plan.get("disable_drs_for_vms", False)is set, callsclone_provider.disable_drs_for_vm()after each clone to prevent DRS from moving the VM before migration.Config
CI
Same-host cloning pinned to ESXi
10.46.29.136, DRS disabled for all VMs on Eco-Cluster.controller_max_vm_inflightrestored from 1→20 on teardown.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests