Move nmstate namespace check to network sanity checks and nodes_active_nics to return node_physical_nics for cloud clusters
#1904
Conversation
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
WalkthroughRefactors NMState namespace handling to be optional rather than required. Removes the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
openshift-virtualization-qe-bot
requested review from
RoniKishner,
geetikakay,
servolkov and
vsibirsk
Collaborator
Author
|
/build-and-push-container |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1904 +/- ##
=======================================
Coverage ? 93.54%
=======================================
Files ? 16
Lines ? 1347
Branches ? 0
=======================================
Hits ? 1260
Misses ? 87
Partials ? 0
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:
|
Contributor
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-1904 published |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
previously approved these changes
Aug 28, 2025
openshift-virtualization-qe-bot
added
the
tox:verify-bugs-are-open:passed
openshift-virtualization-qe-bot
approved these changes
Oct 27, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/network/conftest.py (1)
221-234: Add missingis_baremetal_or_psi_clusterparameter to gate BM/PSI-specific checks.The PR objectives state: "network_sanity updated to check multi-NIC and nmstate installation only on Bare Metal (BM) and PSI clusters." However, the fixture signature is missing the
is_baremetal_or_psi_clusterparameter needed to implement cluster-type conditional logic.Apply this diff to add the missing parameter:
@pytest.fixture(scope="session", autouse=True) def network_sanity( admin_client, hosts_common_available_ports, junitxml_plugin, request, istio_system_namespace, cluster_network_mtu, network_overhead, sriov_workers, ipv4_supported_cluster, ipv6_supported_cluster, conformance_tests, nmstate_namespace, + is_baremetal_or_psi_cluster, ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/network/conftest.py(6 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: rnetser
PR: RedHatQE/openshift-virtualization-tests#0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: For PR #1904 test execution, the critical validation point is test_connectivity_over_migration_between_localnet_vms which should fail gracefully on cloud clusters but pass on bare-metal/PSI clusters, representing the core nmstate conditional logic functionality.
Learnt from: rnetser
PR: RedHatQE/openshift-virtualization-tests#0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: The test execution plan for PR #1904 focuses on cluster-type conditional logic where nmstate functionality is bypassed on cloud clusters (Azure/AWS) but fully functional on bare-metal/PSI clusters, requiring different test strategies for each environment type.
Learnt from: dshchedr
PR: RedHatQE/openshift-virtualization-tests#1840
File: tests/virt/node/workload_density/test_swap.py:88-89
Timestamp: 2025-08-20T23:57:48.380Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, CNV installation and health are verified by sanity tests before other tests run, so hco_namespace is guaranteed to exist in the testing environment. Defensive programming against nil hco_namespace scenarios is not needed in fixtures.
📚 Learning: 2025-09-02T11:18:02.529Z
Learnt from: rnetser
PR: RedHatQE/openshift-virtualization-tests#1904
File: tests/network/conftest.py:319-325
Timestamp: 2025-09-02T11:18:02.529Z
Learning: In tests/network/conftest.py, for the _verify_nmstate_running_pods function, the user rnetser prefers a simple approach of just logging the result from wait_for_pods_running rather than implementing complex logic to distinguish between None and empty list returns.
Applied to files:
tests/network/conftest.py
📚 Learning: 2025-09-02T11:16:59.950Z
Learnt from: rnetser
PR: RedHatQE/openshift-virtualization-tests#1904
File: tests/conftest.py:589-593
Timestamp: 2025-09-02T11:16:59.950Z
Learning: In tests/conftest.py, for non-baremetal/PSI clusters in the nodes_active_nics fixture, the user prefers to populate the "occupied" field with actual physical NICs from node_physical_nics rather than leaving it empty, to provide downstream consumers with visibility into physical NIC inventory even when NMState isn't managing them.
Applied to files:
tests/network/conftest.py
📚 Learning: 2025-09-29T19:05:24.987Z
Learnt from: rnetser
PR: RedHatQE/openshift-virtualization-tests#0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: For PR #1904 test execution, the critical validation point is test_connectivity_over_migration_between_localnet_vms which should fail gracefully on cloud clusters but pass on bare-metal/PSI clusters, representing the core nmstate conditional logic functionality.
Applied to files:
tests/network/conftest.py
📚 Learning: 2025-09-29T19:05:24.987Z
Learnt from: rnetser
PR: RedHatQE/openshift-virtualization-tests#0
File: :0-0
Timestamp: 2025-09-29T19:05:24.987Z
Learning: The test execution plan for PR #1904 focuses on cluster-type conditional logic where nmstate functionality is bypassed on cloud clusters (Azure/AWS) but fully functional on bare-metal/PSI clusters, requiring different test strategies for each environment type.
Applied to files:
tests/network/conftest.py
🧬 Code graph analysis (1)
tests/network/conftest.py (3)
utilities/constants.py (1)
NamespacesNames(689-703)utilities/infra.py (1)
wait_for_pods_running(320-365)tests/conftest.py (3)
admin_client(307-311)nmstate_namespace(2864-2870)namespace(665-678)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
- GitHub Check: can-be-merged
🔇 Additional comments (3)
tests/network/conftest.py (3)
17-17: LGTM: Required imports added correctly.The new imports (
TimeoutExpiredError,NamespacesNames,wait_for_pods_running) are all used in the_verify_nmstate_running_podsfunction and are correctly sourced.Also applies to: 29-29, 36-36
363-363: Verify: Should multi-NIC check also be gated by cluster type?The PR objectives state: "network_sanity updated to check multi-NIC and nmstate installation only on Bare Metal (BM) and PSI clusters." However,
_verify_multi_nicis called unconditionally on line 363, while nmstate verification should be cluster-type gated.The function does have internal marker-based logic (lines 244-256) to skip when
single_nicmarker is used or for conformance tests, which might make cluster-type gating unnecessary. However, this creates a design inconsistency:
- Multi-NIC: marker-based gating (internal to function)
- NMState: cluster-type gating (external, should be added)
Please clarify: Should
_verify_multi_nicalso be moved inside theis_baremetal_or_psi_clusterconditional block for consistency with the PR objectives, or is the current marker-based approach intentional?
243-243: LGTM: Parameter renamed for clarity.Renaming the parameter from
requestto_requesthelps distinguish it from the outernetwork_sanityfixture'srequestparameter, improving code clarity.
Collaborator
Author
|
/build-and-push-container |
Contributor
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-1904 published |
Collaborator
Author
|
/verified |
Contributor
Test execution plan request detailsAs an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
|
rnetser
dismissed stale reviews from coderabbitai[bot] and myakove
test comment
There was a problem hiding this comment.
Test Execution Plan
Directly affected tests:
tests/after_cluster_deploy_sanity/test_after_cluster_deploy_sanity.py::test_pod_sanitytests/network/nmstate/test_connectivity_after_nmstate_changes.pytests/network/localnet/test_default_bridge.pytests/network/localnet/test_ovs_bridge.py
Cluster-specific markers:
-m "gating and single_nic"- Cloud clusters (Azure/AWS)-m "gating"- Bare-metal/PSI clusters
Collaborator
|
/approve |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1074 🏁 Script executed: Length of output: 825 @openshift-virtualization-qe-bot-3 Acknowledged. I’ll post a CHANGES_REQUESTED inline comment on the first modified file with the minimal Test Execution Plan now. I leveraged our stored learnings about nmstate gating and the critical localnet migration test to keep coverage tight. 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 609 🏁 Script executed: Length of output: 2248 @openshift-virtualization-qe-bot-3 I’ve attempted to post the required CHANGES_REQUESTED inline comment at the first line (diff position=1) of the first modified file with the minimal Test Execution Plan. If inline placement was not possible due to API constraints, I retried with the first changed line and finally as a general change request to ensure the plan is attached to the PR. 🏁 Script executed: Length of output: 153 🏁 Script executed: Length of output: 177 🏁 Script executed: Length of output: 252 @openshift-virtualization-qe-bot-3 Here’s the exact content that should be posted as a change request comment (inline on the first file). It follows your “title + list only” requirement: Test Execution Plan
If you want to post it inline on the first diff line of conftest.py (diff position 1) with CHANGES_REQUESTED, run this from a shell with a token that has PR review permissions: 🏁 Script executed: Length of output: 1755 @openshift-virtualization-qe-bot-3 The inline comment contains exactly:
I leveraged our stored learnings for PR #1904 (cluster-type gating and the localnet migration test as the critical nmstate signal) to keep this list focused and minimal. 🧠 Learnings used |
|
Successfully removed PR tag: quay.io/openshift-cnv/openshift-virtualization-tests:pr-1904. |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 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:
In the current implementation, an installed knmstate operator is a requirement.
The knmstate operator is not required:
The following changes have been made to drop the dependency on the operator when not needed:
cluster_health_check,test_pod_sanitywill only verify HCO podsnetwork_sanitywas updated to check knmstate running pods it the namespace exists.More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit