From 611684e9cdcf40ccf8f493ca7fcee58349a67100 Mon Sep 17 00:00:00 2001 From: Mike Fedosin Date: Mon, 2 Jan 2023 13:05:37 +0100 Subject: [PATCH] Provide a truly lazy restmapper Rework WithLazyDiscovery to use DeferredDiscoveryRESTMapper that will lazily query the provided client for discovery information to do REST mappings. --- pkg/client/apiutil/dynamicrestmapper.go | 85 +++++++------------- pkg/client/apiutil/dynamicrestmapper_test.go | 22 ----- 2 files changed, 28 insertions(+), 79 deletions(-) diff --git a/pkg/client/apiutil/dynamicrestmapper.go b/pkg/client/apiutil/dynamicrestmapper.go index 8b7c1c4b68..e3b6282fbb 100644 --- a/pkg/client/apiutil/dynamicrestmapper.go +++ b/pkg/client/apiutil/dynamicrestmapper.go @@ -18,12 +18,12 @@ package apiutil import ( "sync" - "sync/atomic" "golang.org/x/time/rate" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/discovery" + "k8s.io/client-go/discovery/cached/memory" "k8s.io/client-go/rest" "k8s.io/client-go/restmapper" ) @@ -37,9 +37,6 @@ type dynamicRESTMapper struct { newMapper func() (meta.RESTMapper, error) lazy bool - // Used for lazy init. - inited uint32 - initMtx sync.Mutex } // DynamicRESTMapperOption is a functional option on the dynamicRESTMapper. @@ -76,30 +73,43 @@ func WithCustomMapper(newMapper func() (meta.RESTMapper, error)) DynamicRESTMapp // RESTMapper dynamically discovers resource types at runtime. opts // configure the RESTMapper. func NewDynamicRESTMapper(cfg *rest.Config, opts ...DynamicRESTMapperOption) (meta.RESTMapper, error) { - client, err := discovery.NewDiscoveryClientForConfig(cfg) - if err != nil { - return nil, err - } drm := &dynamicRESTMapper{ limiter: rate.NewLimiter(rate.Limit(defaultRefillRate), defaultLimitSize), - newMapper: func() (meta.RESTMapper, error) { - groupResources, err := restmapper.GetAPIGroupResources(client) - if err != nil { - return nil, err - } - return restmapper.NewDiscoveryRESTMapper(groupResources), nil - }, } for _, opt := range opts { - if err = opt(drm); err != nil { + if err := opt(drm); err != nil { return nil, err } } - if !drm.lazy { - if err := drm.setStaticMapper(); err != nil { + + if drm.newMapper == nil { + // If custom mapper is not set, we wse NewDeferredDiscoveryRESTMapper for lazy loading + // or standard NewDiscoveryRESTMapper for other cases. + client, err := discovery.NewDiscoveryClientForConfig(cfg) + if err != nil { return nil, err } + + if !drm.lazy { + drm.newMapper = func() (meta.RESTMapper, error) { + groupResources, err := restmapper.GetAPIGroupResources(client) + if err != nil { + return nil, err + } + + return restmapper.NewDiscoveryRESTMapper(groupResources), nil + } + } else { + drm.newMapper = func() (meta.RESTMapper, error) { + return restmapper.NewDeferredDiscoveryRESTMapper(memory.NewMemCacheClient(client)), nil + } + } + } + + if err := drm.setStaticMapper(); err != nil { + return nil, err } + return drm, nil } @@ -124,23 +134,6 @@ func (drm *dynamicRESTMapper) setStaticMapper() error { return nil } -// init initializes drm only once if drm is lazy. -func (drm *dynamicRESTMapper) init() (err error) { - // skip init if drm is not lazy or has initialized - if !drm.lazy || atomic.LoadUint32(&drm.inited) != 0 { - return nil - } - - drm.initMtx.Lock() - defer drm.initMtx.Unlock() - if drm.inited == 0 { - if err = drm.setStaticMapper(); err == nil { - atomic.StoreUint32(&drm.inited, 1) - } - } - return err -} - // checkAndReload attempts to call the given callback, which is assumed to be dependent // on the data in the restmapper. // @@ -198,9 +191,6 @@ func (drm *dynamicRESTMapper) checkAndReload(checkNeedsReload func() error) erro // TODO: wrap reload errors on NoKindMatchError with go 1.13 errors. func (drm *dynamicRESTMapper) KindFor(resource schema.GroupVersionResource) (schema.GroupVersionKind, error) { - if err := drm.init(); err != nil { - return schema.GroupVersionKind{}, err - } var gvk schema.GroupVersionKind err := drm.checkAndReload(func() error { var err error @@ -211,9 +201,6 @@ func (drm *dynamicRESTMapper) KindFor(resource schema.GroupVersionResource) (sch } func (drm *dynamicRESTMapper) KindsFor(resource schema.GroupVersionResource) ([]schema.GroupVersionKind, error) { - if err := drm.init(); err != nil { - return nil, err - } var gvks []schema.GroupVersionKind err := drm.checkAndReload(func() error { var err error @@ -224,10 +211,6 @@ func (drm *dynamicRESTMapper) KindsFor(resource schema.GroupVersionResource) ([] } func (drm *dynamicRESTMapper) ResourceFor(input schema.GroupVersionResource) (schema.GroupVersionResource, error) { - if err := drm.init(); err != nil { - return schema.GroupVersionResource{}, err - } - var gvr schema.GroupVersionResource err := drm.checkAndReload(func() error { var err error @@ -238,9 +221,6 @@ func (drm *dynamicRESTMapper) ResourceFor(input schema.GroupVersionResource) (sc } func (drm *dynamicRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([]schema.GroupVersionResource, error) { - if err := drm.init(); err != nil { - return nil, err - } var gvrs []schema.GroupVersionResource err := drm.checkAndReload(func() error { var err error @@ -251,9 +231,6 @@ func (drm *dynamicRESTMapper) ResourcesFor(input schema.GroupVersionResource) ([ } func (drm *dynamicRESTMapper) RESTMapping(gk schema.GroupKind, versions ...string) (*meta.RESTMapping, error) { - if err := drm.init(); err != nil { - return nil, err - } var mapping *meta.RESTMapping err := drm.checkAndReload(func() error { var err error @@ -264,9 +241,6 @@ func (drm *dynamicRESTMapper) RESTMapping(gk schema.GroupKind, versions ...strin } func (drm *dynamicRESTMapper) RESTMappings(gk schema.GroupKind, versions ...string) ([]*meta.RESTMapping, error) { - if err := drm.init(); err != nil { - return nil, err - } var mappings []*meta.RESTMapping err := drm.checkAndReload(func() error { var err error @@ -277,9 +251,6 @@ func (drm *dynamicRESTMapper) RESTMappings(gk schema.GroupKind, versions ...stri } func (drm *dynamicRESTMapper) ResourceSingularizer(resource string) (string, error) { - if err := drm.init(); err != nil { - return "", err - } var singular string err := drm.checkAndReload(func() error { var err error diff --git a/pkg/client/apiutil/dynamicrestmapper_test.go b/pkg/client/apiutil/dynamicrestmapper_test.go index 51c8f3ca4b..10fafb825f 100644 --- a/pkg/client/apiutil/dynamicrestmapper_test.go +++ b/pkg/client/apiutil/dynamicrestmapper_test.go @@ -17,7 +17,6 @@ limitations under the License. package apiutil import ( - "fmt" "time" . "github.com/onsi/ginkgo/v2" @@ -145,27 +144,6 @@ var _ = Describe("Dynamic REST Mapper", func() { By("ensuring that it was only refreshed once") Expect(count).To(Equal(1)) }) - - It("should lazily initialize if the lazy option is used", func() { - var err error - var failedOnce bool - mockErr := fmt.Errorf("mock failed once") - mapper, err = NewDynamicRESTMapper(cfg, WithLazyDiscovery, WithCustomMapper(func() (meta.RESTMapper, error) { - // Make newMapper fail once - if !failedOnce { - failedOnce = true - return nil, mockErr - } - baseMapper := meta.NewDefaultRESTMapper(nil) - addToMapper(baseMapper) - return baseMapper, nil - })) - Expect(err).NotTo(HaveOccurred()) - Expect(mapper.(*dynamicRESTMapper).staticMapper).To(BeNil()) - - Expect(callWithTarget()).To(MatchError(mockErr)) - Expect(callWithTarget()).To(Succeed()) - }) } Describe("KindFor", func() {