cache: store namespace names instead of metadata #62
Conversation
Instead of caching namespace metadata, we can instead only cache the namespaces' names and pull their definitions from the client cache. This saves some memory at the expense of a minor increase in response latency (benchmarks indicated on the order of a few extra milliseconds). Signed-off-by: Andy Sadler <[email protected]>
sadlerap
force-pushed
the
cache-improvements
branch
from
d9993de to
4d85759
Compare
| namespace := corev1.Namespace{} | ||
| err := s.namespaceLister.Get(ctx, types.NamespacedName{Name: ns}, &namespace) | ||
| if err != nil { | ||
| slog.Error("failed to retrieve namespace", "namespace", ns, "err", err) |
There was a problem hiding this comment.
this is using the default logger, we should use s.logger instead
| slog.Error("failed to retrieve namespace", "namespace", ns, "err", err) | |
| s.logger.Error("failed to retrieve namespace", "namespace", ns, "err", err) |
| @@ -179,3 +185,21 @@ func (s *SynchronizedAccessCache) Start(ctx context.Context) { | |||
| }() | |||
| }) | |||
| } | |||
|
|
|||
| func (s *SynchronizedAccessCache) List(ctx context.Context, subject rbacv1.Subject) []corev1.Namespace { | |||
There was a problem hiding this comment.
I think this logic should be moved to AccessCache
There was a problem hiding this comment.
I don't agree. I think the AccessCache should solely be concerned with storage. SynchronizedAccessCache, meanwhile, should be able to provide the additional features we need in our cache, relying on AccessCache to provide synchronization.
In other words: I don't see the benefit of pushing this down a layer.
| if _, exists := c[s]; !exists { | ||
| c[s] = sets.New(ns.Name) | ||
| } else { | ||
| c[s].Insert(ns.Name) | ||
| } |
There was a problem hiding this comment.
if we rely on sets here, we can remove the removeDuplicateSubjects function and its invocation at L92
| c[s] = append(c[s], ns) | ||
| if _, exists := c[s]; !exists { | ||
| c[s] = sets.New(ns.Name) | ||
| } else { |
There was a problem hiding this comment.
do you think we can remove this else somehow?
There was a problem hiding this comment.
AIUI no, not directly. I was running into issues with zero-initialized sets, and my understanding is that the constructor (New) needs to be invoked to have a valid data structure.
That said, I may be able to refactor this into two passes, one that makes the sets and one that populates them.
|
|
||
| for _, ns := range namespaces { | ||
| namespace := corev1.Namespace{} | ||
| err := s.namespaceLister.Get(ctx, types.NamespacedName{Name: ns}, &namespace) |
There was a problem hiding this comment.
this way we are getting access from the AccessCache and namespaces from the ResourcesCache. This is coupling the List operation to the two caches.
It may happen that the namespaces in the ResourcesCache were updated before the access in the AccessCache and this may introduce some errors.
I'd suggest to cache the namespaces used during the synch operation in the AccessCache, so that we always provide consistent results.
There was a problem hiding this comment.
Also if we cache namespaces in the AccessCache, we can implement a slightly more performant version of this algorithm by sorting namespaces ahead-of-time (cache Synch/Restock) and using binary search just-in-time (List)
Instead of caching namespace metadata, we can instead only cache the namespaces' names and pull their definitions from the client cache. This saves some memory at the expense of a minor increase in response latency (benchmarks indicated on the order of a few extra milliseconds).