tests, net: mandatory client for OCP wrapper instantiations #2974
+34
−25
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
WalkthroughMultiple test fixtures and utility functions were updated to accept and forward explicit Kubernetes client fixtures ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Suggested labels
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,
azhivovk,
dshchedr,
hmeir,
rlobillo,
servolkov,
vsibirsk and
yossisegev
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
approved these changes
Dec 2, 2025
Collaborator
|
/lgtm |
EdDev
suggested changes
Dec 3, 2025
Contributor
EdDev
left a comment
EdDev
left a comment
There was a problem hiding this comment.
The changes look fine to me.
Please elaborate about the different clients you used (admin vs unpriv). In the intention is explicit, future readers will understand why a specific client was used.
Updated all OCP wrapper class instantiations across the network segment with the relevant client parameter. The choice between admin_client and unprivileged_client follows a clear principle based on the type of resource being accessed and the test scenario being validated: admin_client is used for cluster-level infrastructure and operator-managed resources that require cluster-admin privileges. unprivileged_client is used for user-created resources and scenarios that simulate real user workflows. Using unprivileged_client follows the principle of using the least privilege user possible. Signed-off-by: Anat Wax <[email protected]>
Contributor
Author
|
Change: commit msg |
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
azhivovk
approved these changes
Dec 7, 2025
openshift-virtualization-qe-bot
added
tox:verify-bugs-are-open:running
openshift-virtualization-qe-bot
added
tox:verify-bugs-are-open:passed
openshift-virtualization-qe-bot
approved these changes
Dec 7, 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: 0
🧹 Nitpick comments (1)
tests/network/kubemacpool/test_kubemacpool.py (1)
131-134: Usekmp_downexplicitly to satisfy Ruff while keeping fixture side effectsPassing
unprivileged_clientintoVirtualMachineForTestshere is aligned with the rest of the suite and keeps this negative test in an unprivileged context. The only issue is Ruff’sARG001about the unusedkmp_downargument, since the fixture is relied on purely for its side effects.You can keep the current behavior and silence the warning by explicitly “using” the fixture value:
@pytest.mark.sno @pytest.mark.polarion("CNV-4405") @pytest.mark.single_nic @pytest.mark.s390x def test_kmp_down(unprivileged_client, namespace, kmp_down): - with pytest.raises(ApiException): - with VirtualMachineForTests(name="kmp-down-vm", namespace=namespace.name, client=unprivileged_client): - return + # kmp_down fixture is used for its side effects (scales KMP down) + _ = kmp_down + + with pytest.raises(ApiException): + with VirtualMachineForTests( + name="kmp-down-vm", + namespace=namespace.name, + client=unprivileged_client, + ): + returnThis keeps the fixture behavior unchanged while making the argument observably used for static analysis.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
tests/conftest.py(5 hunks)tests/network/conftest.py(4 hunks)tests/network/general/test_ip_family_services.py(2 hunks)tests/network/kubemacpool/conftest.py(6 hunks)tests/network/kubemacpool/test_kubemacpool.py(1 hunks)tests/network/upgrade/utils.py(1 hunks)tests/network/utils.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/conftest.py
- tests/network/conftest.py
- tests/network/general/test_ip_family_services.py
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 2469
File: utilities/sanity.py:139-142
Timestamp: 2025-11-08T07:36:57.616Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers to keep refactoring PRs (like PR #2469) strictly focused on moving/organizing code into more granular modules without adding new functionality, error handling, or behavioral changes. Such improvements should be handled in separate PRs.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 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
Repo: RedHatQE/openshift-virtualization-tests PR: 2701
File: .github/scripts/coderabbit_workflow.py:45-46
Timestamp: 2025-11-27T11:45:54.224Z
Learning: In the openshift-virtualization-tests repository, when using the PyGithub library or similar API clients, rnetser prefers to rely on API-level validation rather than adding redundant client-side input validation, since the API will handle validation and raise appropriate errors.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/node/workload_density/test_swap.py:71-85
Timestamp: 2025-08-09T01:46:48.039Z
Learning: In the openshift-virtualization-tests repository, user dshchedr prefers that all test setup fixtures be explicitly declared as parameters in the test method itself rather than chaining fixtures through dependencies. This makes all setup steps visible in one place at the test method level, improving test readability and making dependencies explicit, even if a fixture isn't directly used within another fixture's implementation.
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1077
Timestamp: 2025-06-18T09:21:34.315Z
Learning: In tests/observability/metrics/conftest.py, when creating fixtures that modify shared Windows VM state (like changing nodeSelector), prefer using function scope rather than class scope to ensure ResourceEditor context managers properly restore the VM state after each test, maintaining test isolation while still reusing expensive Windows VM fixtures.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:1-5
Timestamp: 2025-09-07T13:16:32.011Z
Learning: In the openshift-virtualization-tests project utilities/oadp.py, DynamicClient instances are passed as parameters rather than created internally, so kubernetes.config import is not needed for client creation fallback patterns.
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/test_mtc_storage_class_migration.py:165-176
Timestamp: 2025-06-17T07:45:37.776Z
Learning: In the openshift-virtualization-tests repository, user jpeimer prefers explicit fixture parameters over composite fixtures in test methods, even when there are many parameters, as they find this approach more readable and maintainable for understanding test dependencies.
Learnt from: SamAlber
Repo: RedHatQE/openshift-virtualization-tests PR: 2507
File: tests/virt/node/general/test_vmi_reset.py:26-29
Timestamp: 2025-11-19T08:13:30.263Z
Learning: In the openshift-virtualization-tests repository, user SamAlber prefers not to define fixture dependencies by chaining fixtures (adding one fixture as a parameter to another). Instead, all fixture dependencies should be explicitly declared as parameters in the test method itself, relying on parameter order to control execution sequence.
Learnt from: azhivovk
Repo: RedHatQE/openshift-virtualization-tests PR: 2119
File: tests/network/localnet/conftest.py:352-366
Timestamp: 2025-10-31T13:05:24.570Z
Learning: In the RedHatQE/openshift-virtualization-tests repository, tests run sequentially by default on the same cluster (with module-scoped fixtures properly cleaned up between modules), and parallel test runs use different machines/clusters. This means cluster-scoped resources (like ClusterUserDefinedNetwork or NodeNetworkConfigurationPolicy) can safely use the same names across different test modules without risk of collision.
Learnt from: vsibirsk
Repo: RedHatQE/openshift-virtualization-tests PR: 2045
File: tests/virt/cluster/vm_lifecycle/conftest.py:46-47
Timestamp: 2025-09-15T06:49:53.478Z
Learning: In the openshift-virtualization-tests repo, large fixture refactoring efforts like the golden image data source migration are handled incrementally by directory/team ownership. The virt/cluster directory is handled separately from virt/node, tests/infra, tests/storage, etc., with each area managed by relevant teams in follow-up PRs.
Learnt from: akri3i
Repo: RedHatQE/openshift-virtualization-tests PR: 1210
File: tests/virt/cluster/general/mass_machine_type_transition_tests/conftest.py:83-97
Timestamp: 2025-06-23T19:19:31.961Z
Learning: In OpenShift Virtualization mass machine type transition tests, the kubevirt_api_lifecycle_automation_job requires cluster-admin privileges to function properly, as confirmed by the test maintainer akri3i.
📚 Learning: 2025-09-02T11:16:59.950Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 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/upgrade/utils.pytests/network/kubemacpool/conftest.py
📚 Learning: 2025-06-18T09:21:34.315Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1077
Timestamp: 2025-06-18T09:21:34.315Z
Learning: In tests/observability/metrics/conftest.py, when creating fixtures that modify shared Windows VM state (like changing nodeSelector), prefer using function scope rather than class scope to ensure ResourceEditor context managers properly restore the VM state after each test, maintaining test isolation while still reusing expensive Windows VM fixtures.
Applied to files:
tests/network/kubemacpool/conftest.pytests/network/kubemacpool/test_kubemacpool.py
📚 Learning: 2025-06-18T09:19:05.769Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/test_vms_metrics.py:129-137
Timestamp: 2025-06-18T09:19:05.769Z
Learning: For Windows VM testing in tests/observability/metrics/test_vms_metrics.py, it's acceptable to have more fixture parameters than typical pylint recommendations when reusing expensive Windows VM fixtures for performance. Windows VMs take a long time to deploy, so reusing fixtures like windows_vm_for_test and adding labels via windows_vm_with_low_bandwidth_migration_policy is preferred over creating separate fixtures that would require additional VM deployments.
Applied to files:
tests/network/kubemacpool/conftest.py
📚 Learning: 2025-08-06T13:57:51.928Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1249-1254
Timestamp: 2025-08-06T13:57:51.928Z
Learning: User rnetser verified that all calls to get_infrastructure() function use the admin_client parameter, confirming that signature changes requiring this parameter don't cause breaking changes in the openshift-virtualization-tests codebase.
Applied to files:
tests/network/kubemacpool/conftest.pytests/network/utils.pytests/network/kubemacpool/test_kubemacpool.py
📚 Learning: 2025-09-29T20:33:51.007Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1932
File: tests/virt/node/migration_and_maintenance/conftest.py:65-72
Timestamp: 2025-09-29T20:33:51.007Z
Learning: In tests/virt/node/migration_and_maintenance/conftest.py, the added_vm_cpu_limit fixture doesn't require ResourceEditor as a context manager because it's the final test to modify the vm_for_multifd_test VM before teardown, so restoration of CPU limits is unnecessary overhead as confirmed by maintainer dshchedr.
Applied to files:
tests/network/kubemacpool/conftest.py
📚 Learning: 2025-08-06T13:57:34.740Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 1028
File: utilities/infra.py:1257-1258
Timestamp: 2025-08-06T13:57:34.740Z
Learning: In the openshift-virtualization-tests repository, all existing calls to the `get_cluster_platform` function in utilities/infra.py already pass the `admin_client` parameter, so the breaking change requiring this parameter does not actually break any existing code.
Applied to files:
tests/network/kubemacpool/conftest.py
📚 Learning: 2025-08-09T01:46:48.039Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/node/workload_density/test_swap.py:71-85
Timestamp: 2025-08-09T01:46:48.039Z
Learning: In the openshift-virtualization-tests repository, user dshchedr prefers that all test setup fixtures be explicitly declared as parameters in the test method itself rather than chaining fixtures through dependencies. This makes all setup steps visible in one place at the test method level, improving test readability and making dependencies explicit, even if a fixture isn't directly used within another fixture's implementation.
Applied to files:
tests/network/kubemacpool/conftest.py
📚 Learning: 2025-08-04T15:27:14.175Z
Learnt from: OhadRevah
Repo: RedHatQE/openshift-virtualization-tests PR: 1166
File: tests/observability/metrics/conftest.py:1065-1082
Timestamp: 2025-08-04T15:27:14.175Z
Learning: In tests/observability/metrics/conftest.py, the `non_existent_node_windows_vm` fixture is used for tier3 Windows VM testing and must use Windows VMs rather than Linux VMs, even though it adds overhead, because it's specifically testing Windows VM status transition metrics as part of dedicated Windows VM test coverage.
Applied to files:
tests/network/kubemacpool/conftest.py
📚 Learning: 2025-09-07T13:16:32.011Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:1-5
Timestamp: 2025-09-07T13:16:32.011Z
Learning: In the openshift-virtualization-tests project utilities/oadp.py, DynamicClient instances are passed as parameters rather than created internally, so kubernetes.config import is not needed for client creation fallback patterns.
Applied to files:
tests/network/kubemacpool/conftest.py
📚 Learning: 2025-06-23T19:19:31.961Z
Learnt from: akri3i
Repo: RedHatQE/openshift-virtualization-tests PR: 1210
File: tests/virt/cluster/general/mass_machine_type_transition_tests/conftest.py:83-97
Timestamp: 2025-06-23T19:19:31.961Z
Learning: In OpenShift Virtualization mass machine type transition tests, the kubevirt_api_lifecycle_automation_job requires cluster-admin privileges to function properly, as confirmed by the test maintainer akri3i.
Applied to files:
tests/network/kubemacpool/conftest.pytests/network/kubemacpool/test_kubemacpool.py
📚 Learning: 2025-09-08T22:06:55.755Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1987
File: tests/virt/node/descheduler/conftest.py:165-170
Timestamp: 2025-09-08T22:06:55.755Z
Learning: In ocp-resources library, the .get() method expects the namespace object (not the string name) when passed as the namespace parameter, as confirmed in VirtualMachineInstanceMigration.get(dyn_client=admin_client, namespace=namespace) usage in tests/virt/node/descheduler/conftest.py.
Applied to files:
tests/network/utils.py
🧬 Code graph analysis (2)
tests/network/upgrade/utils.py (1)
utilities/infra.py (1)
get_node_selector_name(1145-1146)
tests/network/kubemacpool/conftest.py (1)
tests/conftest.py (1)
unprivileged_client(392-424)
🪛 Ruff (0.14.7)
tests/network/kubemacpool/test_kubemacpool.py
131-131: Unused function argument: kmp_down
(ARG001)
⏰ 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). (17)
- 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: 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: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: build-container
- GitHub Check: tox
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
🔇 Additional comments (4)
tests/network/utils.py (2)
186-186: LGTM! NodeNetworkState now properly receives client parameter.The addition of the
clientparameter toNodeNetworkStateis consistent with the PR's objective to make client mandatory for OCP wrapper instantiations.
326-331: Verify all callers ofget_servicehave been updated.The function signature now requires a
clientparameter, which is a breaking change. All call sites across the network test suite must pass the new parameter to avoid runtime errors.tests/network/upgrade/utils.py (1)
30-32: LGTM! NodeNetworkState now properly receives client parameter.The addition of the
clientparameter toNodeNetworkStateis consistent with the PR's objective to make client mandatory for OCP wrapper instantiations.tests/network/kubemacpool/conftest.py (1)
253-270: Client injection into kubemacpool VM fixtures is consistent and least‑privilege alignedThe updates to
disabled_ns_vm,enabled_ns_vm, andno_label_ns_vmto acceptunprivileged_clientand passclient=unprivileged_clientintoVirtualMachineForTestslook correct and consistent with the existingvm_a/vm_bfixtures. VM lifecycle andmac_poolcleanup behavior are preserved, while reservingadmin_clientfor namespace/operator management and using unprivileged access for per‑namespace VMs, matching the PR’s client‑usage model.Based on learnings, this follows the project pattern of passing client instances into helpers instead of constructing them internally.
Also applies to: 272-289, 291-307
Contributor
Author
|
/verified bm02-ibm All network test were executed: tests/network/upgrade/test_upgrade_network.py tests/install_upgrade_operators/must_gather/test_must_gather.py: tests/infrastructure/sap/test_sap_hana_vm.py - As discussed with the infrastructure team, sap hana scenarios are currently not active, no need to run them as part of the verification.
|
Contributor
Test execution plan request detailsCRITICAL: You MUST respond with a review comment on the Files Changed tab, NOT as a regular PR comment. As 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:
|
|
Tip For best results, initiate chat on the files or code changes. @openshift-virtualization-qe-bot-3, I understand you're requesting a test execution plan to be posted as a review comment on the Files Changed tab. However, I'm unable to directly create review comments on specific lines in the Files Changed tab through my available interface - I can only post regular PR comments. That said, I can provide the test execution plan here, and if needed, it can be manually posted as a review comment: Test Execution PlanBased on the fixture modifications in this PR, the following tests are affected: Network Tests
KubeMacPool Tests
Network Upgrade Tests
IP Family Services Tests
Must-Gather Tests
Alternative Marker-Based Approach
Verification Status: ✅ COMPLETE Per @Anatw's verification comments, all affected test areas have been successfully executed with passing results that match the main branch baseline. 🧠 Learnings used |
Collaborator
|
/approve |
|
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
8 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:
Updated all OCP wrapper class instantiations across the network segment with the relevant client parameter.
What this PR does / why we need it:
As of its next releas, openshift-python-wrapper will enforce passing
clientwhen working with cluster resources.openshift-virtualization-tests must align with this change.
All calls in the code to openshift-python-wrapper resources should be updated to pass
clientarg.The choice between admin_client and unprivileged_client follows a clear principle based on the type of resource being accessed and the test scenario being validated:
admin_client is used for cluster-level infrastructure and operator-managed resources that require cluster-admin privileges.
unprivileged_client is used for user-created resources and scenarios that simulate real user workflows. Using unprivileged_client follows the principle of using the least privilege user possible.
jira-ticket:
https://issues.redhat.com/browse/CNV-72392
Summary by CodeRabbit
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.