feat(instances): cache server lookups to reduce API calls#1252
feat(instances): cache server lookups to reduce API calls#1252lukasmetzner wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1252 +/- ##
==========================================
+ Coverage 71.01% 72.11% +1.10%
==========================================
Files 24 26 +2
Lines 2677 2851 +174
==========================================
+ Hits 1901 2056 +155
- Misses 604 618 +14
- Partials 172 177 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
lukasmetzner
force-pushed
the
fix-improve-api-usage
branch
from
7e8edf4 to
1d6ef5f
Compare
jooola
left a comment
jooola
left a comment
There was a problem hiding this comment.
First review! Looks great, only need a bit of polishing.
| nodeLister corelisters.NodeLister | ||
| client *hcloud.Client | ||
| robotClient hrobot.RobotClient | ||
| instanceCache *servercache.Cache[hcloud.Server] |
There was a problem hiding this comment.
| instanceCache *servercache.Cache[hcloud.Server] | |
| serverCache *servercache.Cache[hcloud.Server] |
| hcloudCacheMode = "HCLOUD_CACHE_MODE" | ||
| hcloudCacheTTL = "HCLOUD_CACHE_TTL" |
There was a problem hiding this comment.
Maybe we still want a namespace like "servers"? With "Instances" I understand the instance controller "instance_v2". While with "servers" I understand the API Servers.
We also probably have different caching options for the robot servers.
|
|
||
| if mode, ok := os.LookupEnv(hcloudCacheMode); ok { | ||
| klog.Warningf("Experimental: %s is experimental, breaking changes may occur within minor releases.", hcloudCacheMode) | ||
| cfg.Cache.Mode = servercache.Mode(mode) |
There was a problem hiding this comment.
Should we validate the value provided by the user?
| }, []string{"op"}) | ||
|
|
||
| CacheRequests = prometheus.NewCounterVec(prometheus.CounterOpts{ | ||
| Name: "cloud_controller_manager_cache_requests_total", |
There was a problem hiding this comment.
We probably want to say its a "server" cache.
| func (c *Cache[T]) getFromCache( | ||
| ctx context.Context, | ||
| subsystem string, | ||
| lookup func() *entry[T], | ||
| fetch func() (*T, error), | ||
| opts ...RefreshOption, | ||
| ) (*T, error) { | ||
| refreshOpts := newCacheRefreshOpts(c, opts...) |
There was a problem hiding this comment.
I am tempted to do the following for all functions that pass subsystem, just to unclutter the args:
| func (c *Cache[T]) getFromCache( | |
| ctx context.Context, | |
| subsystem string, | |
| lookup func() *entry[T], | |
| fetch func() (*T, error), | |
| opts ...RefreshOption, | |
| ) (*T, error) { | |
| refreshOpts := newCacheRefreshOpts(c, opts...) | |
| func (c *Cache[T]) getFromCache( | |
| ctx context.Context, | |
| lookup func() *entry[T], | |
| fetch func() (*T, error), | |
| opts ...RefreshOption, | |
| ) (*T, error) { | |
| subsystem := GetSubsystem(ctx) | |
| refreshOpts := newCacheRefreshOpts(c, opts...) |
And if a ctx is missing, we can/should probably add it.
| func (c *Cache[T]) refreshOne( | ||
| subsystem string, | ||
| fetch func() (*T, error), | ||
| ttl time.Duration, | ||
| ) error { |
There was a problem hiding this comment.
We might want to pass a ctx here:
- Have access to the subsystem, without passing
subsystemvia the function args. - Not a big fan of passing a context during in the function definition, instead of from the function caller. The context hierarchy is clearer when passed via the function caller.
| @@ -0,0 +1,26 @@ | |||
| # Instance Cache | |||
There was a problem hiding this comment.
| # Instance Cache | |
| # Server Cache |
2 participants
Feature is marked as experimental.