NO-JIRA: cache kube client instance#30997
Conversation
oc client is a generic acessor for all the OpenShift API clients, as well as the typed kubernetes.Interface client. However the acessor for those clients require reading the configuration file from disk and parsing it again. This not only uses more processing, but also more IO, increasing the chance for an IO failure. This update is caching the clients used during the tests, so converting lots of IO and config parsing into a single one per test.
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
|
@jcmoraisjr: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThese changes refactor two test configuration management files to optimize Kubernetes API client usage by introducing cached client instances. Both files eliminate repeated calls to client initialization methods by storing references to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes ✨ 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 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jcmoraisjr The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/extended/router/config_manager.go (1)
787-793: Consider using a cached client for consistency.This helper function creates a new
kubeClientviaoc.AdminKubeClient()on each invocation. While this function is only called once per test (not in loops), passing the cachedkubeClientfrom the suite scope would maintain consistency with the overall caching strategy.♻️ Optional: Accept cached client as parameter
-func createExecPod(oc *exutil.CLI) (execPod *corev1.Pod, done func()) { - kubeClient := oc.AdminKubeClient() +func createExecPod(kubeClient kubernetes.Interface, oc *exutil.CLI) (execPod *corev1.Pod, done func()) { ns := oc.KubeFramework().Namespace.Name execPod = exutil.CreateExecPodOrFail(kubeClient, ns, "execpod") return execPod, func() { kubeClient.CoreV1().Pods(ns).Delete(context.Background(), execPod.Name, *metav1.NewDeleteOptions(1)) } }Then update call sites:
-execPod, doneExecPod := createExecPod(oc) +execPod, doneExecPod := createExecPod(kubeClient, oc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/config_manager.go` around lines 787 - 793, The createExecPod helper currently calls oc.AdminKubeClient() internally which bypasses the suite-level cached client; change createExecPod to accept a pre-created kube client (e.g., client := oc.AdminKubeClient() passed in) or accept the cached client as an argument and use that instead of calling oc.AdminKubeClient() inside createExecPod, update all call sites that invoke createExecPod to pass the cached kubeClient, and keep the returned execPod and done() behavior the same (references: createExecPod, oc.AdminKubeClient, execPod, done func).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/extended/router/config_manager.go`:
- Around line 787-793: The createExecPod helper currently calls
oc.AdminKubeClient() internally which bypasses the suite-level cached client;
change createExecPod to accept a pre-created kube client (e.g., client :=
oc.AdminKubeClient() passed in) or accept the cached client as an argument and
use that instead of calling oc.AdminKubeClient() inside createExecPod, update
all call sites that invoke createExecPod to pass the cached kubeClient, and keep
the returned execPod and done() behavior the same (references: createExecPod,
oc.AdminKubeClient, execPod, done func).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76501e72-ed1d-4b53-846f-22bcc1ad27b8
📒 Files selected for processing (2)
test/extended/router/config_manager.gotest/extended/router/config_manager_ingress.go
|
Scheduling required tests: |
|
@jcmoraisjr: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
oc client is a generic acessor for all the OpenShift API clients, as well as the typed kubernetes.Interface client. However the acessor for those clients require reading the configuration file from disk and parsing it again. This not only uses more processing, but also more IO, increasing the chance for an IO failure.
This update is caching the clients used during the tests, so converting lots of IO and config parsing into a single one per test.