Use cached discovery client #5908
Conversation
✅ Deploy Preview for odo-docusaurus-preview ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
|
Test failures are real. We are getting the output Logs from failing testsCurrently, this PR replaces |
| @@ -158,6 +161,12 @@ func NewForConfig(config clientcmd.ClientConfig) (client *Client, err error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| config_flags := genericclioptions.NewConfigFlags(true) | |||
| client.cachedDiscoveryClient, err = config_flags.ToDiscoveryClient() | |||
There was a problem hiding this comment.
Do we want to cache between calls to odo, or only during an odo execution.
If we want to cache only during an odo execution, it seems that we could call Invalidate just after creating the client
There was a problem hiding this comment.
If we want to cache only during an odo execution, it seems that we could call
Invalidatejust after creating the client
If we were to invalidate the cache each time we create a client, I think the gain we see in odo list performance in this PR would be gone. But I need to check that to be sure.
There was a problem hiding this comment.
I think the gain we see in
odo listperformance in this PR would be gone. But I need to check that to be sure.
12s without invalidating the cache, 16s if we invalidate it right after creation.
$ odo list
✓ Listing components from namespace 'myproject' [12s]
NAME PROJECT TYPE RUNNING IN MANAGED
* devfile-nodejs-deploy nodejs Deploy odo
$ odo list
✓ Listing components from namespace 'myproject' [16s]
NAME PROJECT TYPE RUNNING IN MANAGED
* devfile-nodejs-deploy nodejs Deploy odoThere was a problem hiding this comment.
What is the client caching? the discovered resources (CRD level), or also the resources themselves (CR level)?
How is the cache consistent: Do we always get the latest versions of what we query (at the price of some back-and-forths to know if the cached one is the last one), of is there some period during which we get old versions?
There was a problem hiding this comment.
What is the client caching? the discovered resources (CRD level), or also the resources themselves (CR level)?
You can look at the cache under ~/.kube/cache/discovery. I took the cue from kubectl api-resources cached=true command.
How is the cache consistent: Do we always get the latest versions of what we query (at the price of some back-and-forths to know if the cached one is the last one), of is there some period during which we get old versions?
There's a ttl, beyond which it falls back to using regular discovery client:
https://github.com/kubernetes/client-go/blob/master/discovery/cached/disk/cached_discovery.go#L155-L157
There was a problem hiding this comment.
what are the solutions if the user doesn't want to wait for 10 minutes
We could document that the user could remove the cache directory from their system using
rm -rfif they don't want to wait. However, this would be relevant for users using minikubs/CRC/kinD. Folks in the more realistic environment might not even have the permissions to install an Operator/CRD.For the time being, I will add this info in
odo listdocs in this PR.
We discussed this topic when being in Paris, with Serena and the team. odo is dedicated to two different kind of people:
- the "lead" developer, who will experiment odo, create and update the Devfile, test different operators, bindings, etc. He will probably have admin access to the cluster to make experiments (on its own CRC cluster for example)
- the "normal" developer, who will have a tailor-made environment (prepared by the first "lead" developer"), and with minimal access to cluster.
The conclusion, as I remember, were that we need mostly to document for the "lead" developer, with the maximum of information for him to understand how to create and update the Devfile efficiently. The doc for the "normal" developer would be very simple, and would be prepared by the "lead" developer.
There was a problem hiding this comment.
The conclusion, as I remember, were that we need mostly to document for the "lead" developer, with the maximum of information for him to understand how to create and update the Devfile efficiently. The doc for the "normal" developer would be very simple, and would be prepared by the "lead" developer.
I wish someone had put this so nicely after that meeting. Anyway, better late than never.
Considering our lead developer scenario here, do you think that covering how to clear the cache manually for the 10-minute scenario would be a good thing to do? Do you think we should cover anything else as well in the docs?
There was a problem hiding this comment.
Yes, I think so. Don't forget to say it is a cache common to the kubectl command
There was a problem hiding this comment.
A cached discovery client refers its cache under `~/.kube/cache` instead of checking with the cluster every time. Signed-off-by: Dharmit Shah <[email protected]>
Signed-off-by: Dharmit Shah <[email protected]>
|
Kudos, SonarCloud Quality Gate passed!
|
openshift-ci
bot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cdrage The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
@dharmit: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/override ci/prow/unit-and-validate-test |
|
@feloy: Overrode contexts on behalf of feloy: ci/prow/unit-and-validate-test, ci/prow/v4.10-integration-e2e In 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 kubernetes/test-infra repository. |
* Use cached discovery client A cached discovery client refers its cache under `~/.kube/cache` instead of checking with the cluster every time. Signed-off-by: Dharmit Shah <[email protected]> * Have both cached and regular discovery clients Signed-off-by: Dharmit Shah <[email protected]> * Docs for odo list's use of cache
Signed-off-by: Dharmit Shah [email protected]
What does this PR do / why we need it:
This PR changes odo to use a cached discovery client.This PR adds a cached discovery client along with the existing discovery
client. Functions can use the client appropriate for their use.
A cached discovery client refers its cache under
~/.kube/cacheinsteadof checking with the cluster every time.
Which issue(s) this PR fixes:
Fixes part of #5872
PR acceptance criteria:
Unit test
Integration test
Documentation
How to test changes:
odo list -v2and look for the line that reads:all.go:119] queried api discovery in. Also, worth noting is the line that reads:all.go:61] all goroutines have returned in.mainbranch.Here are two comparative runs that I did.
odois the binary built using code in this PR, whileodo-mainis the binary built usingmainbranch.