From 6692f683c78d9c929f2eeb7704f754d42415b3bb Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Wed, 27 Jan 2021 15:21:35 -0800 Subject: [PATCH 1/3] refactor: controller uses two level caching to reduce number of redis calls Signed-off-by: Alexander Matyushentsev --- .../commands/argocd_application_controller.go | 1 + util/cache/cache.go | 8 +++ util/cache/twolevelclient.go | 49 +++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 util/cache/twolevelclient.go diff --git a/cmd/argocd-application-controller/commands/argocd_application_controller.go b/cmd/argocd-application-controller/commands/argocd_application_controller.go index 3f2d8c25c3bd5..04e2b515050eb 100644 --- a/cmd/argocd-application-controller/commands/argocd_application_controller.go +++ b/cmd/argocd-application-controller/commands/argocd_application_controller.go @@ -77,6 +77,7 @@ func NewCommand() *cobra.Command { cache, err := cacheSrc() errors.CheckError(err) + cache.Cache.SetClient(cacheutil.NewTwoLevelClient(cache.Cache.GetClient(), time.Hour)) settingsMgr := settings.NewSettingsManager(ctx, kubeClient, namespace) kubectl := kubeutil.NewKubectl() diff --git a/util/cache/cache.go b/util/cache/cache.go index 89015e7b5faf2..95322092c6ae6 100644 --- a/util/cache/cache.go +++ b/util/cache/cache.go @@ -78,6 +78,14 @@ type Cache struct { client CacheClient } +func (c *Cache) GetClient() CacheClient { + return c.client +} + +func (c *Cache) SetClient(client CacheClient) { + c.client = client +} + func (c *Cache) SetItem(key string, item interface{}, expiration time.Duration, delete bool) error { key = fmt.Sprintf("%s|%s", key, common.CacheVersion) if delete { diff --git a/util/cache/twolevelclient.go b/util/cache/twolevelclient.go new file mode 100644 index 0000000000000..f7863d1d9eb18 --- /dev/null +++ b/util/cache/twolevelclient.go @@ -0,0 +1,49 @@ +package cache + +import ( + "context" + "time" +) + +func NewTwoLevelClient(client CacheClient, inMemoryExpiration time.Duration) *twoLevelClient { + return &twoLevelClient{inMemoryCache: NewInMemoryCache(inMemoryExpiration), client: client} +} + +type twoLevelClient struct { + inMemoryCache CacheClient + client CacheClient +} + +func (c *twoLevelClient) Set(item *Item) error { + _ = c.inMemoryCache.Set(item) + return c.client.Set(item) +} + +func (c *twoLevelClient) Get(key string, obj interface{}) error { + err := c.inMemoryCache.Get(key, obj) + if err == nil { + return nil + } + + err = c.client.Get(key, obj) + if err == nil { + _ = c.inMemoryCache.Set(&Item{Key: key, Object: obj}) + } + return err +} + +func (c *twoLevelClient) Delete(key string) error { + err := c.inMemoryCache.Delete(key) + if err != nil { + return err + } + return c.client.Delete(key) +} + +func (c *twoLevelClient) OnUpdated(ctx context.Context, key string, callback func() error) error { + return c.client.OnUpdated(ctx, key, callback) +} + +func (c *twoLevelClient) NotifyUpdated(key string) error { + return c.client.NotifyUpdated(key) +} From 6150cbb4e60c5824a37bc9605127d3b05703c10b Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Thu, 28 Jan 2021 09:44:04 -0800 Subject: [PATCH 2/3] prevent saving the same app cache to redis Signed-off-by: Alexander Matyushentsev --- .../commands/argocd_application_controller.go | 2 +- pkg/apis/application/v1alpha1/types.go | 20 +++++++++++++++++++ .../application-pod-view/pod-view.tsx | 3 ++- util/cache/appstate/cache.go | 7 +++++++ util/cache/inmemory.go | 15 ++++++++++++++ util/cache/twolevelclient.go | 5 ++++- 6 files changed, 49 insertions(+), 3 deletions(-) diff --git a/cmd/argocd-application-controller/commands/argocd_application_controller.go b/cmd/argocd-application-controller/commands/argocd_application_controller.go index 04e2b515050eb..272b186c1e05b 100644 --- a/cmd/argocd-application-controller/commands/argocd_application_controller.go +++ b/cmd/argocd-application-controller/commands/argocd_application_controller.go @@ -77,7 +77,7 @@ func NewCommand() *cobra.Command { cache, err := cacheSrc() errors.CheckError(err) - cache.Cache.SetClient(cacheutil.NewTwoLevelClient(cache.Cache.GetClient(), time.Hour)) + cache.Cache.SetClient(cacheutil.NewTwoLevelClient(cache.Cache.GetClient(), 10*time.Minute)) settingsMgr := settings.NewSettingsManager(ctx, kubeClient, namespace) kubectl := kubeutil.NewKubectl() diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index acf2394c6c7ef..2a8886e358322 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -985,6 +985,18 @@ type ApplicationTree struct { Hosts []HostInfo `json:"hosts,omitempty" protobuf:"bytes,3,rep,name=hosts"` } +func (t *ApplicationTree) Normalize() { + sort.Slice(t.Nodes, func(i, j int) bool { + return t.Nodes[i].FullName() < t.Nodes[j].FullName() + }) + sort.Slice(t.OrphanedNodes, func(i, j int) bool { + return t.OrphanedNodes[i].FullName() < t.OrphanedNodes[j].FullName() + }) + sort.Slice(t.Hosts, func(i, j int) bool { + return t.Hosts[i].Name < t.Hosts[j].Name + }) +} + type ApplicationSummary struct { // ExternalURLs holds all external URLs of application child resources. ExternalURLs []string `json:"externalURLs,omitempty" protobuf:"bytes,1,opt,name=externalURLs"` @@ -1053,6 +1065,10 @@ type ResourceNode struct { CreatedAt *metav1.Time `json:"createdAt,omitempty" protobuf:"bytes,8,opt,name=createdAt"` } +func (n *ResourceNode) FullName() string { + return fmt.Sprintf("%s/%s/%s/%s", n.Group, n.Kind, n.Namespace, n.Name) +} + func (n *ResourceNode) GroupKindVersion() schema.GroupVersionKind { return schema.GroupVersionKind{ Group: n.Group, @@ -1100,6 +1116,10 @@ type ResourceDiff struct { Modified bool `json:"modified,omitempty" protobuf:"bytes,12,opt,name=modified"` } +func (r *ResourceDiff) FullName() string { + return fmt.Sprintf("%s/%s/%s/%s", r.Group, r.Kind, r.Namespace, r.Name) +} + // ConnectionStatus represents connection status type ConnectionStatus = string diff --git a/ui/src/app/applications/components/application-pod-view/pod-view.tsx b/ui/src/app/applications/components/application-pod-view/pod-view.tsx index 3e8962d8941ee..1810bd558c834 100644 --- a/ui/src/app/applications/components/application-pod-view/pod-view.tsx +++ b/ui/src/app/applications/components/application-pod-view/pod-view.tsx @@ -303,7 +303,8 @@ export class PodView extends React.Component { renderMenu: () => this.props.nodeMenu(rnode) }; } - + }); + (tree.nodes || []).forEach((rnode: ResourceTreeNode) => { if (rnode.kind !== 'Pod') { return; } diff --git a/util/cache/appstate/cache.go b/util/cache/appstate/cache.go index 85d527dffd576..9c9cb90dd75c3 100644 --- a/util/cache/appstate/cache.go +++ b/util/cache/appstate/cache.go @@ -3,6 +3,7 @@ package appstate import ( "context" "fmt" + "sort" "time" "github.com/go-redis/redis/v8" @@ -61,6 +62,9 @@ func (c *Cache) GetAppManagedResources(appName string, res *[]*appv1.ResourceDif } func (c *Cache) SetAppManagedResources(appName string, managedResources []*appv1.ResourceDiff) error { + sort.Slice(managedResources, func(i, j int) bool { + return managedResources[i].FullName() < managedResources[j].FullName() + }) return c.SetItem(appManagedResourcesKey(appName), managedResources, c.appStateCacheExpiration, managedResources == nil) } @@ -82,6 +86,9 @@ func (c *Cache) OnAppResourcesTreeChanged(ctx context.Context, appName string, c } func (c *Cache) SetAppResourcesTree(appName string, resourcesTree *appv1.ApplicationTree) error { + if resourcesTree != nil { + resourcesTree.Normalize() + } err := c.SetItem(appResourcesTreeKey(appName), resourcesTree, c.appStateCacheExpiration, resourcesTree == nil) if err != nil { return err diff --git a/util/cache/inmemory.go b/util/cache/inmemory.go index 7064b7108fcbc..fca7761cc30cf 100644 --- a/util/cache/inmemory.go +++ b/util/cache/inmemory.go @@ -29,6 +29,21 @@ func (i *InMemoryCache) Set(item *Item) error { return nil } +func (i *InMemoryCache) HasSame(key string, obj interface{}) bool { + var buf bytes.Buffer + err := gob.NewEncoder(&buf).Encode(obj) + if err != nil { + return false + } + + bufIf, found := i.memCache.Get(key) + if !found { + return false + } + existingBuf := bufIf.(bytes.Buffer) + return bytes.Equal(buf.Bytes(), existingBuf.Bytes()) +} + func (i *InMemoryCache) Get(key string, obj interface{}) error { bufIf, found := i.memCache.Get(key) if !found { diff --git a/util/cache/twolevelclient.go b/util/cache/twolevelclient.go index f7863d1d9eb18..d3e85fad015c6 100644 --- a/util/cache/twolevelclient.go +++ b/util/cache/twolevelclient.go @@ -10,11 +10,14 @@ func NewTwoLevelClient(client CacheClient, inMemoryExpiration time.Duration) *tw } type twoLevelClient struct { - inMemoryCache CacheClient + inMemoryCache *InMemoryCache client CacheClient } func (c *twoLevelClient) Set(item *Item) error { + if c.inMemoryCache.HasSame(item.Key, item.Object) { + return nil + } _ = c.inMemoryCache.Set(item) return c.client.Set(item) } From d12fe5aac6d817f378e2bb99455991a235241da1 Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Fri, 29 Jan 2021 10:53:50 -0800 Subject: [PATCH 3/3] apply reviewer notes Signed-off-by: Alexander Matyushentsev --- pkg/apis/application/v1alpha1/types.go | 4 +++ util/cache/inmemory.go | 15 ++++++++---- util/cache/twolevelclient.go | 34 +++++++++++++++++++------- 3 files changed, 39 insertions(+), 14 deletions(-) diff --git a/pkg/apis/application/v1alpha1/types.go b/pkg/apis/application/v1alpha1/types.go index 2a8886e358322..c7b6789046726 100644 --- a/pkg/apis/application/v1alpha1/types.go +++ b/pkg/apis/application/v1alpha1/types.go @@ -985,6 +985,8 @@ type ApplicationTree struct { Hosts []HostInfo `json:"hosts,omitempty" protobuf:"bytes,3,rep,name=hosts"` } +// Normalize sorts application tree nodes and hosts. The persistent order allows to +// effectively compare previously cached app tree and allows to unnecessary Redis requests. func (t *ApplicationTree) Normalize() { sort.Slice(t.Nodes, func(i, j int) bool { return t.Nodes[i].FullName() < t.Nodes[j].FullName() @@ -1065,6 +1067,7 @@ type ResourceNode struct { CreatedAt *metav1.Time `json:"createdAt,omitempty" protobuf:"bytes,8,opt,name=createdAt"` } +// FullName returns node full name func (n *ResourceNode) FullName() string { return fmt.Sprintf("%s/%s/%s/%s", n.Group, n.Kind, n.Namespace, n.Name) } @@ -1116,6 +1119,7 @@ type ResourceDiff struct { Modified bool `json:"modified,omitempty" protobuf:"bytes,12,opt,name=modified"` } +// FullName returns full name of a node that was used for diffing func (r *ResourceDiff) FullName() string { return fmt.Sprintf("%s/%s/%s/%s", r.Group, r.Kind, r.Namespace, r.Name) } diff --git a/util/cache/inmemory.go b/util/cache/inmemory.go index fca7761cc30cf..38b1ea41b0fbb 100644 --- a/util/cache/inmemory.go +++ b/util/cache/inmemory.go @@ -4,6 +4,7 @@ import ( "bytes" "context" "encoding/gob" + "fmt" "time" gocache "github.com/patrickmn/go-cache" @@ -29,19 +30,23 @@ func (i *InMemoryCache) Set(item *Item) error { return nil } -func (i *InMemoryCache) HasSame(key string, obj interface{}) bool { +// HasSame returns true if key with the same value already present in cache +func (i *InMemoryCache) HasSame(key string, obj interface{}) (bool, error) { var buf bytes.Buffer err := gob.NewEncoder(&buf).Encode(obj) if err != nil { - return false + return false, err } bufIf, found := i.memCache.Get(key) if !found { - return false + return false, nil } - existingBuf := bufIf.(bytes.Buffer) - return bytes.Equal(buf.Bytes(), existingBuf.Bytes()) + existingBuf, ok := bufIf.(bytes.Buffer) + if !ok { + panic(fmt.Errorf("InMemoryCache has unexpected entry: %v", existingBuf)) + } + return bytes.Equal(buf.Bytes(), existingBuf.Bytes()), nil } func (i *InMemoryCache) Get(key string, obj interface{}) error { diff --git a/util/cache/twolevelclient.go b/util/cache/twolevelclient.go index d3e85fad015c6..14a4279e87c89 100644 --- a/util/cache/twolevelclient.go +++ b/util/cache/twolevelclient.go @@ -3,50 +3,66 @@ package cache import ( "context" "time" + + log "github.com/sirupsen/logrus" ) +// NewTwoLevelClient creates cache client that proxies requests to given external cache and tries to minimize +// number of requests to external client by storing cache entries in local in-memory cache. func NewTwoLevelClient(client CacheClient, inMemoryExpiration time.Duration) *twoLevelClient { - return &twoLevelClient{inMemoryCache: NewInMemoryCache(inMemoryExpiration), client: client} + return &twoLevelClient{inMemoryCache: NewInMemoryCache(inMemoryExpiration), externalCache: client} } type twoLevelClient struct { inMemoryCache *InMemoryCache - client CacheClient + externalCache CacheClient } +// Set stores the given value in both in-memory and external cache. +// Skip storing the value in external cache if the same value already exists in memory to avoid requesting external cache. func (c *twoLevelClient) Set(item *Item) error { - if c.inMemoryCache.HasSame(item.Key, item.Object) { + has, err := c.inMemoryCache.HasSame(item.Key, item.Object) + if has { return nil } - _ = c.inMemoryCache.Set(item) - return c.client.Set(item) + if err != nil { + log.Warnf("Failed to check key '%s' in in-memory cache: %v", item.Key, err) + } + err = c.inMemoryCache.Set(item) + if err != nil { + log.Warnf("Failed to save key '%s' in in-memory cache: %v", item.Key, err) + } + return c.externalCache.Set(item) } +// Get returns cache value from in-memory cache if it present. Otherwise loads it from external cache and persists +// in memory to avoid future requests to external cache. func (c *twoLevelClient) Get(key string, obj interface{}) error { err := c.inMemoryCache.Get(key, obj) if err == nil { return nil } - err = c.client.Get(key, obj) + err = c.externalCache.Get(key, obj) if err == nil { _ = c.inMemoryCache.Set(&Item{Key: key, Object: obj}) } return err } +// Delete deletes cache for given key in both in-memory and external cache. func (c *twoLevelClient) Delete(key string) error { err := c.inMemoryCache.Delete(key) if err != nil { return err } - return c.client.Delete(key) + return c.externalCache.Delete(key) } func (c *twoLevelClient) OnUpdated(ctx context.Context, key string, callback func() error) error { - return c.client.OnUpdated(ctx, key, callback) + return c.externalCache.OnUpdated(ctx, key, callback) } func (c *twoLevelClient) NotifyUpdated(key string) error { - return c.client.NotifyUpdated(key) + return c.externalCache.NotifyUpdated(key) }