Virt: replace get_client() usage in utilities/virt.py (part 1) #3328
Conversation
📝 WalkthroughWalkthroughReplaced global Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
|
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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3328 +/- ##
=======================================
Coverage 98.38% 98.38%
=======================================
Files 25 25
Lines 2166 2167 +1
=======================================
+ Hits 2131 2132 +1
Misses 35 35
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:
|
openshift-virtualization-qe-bot-5
requested review from
RoniKishner and
dshchedr
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: 1
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)
1837-1873: Use migration.client instead of cache_admin_client().The
cache_admin_client()docstring states it should be used "ONLY in places whereclientcannot be passed as an argument." Since themigrationobject (VirtualMachineInstanceMigration) has aclientattribute, it should be used instead for consistency.🔎 Recommended fix
if counter >= TIMEOUT_4MIN / sleep: # Get status/events for PODs in non-running or failed state for pod in utilities.infra.get_pod_by_name_prefix( - client=cache_admin_client(), + client=migration.client, pod_prefix=VIRT_LAUNCHER, namespace=namespace, get_all=True, ):Based on learnings, DynamicClient instances should be passed as parameters rather than created internally.
🤖 Fix all issues with AI Agents
In @utilities/virt.py:
- Around line 1509-1512: Replace the ad-hoc admin client with the VM's existing
client: in the Node construction (Node(...)) use self.vm.client instead of
calling cache_admin_client(); this ensures Node(client= self.vm.client,
name=self.vmi.instance.status.nodeName) so Node operations use the same
DynamicClient that created the VM rather than creating a new admin client.
🧹 Nitpick comments (1)
utilities/virt.py (1)
1424-1446: Consider accepting client as a parameter for better flexibility.While using
cache_admin_client()here is acceptable per its docstring (sincefedora_vm_bodydoesn't currently accept a client parameter), consider refactoring this function to accept an optionalclientparameter. This would provide more flexibility and align with the pattern used elsewhere in the codebase where clients are passed as parameters.🔎 Optional refactor
-def fedora_vm_body(name: str) -> dict[str, Any]: +def fedora_vm_body(name: str, client: DynamicClient | None = None) -> dict[str, Any]: pull_secret = utilities.infra.generate_openshift_pull_secret_file() # Make sure we can find the file even if utilities was installed via pip. yaml_file = os.path.abspath("utilities/manifests/vm-fedora.yaml") with open(yaml_file) as fd: data = fd.read() image = Images.Fedora.FEDORA_CONTAINER_IMAGE + node_client = client or cache_admin_client() image_info = get_oc_image_info( image=image, pull_secret=pull_secret, architecture=utilities.cpu.get_nodes_cpu_architecture( - nodes=list(Node.get(client=cache_admin_client())), + nodes=list(Node.get(client=node_client)), ), )Based on learnings, DynamicClient instances are preferably passed as parameters in this codebase.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
utilities/virt.py
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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: 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.
📚 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:
utilities/virt.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:
utilities/virt.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:
utilities/virt.py
📚 Learning: 2025-09-15T06:49:53.478Z
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.
Applied to files:
utilities/virt.py
📚 Learning: 2025-06-23T19:18:12.275Z
Learnt from: akri3i
Repo: RedHatQE/openshift-virtualization-tests PR: 1210
File: tests/virt/cluster/general/mass_machine_type_transition_tests/conftest.py:142-149
Timestamp: 2025-06-23T19:18:12.275Z
Learning: In OpenShift Virtualization machine type transition tests, the kubevirt_api_lifecycle_automation_job updates VM machine types to the latest version based on a MACHINE_TYPE_GLOB pattern, and subsequent fixtures may intentionally revert the machine type to test bidirectional transition behavior.
Applied to files:
utilities/virt.py
📚 Learning: 2025-12-16T20:11:03.645Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.
Applied to files:
utilities/virt.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:
utilities/virt.py
📚 Learning: 2025-09-10T13:25:59.799Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 2014
File: tests/storage/cdi_clone/test_clone.py:0-0
Timestamp: 2025-09-10T13:25:59.799Z
Learning: In the openshift-virtualization-tests repository, when creating DataVolume objects using ocp_resources, use `api_name="storage"` rather than the default `api_name="pvc"`.
Applied to files:
utilities/virt.py
📚 Learning: 2025-12-30T08:14:07.812Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3286
File: tests/utils.py:197-199
Timestamp: 2025-12-30T08:14:07.812Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineInstanceMigration.get() from ocp-resources expects a string for the namespace parameter (not a Namespace object). When calling this method, pass vm.namespace (string) directly rather than extracting .name from a Namespace object.
Applied to files:
utilities/virt.py
📚 Learning: 2025-12-16T15:09:49.597Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T15:09:49.597Z
Learning: In the openshift-virtualization-tests repository, when conftest.py or utilities/bitwarden.py changes affect py_config["os_login_param"], smoke test impact must be determined by: (1) finding all smoke tests using `rg "pytest.mark.smoke"`, (2) checking each for VM creation patterns (VirtualMachineForTests, running_vm, VirtualMachineForTestsFromTemplate), (3) tracing whether running_vm is called with default check_ssh_connectivity=True, which accesses vm.login_params property that reads py_config["os_login_param"][vm.os_flavor]. The dependency chain is: smoke test → VM creation → running_vm → wait_for_ssh_connectivity → vm.login_params → os_login_param. Any smoke test creating VMs with SSH connectivity (the default) depends on os_login_param.
Applied to files:
utilities/virt.py
🧬 Code graph analysis (1)
utilities/virt.py (2)
utilities/cluster.py (1)
cache_admin_client(8-19)tests/conftest.py (2)
namespace(659-672)nodes(421-422)
⏰ 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). (9)
- 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 (2)
utilities/virt.py (2)
31-31: LGTM - Import changes are consistent with the refactoring.The removal of
get_clientfrom the import and the addition ofcache_admin_clientimport align with the PR objective to replaceget_client()usage.Also applies to: 51-51
1099-1100: LGTM - Appropriate use of cache_admin_client() in property.Using
cache_admin_client()here is consistent with the function's docstring guidance, as properties cannot accept client parameters. The privileged nature of this VMI requires admin client access.
utilities/virt.py
Outdated
| import utilities.cpu | ||
| import utilities.data_utils | ||
| import utilities.infra | ||
| from utilities.cluster import cache_admin_client |
There was a problem hiding this comment.
do not use this function. see the function's docsting This usage of this function is limited ONLY in places where client cannot be passed as an argument.
add client arg where needed
There was a problem hiding this comment.
Yes, I did consider whether to use this function.
I ultimately decided to use it because the replaced get_client() returns an admin_client. If we add client directly, cannot guarantee that it will be an admin_client, which could affect the test results.
There was a problem hiding this comment.
@SiboWang1997 Just to double check, the cache_get_client returns an admin client? It seems that it uses the wrapper get_client which returns the client depending on the authentication credentials provided.
Is this a performance improvement by using the cached variant?
There was a problem hiding this comment.
@SamAlber , I assume what you mentioned was cache_admin_client, we can't directly call admin_client/unprivileged_client in utilities code as those are fixture, so that's why I try to call cache_admin_client, but according to function's docsting, we also can't use it in such scenario, so that we need to add client arg where needed
There was a problem hiding this comment.
@SiboWang1997, yes i meant cache_admin_client.
I agree that we need to hardcode the client as the privileged_vmi is a property.
Now we're going to set the client passed to VirtualMachineForTests, right?
If it's an unprivileged_client the privileged_vmi will be unprivileged too?
Did we want the privileged_vmi to be the admin sibling of the created vm (admin_client/unprivileged_client) in the first place ?
Replace the get_client() used in VirtualMachineForTestsFromTemplate object and ServiceForVirtualMachineForTests object with self.client Signed-off-by: Sibo Wang <siwang@redhat.com>
SiboWang1997
force-pushed
the
replace_get_client
branch
from
9e148be to
e1a2788
Compare
SiboWang1997
changed the title
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @utilities/virt.py:
- Around line 1511-1513: The code is using self.client to access a
cluster-scoped Node which requires an admin client; replace the client argument
when calling the Node lookup with cache_admin_client() instead of self.client
(i.e., change client=self.client to client=cache_admin_client()) and ensure you
add the import from ocp_utilities.infra: cache_admin_client at the top of the
file so cluster-scoped Node access always uses the admin client.
- Line 1099: The privileged_vmi property currently instantiates
VirtualMachineInstance with self.client which may be unprivileged; change
privileged_vmi to import and call cache_admin_client() and pass its return value
as the client when constructing VirtualMachineInstance so cluster-scoped Node
access works (update the privileged_vmi property to use cache_admin_client()
instead of self.client and keep name=self.name, namespace=self.namespace).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
utilities/virt.py
🧰 Additional context used
📓 Path-based instructions (2)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: NEVER add linter suppressions like# noqa,# type: ignore,# pylint: disable— fix the code instead of suppressing linter errors
Google-format docstrings are REQUIRED for all public functions with non-obvious return values OR side effects
ALWAYS use absolute imports — NEVER use relative imports in Python files
ALWAYS import specific functions — usefrom module import func, NEVER useimport module
ALWAYS use named arguments for function calls with more than one argument
NEVER use single-letter variable names — ALWAYS use descriptive, meaningful names
No dead code — every function, variable, fixture MUST be used or removed. Code marked with# skip-unused-codeis excluded from dead code analysis
Prefer direct attribute access — usefoo.attrdirectly. Save to variables only when reusing the same attribute multiple times improves readability or extracting clarifies intent
INFO level logging REQUIRED for — test phase transitions, resource creation/deletion, configuration changes, API responses, intermediate state
ERROR level logging REQUIRED for — exceptions with full context: what failed, expected vs actual values, resource state
NEVER use DEBUG level logging — if a log is needed, use INFO
NEVER log secrets, tokens, passwords, or PII
Log format REQUIRED — structured key-value pairs for searchability using format likeLOGGER.info("VM created", extra={"name": vm_name, "namespace": ns})
ALWAYS re-raise exceptions with context — useraise NewError("message") from original_errorto preserve stack trace
Do not catch bareException— catch specific exception types only
NEVER silently swallow exceptions — at minimum, log the error before continuing
ALWAYS usewithfor resources — files, connections, locks MUST use context managers
ALWAYS usetimeout_samplerfromtimeout_samplerpackage for any operation that waits for a condition, NEVER usetime.sleep()in loops
Use implicit boolean checks —if items:NOTif len(items) > 0:or `if items...
Files:
utilities/virt.py
{libs,utilities}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Type hints are MANDATORY — mypy strict mode in
libs/, all new public functions underutilities/MUST be typed
Files:
utilities/virt.py
🧠 Learnings (14)
📓 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: 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: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 3328
File: utilities/virt.py:1509-1512
Timestamp: 2026-01-06T09:35:25.073Z
Learning: In the openshift-virtualization-tests repository, when ServiceForVirtualMachineForTests.service_ip() needs to access Node information (for NODE_PORT service type), it must use cache_admin_client() rather than self.vm.client because Node is a cluster-scoped resource requiring admin permissions, even if the VM was created with an unprivileged client.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3228
File: .coderabbit.yaml:30-41
Timestamp: 2026-01-05T10:33:55.037Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers minimal pre-merge checks in CodeRabbit configuration: only docstrings enforcement (80% threshold) is needed, not title or description checks.
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: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3286
File: tests/utils.py:197-199
Timestamp: 2025-12-30T08:14:07.812Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineInstanceMigration.get() from ocp-resources expects a string for the namespace parameter (not a Namespace object). When calling this method, pass vm.namespace (string) directly rather than extracting .name from a Namespace object.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.
📚 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:
utilities/virt.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:
utilities/virt.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:
utilities/virt.py
📚 Learning: 2026-01-06T09:35:25.073Z
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 3328
File: utilities/virt.py:1509-1512
Timestamp: 2026-01-06T09:35:25.073Z
Learning: In the openshift-virtualization-tests repository, when ServiceForVirtualMachineForTests.service_ip() needs to access Node information (for NODE_PORT service type), it must use cache_admin_client() rather than self.vm.client because Node is a cluster-scoped resource requiring admin permissions, even if the VM was created with an unprivileged client.
Applied to files:
utilities/virt.py
📚 Learning: 2025-12-30T08:14:07.812Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3286
File: tests/utils.py:197-199
Timestamp: 2025-12-30T08:14:07.812Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineInstanceMigration.get() from ocp-resources expects a string for the namespace parameter (not a Namespace object). When calling this method, pass vm.namespace (string) directly rather than extracting .name from a Namespace object.
Applied to files:
utilities/virt.py
📚 Learning: 2025-08-28T11:29:15.768Z
Learnt from: qwang1
Repo: RedHatQE/openshift-virtualization-tests PR: 1678
File: utilities/oadp.py:5-5
Timestamp: 2025-08-28T11:29:15.768Z
Learning: In the openshift-virtualization-tests project, DynamicClient is imported from kubernetes.dynamic, not openshift.dynamic. The standard import pattern is `from kubernetes.dynamic import DynamicClient`.
Applied to files:
utilities/virt.py
📚 Learning: 2025-06-15T13:55:16.878Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 1160
File: tests/storage/storage_migration/conftest.py:324-338
Timestamp: 2025-06-15T13:55:16.878Z
Learning: The `virtctl_volume()` function does not accept a `client` parameter in its signature.
Applied to files:
utilities/virt.py
📚 Learning: 2025-12-16T20:11:03.645Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T20:11:03.645Z
Learning: In the openshift-virtualization-tests repository, when determining smoke test impact for changes affecting py_config["os_login_param"], follow this verification methodology: (1) Find all smoke tests: `rg "pytest.mark.smoke" --type=py -B2 | grep "def test_"`, (2) For each smoke test file, search for VM creation patterns: `rg "VirtualMachineForTests|running_vm|VirtualMachineForTestsFromTemplate|wait_for_ssh|check_ssh_connectivity"`, (3) Trace the dependency chain: smoke test → VirtualMachineForTests/running_vm() → wait_for_ssh_connectivity() (default enabled) → vm.login_params property → py_config["os_login_param"][vm.os_flavor], (4) Check utilities/virt.py for login_params usage: `rg "os_login_param|login_params" utilities/virt.py -C3`. Any smoke test creating VMs with default SSH connectivity checks (running_vm with check_ssh_connectivity=True) depends on os_login_param, even if the test doesn't directly reference it.
Applied to files:
utilities/virt.py
📚 Learning: 2025-09-15T06:49:53.478Z
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.
Applied to files:
utilities/virt.py
📚 Learning: 2025-12-16T15:09:49.597Z
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3062
File: conftest.py:333-333
Timestamp: 2025-12-16T15:09:49.597Z
Learning: In the openshift-virtualization-tests repository, when conftest.py or utilities/bitwarden.py changes affect py_config["os_login_param"], smoke test impact must be determined by: (1) finding all smoke tests using `rg "pytest.mark.smoke"`, (2) checking each for VM creation patterns (VirtualMachineForTests, running_vm, VirtualMachineForTestsFromTemplate), (3) tracing whether running_vm is called with default check_ssh_connectivity=True, which accesses vm.login_params property that reads py_config["os_login_param"][vm.os_flavor]. The dependency chain is: smoke test → VM creation → running_vm → wait_for_ssh_connectivity → vm.login_params → os_login_param. Any smoke test creating VMs with SSH connectivity (the default) depends on os_login_param.
Applied to files:
utilities/virt.py
📚 Learning: 2025-08-09T01:52:26.683Z
Learnt from: dshchedr
Repo: RedHatQE/openshift-virtualization-tests PR: 1716
File: tests/virt/conftest.py:289-297
Timestamp: 2025-08-09T01:52:26.683Z
Learning: When user dshchedr moves working code from one location to another in the openshift-virtualization-tests repository, they prefer not to modify it unless there's a real issue, maintaining the original implementation to avoid introducing unnecessary changes.
Applied to files:
utilities/virt.py
📚 Learning: 2025-11-08T07:36:57.616Z
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.
Applied to files:
utilities/virt.py
📚 Learning: 2025-09-10T13:25:59.799Z
Learnt from: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 2014
File: tests/storage/cdi_clone/test_clone.py:0-0
Timestamp: 2025-09-10T13:25:59.799Z
Learning: In the openshift-virtualization-tests repository, when creating DataVolume objects using ocp_resources, use `api_name="storage"` rather than the default `api_name="pvc"`.
Applied to files:
utilities/virt.py
⏰ 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). (6)
- GitHub Check: build-container
- GitHub Check: tox
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
- GitHub Check: can-be-merged
🔇 Additional comments (1)
utilities/virt.py (1)
1373-1373: No action required. Template processing does not require cluster-admin privileges; it only requirescreatepermission onprocessedtemplates.template.openshift.ioresources (namespace-scoped RBAC is sufficient). The code works correctly with unprivileged clients, as confirmed by active test usage in upgrade and machine-type-transition tests.Likely an incorrect or invalid review comment.
| @property | ||
| def privileged_vmi(self): | ||
| return VirtualMachineInstance(client=get_client(), name=self.name, namespace=self.namespace) | ||
| return VirtualMachineInstance(client=self.client, name=self.name, namespace=self.namespace) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the privileged_vmi property definition
rg -n 'def privileged_vmi' utilities/virt.py -A 5Repository: RedHatQE/openshift-virtualization-tests
Length of output: 426
🏁 Script executed:
# Search for all usages of privileged_vmi in the codebase
rg -n 'privileged_vmi' --type=py -C 2Repository: RedHatQE/openshift-virtualization-tests
Length of output: 29805
🏁 Script executed:
# Find VirtualMachineForTests class definition
rg -n 'class VirtualMachineForTests' utilities/virt.py -A 20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1337
🏁 Script executed:
# Search for instantiation patterns of VirtualMachineForTests to see what clients are passed
rg -n 'VirtualMachineForTests\(' --type=py | head -30Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2478
🏁 Script executed:
# Check the unprivileged client test to see if VirtualMachineForTests is used with unprivileged clients
cat -n tests/install_upgrade_operators/security/scc/test_unprivileged_client_virt_launcher.py | head -40Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1455
🏁 Script executed:
# Check how VirtualMachineForTests __init__ handles client parameter
rg -n 'def __init__' utilities/virt.py -A 35 | grep -A 35 'class VirtualMachineForTests' | head -50Repository: RedHatQE/openshift-virtualization-tests
Length of output: 65
🏁 Script executed:
# Verify that Node access is indeed cluster-scoped and would fail with unprivileged clients
# Search for how Node is typically accessed in the codebase
rg -n 'cache_admin_client' utilities/virt.py -B 2 -A 2 | head -40Repository: RedHatQE/openshift-virtualization-tests
Length of output: 65
🏁 Script executed:
# Check the service_ip() implementation mentioned in the learning to see the pattern
rg -n 'def service_ip' utilities/virt.py -A 10Repository: RedHatQE/openshift-virtualization-tests
Length of output: 655
🏁 Script executed:
# Look for cache_admin_client usage pattern in the codebase
rg -n 'cache_admin_client' --type=py | head -20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1959
🏁 Script executed:
# Check if there's a pattern for accessing Node that requires admin permissions
rg -n '\.node' utilities/virt.py -B 2 | grep -A 2 'privileged_vmi\|cache_admin' | head -40Repository: RedHatQE/openshift-virtualization-tests
Length of output: 331
🏁 Script executed:
# Search for how Pod.node property is implemented
rg -n 'def node' --type=py -A 5 | grep -A 5 'Pod\|class.*node' | head -50Repository: RedHatQE/openshift-virtualization-tests
Length of output: 65
🏁 Script executed:
# Check if Pod class has a node property and what client it uses
python3 << 'EOF'
# Let's trace what happens when accessing .node on a pod
# First check if Pod comes from ocp_resources
import subprocess
result = subprocess.run(
["rg", "-n", "from ocp_resources.pod import", "--type=py"],
capture_output=True,
text=True
)
print("Pod imports:")
print(result.stdout[:500])
# Now check what client is used when accessing node
result2 = subprocess.run(
["rg", "-n", "class Pod", "utilities/virt.py"],
capture_output=True,
text=True
)
print("\nPod class definition in virt.py:")
print(result2.stdout)
EOFRepository: RedHatQE/openshift-virtualization-tests
Length of output: 131
🏁 Script executed:
# Check the actual implementation of VirtualMachineInstance and its node property
rg -n 'class VirtualMachineInstance' utilities/virt.py -A 15Repository: RedHatQE/openshift-virtualization-tests
Length of output: 65
🏁 Script executed:
# Find where VirtualMachineInstance is imported from
rg -n 'VirtualMachineInstance' utilities/virt.py | head -10Repository: RedHatQE/openshift-virtualization-tests
Length of output: 856
🏁 Script executed:
# Check if VirtualMachineInstance is imported and from where
rg -n '^from.*import.*VirtualMachineInstance' utilities/virt.pyRepository: RedHatQE/openshift-virtualization-tests
Length of output: 159
🏁 Script executed:
# Look at the imports at the top of utilities/virt.py
head -100 utilities/virt.py | grep -n 'import\|from'Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2193
🏁 Script executed:
# Check if there's already a pattern in the codebase of using cache_admin_client for Node access
rg -n 'cache_admin_client()' utilities/virt.py -B 3 -A 3Repository: RedHatQE/openshift-virtualization-tests
Length of output: 65
🏁 Script executed:
# Check if there are examples of getting Node through admin_client
rg -n '\.node' --type=py | grep -i admin | head -20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 829
🏁 Script executed:
# Search for service_ip method which was mentioned in the learning as requiring admin client
sed -n '1497,1530p' utilities/virt.pyRepository: RedHatQE/openshift-virtualization-tests
Length of output: 1440
🏁 Script executed:
# Let me verify if privileged_vmi actually accesses .node directly
rg -n 'privileged_vmi\.node' --type=py -C 2Repository: RedHatQE/openshift-virtualization-tests
Length of output: 3980
🏁 Script executed:
# Check the service_ip implementation more carefully - does it have the admin client issue too?
rg -n 'class ServiceForVirtualMachineForTests' utilities/virt.py -A 50 | head -80Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2043
🏁 Script executed:
# Check the git history to see if privileged_vmi recently changed from using cache_admin_client
git log -p --all -S 'privileged_vmi' -- utilities/virt.py | head -100Repository: RedHatQE/openshift-virtualization-tests
Length of output: 65
🏁 Script executed:
# Check if there's a pattern of using cache_admin_client in properties that access cluster resources
rg -n 'def.*cache_admin_client' utilities/ --type=py -A 3Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2854
🏁 Script executed:
# Check if VirtualMachineInstance.node property exists and how it accesses the client
python3 << 'EOF'
import subprocess
result = subprocess.run(
["rg", "-n", "virt_launcher_pod.node", "utilities/virt.py", "-B2", "-A2"],
capture_output=True,
text=True
)
print("Usage pattern of virt_launcher_pod.node:")
print(result.stdout[:1000])
EOFRepository: RedHatQE/openshift-virtualization-tests
Length of output: 394
🏁 Script executed:
# Now let me check if there's a vmi.node property by looking at what properties are accessed on privileged_vmi
rg -n 'privileged_vmi\.' --type=py | grep -v 'virt_launcher_pod' | head -20Repository: RedHatQE/openshift-virtualization-tests
Length of output: 2617
🏁 Script executed:
# Check if VirtualMachineInstance is from ocp_resources and see what it inherits
python3 << 'EOF'
import subprocess
# Search for Node property in ocp_resources VirtualMachineInstance
result = subprocess.run(
["rg", "-n", "def node", "--type=py"],
capture_output=True,
text=True,
cwd="."
)
print("Found 'def node' in:")
for line in result.stdout.split('\n')[:20]:
if line and 'ocp_resources' not in line and 'test' not in line:
print(line)
# Now check if VirtualMachineInstance has a node property by looking at its usage
result2 = subprocess.run(
["rg", "-n", "\.node", "utilities/virt.py", "-B1"],
capture_output=True,
text=True,
)
lines = result2.stdout.split('\n')
# Filter for node access patterns
print("\n\nNode access patterns in virt.py (first 30):")
for i, line in enumerate(lines[:30]):
if line:
print(line)
EOFRepository: RedHatQE/openshift-virtualization-tests
Length of output: 1185
🏁 Script executed:
# Let me check if VirtualMachineInstance comes from a library and if it has a node property
# by checking if the Node access is through inheritance or delegation
rg -n 'class VirtualMachineInstance' --type=py -A 5 | grep -v testRepository: RedHatQE/openshift-virtualization-tests
Length of output: 65
🏁 Script executed:
# Check what .node actually is - is it accessing through Pod or is it a property on VirtualMachineInstance?
rg -n 'vm\.vmi\.node' utilities/virt.py -B 2 -A 2Repository: RedHatQE/openshift-virtualization-tests
Length of output: 1020
🏁 Script executed:
# Check the exact line where vm.vmi.node is used to understand the pattern
sed -n '1810,1890p' utilities/virt.pyRepository: RedHatQE/openshift-virtualization-tests
Length of output: 3391
Use cache_admin_client() for privileged_vmi property to access cluster-scoped Node resources.
The property named privileged_vmi returns a VirtualMachineInstance created with self.client, which may be an unprivileged client. However, callers access cluster-scoped resources like Node through this property (e.g., vm.privileged_vmi.node, vm.privileged_vmi.virt_launcher_pod.node). Since Node is a cluster-scoped resource requiring admin permissions, this will fail when VirtualMachineForTests is instantiated with an unprivileged client.
Change privileged_vmi to use cache_admin_client() instead of self.client:
Suggested fix
@property
def privileged_vmi(self):
from utilities.cluster import cache_admin_client
return VirtualMachineInstance(client=cache_admin_client(), name=self.name, namespace=self.namespace)🤖 Prompt for AI Agents
In @utilities/virt.py at line 1099, The privileged_vmi property currently
instantiates VirtualMachineInstance with self.client which may be unprivileged;
change privileged_vmi to import and call cache_admin_client() and pass its
return value as the client when constructing VirtualMachineInstance so
cluster-scoped Node access works (update the privileged_vmi property to use
cache_admin_client() instead of self.client and keep name=self.name,
namespace=self.namespace).
| client=self.client, | ||
| name=self.vmi.instance.status.nodeName, | ||
| ) |
There was a problem hiding this comment.
CRITICAL: Node access requires admin client, not instance client.
Node is a cluster-scoped resource that requires admin permissions. Using self.client here will fail when the service was created with an unprivileged client (e.g., when vm.client is unprivileged).
Based on learnings, this must use cache_admin_client() for cluster-scoped resources, regardless of the client used to create the VM.
🔧 Proposed fix
First, ensure cache_admin_client is imported at the top of the file:
from ocp_utilities.infra import cache_admin_clientThen apply this diff:
vm_node = Node(
- client=self.client,
+ client=cache_admin_client(),
name=self.vmi.instance.status.nodeName,
)📝 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.
| client=self.client, | |
| name=self.vmi.instance.status.nodeName, | |
| ) | |
| client=cache_admin_client(), | |
| name=self.vmi.instance.status.nodeName, | |
| ) |
🤖 Prompt for AI Agents
In @utilities/virt.py around lines 1511 - 1513, The code is using self.client to
access a cluster-scoped Node which requires an admin client; replace the client
argument when calling the Node lookup with cache_admin_client() instead of
self.client (i.e., change client=self.client to client=cache_admin_client()) and
ensure you add the import from ocp_utilities.infra: cache_admin_client at the
top of the file so cluster-scoped Node access always uses the admin client.
There was a problem hiding this comment.
According to function's docsting, we can't use cache_admin_client() in such scenario
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for cache_admin_client definition and docstring
rg -nP -A 20 'def cache_admin_client' --type=py
Length of output: 620
🏁 Script executed:
#!/bin/bash
# Search for any documentation or comments about cache_admin_client usage
rg -nP -C 5 'cache_admin_client' utilities/virt.py
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for ocp_utilities.infra module to find cache_admin_client documentation
fd -e py infra.py | xargs rg -nP -A 15 'def cache_admin_client'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for service_ip method and surrounding context/comments
rg -nP -B 10 -A 5 'def service_ip' utilities/virt.py
Length of output: 726
@SiboWang1997, you're absolutely right!
The cache_admin_client() docstring explicitly states: "This usage of this function is limited ONLY in places where client cannot be passed as an argument."
Since service_ip() is a regular method where a client parameter can be passed, using cache_admin_client() would violate this design principle.
The Real Issue
The current change from get_client() → self.client is problematic because:
- CRITICAL:
get_client()returns an admin client (needed for cluster-scoped Node access) self.clientmay be an unprivileged client when the Service was created by an unprivileged user- Node is a cluster-scoped resource requiring admin permissions
- This will break NODE_PORT service type when using unprivileged clients
Proper Solution
Following the design pattern, service_ip() should accept an optional admin_client parameter:
def service_ip(self, ip_family=None, admin_client=None):
if self.service_type == Service.Type.CLUSTER_IP:
# ... existing logic ...
return self.instance.spec.clusterIP
# Use passed admin_client or fall back to self.client for backward compatibility
client = admin_client or self.client
vm_node = Node(
client=client,
name=self.vmi.instance.status.nodeName,
)
# ... rest of method ...Then callers requiring NODE_PORT can explicitly pass admin_client:
service_ip = vm.custom_service.service_ip(admin_client=cache_admin_client())This approach:
- ✅ Respects the
cache_admin_client()docstring constraint - ✅ Ensures Node access has admin permissions when needed
- ✅ Maintains backward compatibility
- ✅ Makes the admin requirement explicit at call sites
Would you like me to help update the PR to follow this pattern?
🧬 Code Graph Analysis Results
tests/conftest.py
295-299 def admin_client(): """ Get DynamicClient """ return cache_admin_client()⛔ Learnings removed
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 3328
File: utilities/virt.py:1509-1512
Timestamp: 2026-01-06T09:35:25.073Z
Learning: In the openshift-virtualization-tests repository, when ServiceForVirtualMachineForTests.service_ip() needs to access Node information (for NODE_PORT service type), it must use cache_admin_client() rather than self.vm.client because Node is a cluster-scoped resource requiring admin permissions, even if the VM was created with an unprivileged client.
🧠 Learnings used
Learnt from: SiboWang1997
Repo: RedHatQE/openshift-virtualization-tests PR: 3328
File: utilities/virt.py:1509-1512
Timestamp: 2026-01-06T09:35:25.073Z
Learning: In the openshift-virtualization-tests repository, when ServiceForVirtualMachineForTests.service_ip() needs to access Node information (for NODE_PORT service type), it must use cache_admin_client() rather than self.vm.client because Node is a cluster-scoped resource requiring admin permissions, even if the VM was created with an unprivileged client.
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: 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: 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: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3286
File: tests/utils.py:197-199
Timestamp: 2025-12-30T08:14:07.812Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineInstanceMigration.get() from ocp-resources expects a string for the namespace parameter (not a Namespace object). When calling this method, pass vm.namespace (string) directly rather than extracting .name from a Namespace object.
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: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3228
File: .coderabbit.yaml:30-41
Timestamp: 2026-01-05T10:33:55.037Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers minimal pre-merge checks in CodeRabbit configuration: only docstrings enforcement (80% threshold) is needed, not title or description checks.
There was a problem hiding this comment.
self.client automatically calls get_client() internally which returns an admin client with sufficient permissions to access cluster-scoped Node resources.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how ServiceForVirtualMachineForTests is instantiated to see if client is passed
rg -nP -B 3 -A 10 'ServiceForVirtualMachineForTests\(' utilities/virt.py
Length of output: 1118
🏁 Script executed:
#!/bin/bash
# Search for ocp-resources imports and usage patterns with client
rg -nP 'from ocp_resources' utilities/virt.py | head -20
Length of output: 772
🏁 Script executed:
#!/bin/bash
# Check if there are any instances where Service is created with or without client
rg -nP 'Service\(' utilities/virt.py -A 3 | head -40
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for how get_client() is used vs self.client in the file before this PR
git log --all --pretty=format: --name-only --diff-filter=D | grep -q 'utilities/virt.py' || \
git show HEAD~1:utilities/virt.py | rg -nP 'client=get_client\(\)|client=self\.client' | head -20
Length of output: 550
🏁 Script executed:
#!/bin/bash
# Search for Node() instantiation patterns in the codebase to see how client is typically passed
rg -nP 'Node\(' --type=py -A 2 | grep -E '(client=|Node\()' | head -40
Length of output: 101
🏁 Script executed:
#!/bin/bash
# Search for how Service base class is typically used - does it require client parameter?
rg -nP 'class.*\(Service\)' --type=py -A 10
Length of output: 1903
🏁 Script executed:
#!/bin/bash
# Look for any other places where resources are created without explicit client
rg -nP 'def __init__.*\):' utilities/virt.py -A 5 | grep -A 5 'super().__init__' | head -50
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check VirtualMachineForTests to see how it handles client parameter
rg -nP 'class VirtualMachineForTests' utilities/virt.py -A 50 | grep -A 10 '__init__'
Length of output: 598
🏁 Script executed:
#!/bin/bash
# Search for Resource base class usage and client property patterns
rg -nP '@property.*\n.*def client' --type=py
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for how the ocp_resources library is documented regarding client parameter
rg -nP 'client.*get_client|get_client.*client' --type=py | head -30
Length of output: 1339
🏁 Script executed:
#!/bin/bash
# Check the exact super().__init__ call in ServiceForVirtualMachineForTests
rg -nP 'class ServiceForVirtualMachineForTests' utilities/virt.py -A 25 | grep -A 15 'super().__init__'
Length of output: 408
🏁 Script executed:
#!/bin/bash
# Look for any documentation or comments about Resource class default client behavior
rg -nP 'class.*\(Resource\):.*client|Resource.*default.*client' --type=py -A 5 | head -40
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check how other methods in ServiceForVirtualMachineForTests use self.client
rg -nP 'class ServiceForVirtualMachineForTests' utilities/virt.py -A 60 | grep 'self.client'
Length of output: 36
🏁 Script executed:
#!/bin/bash
# Search for any place where Service is instantiated to see if client is typically passed
rg -nP 'Service\(' --type=py -B 2 -A 5 | grep -E '(client=|Service\()' | head -50
Length of output: 1225
🏁 Script executed:
#!/bin/bash
# Look for the actual Service base class __init__ signature to see if client has a default
fd -e py | xargs rg -l 'from ocp_resources.service import Service' | head -1 | xargs cat | rg -A 30 'class Service'
Length of output: 815
🏁 Script executed:
#!/bin/bash
# Search for actual usage of service_ip() method in tests to see if it works
rg -nP '\.service_ip\(' --type=py -B 2 -A 2
Length of output: 867
🏁 Script executed:
#!/bin/bash
# Look for any test cases that use ServiceForVirtualMachineForTests or custom_service
rg -nP 'custom_service|ServiceForVirtualMachineForTests' --type=py | head -40
Length of output: 3444
🏁 Script executed:
#!/bin/bash
# Check if there's any pattern where ocp-resources resources are used without passing client
rg -nP 'VirtualMachine\(|Pod\(|Node\(|DataVolume\(' --type=py -A 2 | grep -v 'client=' | head -40
Length of output: 1810
🏁 Script executed:
#!/bin/bash
# Look for any imports or usage of ocp_utilities.infra cache_admin_client
rg -nP 'from ocp_utilities.infra import|cache_admin_client' --type=py | head -20
Length of output: 1876
🏁 Script executed:
#!/bin/bash
# Check the PR diff to see what the original code was
git diff HEAD~1 HEAD -- utilities/virt.py | grep -A 5 -B 5 'vm_node = Node'
Length of output: 470
🏁 Script executed:
#!/bin/bash
# Search for any Python package info or documentation about ocp-resources default client
rg -nP 'ocp.resources|ocp_resources' setup.py pyproject.toml requirements.txt 2>/dev/null | head -20
Length of output: 36
🏁 Script executed:
#!/bin/bash
# Check if there's a pattern of Resource instances being created without client parameter
rg -nP '(VirtualMachine|Pod|Node|Service|DataVolume)\(' utilities/ tests/ -A 1 | grep -v 'client=' | head -60
Length of output: 3138
🏁 Script executed:
#!/bin/bash
# Look for any comments or docstrings explaining client parameter behavior
rg -nP 'client.*parameter|default.*client|admin.*client' utilities/virt.py -B 2 -A 2 | head -40
Length of output: 1558
🏁 Script executed:
#!/bin/bash
# Check if Resource class from ocp_resources has a @property client method
python3 -c "from ocp_resources.resource import Resource; import inspect; print(inspect.getsource(Resource.client.fget if hasattr(Resource, 'client') and isinstance(getattr(Resource, 'client'), property) else lambda: 'No client property'))" 2>&1 || echo "Cannot import ocp_resources"
Length of output: 154
🏁 Script executed:
#!/bin/bash
# Search for how VirtualMachineForTests passes client to parent class
rg -nP 'class VirtualMachineForTests.*:' utilities/virt.py -A 80 | grep -A 5 'super().__init__'
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get VirtualMachineForTests __init__ and super().__init__ call
rg -nP 'self.name = utilities.infra.unique_name' utilities/virt.py -A 15
Length of output: 674
🏁 Script executed:
#!/bin/bash
# Check if there are examples of Node being created WITHOUT client parameter that work
rg -nP 'Node\(' --type=py -B 3 -A 3 | grep -v 'client=' -A 3 -B 3 | head -50
Length of output: 328
🏁 Script executed:
#!/bin/bash
# Look for any Resource subclass that doesn't pass client to see the pattern
rg -nP 'super\(\).__init__\(' utilities/virt.py -B 5 -A 3 | grep -v 'client=' | head -50
Length of output: 1291
🏁 Script executed:
#!/bin/bash
# Check for VirtualMachineInstance usage which is also in the same file
rg -nP 'VirtualMachineInstance\(' utilities/virt.py -A 2
Length of output: 184
🏁 Script executed:
#!/bin/bash
# Look at the privileged_vmi property to understand the pattern
rg -nP '@property.*\n.*def privileged_vmi' utilities/virt.py -A 5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the privileged_vmi property definition
rg -nP 'def privileged_vmi' utilities/virt.py -B 2 -A 5
Length of output: 368
🏁 Script executed:
#!/bin/bash
# Look for any documentation or pattern about Resource client property in comments
rg -nP 'Resource.*client|client.*Resource' utilities/virt.py -B 2 -A 2 | head -30
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Check if ocp-resources package has documentation files
fd -e md -e rst -e txt README CHANGELOG | head -10
Length of output: 96
🏁 Script executed:
#!/bin/bash
# Search for any usage pattern that shows Resource has default client
rg -nP 'self\.client' utilities/virt.py | head -30
Length of output: 437
🏁 Script executed:
#!/bin/bash
# Check git history of privileged_vmi to see if it ever used get_client()
git log -p -S 'privileged_vmi' --all -- utilities/virt.py | grep -A 10 -B 5 'privileged_vmi' | head -40
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check git history to see what the original privileged_vmi looked like
git log --all --oneline --grep='privileged_vmi' -- utilities/virt.py | head -5
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for when privileged_vmi was first added to see if it always used self.client
git log --all --diff-filter=A -p -- utilities/virt.py | grep -A 5 -B 5 'def privileged_vmi' | head -20
Length of output: 371
🏁 Script executed:
#!/bin/bash
# Search for any test that verifies Node access works with self.client pattern
rg -nP 'test.*node|def test.*' tests/network/migration/test_migration.py -A 10 | grep -A 10 'service_ip'
Length of output: 289
🏁 Script executed:
#!/bin/bash
# Look for DataVolume or other resource usage patterns without client
rg -nP 'DataVolume\(|VirtualMachineInstance\(' utilities/virt.py -B 2 -A 3 | head -40
Length of output: 596
🏁 Script executed:
#!/bin/bash
# Check what the PR description or commit message says about this change
git log --oneline -1
Length of output: 242
@SiboWang1997, thank you for the clarification! You raise an important point about the Resource base class behavior.
Analysis
Looking at the evidence:
-
ServiceForVirtualMachineForTests structure:
- Does NOT accept or pass
clientparameter tosuper().__init__()(lines 1457-1475) - Yet
service_ip()usesself.clientsuccessfully (line 1511)
- Does NOT accept or pass
-
This pattern works in production:
tests/network/migration/test_migration.py:418usesservice_ip(ip_family=...)successfully
-
Git history shows similar pattern:
privileged_vmiproperty originally usedclient=get_client()- It was changed to
client=self.clientin a similar refactoring
The Key Question
If Resource.client property internally calls get_client() and returns an admin client, then your change is correct and safe. This would mean:
- ✅
self.client→ calls Resource's client property → callsget_client()→ returns admin client - ✅ Node access works (admin client has permissions)
- ✅ Consistent with the broader refactoring pattern
Recommendation
Since the ocp-resources library implementation isn't available in this repository, could you:
- Confirm that
Resource.clientproperty does callget_client()internally when no client is explicitly provided? - Test the NODE_PORT service type scenario to ensure Node lookup works correctly?
If this behavior is confirmed, then the change is appropriate and my earlier concern was unfounded.
🧬 Code Graph Analysis Results
[filename: utilities/virt.py]
-
utilities/virt.py, lines where get_client is imported
-
Snippet (complete):
from ocp_resources.resource import Resource, ResourceEditor, get_client -
Purpose: Shows origin of get_client used to obtain an admin client in subsequent calls.
-
-
utilities/virt.py, usage of get_client() to fetch cluster-scoped resources (examples of admin client usage)
- Snippet (complete):
for pod in utilities.infra.get_pod_by_name_prefix(
client=get_client(),
pod_prefix=VIRT_LAUNCHER,
namespace=namespace,
get_all=True,
):
if pod.status not in (Pod.Status.RUNNING, Pod.Status.COMPLETED, Pod.Status.SUCCEEDED):
... - Purpose: Demonstrates how get_client() is used to access cluster-scoped resources (pods) from within this module.
- Snippet (complete):
-
utilities/virt.py, fedora_vm_body example showing how admin client is obtained for node architecture query
- Snippet (complete):
image_info = get_oc_image_info(
image=image,
pull_secret=pull_secret,
architecture=utilities.cpu.get_nodes_cpu_architecture(
nodes=list(Node.get(client=get_client())),
),
) - Purpose: Illustrates obtaining an admin client via get_client() to query cluster-wide Node information (CPU architecture) as part of VM image info resolution.
- Snippet (complete):
🧠 Learnings used
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: 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: 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: 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.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3286
File: tests/utils.py:197-199
Timestamp: 2025-12-30T08:14:07.812Z
Learning: In the openshift-virtualization-tests repository, VirtualMachineInstanceMigration.get() from ocp-resources expects a string for the namespace parameter (not a Namespace object). When calling this method, pass vm.namespace (string) directly rather than extracting .name from a Namespace object.
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: jpeimer
Repo: RedHatQE/openshift-virtualization-tests PR: 0
File: :0-0
Timestamp: 2025-12-22T15:56:00.157Z
Learning: In the openshift-virtualization-tests repository, when responding to test execution plan requests from openshift-virtualization-qe-bot-3, do NOT use "REQUEST_CHANGES" review type if the PR author has already marked the PR as verified (e.g., with `/verified` command). Test execution plans are informational guides, not blocking requirements. Use COMMENT event for informational test plans, or only REQUEST_CHANGES if there are actual code issues that need to be addressed before merging.
Learnt from: rnetser
Repo: RedHatQE/openshift-virtualization-tests PR: 3228
File: .coderabbit.yaml:30-41
Timestamp: 2026-01-05T10:33:55.037Z
Learning: In the openshift-virtualization-tests repository, user rnetser prefers minimal pre-merge checks in CodeRabbit configuration: only docstrings enforcement (80% threshold) is needed, not title or description checks.
8 participants
Short description:
Replace the get_client() used in VirtualMachineForTestsFromTemplate object and ServiceForVirtualMachineForTests object with self.client
More details:
As drop get_client() in utilities/virt.py will lead to many relevant places change, this PR only updates the objects where get_client() can be directly replaced with self.client
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
https://issues.redhat.com/browse/CNV-73714
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.