[virt] drop get_client() and force passing client arg#4916
Conversation
📝 WalkthroughWalkthroughservice_ip now requires an admin_client and uses privileged vmi.get_node; wait_for_migration_finished accepts only a migration object, detects prolonged SCHEDULING (~4 minutes), logs warning pod diagnostics, and raises MigrationStuckSchedulingError. Call sites and tests updated accordingly. ChangesService IP and Migration Waiting Refactoring
🎯 4 (Complex) | ⏱️ ~45 minutes
🔕 Pre-merge checks override appliedThe pre-merge checks have been overridden successfully. You can now proceed with the merge. Overridden by ❌ Failed checks (1 error)
✅ Passed checks (4 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 |
|
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-4
requested review from
frenzyfriday and
kbidarkar
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4916 +/- ##
==========================================
- Coverage 98.67% 98.66% -0.02%
==========================================
Files 25 25
Lines 2487 2464 -23
==========================================
- Hits 2454 2431 -23
Misses 33 33
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
openshift-virtualization-qe-bot-4
requested review from
EdDev,
RoniKishner,
akri3i,
azhivovk,
dshchedr,
guchen11,
nirdothan,
orelmisan,
rnetser and
sarahbx
openshift-virtualization-qe-bot
removed
the
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
clean up last virt utils from using get_client() Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Signed-off-by: vsibirsk <vsibirsk@redhat.com>
Signed-off-by: vsibirsk <vsibirsk@redhat.com>
move pod event collection to separate module-scope func replaced/updated raised exceptions Signed-off-by: vsibirsk <vsibirsk@redhat.com>
|
Clean rebase detected — no code changes compared to previous head ( |
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/virt.py (1)
1957-1989:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Stop polling once the migration has already failed.
This loop only exits on
Status.SUCCEEDED. If the VMIM reaches a terminal non-success phase, callers likemigrate_vm_and_verify()andverify_hotplug()will still sit here until the outer timeout expires, turning an immediate migration failure into a 10–30 minute delay. Handle terminal failure phases explicitly and reserve the 4-minuteSCHEDULINGpath for the stuck-pending case only.
🤖 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/exceptions.py`:
- Around line 134-138: MigrationStuckSchedulingError.__init__ currently only
sets self.migration_name so Exception.args is empty; update
MigrationStuckSchedulingError.__init__ to call super().__init__ with the error
message (e.g., super().__init__(str(self))) after setting self.migration_name so
the base Exception is initialized and exception.args contains the human-readable
message; refer to the MigrationStuckSchedulingError class and its __init__ and
__str__ methods when making this change.
In `@utilities/unittests/test_exceptions.py`:
- Around line 320-334: Extend the MigrationStuckSchedulingError test to assert
the exception's args tuple to lock the pickling-safe contract: in
test_migration_stuck_scheduling_error_init (or a new small test) add an
assertion like assert error.args == (migration_name,) so that if you later call
super().__init__(...) in utilities/exceptions.py the tests ensure args contains
the expected migration_name value (mirror the pattern used for
DataVolumeConditionMessageNotFoundError).
🪄 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: 92251481-6071-4753-8e6c-182a3aa24a06
📒 Files selected for processing (8)
tests/network/migration/test_migration.pytests/scale/test_scale_benchmark.pytests/utils.pytests/virt/node/descheduler/conftest.pytests/virt/utils.pyutilities/exceptions.pyutilities/unittests/test_exceptions.pyutilities/virt.py
💤 Files with no reviewable changes (1)
- tests/scale/test_scale_benchmark.py
|
/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
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
echo "=== Modified symbols in utilities/ ==="
git diff HEAD~1 --unified=0 -- utilities/virt.py | grep '^[+-]def ' | head -20
git diff HEAD~1 --unified=0 -- utilities/exceptions.py | grep '^[+-]def \|^[+-]class ' | head -20
echo ""
echo "=== Direct callers of wait_for_migration_finished ==="
rg -rn "wait_for_migration_finished" --type py -l
echo ""
echo "=== Direct callers of service_ip ==="
rg -rn "\.service_ip\(" --type py -l
echo ""
echo "=== Direct callers of log_failed_pod_events ==="
rg -rn "log_failed_pod_events" --type py -l
echo ""
echo "=== Direct callers of MigrationStuckSchedulingError ==="
rg -rn "MigrationStuckSchedulingError" --type py -l
echo ""
echo "=== Same-file callers in utilities/virt.py ==="
rg -n "wait_for_migration_finished\|service_ip\|log_failed_pod_events\|MigrationStuckSchedulingError" utilities/virt.py
echo ""
echo "=== Smoke-marked test files ==="
rg -rn "`@pytest.mark.smoke`" --type py -l
echo ""
echo "=== Gating-marked test files ==="
rg -rn "`@pytest.mark.gating`" --type py -l
echo ""
echo "=== Do any smoke files use wait_for_migration_finished or service_ip? ==="
smoke_files=$(rg -rn "`@pytest.mark.smoke`" --type py -l)
for f in $smoke_files; do
if grep -q "wait_for_migration_finished\|service_ip\|MigrationStuckSchedulingError\|migrate_vm_and_verify\|migrate_and_verify_multi_vms\|verify_hotplug" "$f" 2>/dev/null; then
echo "FOUND IN SMOKE: $f"
grep -n "wait_for_migration_finished\|service_ip\|MigrationStuckSchedulingError\|migrate_vm_and_verify\|migrate_and_verify_multi_vms\|verify_hotplug" "$f"
fi
done
echo "=== End smoke check ==="
echo ""
echo "=== Do any gating files use wait_for_migration_finished, service_ip, migrate_vm_and_verify, etc? ==="
gating_files=$(rg -rn "`@pytest.mark.gating`" --type py -l)
for f in $gating_files; do
if grep -q "wait_for_migration_finished\|service_ip\|MigrationStuckSchedulingError\|migrate_vm_and_verify\|migrate_and_verify_multi_vms\|verify_hotplug" "$f" 2>/dev/null; then
echo "FOUND IN GATING: $f"
grep -n "wait_for_migration_finished\|service_ip\|MigrationStuckSchedulingError\|migrate_vm_and_verify\|migrate_and_verify_multi_vms\|verify_hotplug" "$f"
fi
done
echo "=== End gating check ==="
echo ""
echo "=== PR head SHA and first file first changed line ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4916 \
--jq '{sha: .head.sha}'
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4916/files \
--jq '.[0] | {filename: .filename, patch_first_lines: .patch[0:400]}'Length of output: 11322 🏁 Script executed: #!/bin/bash
curl -s -X POST \
-H "Authorization: token ${GITHUB_TOKEN}" \
-H "Accept: application/vnd.github.v3+json" \
"https://api.github.com/repos/RedHatQE/openshift-virtualization-tests/pulls/4916/comments" \
-d '{
"commit_id": "df635af81269324014ccf4dc3a16b46e8a37b01b",
"path": "tests/network/migration/test_migration.py",
"side": "RIGHT",
"line": 361,
"body": "**Test Execution Plan**\n\n- **Run smoke tests: True** — `tests/infrastructure/instance_types/supported_os/test_rhel_os.py` (smoke) → `migrate_vm_and_verify` → `wait_for_migration_finished` (signature + behavior changed: new stuck-scheduling detection raises `MigrationStuckSchedulingError`). Same path for `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py` (smoke).\n- **Run gating tests: True** — Multiple gating tests → `migrate_vm_and_verify` → `wait_for_migration_finished` (changed); `tests/virt/cluster/resource_limits/test_auto_resource_limits.py` (gating) → `hotplug_spec_vm_and_verify_hotplug` → `verify_hotplug` (`tests/utils.py`) → `wait_for_migration_finished`.\n\n**Affected tests to run:**\n- `tests/network/migration/test_migration.py::test_migration_with_masquerade` — `service_ip` now requires `admin_client`.\n- `tests/scale/test_scale_benchmark.py::TestScale::test_mass_vm_live_migration` — direct caller of updated `wait_for_migration_finished`.\n- `tests/virt/node/descheduler/` — conftest `all_existing_migrations_completed` directly calls updated `wait_for_migration_finished`.\n- `tests/infrastructure/instance_types/supported_os/test_rhel_os.py` (smoke + gating) — transitive via `migrate_vm_and_verify`.\n- `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py` (smoke) — transitive via `migrate_vm_and_verify`.\n- `tests/network/migration/test_masquerade_connectivity_after_migration.py` (gating) — transitive via `migrate_vm_and_verify`.\n- `tests/virt/cluster/migration_and_maintenance/test_migration_policy.py` (gating) — transitive via `migrate_vm_and_verify`.\n- `tests/virt/cluster/vm_lifecycle/test_vm_run_strategy.py` (gating) — transitive via `migrate_vm_and_verify`.\n- `tests/virt/upgrade/test_upgrade_virt.py` (gating) — transitive via `migrate_vm_and_verify`.\n- `tests/storage/test_hotplug.py` (gating) — transitive via `migrate_vm_and_verify` and `verify_hotplug`.\n- `tests/virt/cluster/resource_limits/test_auto_resource_limits.py` (gating) — transitive via `hotplug_spec_vm_and_verify_hotplug` → `verify_hotplug` → `wait_for_migration_finished`.\n- `-m migration` — covers remaining callers of `wait_for_migration_finished` via `migrate_and_verify_multi_vms` in `tests/virt/utils.py`."
}' | python3 -c "import sys,json; d=json.load(sys.stdin); print('id:', d.get('id'), 'url:', d.get('html_url'))" 2>&1Length of output: 2838 🏁 Script executed: #!/bin/bash
# Check actual error with raw response first
curl -s -X POST \
-H "Authorization: token ${GITHUB_TOKEN}" \
-H "Accept: application/vnd.github.v3+json" \
"https://api.github.com/repos/RedHatQE/openshift-virtualization-tests/pulls/4916/comments" \
-d '{
"commit_id": "df635af81269324014ccf4dc3a16b46e8a37b01b",
"path": "tests/network/migration/test_migration.py",
"side": "RIGHT",
"line": 361,
"body": "**Test Execution Plan**\n\n- **Run smoke tests: True** — `tests/infrastructure/instance_types/supported_os/test_rhel_os.py` (smoke) \u2192 `migrate_vm_and_verify` \u2192 `wait_for_migration_finished` (signature + behavior changed: new stuck-scheduling detection raises `MigrationStuckSchedulingError`). Same path for `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py` (smoke).\n- **Run gating tests: True** — Multiple gating tests \u2192 `migrate_vm_and_verify` \u2192 `wait_for_migration_finished` (changed); `tests/virt/cluster/resource_limits/test_auto_resource_limits.py` (gating) \u2192 `hotplug_spec_vm_and_verify_hotplug` \u2192 `verify_hotplug` \u2192 `wait_for_migration_finished`.\n\n**Affected tests to run:**\n- `tests/network/migration/test_migration.py::test_migration_with_masquerade`\n- `tests/scale/test_scale_benchmark.py::TestScale::test_mass_vm_live_migration`\n- `tests/virt/node/descheduler/` (conftest `all_existing_migrations_completed`)\n- `tests/infrastructure/instance_types/supported_os/test_rhel_os.py` (smoke + gating)\n- `tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py` (smoke)\n- `tests/network/migration/test_masquerade_connectivity_after_migration.py` (gating)\n- `tests/virt/cluster/migration_and_maintenance/test_migration_policy.py` (gating)\n- `tests/virt/cluster/vm_lifecycle/test_vm_run_strategy.py` (gating)\n- `tests/virt/upgrade/test_upgrade_virt.py` (gating)\n- `tests/storage/test_hotplug.py` (gating)\n- `tests/virt/cluster/resource_limits/test_auto_resource_limits.py` (gating)\n- `-m migration`"
}' 2>&1 | head -100Length of output: 2197 🏁 Script executed: #!/bin/bash
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/4916/comments \
--method POST \
-f commit_id="df635af81269324014ccf4dc3a16b46e8a37b01b" \
-f path="tests/network/migration/test_migration.py" \
-f side="RIGHT" \
-F line=361 \
-f body="**Test Execution Plan**
- **Run smoke tests: True** — \`tests/infrastructure/instance_types/supported_os/test_rhel_os.py\` (smoke) → \`migrate_vm_and_verify\` → \`wait_for_migration_finished\` (signature + behavior changed: new stuck-scheduling detection raises \`MigrationStuckSchedulingError\`). Same path for \`tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py\` (smoke).
- **Run gating tests: True** — Multiple gating tests → \`migrate_vm_and_verify\` → \`wait_for_migration_finished\` (changed); \`tests/virt/cluster/resource_limits/test_auto_resource_limits.py\` (gating) → \`hotplug_spec_vm_and_verify_hotplug\` → \`verify_hotplug\` (\`tests/utils.py\`) → \`wait_for_migration_finished\`.
**Affected tests to run:**
- \`tests/network/migration/test_migration.py::test_migration_with_masquerade\` — \`service_ip\` now requires \`admin_client\`
- \`tests/scale/test_scale_benchmark.py::TestScale::test_mass_vm_live_migration\` — direct caller of updated \`wait_for_migration_finished\`
- \`tests/virt/node/descheduler/\` — conftest \`all_existing_migrations_completed\` directly calls updated \`wait_for_migration_finished\`
- \`tests/infrastructure/instance_types/supported_os/test_rhel_os.py\` (smoke + gating) — transitive via \`migrate_vm_and_verify\`
- \`tests/virt/cluster/common_templates/rhel/test_rhel_os_support.py\` (smoke) — transitive via \`migrate_vm_and_verify\`
- \`tests/network/migration/test_masquerade_connectivity_after_migration.py\` (gating) — transitive via \`migrate_vm_and_verify\`
- \`tests/virt/cluster/migration_and_maintenance/test_migration_policy.py\` (gating) — transitive via \`migrate_vm_and_verify\`
- \`tests/virt/cluster/vm_lifecycle/test_vm_run_strategy.py\` (gating) — transitive via \`migrate_vm_and_verify\`
- \`tests/virt/upgrade/test_upgrade_virt.py\` (gating) — transitive via \`migrate_vm_and_verify\`
- \`tests/storage/test_hotplug.py\` (gating) — transitive via \`migrate_vm_and_verify\` and \`verify_hotplug\`
- \`tests/virt/cluster/resource_limits/test_auto_resource_limits.py\` (gating) — transitive via \`hotplug_spec_vm_and_verify_hotplug\` → \`verify_hotplug\` → \`wait_for_migration_finished\`
- \`-m migration\` — covers remaining callers via \`migrate_and_verify_multi_vms\` in \`tests/virt/utils.py\`" \
--jq '{id: .id, url: .html_url}' 2>&1Length of output: 2806 The Test Execution Plan has been posted as an inline review comment on line 361 of |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4954. Overlapping filestests/scale/test_scale_benchmark.py |
|
/retest tox |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5030. Overlapping filestests/virt/node/descheduler/conftest.py |
|
/approve |
|
/check-can-merge |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
9 participants
What this PR does / why we need it:
clean-up last virt utils from using get_client() in
wait_for_migration_finishedandservice_ipfunction and method.
also refactor
wait_for_migration_finished- take out failed pod logs to separate functionWhich issue(s) this PR fixes:
Aligns last virt utilities 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-73714
Summary by CodeRabbit
Tests
Refactor
Chores