From 6ca441954902deed1eb6bc5e4e555afabe553deb Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Tue, 10 Jun 2025 17:02:54 -0400 Subject: [PATCH] short-circuit reconcile when objects are deleted This is necessary to ensure that we do not keep reconciling the objects as if they were not deleted. The need for this became apparent while trying to use --cascade=orphan with a ClusterExtension. In theory, that should work out of the box because, we set owner references on all managed objects. However, that was not working because our controller was fully reconciling objects with metadata.finalizers: ["orphan"], which was writing owner references back into the objects that the orphan deletion process had just removed. Ultimately this meant that the managed objects would be background deleted because they once again had an owner reference to the now-deleted ClusterExtension, which then caused the kubernetes garbage collector to clean them up. In general, it stands to reason that once we have successfully processed all of our finalizers after a deletion of an object, we should stop reconciling that object. Signed-off-by: Joe Lanford --- .../core/clustercatalog_controller.go | 8 ++ .../core/clustercatalog_controller_test.go | 34 +++++++++ .../clusterextension_controller.go | 8 ++ .../clusterextension_controller_test.go | 73 +++++++++++++++++++ 4 files changed, 123 insertions(+) diff --git a/internal/catalogd/controllers/core/clustercatalog_controller.go b/internal/catalogd/controllers/core/clustercatalog_controller.go index d0597d3ee..ce1636266 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller.go @@ -196,6 +196,14 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *ocv1. return ctrl.Result{}, nil } + if catalog.GetDeletionTimestamp() != nil { + // If we've gotten here, that means the cluster catalog is being deleted, we've handled all of + // _our_ finalizers (above), but the cluster catalog is still present in the cluster, likely + // because there are _other_ finalizers that other controllers need to handle, (e.g. the orphan + // deletion finalizer). + return ctrl.Result{}, nil + } + // TODO: The below algorithm to get the current state based on an in-memory // storedCatalogs map is a hack that helps us keep the ClusterCatalog's // status up-to-date. The fact that we need this setup is indicative of diff --git a/internal/catalogd/controllers/core/clustercatalog_controller_test.go b/internal/catalogd/controllers/core/clustercatalog_controller_test.go index f7b917dc1..95a18733a 100644 --- a/internal/catalogd/controllers/core/clustercatalog_controller_test.go +++ b/internal/catalogd/controllers/core/clustercatalog_controller_test.go @@ -766,6 +766,40 @@ func TestCatalogdControllerReconcile(t *testing.T) { }, }, }, + { + name: "reconcile should be short-circuited if the clustercatalog has a deletion timestamp and all known finalizers have been removed", + catalog: &ocv1.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{"finalizer"}, + DeletionTimestamp: &metav1.Time{Time: time.Date(2025, 6, 10, 16, 43, 0, 0, time.UTC)}, + }, + Spec: ocv1.ClusterCatalogSpec{ + Source: ocv1.CatalogSource{ + Type: ocv1.SourceTypeImage, + Image: &ocv1.ImageSource{ + Ref: "my.org/someimage:latest", + }, + }, + AvailabilityMode: ocv1.AvailabilityModeAvailable, + }, + }, + expectedCatalog: &ocv1.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: "catalog", + Finalizers: []string{"finalizer"}, + DeletionTimestamp: &metav1.Time{Time: time.Date(2025, 6, 10, 16, 43, 0, 0, time.UTC)}}, + Spec: ocv1.ClusterCatalogSpec{ + Source: ocv1.CatalogSource{ + Type: ocv1.SourceTypeImage, + Image: &ocv1.ImageSource{ + Ref: "my.org/someimage:latest", + }, + }, + AvailabilityMode: ocv1.AvailabilityModeAvailable, + }, + }, + }, } { t.Run(tt.name, func(t *testing.T) { reconciler := &ClusterCatalogReconciler{ diff --git a/internal/operator-controller/controllers/clusterextension_controller.go b/internal/operator-controller/controllers/clusterextension_controller.go index e571174b0..9a79e8c75 100644 --- a/internal/operator-controller/controllers/clusterextension_controller.go +++ b/internal/operator-controller/controllers/clusterextension_controller.go @@ -206,6 +206,14 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl return ctrl.Result{}, nil } + if ext.GetDeletionTimestamp() != nil { + // If we've gotten here, that means the cluster extension is being deleted, we've handled all of + // _our_ finalizers (above), but the cluster extension is still present in the cluster, likely + // because there are _other_ finalizers that other controllers need to handle, (e.g. the orphan + // deletion finalizer). + return ctrl.Result{}, nil + } + l.Info("getting installed bundle") installedBundle, err := r.InstalledBundleGetter.GetInstalledBundle(ctx, ext) if err != nil { diff --git a/internal/operator-controller/controllers/clusterextension_controller_test.go b/internal/operator-controller/controllers/clusterextension_controller_test.go index be61891a0..64883c416 100644 --- a/internal/operator-controller/controllers/clusterextension_controller_test.go +++ b/internal/operator-controller/controllers/clusterextension_controller_test.go @@ -48,6 +48,79 @@ func TestClusterExtensionDoesNotExist(t *testing.T) { require.NoError(t, err) } +func TestClusterExtensionShortCircuitsReconcileDuringDeletion(t *testing.T) { + cl, reconciler := newClientAndReconciler(t) + + installedBundleGetterCalledErr := errors.New("installed bundle getter called") + checkInstalledBundleGetterCalled := func(t require.TestingT, err error, args ...interface{}) { + require.Equal(t, installedBundleGetterCalledErr, err) + } + reconciler.InstalledBundleGetter = &MockInstalledBundleGetter{ + err: installedBundleGetterCalledErr, + } + + type testCase struct { + name string + finalizers []string + shouldDelete bool + expectErr require.ErrorAssertionFunc + } + for _, tc := range []testCase{ + { + name: "no finalizers, not deleted", + expectErr: checkInstalledBundleGetterCalled, + }, + { + name: "has finalizers, not deleted", + finalizers: []string{"finalizer"}, + expectErr: checkInstalledBundleGetterCalled, + }, + { + name: "has finalizers, deleted", + finalizers: []string{"finalizer"}, + shouldDelete: true, + expectErr: require.NoError, + }, + } { + t.Run(tc.name, func(t *testing.T) { + pkgName := fmt.Sprintf("test-pkg-%s", rand.String(6)) + + ctx := context.Background() + extKey := types.NamespacedName{Name: fmt.Sprintf("cluster-extension-test-%s", rand.String(8))} + + t.Log("When the cluster extension specifies a non-existent package") + t.Log("By initializing cluster state") + clusterExtension := &ocv1.ClusterExtension{ + ObjectMeta: metav1.ObjectMeta{ + Name: extKey.Name, + Finalizers: tc.finalizers, + }, + Spec: ocv1.ClusterExtensionSpec{ + Source: ocv1.SourceConfig{ + SourceType: "Catalog", + Catalog: &ocv1.CatalogFilter{ + PackageName: pkgName, + }, + }, + Namespace: "default", + ServiceAccount: ocv1.ServiceAccountReference{ + Name: "default", + }, + }, + } + require.NoError(t, cl.Create(ctx, clusterExtension)) + if tc.shouldDelete { + require.NoError(t, cl.Delete(ctx, clusterExtension)) + } + + t.Log("By running reconcile") + res, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: extKey}) + require.Equal(t, ctrl.Result{}, res) + tc.expectErr(t, err) + }) + } +} + func TestClusterExtensionResolutionFails(t *testing.T) { pkgName := fmt.Sprintf("non-existent-%s", rand.String(6)) cl, reconciler := newClientAndReconciler(t)