Refactor node drain function, fix compact cluster drain race#5028
Conversation
|
Warning Review limit reached
More reviews will be available in 47 minutes and 32 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR refactors node management orchestration by removing the polymorphic ChangesNode Management Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
|
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
requested review from
SamAlber,
geetikakay,
sarahbx and
vsibirsk
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5028 +/- ##
==========================================
- 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:
|
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (2)
utilities/virt.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: PR template required sections are not verifiable from provided PR description format.
I can’t find the required template headers/content in the supplied PR description context (especially meaningful content under
What this PR does / why we need it:). Please restore/confirm the full template sections.As per coding guidelines: "
**: Required sections from.github/pull_request_template.mdmust be present; missing/emptyWhat this PR does / why we need it:is HIGH severity."🤖 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` at line 1, The PR description is missing required template sections (particularly the "What this PR does / why we need it:" header) mandated by .github/pull_request_template.md; update the PR description to restore all required headers and content exactly as in the template (including any sections prefixed with "**" and the "What this PR does / why we need it:" block) so the required fields are verifiable, and ensure the PR body contains meaningful content under those headers before marking the PR ready.tests/virt/node/migration_and_maintenance/test_post_copy_migration.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Restore required PR template sections in the PR description.
The provided PR description content is missing the required template headings, and
What this PR does / why we need itmust be present with meaningful text.Please restore:
##### What this PR does / why we need it:##### Which issue(s) this PR fixes:##### Special notes for reviewer:##### jira-ticket:As per coding guidelines:
What this PR does / why we need itmust be present with meaningful content, and all listed required PR template sections must be present.🤖 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` at line 1, The PR description is missing required template headings; restore the following sections in the PR body with meaningful content: "##### What this PR does / why we need it:" (describe the change and rationale), "##### Which issue(s) this PR fixes:" (link issue IDs), "##### Special notes for reviewer:" (any testing/impact notes), and "##### jira-ticket:" (provide the Jira ID); ensure "What this PR does / why we need it" contains a clear, non-empty explanation before marking the PR ready.
🤖 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/virt.py`:
- Around line 2166-2168: The pkill call in the terminate path is too broad and
may kill other "oc adm drain" processes; instead, record and target the specific
drain process you started (e.g., capture the subprocess.Popen object or its PID
when launching the drain) and on termination send the signal only to that PID
(or use the stored Popen.terminate()/kill()). Update the code around the
run(shlex.split('pkill -f "oc adm drain"')) call to use the tracked process/PID
from the drain-start code (reference the subprocess launch where the drain is
started and the termination logic that currently calls run(...)) so only the
intended drain for node.name is stopped. Ensure fallback handling if the PID no
longer exists.
- Around line 2119-2124: The cordon_node and drain_node context managers
currently call subprocess.run(..., shell=True) (e.g., run(f"oc adm cordon
{node.name}", shell=True)) which bypasses the repo command wrapper and may
ignore failures; replace those calls with
pyhelper_utils.shell.run_command([...]) using an explicit argument list and
check=True so failures raise; for drain_node, instead of a global pkill -f "oc
adm drain" in the finally block, capture the specific drain process you start
(e.g., via subprocess.Popen or run_command that returns a handle) and
terminate/kill only that PID on cleanup, ensuring you still log with LOGGER and
handle errors from run_command accordingly.
---
Outside diff comments:
In `@tests/virt/node/migration_and_maintenance/test_post_copy_migration.py`:
- Line 1: The PR description is missing required template headings; restore the
following sections in the PR body with meaningful content: "##### What this PR
does / why we need it:" (describe the change and rationale), "##### Which
issue(s) this PR fixes:" (link issue IDs), "##### Special notes for reviewer:"
(any testing/impact notes), and "##### jira-ticket:" (provide the Jira ID);
ensure "What this PR does / why we need it" contains a clear, non-empty
explanation before marking the PR ready.
In `@utilities/virt.py`:
- Line 1: The PR description is missing required template sections (particularly
the "What this PR does / why we need it:" header) mandated by
.github/pull_request_template.md; update the PR description to restore all
required headers and content exactly as in the template (including any sections
prefixed with "**" and the "What this PR does / why we need it:" block) so the
required fields are verifiable, and ensure the PR body contains meaningful
content under those headers before marking the PR ready.
🪄 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: 4b2225fe-031b-4981-b895-73a9176fcf84
📒 Files selected for processing (6)
tests/chaos/oadp/conftest.pytests/virt/cluster/migration_and_maintenance/test_evictionstrategy.pytests/virt/node/migration_and_maintenance/test_node_maintenance.pytests/virt/node/migration_and_maintenance/test_post_copy_migration.pytests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.pyutilities/virt.py
dshchedr
force-pushed
the
fix-compact-cluster-virt-api-race
branch
from
aac1211 to
d24aab4
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
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)
1628-1637:⚠️ Potential issue | 🟠 MajorHIGH: Fix
NODE_PORTip_familyparsing to use NodeAddress fields (type/address), not raw entries.
ServiceForVirtualMachineForTests.service_ip()iteratesvm_node.instance.status.addresses, but Node status addresses areNodeAddressobjects (withtype+address). Passing each entry directly toipaddress.ip_address(...)will fail/mis-filter, andreturn internal_ips[0]may not be an IP string. Filtertype == "InternalIP"and passnode_address.addressintoipaddress.ip_address(...).Suggested fix
if self.service_type == Service.Type.NODE_PORT: vm_node = self.vm.vmi.get_node(privileged_client=admin_client) if ip_family: internal_ips = [ - internal_ip - for internal_ip in vm_node.instance.status.addresses - if str(ipaddress.ip_address(internal_ip).version) in ip_family + node_address.address + for node_address in vm_node.instance.status.addresses + if node_address.type == "InternalIP" + and str(ipaddress.ip_address(node_address.address).version) in ip_family ] assert internal_ips, f"No {ip_family} addresses in node {vm_node.name}" return internal_ips[0]🤖 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 1628 - 1637, In the NODE_PORT branch of ServiceForVirtualMachineForTests.service_ip() (the block that checks self.service_type == Service.Type.NODE_PORT and reads vm_node.instance.status.addresses), you must treat each entry as a NodeAddress object and not pass the raw entry to ipaddress.ip_address; instead filter addresses where node_address.type == "InternalIP" (or the requested type), call ipaddress.ip_address(node_address.address) to check the IP family, and return node_address.address (a string) rather than the raw object; update the comprehension/loop to use node_address.address for parsing and ensure an assertion/error if no matching addresses are found.
🤖 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/virt.py`:
- Around line 2182-2195: The current compact_cluster branch only deletes the
first matching virt-api pod (inside the loop over utilities.infra.get_pods) then
breaks, which leaves a second co-located replica to be evicted during drain;
instead, cordon the node once (use cordon_node(admin_client=admin_client,
node=node) outside or around the loop) and iterate over all pods from
utilities.infra.get_pods(filtering by Pod.ApiGroup.KUBEVIRT_IO and VIRT_API and
pod.node.name == node.name), calling pod.delete(wait=True) for each and logging
via LOGGER.info for each deletion; remove the break so all matching pods on that
node are deleted while cordoned before proceeding to drain.
- Around line 2148-2154: The _uncordon_and_stabilize function is currently
decorated with `@contextmanager` but lacks a yield so its uncordon/stabilize steps
never run when called directly from cordon_node()/drain_node(); fix this by
removing the `@contextmanager` decorator and keeping _uncordon_and_stabilize as a
normal function (so run_command(...), wait_for_node_schedulable_status(...), and
wait_for_kv_stabilize(...) execute when cordon_node()/drain_node() call it), or
alternatively implement a proper context manager by adding a yield and changing
callers to use "with _uncordon_and_stabilize(...):"—prefer removing the
decorator for the minimal change.
---
Outside diff comments:
In `@utilities/virt.py`:
- Around line 1628-1637: In the NODE_PORT branch of
ServiceForVirtualMachineForTests.service_ip() (the block that checks
self.service_type == Service.Type.NODE_PORT and reads
vm_node.instance.status.addresses), you must treat each entry as a NodeAddress
object and not pass the raw entry to ipaddress.ip_address; instead filter
addresses where node_address.type == "InternalIP" (or the requested type), call
ipaddress.ip_address(node_address.address) to check the IP family, and return
node_address.address (a string) rather than the raw object; update the
comprehension/loop to use node_address.address for parsing and ensure an
assertion/error if no matching addresses are found.
🪄 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: 91786c68-af80-4c0c-9a7d-2550cf7184f6
📒 Files selected for processing (6)
tests/chaos/oadp/conftest.pytests/virt/cluster/migration_and_maintenance/test_evictionstrategy.pytests/virt/node/migration_and_maintenance/test_node_maintenance.pytests/virt/node/migration_and_maintenance/test_post_copy_migration.pytests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.pyutilities/virt.py
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5028 published |
|
/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
# 6a: Find modified symbols in utilities/virt.py
echo "=== Modified symbols in utilities/virt.py ==="
git diff HEAD~1 --unified=0 -- utilities/virt.py | grep '^[+-]def '
# 6b: Find callers of node_mgmt_console, cordon_node, drain_node in tests/ and conftest files
echo ""
echo "=== Callers of node_mgmt_console ==="
rg -rn 'node_mgmt_console' tests/ --include='*.py' -l
echo ""
echo "=== Callers of cordon_node ==="
rg -rn 'cordon_node' tests/ --include='*.py' -l
echo ""
echo "=== Callers of drain_node ==="
rg -rn 'drain_node' tests/ --include='*.py' -l
# 6d: Same-file callers in utilities/virt.py
echo ""
echo "=== Same-file callers in utilities/virt.py ==="
rg -n 'node_mgmt_console|cordon_node|drain_node|_uncordon_and_stabilize' utilities/virt.py
# 7: Smoke test files
echo ""
echo "=== Smoke marked test files ==="
rg -l '`@pytest.mark.smoke`' tests/
# 8: Gating test files
echo ""
echo "=== Gating marked test files ==="
rg -l '`@pytest.mark.gating`' tests/Length of output: 6581 🏁 Script executed: #!/bin/bash
# Get the list of files changed and their first diff lines for PR 5028
gh pr diff 5028 --repo RedHatQE/openshift-virtualization-tests | head -100
# Also get the PR commits for the review API
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5028 --jq '.head.sha'Length of output: 5034 🏁 Script executed: #!/bin/bash
# Post inline review comment on PR 5028 on the first changed line of tests/chaos/oadp/conftest.py
COMMIT_SHA="f86011644595739725dea2c33a8e387241df10fc"
PR_NUMBER=5028
REPO="RedHatQE/openshift-virtualization-tests"
BODY='**Test Execution Plan**
**Run smoke tests: False**
No smoke-marked test file has a traced dependency path through `drain_node`, `cordon_node`, or `_uncordon_and_stabilize`.
**Run gating tests: True**
- `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` → `cordon_node` (gating): test file directly calls `cordon_node` from `utilities/virt.py`
- `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` → fixture `drained_node` → `drain_node` (gating): fixture uses `drain_node` from `utilities/virt.py`
**Affected tests to run**
| Test path | Reason |
|---|---|
| `tests/chaos/oadp/conftest.py` + callers | fixture `drain_vm_source_node` now uses `drain_node` |
| `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` | fixture `drained_node` → `drain_node` (gating) |
| `tests/virt/node/migration_and_maintenance/test_node_maintenance.py` | `drain_using_console` / `drain_using_console_windows` → `drain_node`; cordon steps → `cordon_node` |
| `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py` | `drained_node_with_hotplugged_vm` → `drain_node` |
| `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` | directly calls `cordon_node` (gating) |
**Real tests (cluster required)**
Error/fix path — compact cluster virt-api race (the scenario this PR fixes):Length of output: 1901 🏁 Script executed: #!/bin/bash
COMMIT_SHA="f86011644595739725dea2c33a8e387241df10fc"
PR_NUMBER=5028
REPO="RedHatQE/openshift-virtualization-tests"
# Write body to file to avoid quoting issues
cat > /tmp/review_body.md << 'ENDOFBODY'
**Test Execution Plan**
**Run smoke tests: False**
No smoke-marked test file has a traced dependency path through `drain_node`, `cordon_node`, or `_uncordon_and_stabilize`.
**Run gating tests: True**
- `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` → `cordon_node` (gating): test file directly calls `cordon_node` from `utilities/virt.py`
- `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` → fixture `drained_node` → `drain_node` (gating): fixture uses `drain_node` from `utilities/virt.py`
**Affected tests to run**
| Test path | Reason |
|---|---|
| `tests/chaos/oadp/conftest.py` + callers | fixture `drain_vm_source_node` now uses `drain_node` |
| `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` | fixture `drained_node` → `drain_node` (gating) |
| `tests/virt/node/migration_and_maintenance/test_node_maintenance.py` | `drain_using_console` / `drain_using_console_windows` → `drain_node`; cordon steps → `cordon_node` |
| `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py` | `drained_node_with_hotplugged_vm` → `drain_node` |
| `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` | directly calls `cordon_node` (gating) |
**Real tests (cluster required)**
Error/fix path — compact cluster virt-api race (the scenario this PR fixes):Length of output: 1914 🏁 Script executed: #!/bin/bash
COMMIT_SHA="f86011644595739725dea2c33a8e387241df10fc"
PR_NUMBER=5028
REPO="RedHatQE/openshift-virtualization-tests"
python3 << 'PYEOF'
import json, subprocess
comment_body = (
"**Test Execution Plan**\n\n"
"**Run smoke tests: False**\n"
"No smoke-marked test file has a traced dependency path through `drain_node`, `cordon_node`, or `_uncordon_and_stabilize`.\n\n"
"**Run gating tests: True**\n"
"- `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` \u2192 `cordon_node` (gating): test file directly calls `cordon_node` from `utilities/virt.py`\n"
"- `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` \u2192 fixture `drained_node` \u2192 `drain_node` (gating): fixture uses `drain_node` from `utilities/virt.py`\n\n"
"**Affected tests to run**\n\n"
"| Test path | Reason |\n"
"|---|---|\n"
"| `tests/chaos/oadp/conftest.py` + callers | fixture `drain_vm_source_node` now uses `drain_node` |\n"
"| `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` | fixture `drained_node` -> `drain_node` (gating) |\n"
"| `tests/virt/node/migration_and_maintenance/test_node_maintenance.py` | `drain_using_console` / `drain_using_console_windows` -> `drain_node`; cordon steps -> `cordon_node` |\n"
"| `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py` | `drained_node_with_hotplugged_vm` -> `drain_node` |\n"
"| `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` | directly calls `cordon_node` (gating) |\n\n"
"**Real tests (cluster required)**\n\n"
"Error/fix path \u2014 compact cluster virt-api race (the scenario this PR fixes):\n\n"
"```\npytest tests/virt/node/migration_and_maintenance/test_node_maintenance.py --tc-config compact_cluster=true -k drain\n```\n\n"
"Expected: no webhook timeout from concurrent virt-api/VM eviction; drain completes cleanly.\n\n"
"Happy path (regression \u2014 standard cluster drain still works):\n\n"
"```\npytest tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py tests/virt/node/migration_and_maintenance/test_node_maintenance.py tests/virt/node/migration_and_maintenance/test_post_copy_migration.py tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py\n```\n\n"
"Expected: all node-drain and cordon tests pass on non-compact cluster without regression."
)
payload = {
"commit_id": "f86011644595739725dea2c33a8e387241df10fc",
"body": "",
"event": "COMMENT",
"comments": [
{
"path": "tests/chaos/oadp/conftest.py",
"line": 22,
"side": "RIGHT",
"body": comment_body
}
]
}
with open("/tmp/review_payload.json", "w") as f:
json.dump(payload, f)
result = subprocess.run(
["gh", "api", "--method", "POST",
"-H", "Accept: application/vnd.github+json",
"/repos/RedHatQE/openshift-virtualization-tests/pulls/5028/reviews",
"--input", "/tmp/review_payload.json",
"--jq", ".id, .html_url"],
capture_output=True, text=True
)
print("STDOUT:", result.stdout)
print("STDERR:", result.stderr)
print("RC:", result.returncode)
PYEOFLength of output: 3380 |
|
/lgtm |
Split node_mgmt_console() into cordon_node() and drain_node(). On compact clusters, draining a node with a virt-api pod can cause webhook timeouts during VM eviction (both virt-api and VM evicted simultaneously). Add logic to drain_node() to detect and relocate virt-api pods before draining: cordon the node, delete the pod (forces rescheduling while the other replica continues serving), then proceed with drain. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Denys Shchedrivyi <dshchedr@redhat.com>
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5028 published |
|
/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
set -euo pipefail
# Get current PR head SHA and diff info
echo "=== Current head SHA ==="
gh api repos/RedHatQE/openshift-virtualization-tests/pulls/5028 --jq '.head.sha'
echo ""
echo "=== First changed file and lines ==="
gh pr diff 5028 --repo RedHatQE/openshift-virtualization-tests | head -50
echo ""
echo "=== Modified symbols in utilities/virt.py ==="
git diff HEAD~1 --unified=0 -- utilities/virt.py | grep '^[+-]def ' || true
echo ""
echo "=== Callers of cordon_node in tests ==="
rg -rn 'cordon_node' tests/ --include='*.py' -l
echo ""
echo "=== Callers of drain_node in tests ==="
rg -rn 'drain_node' tests/ --include='*.py' -l
echo ""
echo "=== Gating marked test files ==="
rg -l '`@pytest.mark.gating`' tests/
echo ""
echo "=== Smoke marked test files ==="
rg -l '`@pytest.mark.smoke`' tests/
echo ""
echo "=== Check if affected files have gating marks ==="
rg '`@pytest.mark.gating`' tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py tests/virt/node/migration_and_maintenance/test_node_maintenance.py tests/virt/node/migration_and_maintenance/test_post_copy_migration.py tests/chaos/oadp/conftest.py || true
echo ""
echo "=== Check if affected files have smoke marks ==="
rg '`@pytest.mark.smoke`' tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py tests/virt/node/migration_and_maintenance/test_node_maintenance.py tests/virt/node/migration_and_maintenance/test_post_copy_migration.py tests/chaos/oadp/conftest.py || trueLength of output: 4348 🏁 Script executed: #!/bin/bash
# Get gating and smoke info, then post inline review
echo "=== Gating marks in affected files ==="
rg -n '`@pytest.mark.gating`' \
tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py \
tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py \
tests/virt/node/migration_and_maintenance/test_node_maintenance.py \
tests/virt/node/migration_and_maintenance/test_post_copy_migration.py 2>/dev/null || echo "(none found)"
echo ""
echo "=== Smoke marks in affected files ==="
rg -n '`@pytest.mark.smoke`' \
tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py \
tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py \
tests/virt/node/migration_and_maintenance/test_node_maintenance.py \
tests/virt/node/migration_and_maintenance/test_post_copy_migration.py 2>/dev/null || echo "(none found)"
echo ""
echo "=== All callers of drain_node ==="
rg -rn 'drain_node' tests/ --include='*.py' -l 2>/dev/null || echo "(none)"
echo ""
echo "=== All callers of cordon_node ==="
rg -rn 'cordon_node' tests/ --include='*.py' -l 2>/dev/null || echo "(none)"
echo ""
echo "=== Smoke test files in repo ==="
rg -l '`@pytest.mark.smoke`' tests/ 2>/dev/null | head -20
echo ""
echo "=== Gating test files in repo ==="
rg -l '`@pytest.mark.gating`' tests/ 2>/dev/null | head -20Length of output: 3362 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 << 'PYEOF'
import json
import subprocess
COMMIT_SHA = "3092913480940b2b40212ec576d2690d1a6dc6cf"
comment_body = """\
**Test Execution Plan**
**Run smoke tests: False**
No traced path from any smoke-marked test file → fixture → `drain_node`/`cordon_node`/`_uncordon_and_stabilize` in `utilities/virt.py`. Smoke files (`test_rhel_os_support.py`, `test_container_disk_vm.py`, etc.) have no fixture dependency on the changed symbols.
**Run gating tests: True**
- `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` → fixture `drained_node` → `drain_node` (`utilities/virt.py`) — `@pytest.mark.gating` at line 104
- `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` → directly calls `cordon_node` (`utilities/virt.py`) — `@pytest.mark.gating` at line 31
**Affected tests to run**
| Test path | Marker | Reason |
|---|---|---|
| `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` | gating | fixture `drained_node` → `drain_node` |
| `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` | gating | directly calls `cordon_node` |
| `tests/virt/node/migration_and_maintenance/test_node_maintenance.py` | — | `drain_using_console`/`drain_using_console_windows` → `drain_node`; cordon steps → `cordon_node` |
| `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py` | — | `drained_node_with_hotplugged_vm` → `drain_node` |
| `tests/chaos/oadp/conftest.py` + tests under `tests/chaos/oadp/` | — | fixture `drain_vm_source_node` → `drain_node` |
**Real tests (cluster required)**
Error/fix path — compact cluster virt-api race (the scenario this PR fixes):Length of output: 2174 🏁 Script executed: #!/bin/bash
set -euo pipefail
cat > /tmp/post_review.py << 'EOF'
import json
import subprocess
COMMIT_SHA = "3092913480940b2b40212ec576d2690d1a6dc6cf"
comment_body = (
"**Test Execution Plan**\n\n"
"**Run smoke tests: False**\n"
"No traced path from any smoke-marked test file to fixture to `drain_node`/`cordon_node`/`_uncordon_and_stabilize` "
"in `utilities/virt.py`. Smoke files (`test_rhel_os_support.py`, `test_container_disk_vm.py`, etc.) "
"have no fixture dependency on the changed symbols.\n\n"
"**Run gating tests: True**\n"
"- `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` \u2192 fixture `drained_node` "
"\u2192 `drain_node` (`utilities/virt.py`) \u2014 `@pytest.mark.gating` at line 104\n"
"- `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` \u2192 directly calls "
"`cordon_node` (`utilities/virt.py`) \u2014 `@pytest.mark.gating` at line 31\n\n"
"**Affected tests to run**\n\n"
"| Test path | Marker | Reason |\n"
"|---|---|---|\n"
"| `tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py` | gating | fixture `drained_node` \u2192 `drain_node` |\n"
"| `tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py` | gating | directly calls `cordon_node` |\n"
"| `tests/virt/node/migration_and_maintenance/test_node_maintenance.py` | \u2014 | `drain_using_console`/`drain_using_console_windows` \u2192 `drain_node`; cordon steps \u2192 `cordon_node` |\n"
"| `tests/virt/node/migration_and_maintenance/test_post_copy_migration.py` | \u2014 | `drained_node_with_hotplugged_vm` \u2192 `drain_node` |\n"
"| `tests/chaos/oadp/conftest.py` + tests under `tests/chaos/oadp/` | \u2014 | fixture `drain_vm_source_node` \u2192 `drain_node` |\n\n"
"**Real tests (cluster required)**\n\n"
"Error/fix path \u2014 compact cluster virt-api race (the scenario this PR fixes):\n\n"
"```\npytest tests/virt/node/migration_and_maintenance/test_node_maintenance.py -k drain --tc-config compact_cluster=true\n```\n\n"
"Expected: drain completes without webhook timeout from concurrent virt-api/VM eviction; "
"virt-api pod is relocated before drain begins.\n\n"
"Happy path (regression \u2014 standard cluster drain still works):\n\n"
"```\npytest tests/virt/cluster/migration_and_maintenance/test_evictionstrategy.py "
"tests/virt/node/migration_and_maintenance/test_node_maintenance.py "
"tests/virt/node/migration_and_maintenance/test_post_copy_migration.py "
"tests/virt/node/migration_and_maintenance/test_vm_unscheduled_node.py\n```\n\n"
"Expected: all node-drain and cordon tests pass on non-compact cluster without regression."
)
payload = {
"commit_id": COMMIT_SHA,
"body": "",
"event": "COMMENT",
"comments": [
{
"path": "tests/chaos/oadp/conftest.py",
"line": 22,
"side": "RIGHT",
"body": comment_body
}
]
}
with open("/tmp/review_payload.json", "w") as f:
json.dump(payload, f)
result = subprocess.run(
[
"gh", "api", "--method", "POST",
"-H", "Accept: application/vnd.github+json",
"/repos/RedHatQE/openshift-virtualization-tests/pulls/5028/reviews",
"--input", "/tmp/review_payload.json",
"--jq", ".id, .html_url, .state"
],
capture_output=True, text=True
)
print("STDOUT:", result.stdout)
print("STDERR:", result.stderr)
print("RC:", result.returncode)
EOF
python3 /tmp/post_review.pyLength of output: 3722 |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4873. Overlapping filesutilities/virt.py |
|
/approve |
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #5219. Overlapping filesutilities/virt.py |
|
/retest verify-bugs-are-open |
|
/check-can-merge |
|
/lgtm |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5028. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
10 participants
What this PR does / why we need it:
Split node_mgmt_console() into cordon_node() and drain_node()
Fix the flakyness when executed on compact cluster
On compact clusters, draining a node with a virt-api pod can cause webhook timeouts during VM eviction (both virt-api and VM evicted simultaneously). Add logic to drain_node() to detect and relocate virt-api pods before draining: cordon the node, delete the pod (forces rescheduling while the other replica continues serving), then proceed with drain.
Co-Authored-By: Claude Sonnet 4.5
Summary by CodeRabbit