From 037ed11f649f356efc4363db32b9f85ef02ca106 Mon Sep 17 00:00:00 2001 From: Mikhail Mazurskiy Date: Sun, 25 Jul 2021 19:31:58 +1000 Subject: [PATCH] Cache rest mapper and discovery client They are both stateful so it is beneficial for performance to cache them. --- .../pkg/genericclioptions/config_flags.go | 66 ++++++++++++++++--- .../cli-runtime/pkg/resource/builder.go | 24 +------ .../k8s.io/client-go/restmapper/discovery.go | 2 +- 3 files changed, 59 insertions(+), 33 deletions(-) diff --git a/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go b/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go index 86a601a0ded..3a715c06d7e 100644 --- a/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go +++ b/staging/src/k8s.io/cli-runtime/pkg/genericclioptions/config_flags.go @@ -102,11 +102,18 @@ type ConfigFlags struct { // before it is returned in ToRESTConfig function. WrapConfigFn func(*rest.Config) *rest.Config - clientConfig clientcmd.ClientConfig - lock sync.Mutex - // If set to true, will use persistent client config and - // propagate the config to the places that need it, rather than - // loading the config multiple times + clientConfig clientcmd.ClientConfig + clientConfigLock sync.Mutex + + restMapper meta.RESTMapper + restMapperLock sync.Mutex + + discoveryClient discovery.CachedDiscoveryInterface + discoveryClientLock sync.Mutex + + // If set to true, will use persistent client config, rest mapper, discovery client, and + // propagate them to the places that need them, rather than + // instantiating them multiple times. usePersistentConfig bool // Allows increasing burst used for discovery, this is useful // in clusters with many registered resources @@ -216,8 +223,8 @@ func (f *ConfigFlags) toRawKubeConfigLoader() clientcmd.ClientConfig { // toRawKubePersistentConfigLoader binds config flag values to config overrides // Returns a persistent clientConfig for propagation. func (f *ConfigFlags) toRawKubePersistentConfigLoader() clientcmd.ClientConfig { - f.lock.Lock() - defer f.lock.Unlock() + f.clientConfigLock.Lock() + defer f.clientConfigLock.Unlock() if f.clientConfig == nil { f.clientConfig = f.toRawKubeConfigLoader() @@ -230,6 +237,27 @@ func (f *ConfigFlags) toRawKubePersistentConfigLoader() clientcmd.ClientConfig { // Expects the AddFlags method to have been called. // Returns a CachedDiscoveryInterface using a computed RESTConfig. func (f *ConfigFlags) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { + if f.usePersistentConfig { + return f.toPersistentDiscoveryClient() + } + return f.toDiscoveryClient() +} + +func (f *ConfigFlags) toPersistentDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { + f.discoveryClientLock.Lock() + defer f.discoveryClientLock.Unlock() + + if f.discoveryClient == nil { + discoveryClient, err := f.toDiscoveryClient() + if err != nil { + return nil, err + } + f.discoveryClient = discoveryClient + } + return f.discoveryClient, nil +} + +func (f *ConfigFlags) toDiscoveryClient() (discovery.CachedDiscoveryInterface, error) { config, err := f.ToRESTConfig() if err != nil { return nil, err @@ -255,6 +283,27 @@ func (f *ConfigFlags) ToDiscoveryClient() (discovery.CachedDiscoveryInterface, e // ToRESTMapper returns a mapper. func (f *ConfigFlags) ToRESTMapper() (meta.RESTMapper, error) { + if f.usePersistentConfig { + return f.toPersistentRESTMapper() + } + return f.toRESTMapper() +} + +func (f *ConfigFlags) toPersistentRESTMapper() (meta.RESTMapper, error) { + f.restMapperLock.Lock() + defer f.restMapperLock.Unlock() + + if f.restMapper == nil { + restMapper, err := f.toRESTMapper() + if err != nil { + return nil, err + } + f.restMapper = restMapper + } + return f.restMapper, nil +} + +func (f *ConfigFlags) toRESTMapper() (meta.RESTMapper, error) { discoveryClient, err := f.ToDiscoveryClient() if err != nil { return nil, err @@ -324,7 +373,6 @@ func (f *ConfigFlags) AddFlags(flags *pflag.FlagSet) { if f.Timeout != nil { flags.StringVar(f.Timeout, flagTimeout, *f.Timeout, "The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests.") } - } // WithDeprecatedPasswordFlag enables the username and password config flags @@ -377,7 +425,7 @@ func stringptr(val string) *string { } // overlyCautiousIllegalFileCharacters matches characters that *might* not be supported. Windows is really restrictive, so this is really restrictive -var overlyCautiousIllegalFileCharacters = regexp.MustCompile(`[^(\w/\.)]`) +var overlyCautiousIllegalFileCharacters = regexp.MustCompile(`[^(\w/.)]`) // computeDiscoverCacheDir takes the parentDir and the host and comes up with a "usually non-colliding" name. func computeDiscoverCacheDir(parentDir, host string) string { diff --git a/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go b/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go index 87922680ed2..f206f3b3ef3 100644 --- a/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go +++ b/staging/src/k8s.io/cli-runtime/pkg/resource/builder.go @@ -212,7 +212,7 @@ func NewBuilder(restClientGetter RESTClientGetter) *Builder { return newBuilder( restClientGetter.ToRESTConfig, - (&cachingRESTMapperFunc{delegate: restClientGetter.ToRESTMapper}).ToRESTMapper, + restClientGetter.ToRESTMapper, (&cachingCategoryExpanderFunc{delegate: categoryExpanderFn}).ToCategoryExpander, ) } @@ -1187,28 +1187,6 @@ func HasNames(args []string) (bool, error) { return hasCombinedTypes || len(args) > 1, nil } -type cachingRESTMapperFunc struct { - delegate RESTMapperFunc - - lock sync.Mutex - cached meta.RESTMapper -} - -func (c *cachingRESTMapperFunc) ToRESTMapper() (meta.RESTMapper, error) { - c.lock.Lock() - defer c.lock.Unlock() - if c.cached != nil { - return c.cached, nil - } - - ret, err := c.delegate() - if err != nil { - return nil, err - } - c.cached = ret - return c.cached, nil -} - type cachingCategoryExpanderFunc struct { delegate CategoryExpanderFunc diff --git a/staging/src/k8s.io/client-go/restmapper/discovery.go b/staging/src/k8s.io/client-go/restmapper/discovery.go index 19ae95e1b5b..d560db84dc9 100644 --- a/staging/src/k8s.io/client-go/restmapper/discovery.go +++ b/staging/src/k8s.io/client-go/restmapper/discovery.go @@ -205,7 +205,7 @@ func (d *DeferredDiscoveryRESTMapper) getDelegate() (meta.RESTMapper, error) { } d.delegate = NewDiscoveryRESTMapper(groupResources) - return d.delegate, err + return d.delegate, nil } // Reset resets the internally cached Discovery information and will