short-circuit reconcile when objects are deleted (#2022)

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 <[email protected]>

Author: joelanford

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

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{

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 {

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)