Repository navigation
feat(MTV-5654): add copy-offload populator throttling test - #550
Merged
myakove merged 27 commits intoJun 19, 2026
Merged
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? |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughImplements MTV-696 end-to-end copy-offload populator throttling tests by adding constants, a new ChangesMTV-696 Populator Throttling Migration Test
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over Test,populator_inflight_limit: Fixture Setup
Test->>populator_inflight_forkliftcontroller: class-scoped fixture entry
populator_inflight_forkliftcontroller->>FileLock: acquire(timeout=POPULATOR_INFLIGHT_LOCK_TIMEOUT)
populator_inflight_forkliftcontroller->>ForkliftController: wait for RUNNING state
populator_inflight_forkliftcontroller->>get_deployment_populator_inflight_limit: read current MAX_POPULATOR_INFLIGHT
populator_inflight_forkliftcontroller->>populator_inflight_limit: apply POPULATOR_INFLIGHT_LIMIT=2
populator_inflight_limit->>ForkliftController: patch spec.controller_max_populator_inflight=2
populator_inflight_limit->>wait_for_populator_inflight_deployment: poll until Deployment env-var matches
end
rect rgba(60, 179, 113, 0.5)
Note over Test,verify_populator_throttling: Migration Execution and Verification
Test->>execute_migration_monitoring_populator_inflight: plan, target_namespace
execute_migration_monitoring_populator_inflight->>wait_for_copyoffload_plan_secret: wait for plan populator Secret
execute_migration_monitoring_populator_inflight->>wait_for_migration_complate: on_status_poll=tracker.poll
wait_for_migration_complate->_PopulatorConcurrencyTracker: poll per iteration
_PopulatorConcurrencyTracker->>_count_active_populator_pods_by_host: count Running/Pending pods by SOURCE_HOST_LABEL
execute_migration_monitoring_populator_inflight-->>Test: max_concurrent_by_host dict
Test->>verify_populator_throttling: max_concurrent_by_host, max_populator_inflight=2
verify_populator_throttling->>_verify_source_host_labels_on_pods: assert single SOURCE_HOST_LABEL per pod
verify_populator_throttling->>_verify_throttled_events_on_pods: assert POPULATOR_THROTTLED_EVENT_REASON on PVCs
verify_populator_throttling->>_verify_populator_inflight_observed: assert peak ≤ 2 and peak ≥ min(2, disk_count)
end
rect rgba(255, 165, 0, 0.5)
Note over populator_inflight_limit,ForkliftController: Fixture Teardown
populator_inflight_limit->>ForkliftController: restore original_deployment_limit
populator_inflight_limit->>wait_for_populator_inflight_deployment: confirm Deployment restored
FileLock->>FileLock: release
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
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)
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
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
utilities/mtv_migration.py (1)
367-404:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
Raisessection to docstring.The function raises
MigrationPlanExecErroron failure/timeout (lines 399-404), but the docstring omits theRaisessection. Per coding guidelines, includeRaiseswhen exceptions are actually raised.📝 Proposed fix
def wait_for_migration_complate( plan: Plan, *, on_status_poll: Callable[[str], None] | None = None, ) -> None: """Wait for a Plan migration to reach Succeeded or Failed. Args: plan (Plan): The Plan resource to monitor. on_status_poll (Callable[[str], None] | None): Optional callback invoked on each poll with the current migration status string. + + Raises: + MigrationPlanExecError: If the migration fails or times out. """🤖 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/mtv_migration.py` around lines 367 - 404, Update the docstring for wait_for_migration_complate to include a Raises section that documents MigrationPlanExecError being raised when the plan migration fails or when a timeout occurs (the except branch catching TimeoutExpiredError and MigrationPlanExecError re-raises MigrationPlanExecError); reference the Plan parameter and mention the function uses TimeoutSampler and py_config.get("plan_wait_timeout") for timing so readers know the timeout source.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 `@guides/copyoffload/how-to-run-copyoffload-tests.md`:
- Around line 424-432: Update the pytest example so it explicitly
disables/limits xdist parallelism to enforce isolation for
TestCopyoffloadPopulatorThrottlingMigration; modify the suggested command that
runs pytest -m copyoffload -k TestCopyoffloadPopulatorThrottlingMigration to
include an explicit xdist flag (e.g., add -n 0 or -n 1) so the test never runs
concurrently with other tests that rely on the default populator limit.
In `@tests/copyoffload/test_copyoffload_migration.py`:
- Around line 3514-3554: Add full Python type annotations to each new test
method in this class (including test_create_storagemap, test_create_networkmap,
test_create_plan, test_check_vms and the other methods mentioned around lines
3556-3609 and 3650-3675): annotate every fixture parameter with appropriate
built-in typing (e.g., dict, list, tuple, str, int, or specific types used by
your test fixtures) and add a return type of -> None on each function signature;
ensure you use only built-in typing names (dict, list, tuple) as required by the
repo policy and update any untyped fixture parameters in
get_storage_migration_map callers (and related helper calls) to match the new
annotated signatures so the test signatures are consistent with test_migrate_vms
and test_verify_populator_throttling.
---
Outside diff comments:
In `@utilities/mtv_migration.py`:
- Around line 367-404: Update the docstring for wait_for_migration_complate to
include a Raises section that documents MigrationPlanExecError being raised when
the plan migration fails or when a timeout occurs (the except branch catching
TimeoutExpiredError and MigrationPlanExecError re-raises
MigrationPlanExecError); reference the Plan parameter and mention the function
uses TimeoutSampler and py_config.get("plan_wait_timeout") for timing so readers
know the timeout source.
🪄 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: 07f2d88a-ce6c-48bf-98fc-e62666fe7f9a
📒 Files selected for processing (8)
README.mdguides/copyoffload/how-to-run-copyoffload-tests.mdtests/copyoffload/conftest.pytests/copyoffload/test_copyoffload_migration.pytests/tests_config/config.pyutilities/copyoffload_constants.pyutilities/copyoffload_migration.pyutilities/mtv_migration.py
prabinovRedhat
force-pushed
the
feat/MTV-5654-populator-throttling
branch
from
June 10, 2026 07:27
b9c4dc0 to
b91d9f5
Compare
Collaborator
Author
|
cc: @tshefi |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
utilities/copyoffload_migration.py (1)
925-946:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Poll for
PopulatorThrottledevents instead of reading them once.
Event.list()is eventually consistent, so this one-shot read can miss throttling events that were emitted during the migration but have not been surfaced yet when verification runs. That makes the new copy-offload test flaky under cluster load.A short retry window here is enough: keep collecting unique PVCs with
PopulatorThrottleduntil you reachmin_expected_throttled, then assert.Suggested fix
- throttled_pvc_names: list[str] = [] - for pod in populate_pods: - labels: dict[str, str] = pod.instance.metadata.labels or {} - pvc_name: str = labels.get("pvcName", pod.name) - field_selector = f"involvedObject.name={pvc_name},involvedObject.kind=PersistentVolumeClaim" - for event in Event.list( - client=ocp_admin_client, - namespace=target_namespace, - field_selector=field_selector, - since_seconds=py_config["plan_wait_timeout"], - ): - if event.get("reason") == POPULATOR_THROTTLED_EVENT_REASON: - throttled_pvc_names.append(pvc_name) - LOGGER.info(f"PVC '{pvc_name}' has {POPULATOR_THROTTLED_EVENT_REASON} event") - break + def _collect_throttled_pvc_names() -> set[str]: + seen: set[str] = set() + for pod in populate_pods: + labels: dict[str, str] = pod.instance.metadata.labels or {} + pvc_name: str = labels.get("pvcName", pod.name) + field_selector = f"involvedObject.name={pvc_name},involvedObject.kind=PersistentVolumeClaim" + for event in Event.list( + client=ocp_admin_client, + namespace=target_namespace, + field_selector=field_selector, + since_seconds=py_config["plan_wait_timeout"], + ): + if event.get("reason") == POPULATOR_THROTTLED_EVENT_REASON: + seen.add(pvc_name) + break + return seen + + throttled_pvc_names: set[str] = set() + for throttled_pvc_names in TimeoutSampler(wait_timeout=30, sleep=2, func=_collect_throttled_pvc_names): + if len(throttled_pvc_names) >= min_expected_throttled: + break if len(throttled_pvc_names) < min_expected_throttled: raise ValueError( f"Expected at least {min_expected_throttled} PVC(s) with {POPULATOR_THROTTLED_EVENT_REASON} " f"events for migration '{migration_uid}' ({len(populate_pods)} disks, " f"limit={max_populator_inflight}); found {len(throttled_pvc_names)}: {throttled_pvc_names}" )As per coding guidelines, tests must be deterministic.
🤖 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 925 - 946, The one-time read of Event.list can miss eventual events; change the block that builds throttled_pvc_names (iterating populate_pods) into a short retry/poll loop that repeatedly queries Event.list for each pod (using the same field_selector and since_seconds) until either a deadline (e.g. now + a small retry window) is reached or throttled_pvc_names contains at least min_expected_throttled unique PVC names. Use a set to dedupe PVCs (convert to list for existing callers), check event.get("reason") == POPULATOR_THROTTLED_EVENT_REASON to add the pvc, log when discovered, and sleep a small interval between iterations; after the loop assert the same ValueError if the count is still below min_expected_throttled. Ensure you update references to throttled_pvc_names, populate_pods, Event.list, POPULATOR_THROTTLED_EVENT_REASON, and min_expected_throttled when implementing the retry loop.Source: Coding guidelines
tests/copyoffload/test_copyoffload_migration.py (1)
3581-3608:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Fail fast if the plan config loses
copyoffload=True.This class is specifically validating copy-offload throttling, so
copyoffloadis not optional here.prepared_plan.get("copyoffload", False)silently downgrades the run to a non-copy-offload migration if the config key is missing or renamed, and the suite then fails much later in throttling/XCOPY assertions with a misleading signal.Proposed fix
self.__class__.plan_resource = create_plan_resource( ocp_admin_client=ocp_admin_client, fixture_store=fixture_store, source_provider=source_provider, destination_provider=destination_provider, storage_map=self.storage_map, network_map=self.network_map, virtual_machines_list=prepared_plan["virtual_machines"], target_namespace=target_namespace, warm_migration=prepared_plan.get("warm_migration", False), - copyoffload=prepared_plan.get("copyoffload", False), + copyoffload=prepared_plan["copyoffload"], )As per coding guidelines, "For configurations controlled by the codebase (
py_config,plan,tests_params), never use defaults; always use direct access likepy_config['storage_class']not.get()with defaults."🤖 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/copyoffload/test_copyoffload_migration.py` around lines 3581 - 3608, In test_create_plan, do not use prepared_plan.get("copyoffload", False); instead access prepared_plan["copyoffload"] directly (or assert "copyoffload" in prepared_plan before calling create_plan_resource) so a missing/renamed key fails fast; update the create_plan_resource call in the test_create_plan function to pass prepared_plan["copyoffload"] (or perform an explicit assertion on prepared_plan) to ensure copyoffload must be present.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.
Outside diff comments:
In `@tests/copyoffload/test_copyoffload_migration.py`:
- Around line 3581-3608: In test_create_plan, do not use
prepared_plan.get("copyoffload", False); instead access
prepared_plan["copyoffload"] directly (or assert "copyoffload" in prepared_plan
before calling create_plan_resource) so a missing/renamed key fails fast; update
the create_plan_resource call in the test_create_plan function to pass
prepared_plan["copyoffload"] (or perform an explicit assertion on prepared_plan)
to ensure copyoffload must be present.
In `@utilities/copyoffload_migration.py`:
- Around line 925-946: The one-time read of Event.list can miss eventual events;
change the block that builds throttled_pvc_names (iterating populate_pods) into
a short retry/poll loop that repeatedly queries Event.list for each pod (using
the same field_selector and since_seconds) until either a deadline (e.g. now + a
small retry window) is reached or throttled_pvc_names contains at least
min_expected_throttled unique PVC names. Use a set to dedupe PVCs (convert to
list for existing callers), check event.get("reason") ==
POPULATOR_THROTTLED_EVENT_REASON to add the pvc, log when discovered, and sleep
a small interval between iterations; after the loop assert the same ValueError
if the count is still below min_expected_throttled. Ensure you update references
to throttled_pvc_names, populate_pods, Event.list,
POPULATOR_THROTTLED_EVENT_REASON, and min_expected_throttled when implementing
the retry loop.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8af482da-8ef9-48c9-be17-94702420c189
📒 Files selected for processing (4)
README.mdtests/copyoffload/test_copyoffload_migration.pyutilities/copyoffload_constants.pyutilities/copyoffload_migration.py
Collaborator
Author
|
/verified |
Collaborator
|
/lgtm |
This was referenced Jul 8, 2026
This was referenced Jul 21, 2026
This was referenced Jul 28, 2026
This was referenced Aug 14, 2026
2 tasks
This was referenced Sep 29, 2026
This was referenced Oct 7, 2026
This was referenced Oct 19, 2026
This was referenced Nov 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds MTV-5654 copy-offload (Case 1 + Case 3) coverage for per-ESXi-host populator throttling and
sourceHostlabels.TestCopyoffloadPopulatorThrottlingMigration: 7-step flow (storagemap → networkmap → plan → migrate with peak monitoring → verify throttling → check xcopy → check VMs)populator_inflight_forkliftcontrollerfixture: class-scoped patch ofForkliftController.spec.controller_max_populator_inflightand deployment env to limit 2, with pre-test snapshot restore on teardown and xdist file lockPopulatorThrottledevents (≥ disks − limit), singlesourceHostlabel on all populate podswait_for_migration_complate(on_status_poll=...)and publicget_migration_for_plan()for migration-scoped monitoringRebased on latest
main(includesmerge_storage_secret_extraintegration).Test plan
uv run pytest -m copyoffload -k TestCopyoffloadPopulatorThrottlingMigration— 7/7 passed (~17 min)PopulatorThrottledevents with(2/2 in-flight)Notes
Summary by CodeRabbit