add: caching for Discovery Client #682
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| if err = discovery.InitializeGlobalDiscoveryClient(restConfig); err != nil { | ||
| klog.Errorf("Error initializing global discovery client: %s", err) | ||
| } else { | ||
| klog.Infof("Initializing global discovery client") |
There was a problem hiding this comment.
I think adding the log level here makes sense as this is informational, so can be filtered out at other log levels.
| klog.Infof("Initializing global discovery client") | |
| klog.V(4).Infof("Initializing global discovery client") |
There was a problem hiding this comment.
Hey Fiona,
Thanks for the review. I'll update the log :)
The refresh timeframe set to 5 hours is an arbitrary number, I am aiming for a timeframe which both promotes the efficiency of the cache, whist not holding cache for too long, therefor I chose to half that of the controller-runtime resync period of 10 hours.
Although I am open for discussion if you have any suggestions towards the timeframe :)
There was a problem hiding this comment.
Just thinking about it, and how it would work. From what I understand the client only initializes once, and then refreshes after 5 hours. If resources are created soon after a refresh, will they then only be available for manipulation after the next refresh?
There was a problem hiding this comment.
If you query for an object type which doesn't exist in the cache it would cause a cache miss and invalidation triggering queries to the k8 discovery client to repopulate the cache.
There was a problem hiding this comment.
Thanks Kevin. So if a new object of an object type that does exist is created, this will be discoverable?
There was a problem hiding this comment.
Yes, but the discovery client cache doesn't cache individual k8 objects. As I understand it, the Discovery Client is the piece of k8 which resolves names such as pod or po into a fully resolved kubernetes API endpoint. Without a cache, every time one tries to get a pod from kubernetes it will iterate through the discovery API until it finds an entry with that name. This means that in the worst case that every resource will be checked using a GET request which can cause client throttling. Without a cache this would happen every time.
There was a problem hiding this comment.
Thanks for explaining!
|
Hey @KPostOffice, |
|
@VanillaSpoon I'm unclear on exactly how the disk cache works if I'm being honest, because as part of the struct it wraps the memcache which makes me think that most requests will still go through the memcache. |
|
|
||
| func RefreshDiscoveryCache() { | ||
| klog.Infof("Invalidating discovery cache") | ||
| GlobalCachedDiscoveryClient.Invalidate() |
There was a problem hiding this comment.
Perhaps we should have some error handling or logging in case any errors arise when attempting to invalidate the discovery cache? WDYT
There was a problem hiding this comment.
Invalidate doesn't return anything so I don't think this is possible
There was a problem hiding this comment.
Gotcha, you're right! Thanks Kevin.
Issue link
closes #611
What changes have been made
Adding a Discovery Client cache, which will reduce the calls to the Discovery Client, and prevent throttling. The cache is then refreshed by a ticker every 5 hours.
Checks