From 2510906413f6d15351ab959e2227d8091a6d51e2 Mon Sep 17 00:00:00 2001 From: Donnie Adams Date: Mon, 27 Jan 2025 08:17:35 -0500 Subject: [PATCH] chore: small performance updates Allow client operations to be cached, but not add triggers. Use a map instead of a slice for storing trigger matchers for quicker retrieval. Signed-off-by: Donnie Adams --- pkg/router/handler.go | 4 +- pkg/router/matcher.go | 11 +++ pkg/router/tester/types.go | 6 +- pkg/router/trigger.go | 42 +++++++----- pkg/runtime/backend.go | 4 +- pkg/runtime/cached.go | 58 +++++++++++----- pkg/runtime/multi/client.go | 4 +- .../untriggered.go} | 68 ++++++++++++++----- 8 files changed, 137 insertions(+), 60 deletions(-) rename pkg/{uncached/uncached.go => untriggered/untriggered.go} (62%) diff --git a/pkg/router/handler.go b/pkg/router/handler.go index 79334e4..7f9363f 100644 --- a/pkg/router/handler.go +++ b/pkg/router/handler.go @@ -59,7 +59,7 @@ func NewHandlerSet(name string, scheme *runtime.Scheme, backend backend.Backend) handlers: map[schema.GroupVersionKind][]Handler{}, }, triggers: triggers{ - matchers: map[schema.GroupVersionKind]map[enqueueTarget][]objectMatcher{}, + matchers: map[schema.GroupVersionKind]map[enqueueTarget]map[string]objectMatcher{}, trigger: backend, gvkLookup: backend, scheme: scheme, @@ -317,7 +317,7 @@ func (m *HandlerSet) handle(gvk schema.GroupVersionKind, key string, unmodifiedO } if unmodifiedObject == nil { - // A nil object here means tha the object was deleted, so unregister the triggers + // A nil object here means that the object was deleted, so unregister the triggers m.triggers.UnregisterAndTrigger(req) } else { m.triggers.Trigger(req) diff --git a/pkg/router/matcher.go b/pkg/router/matcher.go index a764438..5c69c7e 100644 --- a/pkg/router/matcher.go +++ b/pkg/router/matcher.go @@ -13,6 +13,17 @@ type objectMatcher struct { Fields fields.Selector } +func (o *objectMatcher) String() string { + s := o.Name + "/" + o.Namespace + if o.Selector != nil { + s += "/label selectors" + o.Selector.String() + } + if o.Fields != nil { + s += "/field selectors" + o.Fields.String() + } + return s +} + func (o *objectMatcher) Equals(other objectMatcher) bool { if o.Name != other.Name { return false diff --git a/pkg/router/tester/types.go b/pkg/router/tester/types.go index fcfb6fa..e358bdb 100644 --- a/pkg/router/tester/types.go +++ b/pkg/router/tester/types.go @@ -9,7 +9,7 @@ import ( "github.com/google/uuid" "github.com/obot-platform/nah/pkg/router" - "github.com/obot-platform/nah/pkg/uncached" + "github.com/obot-platform/nah/pkg/untriggered" "golang.org/x/exp/maps" "k8s.io/apimachinery/pkg/api/errors" meta2 "k8s.io/apimachinery/pkg/api/meta" @@ -38,7 +38,7 @@ func (c Client) objects() []kclient.Object { } func (c *Client) Get(ctx context.Context, key kclient.ObjectKey, out kclient.Object, opts ...kclient.GetOption) error { - if u, ok := out.(*uncached.Holder); ok { + if u, ok := out.(*untriggered.Holder); ok { out = u.Object } t := reflect.TypeOf(out) @@ -73,7 +73,7 @@ func copy(dest, src kclient.Object) { } func (c *Client) List(ctx context.Context, objList kclient.ObjectList, opts ...kclient.ListOption) error { - if u, ok := objList.(*uncached.HolderList); ok { + if u, ok := objList.(*untriggered.HolderList); ok { objList = u.ObjectList } diff --git a/pkg/router/trigger.go b/pkg/router/trigger.go index c4882a9..8665332 100644 --- a/pkg/router/trigger.go +++ b/pkg/router/trigger.go @@ -6,7 +6,7 @@ import ( "github.com/obot-platform/nah/pkg/backend" "github.com/obot-platform/nah/pkg/log" - "github.com/obot-platform/nah/pkg/uncached" + "github.com/obot-platform/nah/pkg/untriggered" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" @@ -16,7 +16,7 @@ import ( type triggers struct { lock sync.RWMutex - matchers map[schema.GroupVersionKind]map[enqueueTarget][]objectMatcher + matchers map[schema.GroupVersionKind]map[enqueueTarget]map[string]objectMatcher trigger backend.Trigger gvkLookup backend.Backend scheme *runtime.Scheme @@ -36,15 +36,15 @@ func (m *triggers) invokeTriggers(req Request) { m.lock.RLock() defer m.lock.RUnlock() - for enqueueTarget, matchers := range m.matchers[req.GVK] { - if enqueueTarget.gvk == req.GVK && - enqueueTarget.key == req.Key { + for et, matchers := range m.matchers[req.GVK] { + if et.gvk == req.GVK && + et.key == req.Key { continue } for _, matcher := range matchers { if matcher.Match(req.Namespace, req.Name, req.Object) { - log.Debugf("Triggering [%s] [%v] from [%s] [%v]", enqueueTarget.key, enqueueTarget.gvk, req.Key, req.GVK) - _ = m.trigger.Trigger(enqueueTarget.gvk, enqueueTarget.key, 0) + log.Debugf("Triggering [%s] [%v] from [%s] [%v]", et.key, et.gvk, req.Key, req.GVK) + _ = m.trigger.Trigger(et.gvk, et.key, 0) break } } @@ -61,15 +61,20 @@ func (m *triggers) register(gvk schema.GroupVersionKind, key string, targetGVK s } matchers, ok := m.matchers[targetGVK] if !ok { - matchers = map[enqueueTarget][]objectMatcher{} + matchers = map[enqueueTarget]map[string]objectMatcher{} m.matchers[targetGVK] = matchers } - for _, existing := range matchers[target] { - if existing.Equals(mr) { - return - } + + matcherKey := mr.String() + if _, ok := matchers[target][matcherKey]; ok { + return + } + + if matchers[target] == nil { + matchers[target] = map[string]objectMatcher{} } - matchers[target] = append(matchers[target], mr) + + matchers[target][matcherKey] = mr } func (m *triggers) Trigger(req Request) { @@ -79,7 +84,7 @@ func (m *triggers) Trigger(req Request) { } func (m *triggers) Register(sourceGVK schema.GroupVersionKind, key string, obj runtime.Object, namespace, name string, selector labels.Selector, fields fields.Selector) (schema.GroupVersionKind, bool, error) { - if uncached.IsWrapped(obj) { + if untriggered.IsWrapped(obj) { return schema.GroupVersionKind{}, false, nil } gvk, err := m.gvkLookup.GVKForObject(obj, m.scheme) @@ -107,7 +112,7 @@ func (m *triggers) UnregisterAndTrigger(req Request) { m.lock.Lock() defer m.lock.Unlock() - remainingMatchers := map[schema.GroupVersionKind]map[enqueueTarget][]objectMatcher{} + remainingMatchers := map[schema.GroupVersionKind]map[enqueueTarget]map[string]objectMatcher{} for targetGVK, matchers := range m.matchers { for target, mts := range matchers { @@ -119,9 +124,12 @@ func (m *triggers) UnregisterAndTrigger(req Request) { if targetGVK != req.GVK || mt.Namespace != req.Namespace || mt.Name != req.Name { // If the matcher matches the deleted object exactly, then skip the matcher. if remainingMatchers[targetGVK] == nil { - remainingMatchers[targetGVK] = make(map[enqueueTarget][]objectMatcher) + remainingMatchers[targetGVK] = make(map[enqueueTarget]map[string]objectMatcher) + } + if remainingMatchers[targetGVK][target] == nil { + remainingMatchers[targetGVK][target] = make(map[string]objectMatcher) } - remainingMatchers[targetGVK][target] = append(remainingMatchers[targetGVK][target], mt) + remainingMatchers[targetGVK][target][mt.String()] = mt } if targetGVK == req.GVK && mt.Match(req.Namespace, req.Name, req.Object) { log.Debugf("Triggering [%s] [%v] from [%s] [%v] on delete", target.key, target.gvk, req.Key, req.GVK) diff --git a/pkg/runtime/backend.go b/pkg/runtime/backend.go index a7ac092..b7c9119 100644 --- a/pkg/runtime/backend.go +++ b/pkg/runtime/backend.go @@ -12,7 +12,7 @@ import ( "github.com/obot-platform/nah/pkg/backend" "github.com/obot-platform/nah/pkg/fields" "github.com/obot-platform/nah/pkg/router" - "github.com/obot-platform/nah/pkg/uncached" + "github.com/obot-platform/nah/pkg/untriggered" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" kcache "k8s.io/client-go/tools/cache" @@ -167,7 +167,7 @@ func (b *Backend) IsObjectNamespaced(obj runtime.Object) (bool, error) { } func (b *Backend) GVKForObject(obj runtime.Object, scheme *runtime.Scheme) (schema.GroupVersionKind, error) { - return apiutil.GVKForObject(uncached.Unwrap(obj), scheme) + return apiutil.GVKForObject(untriggered.Unwrap(obj), scheme) } func (b *Backend) IndexField(ctx context.Context, obj kclient.Object, field string, extractValue kclient.IndexerFunc) error { diff --git a/pkg/runtime/cached.go b/pkg/runtime/cached.go index 7d07e08..c43b382 100644 --- a/pkg/runtime/cached.go +++ b/pkg/runtime/cached.go @@ -7,7 +7,7 @@ import ( "sync" "time" - "github.com/obot-platform/nah/pkg/uncached" + "github.com/obot-platform/nah/pkg/untriggered" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" @@ -115,8 +115,11 @@ func (c *cacheClient) store(obj kclient.Object) { } func (c *cacheClient) Get(ctx context.Context, key kclient.ObjectKey, obj kclient.Object, opts ...kclient.GetOption) error { - if u, ok := obj.(*uncached.Holder); ok { - return c.uncached.Get(ctx, key, u.Object, opts...) + if u, ok := obj.(*untriggered.Holder); ok { + obj = u.Object + if u.IsUncached() { + return c.uncached.Get(ctx, key, obj, opts...) + } } getErr := c.cached.Get(ctx, key, obj) @@ -155,15 +158,21 @@ func (c *cacheClient) Get(ctx context.Context, key kclient.ObjectKey, obj kclien } func (c *cacheClient) List(ctx context.Context, list kclient.ObjectList, opts ...kclient.ListOption) error { - if u, ok := list.(*uncached.HolderList); ok { - return c.uncached.List(ctx, u.ObjectList, opts...) + if u, ok := list.(*untriggered.HolderList); ok { + list = u.ObjectList + if u.IsUncached() { + return c.uncached.List(ctx, u, opts...) + } } return c.cached.List(ctx, list, opts...) } func (c *cacheClient) Create(ctx context.Context, obj kclient.Object, opts ...kclient.CreateOption) error { - if u, ok := obj.(*uncached.Holder); ok { - return c.uncached.Create(ctx, u.Object, opts...) + if u, ok := obj.(*untriggered.Holder); ok { + obj = u.Object + if u.IsUncached() { + return c.uncached.Create(ctx, obj, opts...) + } } err := c.cached.Create(ctx, obj, opts...) if err != nil { @@ -174,8 +183,11 @@ func (c *cacheClient) Create(ctx context.Context, obj kclient.Object, opts ...kc } func (c *cacheClient) Delete(ctx context.Context, obj kclient.Object, opts ...kclient.DeleteOption) error { - if u, ok := obj.(*uncached.Holder); ok { - return c.uncached.Delete(ctx, u.Object, opts...) + if u, ok := obj.(*untriggered.Holder); ok { + obj = u.Object + if u.IsUncached() { + return c.uncached.Delete(ctx, obj, opts...) + } } err := c.cached.Delete(ctx, obj, opts...) if err != nil { @@ -186,8 +198,11 @@ func (c *cacheClient) Delete(ctx context.Context, obj kclient.Object, opts ...kc } func (c *cacheClient) Update(ctx context.Context, obj kclient.Object, opts ...kclient.UpdateOption) error { - if u, ok := obj.(*uncached.Holder); ok { - return c.uncached.Update(ctx, u.Object, opts...) + if u, ok := obj.(*untriggered.Holder); ok { + obj = u.Object + if u.IsUncached() { + return c.uncached.Update(ctx, obj, opts...) + } } err := c.cached.Update(ctx, obj, opts...) if err != nil { @@ -198,8 +213,11 @@ func (c *cacheClient) Update(ctx context.Context, obj kclient.Object, opts ...kc } func (c *cacheClient) Patch(ctx context.Context, obj kclient.Object, patch kclient.Patch, opts ...kclient.PatchOption) error { - if u, ok := obj.(*uncached.Holder); ok { - return c.uncached.Patch(ctx, u.Object, patch, opts...) + if u, ok := obj.(*untriggered.Holder); ok { + obj = u.Object + if u.IsUncached() { + return c.uncached.Patch(ctx, obj, patch, opts...) + } } err := c.cached.Patch(ctx, obj, patch, opts...) if err != nil { @@ -210,6 +228,12 @@ func (c *cacheClient) Patch(ctx context.Context, obj kclient.Object, patch kclie } func (c *cacheClient) DeleteAllOf(ctx context.Context, obj kclient.Object, opts ...kclient.DeleteAllOfOption) error { + if u, ok := obj.(*untriggered.Holder); ok { + obj = u.Object + if u.IsUncached() { + return c.uncached.DeleteAllOf(ctx, obj, opts...) + } + } return c.cached.DeleteAllOf(ctx, obj, opts...) } @@ -248,15 +272,15 @@ type subResourceClient struct { } func (s *subResourceClient) Get(ctx context.Context, obj kclient.Object, subResource kclient.Object, opts ...kclient.SubResourceGetOption) error { - return s.reader.Get(ctx, uncached.Unwrap(obj).(kclient.Object), subResource, opts...) + return s.reader.Get(ctx, untriggered.Unwrap(obj).(kclient.Object), subResource, opts...) } func (s *subResourceClient) Create(ctx context.Context, obj kclient.Object, subResource kclient.Object, opts ...kclient.SubResourceCreateOption) error { - return s.writer.Create(ctx, uncached.Unwrap(obj).(kclient.Object), subResource, opts...) + return s.writer.Create(ctx, untriggered.Unwrap(obj).(kclient.Object), subResource, opts...) } func (s *subResourceClient) Update(ctx context.Context, obj kclient.Object, opts ...kclient.SubResourceUpdateOption) error { - err := s.writer.Update(ctx, uncached.Unwrap(obj).(kclient.Object), opts...) + err := s.writer.Update(ctx, untriggered.Unwrap(obj).(kclient.Object), opts...) if err != nil { return err } @@ -265,7 +289,7 @@ func (s *subResourceClient) Update(ctx context.Context, obj kclient.Object, opts } func (s *subResourceClient) Patch(ctx context.Context, obj kclient.Object, patch kclient.Patch, opts ...kclient.SubResourcePatchOption) error { - err := s.writer.Patch(ctx, uncached.Unwrap(obj).(kclient.Object), patch, opts...) + err := s.writer.Patch(ctx, untriggered.Unwrap(obj).(kclient.Object), patch, opts...) if err != nil { return err } diff --git a/pkg/runtime/multi/client.go b/pkg/runtime/multi/client.go index 8ba8ce5..4fe04fe 100644 --- a/pkg/runtime/multi/client.go +++ b/pkg/runtime/multi/client.go @@ -4,7 +4,7 @@ import ( "context" "fmt" - "github.com/obot-platform/nah/pkg/uncached" + "github.com/obot-platform/nah/pkg/untriggered" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -155,7 +155,7 @@ func (m multiClient) Watch(ctx context.Context, obj kclient.ObjectList, opts ... } func (m multiClient) getClient(obj runtime.Object) (kclient.WithWatch, error) { - gvk, err := m.GroupVersionKindFor(uncached.Unwrap(obj)) + gvk, err := m.GroupVersionKindFor(untriggered.Unwrap(obj)) if err != nil { return nil, err } diff --git a/pkg/uncached/uncached.go b/pkg/untriggered/untriggered.go similarity index 62% rename from pkg/uncached/uncached.go rename to pkg/untriggered/untriggered.go index 7b8ebd3..333fcdc 100644 --- a/pkg/uncached/uncached.go +++ b/pkg/untriggered/untriggered.go @@ -1,22 +1,62 @@ -package uncached +package untriggered import ( "k8s.io/apimachinery/pkg/runtime" kclient "sigs.k8s.io/controller-runtime/pkg/client" ) +type Holder struct { + kclient.Object + uncached bool +} + +func (h *Holder) DeepCopyObject() runtime.Object { + return &Holder{Object: h.Object.DeepCopyObject().(kclient.Object), uncached: h.uncached} +} + +func (h *Holder) IsUncached() bool { + return h.uncached +} + +type HolderList struct { + kclient.ObjectList + uncached bool +} + +func (h *HolderList) DeepCopyObject() runtime.Object { + return &HolderList{ObjectList: h.ObjectList.DeepCopyObject().(kclient.ObjectList), uncached: h.uncached} +} + +func (h *HolderList) IsUncached() bool { + return h.uncached +} + func List(obj kclient.ObjectList) kclient.ObjectList { return &HolderList{ ObjectList: obj, } } +func UncachedList(obj kclient.ObjectList) kclient.ObjectList { + return &HolderList{ + ObjectList: obj, + uncached: true, + } +} + func Get(obj kclient.Object) kclient.Object { return &Holder{ Object: obj, } } +func UncachedGet(obj kclient.Object) kclient.Object { + return &Holder{ + Object: obj, + uncached: true, + } +} + func IsWrapped(obj runtime.Object) bool { if _, ok := obj.(*Holder); ok { return true @@ -27,6 +67,16 @@ func IsWrapped(obj runtime.Object) bool { return false } +func IsUncached(obj runtime.Object) bool { + if h, ok := obj.(*Holder); ok { + return h.uncached + } + if h, ok := obj.(*HolderList); ok { + return h.uncached + } + return false +} + func Unwrap(obj runtime.Object) runtime.Object { if h, ok := obj.(*Holder); ok { return h.Object @@ -43,19 +93,3 @@ func UnwrapList(obj kclient.ObjectList) kclient.ObjectList { } return obj } - -type Holder struct { - kclient.Object -} - -func (h *Holder) DeepCopyObject() runtime.Object { - return &Holder{Object: h.Object.DeepCopyObject().(kclient.Object)} -} - -type HolderList struct { - kclient.ObjectList -} - -func (h *HolderList) DeepCopyObject() runtime.Object { - return &HolderList{ObjectList: h.ObjectList.DeepCopyObject().(kclient.ObjectList)} -}