cno: Check CNO network CRD status#4770
Open
qinqon wants to merge 3 commits intoRedHatQE:mainfrom
Open
Conversation
Contributor
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR shifts network operator readiness checking from polling a MNP CustomResourceDefinition to monitoring ClusterOperator/network condition states. The fixture now takes explicit reference timestamps before patch operations to verify that operator reconciliation occurred after configuration changes, removing admin_client dependency. ChangesNetwork Operator Stability Waiting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Contributor
|
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-2
requested review from
Anatw,
EdDev,
azhivovk,
orelmisan and
yossisegev
![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: 1
🤖 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 `@tests/network/flat_overlay/utils.py`:
- Around line 71-79: Add a Google-style "Args" and "Raises" section to the
docstring of wait_for_network_operator_stable_conditions describing the
parameters (reference_time: timezone-aware datetime required), what the function
does (blocks until ClusterOperator/network has reconciled and is stable, up to
TIMEOUT_10MIN), and its return/side effects; in "Raises" document that it raises
TimeoutExpiredError on timeout and any other exceptions it may propagate. Ensure
the Args entry notes the timezone-aware requirement for reference_time and that
the function may block for up to TIMEOUT_10MIN so callers can handle waiting
appropriately.
🪄 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: fcf90a57-a8a0-4cde-91cc-7e3270221077
📒 Files selected for processing (2)
tests/network/flat_overlay/conftest.pytests/network/flat_overlay/utils.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
qinqon
force-pushed
the
wait-for-cno-crd-status
branch
from
8117d28 to
c02e853
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/network/flat_overlay/utils.py (1)
71-79: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winGoogle-format
ArgsandRaisessections are still missing from the docstring.🤖 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/network/flat_overlay/utils.py` around lines 71 - 79, The docstring shown in tests/network/flat_overlay/utils.py is missing Google-style "Args" and "Raises" sections; update this docstring (the function that waits for ClusterOperator/network reconciliation) to include a Google-format Args section listing parameters (e.g., reference_time, timeout, poll_interval or whatever parameters the function signature uses) with types and brief descriptions, and a Raises section that documents exceptions the function can raise (e.g., TimeoutError or other runtime exceptions), ensuring formatting follows Google docstring conventions.
🤖 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.
Duplicate comments:
In `@tests/network/flat_overlay/utils.py`:
- Around line 71-79: The docstring shown in tests/network/flat_overlay/utils.py
is missing Google-style "Args" and "Raises" sections; update this docstring (the
function that waits for ClusterOperator/network reconciliation) to include a
Google-format Args section listing parameters (e.g., reference_time, timeout,
poll_interval or whatever parameters the function signature uses) with types and
brief descriptions, and a Raises section that documents exceptions the function
can raise (e.g., TimeoutError or other runtime exceptions), ensuring formatting
follows Google docstring conventions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b1ba7daf-8315-4bc0-813b-704ae4a244c2
📒 Files selected for processing (3)
tests/network/flat_overlay/conftest.pytests/network/flat_overlay/test_multi_network_policy.pytests/network/flat_overlay/utils.py
💤 Files with no reviewable changes (1)
- tests/network/flat_overlay/test_multi_network_policy.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
qinqon
added a commit
to qinqon/openshift-virtualization-tests
that referenced
this pull request
May 8, 2026
Add Google-format Args and Raises sections to the docstring to address CodeRabbit review feedback on PR RedHatQE#4770. Documents the timezone-aware requirement for reference_time and the TIMEOUT_10MIN blocking behavior. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/network/flat_overlay/utils.py`:
- Line 95: Update the function signature for _is_reconciled_and_healthy to
include an explicit boolean return type (add -> bool) so the definition reads
with a return annotation; locate def _is_reconciled_and_healthy() in
tests/network/flat_overlay/utils.py and change it to def
_is_reconciled_and_healthy() -> bool:, leaving the implementation unchanged.
🪄 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: f153dfa2-8438-46e7-872a-680a06be49f0
📒 Files selected for processing (1)
tests/network/flat_overlay/utils.py
| """ | ||
| network_co = ClusterOperator(name="network") | ||
|
|
||
| def _is_reconciled_and_healthy(): |
Contributor
There was a problem hiding this comment.
Add -> bool return type annotation to _is_reconciled_and_healthy.
Ruff ANN202 flags the missing return type. Per the project guideline — fix the code, never suppress the linter.
🔧 Proposed fix
- def _is_reconciled_and_healthy():
+ def _is_reconciled_and_healthy() -> bool:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _is_reconciled_and_healthy(): | |
| def _is_reconciled_and_healthy() -> bool: |
🧰 Tools
🪛 Ruff (0.15.12)
[warning] 95-95: Missing return type annotation for private function _is_reconciled_and_healthy
(ANN202)
🤖 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/network/flat_overlay/utils.py` at line 95, Update the function
signature for _is_reconciled_and_healthy to include an explicit boolean return
type (add -> bool) so the definition reads with a return annotation; locate def
_is_reconciled_and_healthy() in tests/network/flat_overlay/utils.py and change
it to def _is_reconciled_and_healthy() -> bool:, leaving the implementation
unchanged.
When multi network policy is activated the tests are just checking that CRD exist, this is not enough since ovn-kubernetes pods get restarted, the proper way is to check cluster network operator network CRD status. Signed-off-by: Enrique Llorente <ellorent@redhat.com>
Add Google-format Args and Raises sections to the docstring to address CodeRabbit review feedback on PR RedHatQE#4770. Documents the timezone-aware requirement for reference_time and the TIMEOUT_10MIN blocking behavior. Assisted-By: Claude <noreply@anthropic.com> Signed-off-by: Enrique Llorente <ellorent@redhat.com>
qinqon
force-pushed
the
wait-for-cno-crd-status
branch
from
e918b1d to
428e47e
Compare
Contributor
|
Clean rebase detected — no code changes compared to previous head ( |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
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
6 participants
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.
Short description:
Replace the multi-network-policy CRD existence check with a Cluster Network Operator (CNO) status check when toggling
useMultiNetworkPolicy.More details:
The previous wait only verified the
multi-networkpolicies.k8s.cni.cncf.ioCRD existed/disappeared. That signal is insufficient: enabling/disablinguseMultiNetworkPolicytriggers a CNO reconcile that restarts ovn-kubernetes pods, and the CRD can flip ahead of the operator finishing its rollout. Tests proceeded while the cluster network was still converging, leading to flakes.What this PR does / why we need it:
When multi network policy is activated the tests were just checking that the CRD exists. This is not enough since ovn-kubernetes pods get restarted; the proper way is to wait for the cluster network operator to finish reconciling via its status conditions.
Which issue(s) this PR fixes:
NONE
Special notes for reviewer:
NONE
jira-ticket:
https://redhat.atlassian.net/browse/CNV-83350
Summary by CodeRabbit