replace shared-informers' methods with cached client #461
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -11,7 +11,6 @@ import ( | |||||||
| toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" | ||||||||
| "github.com/codeready-toolchain/registration-service/pkg/auth" | ||||||||
| "github.com/codeready-toolchain/registration-service/pkg/configuration" | ||||||||
| "github.com/codeready-toolchain/registration-service/pkg/log" | ||||||||
| "github.com/codeready-toolchain/registration-service/pkg/proxy" | ||||||||
| "github.com/codeready-toolchain/registration-service/pkg/proxy/metrics" | ||||||||
|
|
@@ -85,12 +84,7 @@ func main() { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| app, err := server.NewInClusterApplication(cl) | ||||||||
| if err != nil { | ||||||||
| panic(err.Error()) | ||||||||
| } | ||||||||
|
|
@@ -121,11 +115,6 @@ func main() { | |||||||
| } | ||||||||
| proxySrv := p.StartProxy(proxy.DefaultPort) | ||||||||
|
|
||||||||
| // --------------------------------------------- | ||||||||
| // Registration Service | ||||||||
| // --------------------------------------------- | ||||||||
|
|
@@ -202,9 +191,22 @@ func newCachedClient(ctx context.Context, cfg *rest.Config) (client.Client, erro | |||||||
|
|
||||||||
| // populate the cache backed by shared informers that are initialized lazily on the first call | ||||||||
| // for the given GVK with all resources we are interested in from the host-operator namespace | ||||||||
| objectsToList := map[string]client.ObjectList{ | ||||||||
| "MasterUserRecord": &toolchainv1alpha1.MasterUserRecordList{}, | ||||||||
| "Space": &toolchainv1alpha1.SpaceList{}, | ||||||||
| "SpaceBinding": &toolchainv1alpha1.SpaceBindingList{}, | ||||||||
| "ToolchainStatus": &toolchainv1alpha1.ToolchainStatusList{}, | ||||||||
| "UserSignup": &toolchainv1alpha1.UserSignupList{}, | ||||||||
| "ProxyPlugin": &toolchainv1alpha1.ProxyPluginList{}, | ||||||||
| "NSTemplateTier": &toolchainv1alpha1.NSTemplateTierList{}, | ||||||||
| "ToolchainConfig": &toolchainv1alpha1.ToolchainConfigList{}, | ||||||||
| "BannedUser": &toolchainv1alpha1.BannedUserList{}, | ||||||||
| "Secret": &corev1.SecretList{}} | ||||||||
|
There was a problem hiding this comment.
Suggested change
While I don't see we actually access CMs in reg-service it's listed here: There was a problem hiding this comment. I believe Matous added these resources based on the informers we were creating in informers.go. If my memory serves me correctly, the list of resources there are those that are needed in the proxy flow and so we pre-populate the cache for those resources so that the proxy is as fast as possible even on first time use by a user. I don't remember whether ConfigMaps are part of that flow but I don't think it hurts to add it. There was a problem hiding this comment. @rajivnathan is correct, we want to cache only those resources we access in reg-service. Caching anything else doesn't make much sense. We could theoretically drop a list of some reources like Secrets, NSTemplateTiers, ProxyPlugins, ToolchainStatuses, and ToolchainConfigs because we don't expect many resources to be present in the namespace, so the very first call wouldn't take much more time compared to when it is already cached. But I thought that it's also completely fine to pre-populate the cache with all resources the reg-service and proxy touch. There was a problem hiding this comment. As for the ConfigMap, if I'm not mistaken, the reg-service doesn't touch any ConfigMap in the toolchain-host-operator namespace. The permission in the There was a problem hiding this comment. Cache can be crucial for performance. Especially for the proxy. And it's easy to miss this (don't add specific resources to the initialization step) during development. We don't have any performance tests in our CI. I checked it looks like the CM are not used by reg-service and proxy. So let's create a separate PR to remove it from the role. |
||||||||
|
|
||||||||
| for resourceName := range objectsToList { | ||||||||
| log.Infof(nil, "Syncing informer cache with %s resources", resourceName) | ||||||||
| if err := hostCluster.GetClient().List(ctx, objectsToList[resourceName], client.InNamespace(configuration.Namespace())); err != nil { | ||||||||
| log.Errorf(nil, err, "Informer cache sync failed for %s", resourceName) | ||||||||
| return nil, err | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how we could make it easier to NOT forget to update this list when we need to access more resource kinds via the client... Maybe add a comment to https://github.com/codeready-toolchain/host-operator/blob/master/deploy/registration-service/registration-service.yaml#L20 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We list only those objects we want to cache upfront. As it is mentioned in the comment above this code, the cache works in a lazy way so it starts caching GVK resources only after it is fetched for the first time.
In other words, the cache is populated only after making the first call on the GVK resource.
If we didn't do the list here, then the very first call to the reg-service/proxy would take a lot of time because the client would start populating the cache with the resources.
By listing the resources before the reg-service pod gets ready, we ensure that the cache is fully synced with all resources we need to have cached.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got that. My point is that it's easy to forget to add a new resource to this list. Let's say we are introducing some change to the proxy so it now needs to access a new resource with kind
X. If we don't add it to this list then the proxy performance may degrade for the first request. It will degrade again after the proxy is restarted for every replica. So for example if there is 10 replicas of reg-service/proxy then up to 10 requests per deployment/version would be impacted.You can't access a new kind without adding this
Xresource to the operator role in https://github.com/codeready-toolchain/host-operator/blob/master/deploy/registration-service/registration-service.yaml#L20So I was wondering if that yaml is a good place to add a reminder to update the cache initialization if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in the call:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for 1. and 2. see the PRs:
codeready-toolchain/host-operator#1089
#463
As for the 3. item, it's easily doable to go over all kinds of the toolchain api, but there is one problem. The scheme contains api of both host & member, and at the level of the scheme we cannot decide which one is the host resource and which one belongs to the member. We could list available api groups from the cluster and exclude those kinds that are not available, but this wouldn't work in single-cluster environments. In addition to that, host-operator SA doesn't have permission to read/list member-operator CRDs, so it would fail on authorization as well.
In other words, there are too many complications in doing it in a generic way so it's not worth it.