[VIRT] Fix fixture scope race condition in node labeller tests#5235
Conversation
Change labelled_worker_node1 from class to function scope to prevent race where kubevirt reconciles label before skip annotation is applied. Co-Authored-By: Claude Sonnet 4.5 Signed-off-by: Denys Shchedrivyi <dshchedr@redhat.com>
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
📝 WalkthroughWalkthroughThe ChangesNode Labeller Annotation Fixture Scope
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes HIGH severity note: Fixture scope changes carry real behavioral impact. Here's why this matters:
If Verify: Does 🔕 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, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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 |
PR Summary by Qodo[VIRT] Fix node labeller test race by making labelled node fixture function-scoped WalkthroughsDescription• Change labelled node fixture to function scope to avoid cross-test state leakage. • Prevent race where KubeVirt reconciles labels before the skip annotation is applied. • Improve determinism of node labeller annotation tests under parallel/fast execution. Diagramgraph TD
T[Test: node labeller] --> F["pytest fixture: labelled_worker_node1"] --> K[KubeVirt reconcile] --> N[(Worker node labels)]
F --> A[Skip annotation]
subgraph Legend
direction LR
_t["Test"] ~~~ _f["Fixture"] ~~~ _svc["Controller"] ~~~ _db[("Cluster state")]
end
High-Level AssessmentThe following are alternative approaches to this PR: 1. Add explicit synchronization around annotation/label reconciliation
2. Force serial execution / isolate via dedicated node resource
Recommendation: Making the fixture function-scoped is the simplest and most targeted fix to eliminate cross-test shared-state and reduce the reconciliation race. Consider adding a small state-wait helper only if flakes persist due to controller timing even with per-test isolation. File ChangesTests (1)
|
openshift-virtualization-qe-bot
added
the
tox:verify-tc-requirement-polarion:running
|
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. |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
tests/virt/node/node_labeller/test_node_labeller_annotation.py (1)
69-70:⚠️ Potential issue | 🟠 MajorAdd required comment explaining why the dependency exists.
The coding guidelines mandate: "When using
@pytest.mark.dependency, a comment explaining WHY the dependency exists is REQUIRED." Line 69 is missing this comment.Clarify whether the second test truly depends on the first test's state or if it's an independent scenario that happens to verify the opposite outcome. With function-scoped fixtures, each test gets fresh setup (DISABLED label), so the dependency is likely for ordering/workflow clarity rather than state inheritance. Either add the required comment explaining the dependency's purpose, or remove the marker if these are truly independent test scenarios.
🤖 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/node_labeller/test_node_labeller_annotation.py` around lines 69 - 70, The test_node_labeller_removed_skip_node_annotation method has a `@pytest.mark.dependency` decorator that lacks the required comment explaining why the dependency exists. Review whether this test truly depends on the state from test_node_labeller_added_skip_node_annotation or if it's an independent test scenario with opposite behavior. If the dependency is necessary, add a comment above or inline with the decorator clarifying the purpose (e.g., for ordering, workflow verification, or state inheritance). If the tests are truly independent with function-scoped fixtures providing fresh setup, remove the `@pytest.mark.dependency` marker entirely.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/virt/node/node_labeller/test_node_labeller_annotation.py`:
- Around line 69-70: The test_node_labeller_removed_skip_node_annotation method
has a `@pytest.mark.dependency` decorator that lacks the required comment
explaining why the dependency exists. Review whether this test truly depends on
the state from test_node_labeller_added_skip_node_annotation or if it's an
independent test scenario with opposite behavior. If the dependency is
necessary, add a comment above or inline with the decorator clarifying the
purpose (e.g., for ordering, workflow verification, or state inheritance). If
the tests are truly independent with function-scoped fixtures providing fresh
setup, remove the `@pytest.mark.dependency` marker entirely.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ed3fdbda-77c1-4c23-8e85-fed37cf93ab8
📒 Files selected for processing (1)
tests/virt/node/node_labeller/test_node_labeller_annotation.py
|
/build-and-push-container |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-5235 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
# Get the content of the changed file
cat tests/virt/node/node_labeller/test_node_labeller_annotation.py
echo "---SMOKE MARKERS IN THIS FILE---"
rg '`@pytest.mark.smoke`' tests/virt/node/node_labeller/test_node_labeller_annotation.py
echo "---GATING MARKERS IN THIS FILE---"
rg '`@pytest.mark.gating`' tests/virt/node/node_labeller/test_node_labeller_annotation.py
echo "---FIXTURE USAGE IN OTHER FILES---"
rg -rn 'labelled_worker_node1' tests/ --include="*.py"
echo "---SMOKE MARKERS INTERSECT---"
rg -l '`@pytest.mark.smoke`' tests/virt/node/node_labeller/
echo "---GATING MARKERS INTERSECT---"
rg -l '`@pytest.mark.gating`' tests/virt/node/node_labeller/Length of output: 4937 🏁 Script executed: #!/bin/bash
# Get the PR diff to find the commit SHA and line position for inline comment
PR_NUMBER=5235
REPO="RedHatQE/openshift-virtualization-tests"
# Get PR details
gh api repos/$REPO/pulls/$PR_NUMBER --jq '{head_sha: .head.sha, base_sha: .base.sha}'
# Get the diff to find file position
gh api repos/$REPO/pulls/$PR_NUMBER/files --jq '.[] | {filename, patch}'Length of output: 1000 🏁 Script executed: #!/bin/bash
PR_NUMBER=5235
REPO="RedHatQE/openshift-virtualization-tests"
HEAD_SHA="e7b45d22e330b4b527d589e24b3caf1a72ea40c1"
FILE="tests/virt/node/node_labeller/test_node_labeller_annotation.py"
BODY="**Test Execution Plan**
- **Run smoke tests: False** — No smoke-marked tests in \`test_node_labeller_annotation.py\` and no traced path from the fixture change to any smoke-marked test.
- **Run gating tests: False** — The gating-marked files in \`tests/virt/node/node_labeller/cpu_features/\` do not use the \`labelled_worker_node1\` fixture and are unaffected by this scope change.
**Affected tests to run:**
The scope change from \`class\` → \`function\` affects both tests that consume \`labelled_worker_node1\`. Each test now gets a fresh fixture instance (label manipulation + teardown) instead of sharing one across the class:
- \`tests/virt/node/node_labeller/test_node_labeller_annotation.py::TestNodeLabellerSkipAnnotation::test_node_labeller_added_skip_node_annotation\`
- \`tests/virt/node/node_labeller/test_node_labeller_annotation.py::TestNodeLabellerSkipAnnotation::test_node_labeller_removed_skip_node_annotation\`
Note: \`test_node_labeller_removed_skip_node_annotation\` has a \`@pytest.mark.dependency\` on the first test. With function-scoped \`labelled_worker_node1\`, each test independently sets up/tears down the label, which is precisely the fix for the race condition."
gh api repos/$REPO/pulls/$PR_NUMBER/reviews \
--method POST \
--field commit_id="$HEAD_SHA" \
--field body="" \
--field event="COMMENT" \
--field "comments[][path]=$FILE" \
--field "comments[][position]=6" \
--field "comments[][body]=$BODY"Length of output: 3445 |
|
/approved |
|
/approve |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-5235. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
9 participants
Change labelled_worker_node1 from class to function scope to prevent race where kubevirt reconciles label before skip annotation is applied.
Co-Authored-By: Claude Sonnet 4.5
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit
Note: This release contains only internal testing improvements with no user-facing changes.