Repository: vmware-tanzu/octant
Pull request #2105 (2021-03-05 22:47:10)

Memory caching for CRD navigation entries
During performance testing, I noticed that we are rebuilding a list of CRD navigation entries twice per second, for cluster scope and current namespace. That is very costly operation taking ~500ms for larger CRD sets. To address the issue, I added a memory cache holding CRD navigation entries. Cache is invalidated every time new CRD appears or goes away.

Performance improvement is considerable but it depends on number of CRDs in cluster/namespace - I am seeing 15-20% improvement on larger sets on my mac.

Signed-off-by: Milan Klanjsek <mklanjsek@pivotal.io>





wwitzel3: Good start, this will need to be tied in to the Add/Update/Delete events of crd_watcher.go so that it removes old CRDS and adds new CRDs as they are applied. This will only cache the CRDs that are installed at the time you startup Octant.
mklanjsek: @wwitzel3 I already handle Add/Delete in `crd_watcher`, invalidating the cache there so it's recreated during next call.
wwitzel3: > @wwitzel3 I already handle Add/Delete in `crd_watcher`, invalidating the cache there so it's recreated during next call.

I see it now, probably was just GitHub not showing me the whole diff.
wwitzel3: Current:
![image](https://user-images.githubusercontent.com/27317/110213880-ec8c0100-7e6f-11eb-9fc0-1180be48acd0.png)

This change alone:
![image](https://user-images.githubusercontent.com/27317/110213741-53f58100-7e6f-11eb-9246-05a6d062e5d3.png)

This change combined with the #2102 branch
![image](https://user-images.githubusercontent.com/27317/110213757-6374ca00-7e6f-11eb-9151-a0a31b14cadb.png)

mklanjsek: Good catch Wayne, missed the race condition.  I eliminated the redundant sort on cached object and as a result the race condition is gone now.

Discouraged by negligible performance gain in your tests, makes me wonder if it's worth it... 
wwitzel3: Negligible? Avg. 50% improvement once cached, that's a big improvement.
mklanjsek: Ooops my bad, didn't read that well 🤦  Definitely not negligible!
wwitzel3: I want to think about this a bit more, the idea is solid and effective. I'm not sure I like the current implementation of just slapping a global in to navigation and calling it from the watcher callbacks and accessing it in `CRDEntires` and `CustomResourceDefinitions`.

Also there will need to be at least a simple assertion test: non-cache call, cache call, clear cache, non-cache call, cache call.


wwitzel3: @mklanjsek and I talked about this, we will implement a cache layer and then overview and custom overview will maintain their own cache of the `CRDEntries` call. This is because the modules actually care about different results and the CRDEntires function is bi-modal depending of if you want namespace scoped CRDs or not.
mklanjsek: Waiting until #2144 is merged before finishing this.

Patch:
diff --git a/internal/describer/crd_watcher.go b/internal/describer/crd_watcher.go
index 8acb2e29..ed2aa553 100644
--- a/internal/describer/crd_watcher.go
+++ b/internal/describer/crd_watcher.go
@@ -78,7 +78,6 @@ func (cw *DefaultCRDWatcher) Watch(ctx context.Context) error {
 			}
 
 			cw.clusterClient.ResetMapper()
-
 			for _, watchConfig := range cw.watchConfigs {
 				if watchConfig.CanPerform(u) {
 					watchConfig.Add(ctx, u)
diff --git a/internal/modules/clusteroverview/clusteroverview.go b/internal/modules/clusteroverview/clusteroverview.go
index fe617137..7f70005a 100644
--- a/internal/modules/clusteroverview/clusteroverview.go
+++ b/internal/modules/clusteroverview/clusteroverview.go
@@ -42,9 +42,28 @@ type ClusterOverview struct {
 	pathMatcher *describer.PathMatcher
 	watchedCRDs []*unstructured.Unstructured
 
+	navigationCrdCache []navigation.Navigation
+	navigationCrdLock  sync.Mutex
+
 	mu sync.Mutex
 }
 
+func (co *ClusterOverview) CRDEntries(ctx context.Context, prefix, namespace string, objectStore store.Store, wantsClusterScoped bool) ([]navigation.Navigation, bool, error) {
+	if co.navigationCrdCache != nil {
+		return co.navigationCrdCache, false, nil
+	}
+
+	co.navigationCrdLock.Lock()
+	defer co.navigationCrdLock.Unlock()
+
+	entries, loading, err := navigation.CRDEntries(ctx, prefix, namespace, objectStore, wantsClusterScoped)
+	if len(entries) > 0 {
+		co.navigationCrdCache = entries
+		return co.navigationCrdCache, loading, err
+	}
+	return []navigation.Navigation{}, false, nil
+}
+
 var _ module.Module = (*ClusterOverview)(nil)
 
 func New(ctx context.Context, options Options) (*ClusterOverview, error) {
@@ -83,6 +102,7 @@ func New(ctx context.Context, options Options) (*ClusterOverview, error) {
 				}
 				describer.AddCRD(ctx, object, pathMatcher, customResourcesDescriber, co, options.DashConfig.ObjectStore())
 				co.watchedCRDs = append(co.watchedCRDs, object)
+				co.navigationCrdCache = nil
 			}
 		}(pathMatcher, customResourcesDescriber),
 		Delete: func(_ *describer.PathMatcher, csd *describer.CRDSection) config.ObjectHandler {
@@ -102,6 +122,7 @@ func New(ctx context.Context, options Options) (*ClusterOverview, error) {
 					list = append(list, co.watchedCRDs[i])
 				}
 				co.watchedCRDs = list
+				co.navigationCrdCache = nil
 			}
 		}(pathMatcher, customResourcesDescriber),
 		IsNamespaced: false,
@@ -196,7 +217,7 @@ func (co *ClusterOverview) Navigation(ctx context.Context, _ string, root string
 		EntriesFuncs: map[string]octant.EntriesFunc{
 			"Cluster Overview":            nil,
 			"Namespaces":                  nil,
-			"Custom Resources":            navigation.CRDEntries,
+			"Custom Resources":            co.CRDEntries,
 			"Custom Resource Definitions": nil,
 			"RBAC":                        rbacEntries,
 			"Webhooks":                    webhookEntries,
@@ -313,5 +334,6 @@ func (co *ClusterOverview) SetContext(ctx context.Context, _ string) error {
 	}
 
 	co.watchedCRDs = []*unstructured.Unstructured{}
+	co.navigationCrdCache = nil
 	return nil
 }
diff --git a/internal/modules/overview/overview.go b/internal/modules/overview/overview.go
index f7eeac00..938a030e 100644
--- a/internal/modules/overview/overview.go
+++ b/internal/modules/overview/overview.go
@@ -9,6 +9,8 @@ import (
 	"context"
 	"sync"
 
+	"github.com/vmware-tanzu/octant/pkg/store"
+
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 
@@ -42,9 +44,31 @@ type Overview struct {
 
 	watchedCRDs []*unstructured.Unstructured
 
+	navigationCrdCache map[string][]navigation.Navigation
+	navigationCrdLock  sync.Mutex
+
 	mu sync.Mutex
 }
 
+func (co *Overview) CRDEntries(ctx context.Context, prefix, namespace string, objectStore store.Store, wantsClusterScoped bool) ([]navigation.Navigation, bool, error) {
+	if _, ok := co.navigationCrdCache[namespace]; ok {
+		return co.navigationCrdCache[namespace], false, nil
+	}
+
+	co.navigationCrdLock.Lock()
+	defer co.navigationCrdLock.Unlock()
+
+	entries, loading, err := navigation.CRDEntries(ctx, prefix, namespace, objectStore, wantsClusterScoped)
+	if len(entries) > 0 {
+		if co.navigationCrdCache == nil {
+			co.navigationCrdCache = make(map[string][]navigation.Navigation)
+		}
+		co.navigationCrdCache[namespace] = entries
+		return co.navigationCrdCache[namespace], loading, err
+	}
+	return []navigation.Navigation{}, false, nil
+}
+
 var _ module.Module = (*Overview)(nil)
 var _ module.ActionReceiver = (*Overview)(nil)
 
@@ -84,6 +108,7 @@ func (co *Overview) SetContext(ctx context.Context, contextName string) error {
 	crdWatcher := co.dashConfig.CRDWatcher()
 	crdWatcher.Watch(ctx)
 
+	co.navigationCrdCache = nil
 	return nil
 }
 
@@ -134,6 +159,7 @@ func (co *Overview) bootstrap(ctx context.Context) error {
 				}
 				describer.AddCRD(ctx, object, pathMatcher, customResourcesDescriber, co, co.dashConfig.ObjectStore())
 				co.watchedCRDs = append(co.watchedCRDs, object)
+				delete(co.navigationCrdCache, object.GetNamespace())
 			}
 		}(pathMatcher, customResourcesDescriber),
 		Delete: func(pathMatcher *describer.PathMatcher, csd *describer.CRDSection) config.ObjectHandler {
@@ -153,6 +179,7 @@ func (co *Overview) bootstrap(ctx context.Context) error {
 					list = append(list, co.watchedCRDs[i])
 				}
 				co.watchedCRDs = list
+				delete(co.navigationCrdCache, object.GetNamespace())
 			}
 		}(pathMatcher, customResourcesDescriber),
 		IsNamespaced: true,
@@ -195,7 +222,7 @@ func (co *Overview) Navigation(ctx context.Context, namespace, root string) ([]n
 			"Workloads":                    workloadEntries,
 			"Discovery and Load Balancing": discoAndLBEntries,
 			"Config and Storage":           configAndStorageEntries,
-			"Custom Resources":             navigation.CRDEntries,
+			"Custom Resources":             co.CRDEntries,
 			"RBAC":                         rbacEntries,
 			"Events":                       nil,
 		},
diff --git a/pkg/navigation/navigation.go b/pkg/navigation/navigation.go
index de47b52a..c0dc58a9 100644
--- a/pkg/navigation/navigation.go
+++ b/pkg/navigation/navigation.go
@@ -88,10 +88,6 @@ func CRDEntries(ctx context.Context, prefix, namespace string, objectStore store
 		return nil, false, errors.Wrap(err, "retrieving CRDs")
 	}
 
-	sort.Slice(crds, func(i, j int) bool {
-		return crds[i].Name < crds[j].Name
-	})
-
 	for i := range crds {
 		if wantsClusterScoped && crds[i].Spec.Scope != apiextv1.ClusterScoped {
 			continue
@@ -158,6 +154,9 @@ func CustomResourceDefinitions(ctx context.Context, o store.Store) ([]*apiextv1.
 		}
 		list = append(list, crd)
 	}
+	sort.Slice(list, func(i, j int) bool {
+		return list[i].Name < list[j].Name
+	})
 
 	return list, hasSynced, nil
 }


Test patch:
diff --git a/internal/modules/overview/overview_test.go b/internal/modules/overview/overview_test.go
new file mode 100644
index 00000000..97b8feba
--- /dev/null
+++ b/internal/modules/overview/overview_test.go
@@ -0,0 +1,109 @@
+package overview
+
+import (
+	"context"
+	"path"
+	"testing"
+
+	"github.com/golang/mock/gomock"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/client-go/tools/cache"
+
+	clusterFake "github.com/vmware-tanzu/octant/internal/cluster/fake"
+	"github.com/vmware-tanzu/octant/internal/config"
+	configFake "github.com/vmware-tanzu/octant/internal/config/fake"
+	"github.com/vmware-tanzu/octant/internal/describer"
+	"github.com/vmware-tanzu/octant/internal/log"
+	"github.com/vmware-tanzu/octant/internal/testutil"
+	"github.com/vmware-tanzu/octant/pkg/icon"
+	"github.com/vmware-tanzu/octant/pkg/navigation"
+	"github.com/vmware-tanzu/octant/pkg/store"
+	objectStoreFake "github.com/vmware-tanzu/octant/pkg/store/fake"
+)
+
+func Test_Overview_CRD_Navigation_Cache(t *testing.T) {
+	controller := gomock.NewController(t)
+	defer controller.Finish()
+
+	ctx := context.Background()
+	crdKey := store.Key{
+		APIVersion: "apiextensions.k8s.io/v1",
+		Kind:       "CustomResourceDefinition",
+	}
+
+	client := clusterFake.NewMockClientInterface(controller)
+	objectStore := objectStoreFake.NewMockStore(controller)
+	objectStore.EXPECT().
+		Watch(ctx, crdKey, gomock.Any()).
+		DoAndReturn(func(_ context.Context, key store.Key, c *cache.ResourceEventHandlerFuncs) error {
+			assert.NotNil(t, c.AddFunc)
+			assert.NotNil(t, c.DeleteFunc)
+			return nil
+		})
+
+	dashConfig := configFake.NewMockDash(controller)
+	dashConfig.EXPECT().ObjectStore().Return(objectStore).AnyTimes()
+
+	crdWatcher, err := describer.NewDefaultCRDWatcher(ctx, client, objectStore, nil)
+	require.NoError(t, err)
+
+	watchConfig := &config.CRDWatchConfig{
+		Add:          func(ctx context.Context, object *unstructured.Unstructured) {},
+		Delete:       func(ctx context.Context, object *unstructured.Unstructured) {},
+		IsNamespaced: false,
+	}
+
+	require.NoError(t, crdWatcher.AddConfig(watchConfig))
+	require.NoError(t, crdWatcher.Watch(ctx))
+
+	dashConfig.EXPECT().Validate().Return(nil).AnyTimes()
+	dashConfig.EXPECT().Logger().Return(log.NopLogger()).AnyTimes()
+	dashConfig.EXPECT().CRDWatcher().Return(crdWatcher).AnyTimes()
+
+	overviewOptions := Options{
+		Namespace:  "test-namespace",
+		DashConfig: dashConfig,
+	}
+
+	overviewModule, err := New(ctx, overviewOptions)
+	require.NoError(t, err)
+	assert.NotNil(t, overviewModule)
+	assert.Nil(t, overviewModule.navigationCrdCache)
+
+	crd := testutil.CreateCRDWithKind("namespace-scoped", "NamespaceScoped", false)
+	crds := testutil.ToUnstructuredList(t, crd)
+	objectStore.EXPECT().
+		List(gomock.Any(), crdKey).
+		Return(crds, false, nil).
+		AnyTimes()
+
+	namespaceName := "test"
+	crNamespaceKey := store.Key{
+		Namespace:  namespaceName,
+		APIVersion: "testing/v1",
+		Kind:       "NamespaceScoped",
+	}
+
+	cr := testutil.CreateCR("testing", "v1", "NamespaceScoped", "namespace-scoped")
+	crs := testutil.ToUnstructuredList(t, cr)
+
+	objectStore.EXPECT().
+		List(gomock.Any(), crNamespaceKey).
+		Return(crs, false, nil).
+		AnyTimes()
+
+	nav, err := navigation.New(cr.GetName(), path.Join("/prefix", cr.GetName()), navigation.SetNavigationIcon(icon.CustomResourceDefinition))
+	require.NoError(t, err)
+
+	namespaceGot, _, err := overviewModule.CRDEntries(ctx, "/prefix", namespaceName, objectStore, false)
+	require.NoError(t, err)
+
+	namespaceExpected := []navigation.Navigation{*nav}
+	assert.Equal(t, namespaceExpected, namespaceGot)
+	assert.NotNil(t, overviewModule.navigationCrdCache)
+
+	expectedCache := map[string][]navigation.Navigation{namespaceName: namespaceExpected}
+	assert.Equal(t, expectedCache, overviewModule.navigationCrdCache)
+}
diff --git a/internal/testutil/crd.go b/internal/testutil/crd.go
new file mode 100644
index 00000000..4c801e17
--- /dev/null
+++ b/internal/testutil/crd.go
@@ -0,0 +1,44 @@
+package testutil
+
+import (
+	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+)
+
+func CreateCR(group, version, kind, name string) *unstructured.Unstructured {
+	m := make(map[string]interface{})
+	u := &unstructured.Unstructured{Object: m}
+
+	u.SetName(name)
+	u.SetGroupVersionKind(schema.GroupVersionKind{
+		Group:   group,
+		Version: version,
+		Kind:    kind,
+	})
+
+	return u
+}
+
+func CreateCRDWithKind(name, kind string, isClusterScoped bool) *apiextv1.CustomResourceDefinition {
+	scope := apiextv1.ClusterScoped
+	if !isClusterScoped {
+		scope = apiextv1.NamespaceScoped
+	}
+
+	crd := CreateCRD(name)
+	crd.Spec.Scope = scope
+	crd.Spec.Group = "testing"
+	crd.Spec.Names = apiextv1.CustomResourceDefinitionNames{
+		Kind: kind,
+	}
+	crd.Spec.Versions = []apiextv1.CustomResourceDefinitionVersion{
+		{
+			Name:    "v1",
+			Served:  true,
+			Storage: true,
+		},
+	}
+
+	return crd
+}
diff --git a/pkg/navigation/navigation_test.go b/pkg/navigation/navigation_test.go
index 85c5e4a4..af93452b 100644
--- a/pkg/navigation/navigation_test.go
+++ b/pkg/navigation/navigation_test.go
@@ -10,15 +10,13 @@ import (
 	"path"
 	"testing"
 
+	"github.com/vmware-tanzu/octant/pkg/icon"
+
 	"github.com/golang/mock/gomock"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"github.com/vmware-tanzu/octant/internal/testutil"
-	"github.com/vmware-tanzu/octant/pkg/icon"
 	"github.com/vmware-tanzu/octant/pkg/store"
 	"github.com/vmware-tanzu/octant/pkg/store/fake"
 )
@@ -59,8 +57,8 @@ func TestCRDEntries_namespace_scoped(t *testing.T) {
 	defer controller.Finish()
 
 	objectStore := fake.NewMockStore(controller)
-	clusterScopedCRD := createCRD("cluster-scoped", "ClusterScoped", true)
-	namespaceScopedCRD := createCRD("namespace-scoped", "NamespaceScoped", false)
+	clusterScopedCRD := testutil.CreateCRDWithKind("cluster-scoped", "ClusterScoped", true)
+	namespaceScopedCRD := testutil.CreateCRDWithKind("namespace-scoped", "NamespaceScoped", false)
 
 	crds := testutil.ToUnstructuredList(t, clusterScopedCRD, namespaceScopedCRD)
 	crdKey := store.Key{
@@ -72,9 +70,9 @@ func TestCRDEntries_namespace_scoped(t *testing.T) {
 		Return(crds, false, nil).
 		AnyTimes()
 
-	clusterCR := createCR("testing", "v1", "ClusterScoped", "cluster-scoped")
+	clusterCR := testutil.CreateCR("testing", "v1", "ClusterScoped", "cluster-scoped")
 	clusterCRs := testutil.ToUnstructuredList(t, clusterCR)
-	namespaceCR := createCR("testing", "v1", "NamespaceScoped", "namespace-scoped")
+	namespaceCR := testutil.CreateCR("testing", "v1", "NamespaceScoped", "namespace-scoped")
 	namespaceCRs := testutil.ToUnstructuredList(t, namespaceCR)
 
 	crNamespaceKey := store.Key{
@@ -106,12 +104,14 @@ func TestCRDEntries_namespace_scoped(t *testing.T) {
 
 	assert.Equal(t, namespaceExpected, namespaceGot)
 
-	//clusterGot, _, err := CRDEntries(ctx, "/prefix", "default", objectStore, true)
-	//require.NoError(t, err)
-	//
-	//clusterExpected := []Navigation{}
+	clusterGot, _, err := CRDEntries(ctx, "/prefix", "default", objectStore, true)
+	require.NoError(t, err)
 
-	//assert.Equal(t, clusterExpected, clusterGot)
+	clusterExpected := []Navigation{
+		createNavForCR(t, clusterCR.GetName()),
+	}
+
+	assert.Equal(t, clusterExpected, clusterGot)
 }
 
 func createNavForCR(t *testing.T, name string) Navigation {
@@ -120,40 +120,3 @@ func createNavForCR(t *testing.T, name string) Navigation {
 
 	return *nav
 }
-
-func createCRD(name, kind string, isClusterScoped bool) *apiextv1.CustomResourceDefinition {
-	scope := apiextv1.ClusterScoped
-	if !isClusterScoped {
-		scope = apiextv1.NamespaceScoped
-	}
-
-	crd := testutil.CreateCRD(name)
-	crd.Spec.Scope = scope
-	crd.Spec.Group = "testing"
-	crd.Spec.Names = apiextv1.CustomResourceDefinitionNames{
-		Kind: kind,
-	}
-	crd.Spec.Versions = []apiextv1.CustomResourceDefinitionVersion{
-		{
-			Name:    "v1",
-			Served:  true,
-			Storage: true,
-		},
-	}
-
-	return crd
-}
-
-func createCR(group, version, kind, name string) *unstructured.Unstructured {
-	m := make(map[string]interface{})
-	u := &unstructured.Unstructured{Object: m}
-
-	u.SetName(name)
-	u.SetGroupVersionKind(schema.GroupVersionKind{
-		Group:   group,
-		Version: version,
-		Kind:    kind,
-	})
-
-	return u
-}