From a0f12e6b9a10a6130ba7e716c4b282c3f2c061ae Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk <509198+m1kola@users.noreply.github.com> Date: Fri, 1 Nov 2024 12:45:07 +0100 Subject: [PATCH] Populate/update cache on ClusterCatalog reconcile (#1284) Currently we fetch catalog data and populate cache on demand during ClusterExtension reconciliation. This works but the first reconciliation after ClusterCatalog creation or update is slow due to the need to fetch data. With this change we proactively populate cache on ClusterCatalog creation and check if cache needs to be updated on ClusterCatalog update. Signed-off-by: Mikalai Radchuk --- cmd/manager/main.go | 5 +- internal/catalogmetadata/client/client.go | 11 +- .../catalogmetadata/client/client_test.go | 19 +- .../controllers/clustercatalog_controller.go | 58 ++++-- .../clustercatalog_controller_test.go | 188 +++++++++++++++--- 5 files changed, 206 insertions(+), 75 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index b03472dfc..c353a4cb0 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -314,8 +314,9 @@ func main() { } if err = (&controllers.ClusterCatalogReconciler{ - Client: cl, - Cache: catalogClientBackend, + Client: cl, + CatalogCache: catalogClientBackend, + CatalogCachePopulator: catalogClient, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterCatalog") os.Exit(1) diff --git a/internal/catalogmetadata/client/client.go b/internal/catalogmetadata/client/client.go index fee3b1cc2..a68c6c989 100644 --- a/internal/catalogmetadata/client/client.go +++ b/internal/catalogmetadata/client/client.go @@ -65,17 +65,10 @@ func (c *Client) GetPackage(ctx context.Context, catalog *catalogd.ClusterCatalo catalogFsys, err := c.cache.Get(catalog.Name, catalog.Status.ResolvedSource.Image.Ref) if err != nil { - return nil, fmt.Errorf("error retrieving catalog cache: %v", err) + return nil, fmt.Errorf("error retrieving cache for catalog %q: %v", catalog.Name, err) } if catalogFsys == nil { - // TODO: https://github.com/operator-framework/operator-controller/pull/1284 - // For now we are still populating cache (if absent) on-demand, - // but we might end up just returning a "cache not found" error here - // once we implement cache population in the controller. - catalogFsys, err = c.PopulateCache(ctx, catalog) - if err != nil { - return nil, fmt.Errorf("error fetching catalog contents: %v", err) - } + return nil, fmt.Errorf("cache for catalog %q not found", catalog.Name) } pkgFsys, err := fs.Sub(catalogFsys, pkgName) diff --git a/internal/catalogmetadata/client/client_test.go b/internal/catalogmetadata/client/client_test.go index 33f43c1cf..15430d7c1 100644 --- a/internal/catalogmetadata/client/client_test.go +++ b/internal/catalogmetadata/client/client_test.go @@ -62,7 +62,7 @@ func TestClientGetPackage(t *testing.T) { catalog: defaultCatalog, cache: &fakeCache{getErr: errors.New("fetch error")}, assert: func(t *testing.T, dc *declcfg.DeclarativeConfig, err error) { - assert.ErrorContains(t, err, `error retrieving catalog cache`) + assert.ErrorContains(t, err, `error retrieving cache for catalog "catalog-1"`) }, }, { @@ -114,18 +114,7 @@ func TestClientGetPackage(t *testing.T) { return testFS, nil }}, assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) { - require.NoError(t, err) - assert.Equal(t, &declcfg.DeclarativeConfig{Packages: []declcfg.Package{{Schema: declcfg.SchemaPackage, Name: "pkg-present"}}}, fbc) - }, - }, - { - name: "cache unpopulated and fails to populate", - catalog: defaultCatalog, - pkgName: "pkg-present", - cache: &fakeCache{putErr: errors.New("fake cache put error")}, - assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) { - assert.Nil(t, fbc) - assert.ErrorContains(t, err, "error fetching catalog contents") + assert.ErrorContains(t, err, `cache for catalog "catalog-1" not found`) }, }, } { @@ -278,7 +267,6 @@ type fakeCache struct { getErr error putFunc func(source string, errToCache error) (fs.FS, error) - putErr error } func (c *fakeCache) Get(catalogName, resolvedRef string) (fs.FS, error) { @@ -293,9 +281,6 @@ func (c *fakeCache) Put(catalogName, resolvedRef string, source io.Reader, errTo } return c.putFunc(buf.String(), errToCache) } - if c.putErr != nil { - return nil, c.putErr - } return nil, errors.New("unexpected error") } diff --git a/internal/controllers/clustercatalog_controller.go b/internal/controllers/clustercatalog_controller.go index 0f7a26a6c..dc86ed59f 100644 --- a/internal/controllers/clustercatalog_controller.go +++ b/internal/controllers/clustercatalog_controller.go @@ -18,25 +18,30 @@ package controllers import ( "context" + "fmt" + "io/fs" apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" ) -type CatalogCacheRemover interface { +type CatalogCache interface { + Get(catalogName, resolvedRef string) (fs.FS, error) Remove(catalogName string) error } +type CatalogCachePopulator interface { + PopulateCache(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) +} + // ClusterCatalogReconciler reconciles a ClusterCatalog object type ClusterCatalogReconciler struct { client.Client - Cache CatalogCacheRemover + CatalogCache CatalogCache + CatalogCachePopulator CatalogCachePopulator } //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clustercatalogs,verbs=get;list;watch @@ -45,31 +50,44 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque existingCatalog := &catalogd.ClusterCatalog{} err := r.Client.Get(ctx, req.NamespacedName, existingCatalog) if apierrors.IsNotFound(err) { - return ctrl.Result{}, r.Cache.Remove(req.Name) + if err := r.CatalogCache.Remove(req.Name); err != nil { + return ctrl.Result{}, fmt.Errorf("error removing cache for catalog %q: %v", req.Name, err) + } + return ctrl.Result{}, nil } if err != nil { return ctrl.Result{}, err } + + if existingCatalog.Status.ResolvedSource == nil || + existingCatalog.Status.ResolvedSource.Image == nil || + existingCatalog.Status.ResolvedSource.Image.Ref == "" { + // Reference is not known yet - skip cache population with no error. + // Once the reference is resolved another reconcile cycle + // will be triggered and we will progress further. + return ctrl.Result{}, nil + } + + catalogFsys, err := r.CatalogCache.Get(existingCatalog.Name, existingCatalog.Status.ResolvedSource.Image.Ref) + if err != nil { + return ctrl.Result{}, fmt.Errorf("error retrieving cache for catalog %q: %v", existingCatalog.Name, err) + } + if catalogFsys != nil { + // Cache already exists so we do not need to populate it + return ctrl.Result{}, nil + } + + if _, err = r.CatalogCachePopulator.PopulateCache(ctx, existingCatalog); err != nil { + return ctrl.Result{}, fmt.Errorf("error populating cache for catalog %q: %v", existingCatalog.Name, err) + } + return ctrl.Result{}, nil } // SetupWithManager sets up the controller with the Manager. func (r *ClusterCatalogReconciler) SetupWithManager(mgr ctrl.Manager) error { _, err := ctrl.NewControllerManagedBy(mgr). - For(&catalogd.ClusterCatalog{}, builder.WithPredicates(predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { - return false - }, - UpdateFunc: func(e event.UpdateEvent) bool { - return false - }, - DeleteFunc: func(e event.DeleteEvent) bool { - return true - }, - GenericFunc: func(e event.GenericEvent) bool { - return false - }, - })). + For(&catalogd.ClusterCatalog{}). Build(r) return err diff --git a/internal/controllers/clustercatalog_controller_test.go b/internal/controllers/clustercatalog_controller_test.go index 762fa15ec..639f20c96 100644 --- a/internal/controllers/clustercatalog_controller_test.go +++ b/internal/controllers/clustercatalog_controller_test.go @@ -3,7 +3,9 @@ package controllers_test import ( "context" "errors" + "io/fs" "testing" + "testing/fstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -19,38 +21,142 @@ import ( ) func TestClusterCatalogReconcilerFinalizers(t *testing.T) { + const fakeResolvedRef = "fake/catalog@sha256:fakesha1" catalogKey := types.NamespacedName{Name: "test-catalog"} for _, tt := range []struct { - name string - catalog *catalogd.ClusterCatalog - cacheRemoveFunc func(catalogName string) error - wantCacheRemoveCalled bool - wantErr string + name string + catalog *catalogd.ClusterCatalog + catalogCache mockCatalogCache + catalogCachePopulator mockCatalogCachePopulator + wantGetCacheCalled bool + wantRemoveCacheCalled bool + wantPopulateCacheCalled bool + wantErr string }{ { - name: "catalog exists", + name: "catalog exists - cache unpopulated", catalog: &catalogd.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: catalogKey.Name, }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, + }, + catalogCachePopulator: mockCatalogCachePopulator{ + populateCacheFunc: func(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalog.Name) + return nil, nil + }, + }, + wantGetCacheCalled: true, + wantPopulateCacheCalled: true, + }, + { + name: "catalog exists - cache already populated", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, + }, + catalogCache: mockCatalogCache{ + getFunc: func(catalogName, resolvedRef string) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalogName) + assert.Equal(t, fakeResolvedRef, resolvedRef) + // Just any non-nil fs.FS to simulate existence of cache + return fstest.MapFS{}, nil + }, + }, + wantGetCacheCalled: true, + }, + { + name: "catalog exists - catalog not yet resolved", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + }, + }, + { + name: "catalog exists - error on cache population", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, }, + catalogCachePopulator: mockCatalogCachePopulator{ + populateCacheFunc: func(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalog.Name) + return nil, errors.New("fake error from populate cache function") + }, + }, + wantGetCacheCalled: true, + wantPopulateCacheCalled: true, + wantErr: "error populating cache for catalog", + }, + { + name: "catalog exists - error on cache get", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, + }, + catalogCache: mockCatalogCache{ + getFunc: func(catalogName, resolvedRef string) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalogName) + assert.Equal(t, fakeResolvedRef, resolvedRef) + return nil, errors.New("fake error from cache get function") + }, + }, + wantGetCacheCalled: true, + wantErr: "error retrieving cache for catalog", }, { name: "catalog does not exist", - cacheRemoveFunc: func(catalogName string) error { - assert.Equal(t, catalogKey.Name, catalogName) - return nil + catalogCache: mockCatalogCache{ + removeFunc: func(catalogName string) error { + assert.Equal(t, catalogKey.Name, catalogName) + return nil + }, }, - wantCacheRemoveCalled: true, + wantRemoveCacheCalled: true, }, { name: "catalog does not exist - error on removal", - cacheRemoveFunc: func(catalogName string) error { - return errors.New("fake error from remove") + catalogCache: mockCatalogCache{ + removeFunc: func(catalogName string) error { + assert.Equal(t, catalogKey.Name, catalogName) + return errors.New("fake error from remove") + }, }, - wantCacheRemoveCalled: true, - wantErr: "fake error from remove", + wantRemoveCacheCalled: true, + wantErr: "error removing cache for catalog", }, } { t.Run(tt.name, func(t *testing.T) { @@ -62,13 +168,10 @@ func TestClusterCatalogReconcilerFinalizers(t *testing.T) { } cl := clientBuilder.Build() - cacheRemover := &mockCatalogCacheRemover{ - removeFunc: tt.cacheRemoveFunc, - } - reconciler := &controllers.ClusterCatalogReconciler{ - Client: cl, - Cache: cacheRemover, + Client: cl, + CatalogCache: controllers.CatalogCache(&tt.catalogCache), + CatalogCachePopulator: controllers.CatalogCachePopulator(&tt.catalogCachePopulator), } result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) @@ -79,17 +182,48 @@ func TestClusterCatalogReconcilerFinalizers(t *testing.T) { } require.Equal(t, ctrl.Result{}, result) - assert.Equal(t, tt.wantCacheRemoveCalled, cacheRemover.called) + assert.Equal(t, tt.wantRemoveCacheCalled, tt.catalogCache.removeFuncCalled) + assert.Equal(t, tt.wantGetCacheCalled, tt.catalogCache.getFuncCalled) + assert.Equal(t, tt.wantPopulateCacheCalled, tt.catalogCachePopulator.populateCacheCalled) }) } } -type mockCatalogCacheRemover struct { - called bool - removeFunc func(catalogName string) error +type mockCatalogCache struct { + removeFuncCalled bool + removeFunc func(catalogName string) error + getFuncCalled bool + getFunc func(catalogName, resolvedRef string) (fs.FS, error) +} + +func (m *mockCatalogCache) Remove(catalogName string) error { + m.removeFuncCalled = true + if m.removeFunc != nil { + return m.removeFunc(catalogName) + } + + return nil +} + +func (m *mockCatalogCache) Get(catalogName, resolvedRef string) (fs.FS, error) { + m.getFuncCalled = true + if m.getFunc != nil { + return m.getFunc(catalogName, resolvedRef) + } + + return nil, nil } -func (m *mockCatalogCacheRemover) Remove(catalogName string) error { - m.called = true - return m.removeFunc(catalogName) +type mockCatalogCachePopulator struct { + populateCacheCalled bool + populateCacheFunc func(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) +} + +func (m *mockCatalogCachePopulator) PopulateCache(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) { + m.populateCacheCalled = true + if m.populateCacheFunc != nil { + return m.populateCacheFunc(ctx, catalog) + } + + return nil, nil }