feat(kuma-cp) k8s performance optimisation #1113
Conversation
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]> # Conflicts: # app/kuma-cp/cmd/run.go
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]>
| */ | ||
|
|
||
| // Package cache is a copy of the package sigs.k8s.io/controller-runtime/pkg/cache | ||
| // but without deepCopy on Get and List methods |
There was a problem hiding this comment.
Although I understand the importance of having this K8S cache with modifications I want to discuss the long term strategy. I find such copy paste hard to maintain. Action items that IMHO needs to be applied here
- Provide a more explanation why deepCopy was removed
- Clearly mark fragments in the code that were modified for example
// Kuma modification start
// Kuma modification end
- Please start the conversation with Kubernetes community and let's try to put this in upstream. I don't expect this to be easy, but let's at least try to raise the problem.
- We need to remember about this when upgrading Kubernetes library. Any ideas?
| runtime.SetClusterId(clusterId) | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
What do you think of putting this in defaults? I'd love to avoid having a separate package in pkg just for this.
There was a problem hiding this comment.
I did so, but then figured out defaults and clusterid work in different modes, defaults runs everywhere, clusterid runs only in Global and Standalone mode.
I don't mind having clusterid as a separate component (even if it's tiny). I think we can create dir pkg/components and move all components here. Every component should have func Setup(rt runtime.Runtime) error function, own logger, documentation, and component's level tests. That's pretty much what we have today but it's not really ordered
There was a problem hiding this comment.
I don't know if you remember my presentation on one of the community call, but I also wanted to segment into a couple of main packages (util, core, api, components, plugins), where there are clear rules about dependencies between all packages. Components leave in separate components package. The main thing that I would like to see is to have 0 dependencies between components. They should be like separate processes that can be pulled off from the code.
https://docs.google.com/presentation/d/1EyEGpji6UHFYPyD4krORcIsD8aubDE9GXfDVlhhUNOo/edit?usp=sharing
but in terms of clusterid and defaults. Look that signing key is created also in global and standalone, but I put this in defaults. If you think about this, it's a matter of where you put the "if".
- Put if whether to run small component
- Put if in bigger component that creates defaults.
I prefer the second one, in this case, I think we "overpackage" many places in the codebase.
But I leave your judgment on which one to do, I'm not strongly attached to the second option.
There was a problem hiding this comment.
Yeah, of course, I remember your presentation. I totally get your idea about bigger components with less packaging. But unlike signingKey this clusterid doesn't really have "defaulting" functionality. It's more like "cluster init" functionality. So I'm not sure it belongs here.
There was a problem hiding this comment.
ClusterID is the same "default" as Signing Key which is a random value.
Signed-off-by: Ilya Lobkov <[email protected]>
Signed-off-by: Ilya Lobkov <[email protected]> # Conflicts: # app/kuma-cp/cmd/run.go
Signed-off-by: Ilya Lobkov <[email protected]>
Summary
Add cached k8s client
Kuma CP did an unlimited number of requests to Kubernetes API Server. That was leading to the "throttling" of Kubernetes API Server. Cached K8s client solves the problem. But default Cache is inefficient from a memory usage perspective. Every Get/List request returns a deep copy. I copied this Cache to Kuma codebase and got rid of
deepCopy(). Also, since cached k8s client needs to be explicitly started (as a Component) I moved ClusterId creation into a separate component.Add cache for marshaled resources
Alongside
SimpleConverterI addedCachedConverterwhich caches Marshal operation. We can't use it for webhooks because they take objects from Request (not from the Store) and these objects don't have ResourceVersion which makes such caching impossible.Documentation