Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use cached discovery client #5908

Merged
merged 3 commits into from Jul 7, 2022
Merged

Use cached discovery client #5908

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b5f7f1
Use cached discovery client
dharmit Jul 1, 2022
6414968
Have both cached and regular discovery clients
dharmit Jul 4, 2022
5edd7a0
Docs for odo list's use of cache
dharmit Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/website/docs/command-reference/list.md
View file
Open in desktop
Expand Up @@ -18,3 +18,18 @@ defined in the local Devfile,

* `--namespace` - Namespace to list the components from (optional). By default, the current namespace defined in kubeconfig is used
* `-o json` - Outputs the list in JSON format. See [JSON output](json-output.md) for more information

:::tip use of cache

`odo list` makes use of cache for performance reasons. This is the same cache that is referred by `kubectl` command
when you do `kubectl api-resources --cached=true`. As a result, if you were to install an Operator/CRD on the
Kubernetes cluster, and create a resource from it using odo, you might not see it in the `odo list` output. This
would be the case for 10 minutes timeframe for which the cache is considered valid. Beyond this 10 minutes, the
cache is updated anyway.

If you would like to invalidate the cache before the 10 minutes timeframe, you could manually delete it by doing:
```shell
rm -rf ~/.kube/cache/discovery/api.crc.testing_6443/
```
Above example shows how to invalidate the cache for a CRC cluster. Note that you will have to modify the `api.crc.
testing_6443` part based on the cluster you are working against.
2 changes: 1 addition & 1 deletion pkg/kclient/all.go
View file
Open in desktop
Expand Up @@ -18,7 +18,7 @@ import (
// Code into this file is heavily inspired from https://github.com/ahmetb/kubectl-tree

func (c *Client) GetAllResourcesFromSelector(selector string, ns string) ([]unstructured.Unstructured, error) {
apis, err := findAPIs(c.discoveryClient)
apis, err := findAPIs(c.cachedDiscoveryClient)
if err != nil {
return nil, err
}
Expand Down
15 changes: 12 additions & 3 deletions pkg/kclient/kclient.go
View file
Open in desktop
Expand Up @@ -4,6 +4,8 @@ import (
"fmt"
"strings"

"k8s.io/cli-runtime/pkg/genericclioptions"

"github.com/blang/semver"

kerrors "k8s.io/apimachinery/pkg/api/errors"
Expand Down Expand Up @@ -51,9 +53,10 @@ type Client struct {
// DynamicClient interacts with client-go's `dynamic` package. It is used
// to dynamically create service from an operator. It can take an arbitrary
// yaml and create k8s/OpenShift resource from it.
DynamicClient dynamic.Interface
discoveryClient discovery.DiscoveryInterface
restmapper *restmapper.DeferredDiscoveryRESTMapper
DynamicClient dynamic.Interface
discoveryClient discovery.DiscoveryInterface
cachedDiscoveryClient discovery.CachedDiscoveryInterface
restmapper *restmapper.DeferredDiscoveryRESTMapper

supportedResources map[string]bool
// Is server side apply supported by cluster
Expand Down Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to cache only during an odo execution, it seems that we could call Invalidate just 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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      odo

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 -rf if 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 list docs 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think so. Don't forget to say it is a cache common to the kubectl command

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://deploy-preview-5908--odo-docusaurus-preview.netlify.app/docs/command-reference/list

if err != nil {
return nil, err
}

client.restmapper = restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(client.discoveryClient))

client.checkIngressSupports = true
Expand Down