From 33fa1889ec962ed3d0c7cc848298d4e055932304 Mon Sep 17 00:00:00 2001 From: Anik Bhattacharjee Date: Thu, 19 Sep 2024 20:31:40 +0530 Subject: [PATCH 1/3] =?UTF-8?q?=F0=9F=90=9B=20Handle=20finalizers=20(creat?= =?UTF-8?q?ion=20and=20deletion)=20better?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes [409](https://github.com/operator-framework/catalogd/issues/409) --- cmd/manager/main.go | 34 +++++++- .../core/clustercatalog_controller.go | 82 ++++++++++++------- .../core/clustercatalog_controller_test.go | 50 +++++------ 3 files changed, 108 insertions(+), 58 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index 10d97187..fd9346ef 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -17,6 +17,7 @@ limitations under the License. package main import ( + "context" "crypto/tls" "flag" "fmt" @@ -35,6 +36,8 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/certwatcher" + "sigs.k8s.io/controller-runtime/pkg/client" + crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -222,10 +225,29 @@ func main() { os.Exit(1) } + clusterCatalogFinalizers := crfinalizer.NewFinalizers() + if err := clusterCatalogFinalizers.Register(corecontrollers.FbcDeletionFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + catalog, ok := obj.(*v1alpha1.ClusterCatalog) + if !ok { + panic("could not convert object to clusterCatalog") + } + if err := localStorage.Delete(catalog.Name); err != nil { + return crfinalizer.Result{}, corecontrollers.UpdateStatusStorageDeleteError(&catalog.Status, err) + } + if err := unpacker.Cleanup(ctx, catalog); err != nil { + return crfinalizer.Result{}, corecontrollers.UpdateStatusStorageDeleteError(&catalog.Status, err) + } + return crfinalizer.Result{}, nil + })); err != nil { + setupLog.Error(err, "unable to register finalizer", "finalizerKey", corecontrollers.FbcDeletionFinalizer) + os.Exit(1) + } + if err = (&corecontrollers.ClusterCatalogReconciler{ - Client: mgr.GetClient(), - Unpacker: unpacker, - Storage: localStorage, + Client: mgr.GetClient(), + Unpacker: unpacker, + Storage: localStorage, + Finalizers: clusterCatalogFinalizers, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterCatalog") os.Exit(1) @@ -279,3 +301,9 @@ func podNamespace() string { } return string(namespace) } + +type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) + +func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + return f(ctx, obj) +} diff --git a/internal/controllers/core/clustercatalog_controller.go b/internal/controllers/core/clustercatalog_controller.go index 6b889cf9..65ccd5f1 100644 --- a/internal/controllers/core/clustercatalog_controller.go +++ b/internal/controllers/core/clustercatalog_controller.go @@ -18,17 +18,17 @@ package core import ( "context" // #nosec + "errors" "fmt" "time" "k8s.io/apimachinery/pkg/api/equality" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - apimacherrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/wait" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" "sigs.k8s.io/controller-runtime/pkg/log" "github.com/operator-framework/catalogd/api/core/v1alpha1" @@ -37,7 +37,7 @@ import ( ) const ( - fbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" + FbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" // CatalogSources are polled if PollInterval is mentioned, in intervals of wait.Jitter(pollDuration, maxFactor) // wait.Jitter returns a time.Duration between pollDuration and pollDuration + maxFactor * pollDuration. requeueJitterMaxFactor = 0.01 @@ -46,8 +46,9 @@ const ( // ClusterCatalogReconciler reconciles a Catalog object type ClusterCatalogReconciler struct { client.Client - Unpacker source.Unpacker - Storage storage.Instance + Unpacker source.Unpacker + Storage storage.Instance + Finalizers crfinalizer.Finalizers } //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clustercatalogs,verbs=get;list;watch;create;update;patch;delete @@ -73,23 +74,35 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque reconciledCatsrc := existingCatsrc.DeepCopy() res, reconcileErr := r.reconcile(ctx, reconciledCatsrc) - // Update the status subresource before updating the main object. This is - // necessary because, in many cases, the main object update will remove the - // finalizer, which will cause the core Kubernetes deletion logic to - // complete. Therefore, we need to make the status update prior to the main - // object update to ensure that the status update can be processed before - // a potential deletion. - if !equality.Semantic.DeepEqual(existingCatsrc.Status, reconciledCatsrc.Status) { - if updateErr := r.Client.Status().Update(ctx, reconciledCatsrc); updateErr != nil { - return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr}) + // Do checks before any Update()s, as Update() may modify the resource structure! + updateStatus := !equality.Semantic.DeepEqual(existingCatsrc.Status, reconciledCatsrc.Status) + updateFinalizers := !equality.Semantic.DeepEqual(existingCatsrc.Finalizers, reconciledCatsrc.Finalizers) + unexpectedFieldsChanged := checkForUnexpectedFieldChange(existingCatsrc, *reconciledCatsrc) + + if unexpectedFieldsChanged { + panic("spec or metadata changed by reconciler") + } + + // Save the finalizers off to the side. If we update the status, the reconciledCatsrc will be updated + // to contain the new state of the ClusterCatalog, which contains the status update, but (critically) + // does not contain the finalizers. After the status update, we need to re-add the finalizers to the + // reconciledCatsrc before updating the object. + finalizers := reconciledCatsrc.Finalizers + + if updateStatus { + if err := r.Client.Status().Update(ctx, reconciledCatsrc); err != nil { + reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating status: %v", err)) } } - existingCatsrc.Status, reconciledCatsrc.Status = v1alpha1.ClusterCatalogStatus{}, v1alpha1.ClusterCatalogStatus{} - if !equality.Semantic.DeepEqual(existingCatsrc, reconciledCatsrc) { - if updateErr := r.Client.Update(ctx, reconciledCatsrc); updateErr != nil { - return res, apimacherrors.NewAggregate([]error{reconcileErr, updateErr}) + + reconciledCatsrc.Finalizers = finalizers + + if updateFinalizers { + if err := r.Client.Update(ctx, reconciledCatsrc); err != nil { + reconcileErr = errors.Join(reconcileErr, fmt.Errorf("error updating finalizers: %v", err)) } } + return res, reconcileErr } @@ -109,18 +122,20 @@ func (r *ClusterCatalogReconciler) SetupWithManager(mgr ctrl.Manager) error { // linting from the linter that was fussing about this. // nolint:unparam func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *v1alpha1.ClusterCatalog) (ctrl.Result, error) { - if catalog.DeletionTimestamp.IsZero() && !controllerutil.ContainsFinalizer(catalog, fbcDeletionFinalizer) { - controllerutil.AddFinalizer(catalog, fbcDeletionFinalizer) - return ctrl.Result{}, nil + finalizeResult, err := r.Finalizers.Finalize(ctx, catalog) + if err != nil { + // TODO: For now, this error handling follows the pattern of other error handling. + // Namely: zero just about everything out, throw our hands up, and return an error. + // This is not ideal, and we should consider a more nuanced approach that resolves + // as much status as possible before returning, or at least keeps previous state if + // it is properly labeled with its observed generation. + updateStatusStorageError(&catalog.Status, err) + updateStatusUnpackFailing(&catalog.Status, err) + return ctrl.Result{}, err } - if !catalog.DeletionTimestamp.IsZero() { - if err := r.Storage.Delete(catalog.Name); err != nil { - return ctrl.Result{}, updateStatusStorageDeleteError(&catalog.Status, err) - } - if err := r.Unpacker.Cleanup(ctx, catalog); err != nil { - return ctrl.Result{}, updateStatusStorageDeleteError(&catalog.Status, err) - } - controllerutil.RemoveFinalizer(catalog, fbcDeletionFinalizer) + if finalizeResult.Updated || finalizeResult.StatusUpdated { + // On create: make sure the finalizer is applied before we do anything + // On delete: make sure we do nothing after the finalizer is removed return ctrl.Result{}, nil } @@ -230,7 +245,7 @@ func updateStatusStorageError(status *v1alpha1.ClusterCatalogStatus, err error) return err } -func updateStatusStorageDeleteError(status *v1alpha1.ClusterCatalogStatus, err error) error { +func UpdateStatusStorageDeleteError(status *v1alpha1.ClusterCatalogStatus, err error) error { meta.SetStatusCondition(&status.Conditions, metav1.Condition{ Type: v1alpha1.TypeDelete, Status: metav1.ConditionFalse, @@ -271,3 +286,10 @@ func (r *ClusterCatalogReconciler) needsUnpacking(catalog *v1alpha1.ClusterCatal // time to unpack return true } + +// Compare resources - ignoring status & metadata.finalizers +func checkForUnexpectedFieldChange(a, b v1alpha1.ClusterCatalog) bool { + a.Status, b.Status = v1alpha1.ClusterCatalogStatus{}, v1alpha1.ClusterCatalogStatus{} + a.Finalizers, b.Finalizers = []string{}, []string{} + return !equality.Semantic.DeepEqual(a, b) +} diff --git a/internal/controllers/core/clustercatalog_controller_test.go b/internal/controllers/core/clustercatalog_controller_test.go index c5edb742..286a1244 100644 --- a/internal/controllers/core/clustercatalog_controller_test.go +++ b/internal/controllers/core/clustercatalog_controller_test.go @@ -96,7 +96,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -108,7 +108,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -135,7 +135,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -149,7 +149,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -179,7 +179,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -193,7 +193,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -224,7 +224,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -238,7 +238,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -269,7 +269,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -283,7 +283,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -316,7 +316,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -330,7 +330,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -367,7 +367,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -381,7 +381,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -422,7 +422,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -441,7 +441,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -479,7 +479,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -494,7 +494,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -553,7 +553,7 @@ func TestPollingRequeue(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -570,7 +570,7 @@ func TestPollingRequeue(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -612,7 +612,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -630,7 +630,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, Generation: 2, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -667,7 +667,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, Generation: 2, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -705,7 +705,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, Generation: 2, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -743,7 +743,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{fbcDeletionFinalizer}, + Finalizers: []string{FbcDeletionFinalizer}, Generation: 3, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ From 22580fd2603efe591ceb823e176d3b2f2e36da85 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 19 Sep 2024 14:15:53 -0400 Subject: [PATCH 2/3] Move the finalizer setup Move the finalizer setup out from main.go, and into the controller code. This allows the finalizers to be initialized and used in the test code. Signed-off-by: Todd Short --- cmd/manager/main.go | 27 ++-------- .../core/clustercatalog_controller.go | 33 ++++++++++-- .../core/clustercatalog_controller_test.go | 51 ++++++++++++++++--- 3 files changed, 76 insertions(+), 35 deletions(-) diff --git a/cmd/manager/main.go b/cmd/manager/main.go index fd9346ef..bb0b6df3 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -17,7 +17,6 @@ limitations under the License. package main import ( - "context" "crypto/tls" "flag" "fmt" @@ -36,8 +35,6 @@ import ( _ "k8s.io/client-go/plugin/pkg/client/auth" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/certwatcher" - "sigs.k8s.io/controller-runtime/pkg/client" - crfinalizer "sigs.k8s.io/controller-runtime/pkg/finalizer" "sigs.k8s.io/controller-runtime/pkg/healthz" "sigs.k8s.io/controller-runtime/pkg/log/zap" "sigs.k8s.io/controller-runtime/pkg/metrics" @@ -225,21 +222,9 @@ func main() { os.Exit(1) } - clusterCatalogFinalizers := crfinalizer.NewFinalizers() - if err := clusterCatalogFinalizers.Register(corecontrollers.FbcDeletionFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { - catalog, ok := obj.(*v1alpha1.ClusterCatalog) - if !ok { - panic("could not convert object to clusterCatalog") - } - if err := localStorage.Delete(catalog.Name); err != nil { - return crfinalizer.Result{}, corecontrollers.UpdateStatusStorageDeleteError(&catalog.Status, err) - } - if err := unpacker.Cleanup(ctx, catalog); err != nil { - return crfinalizer.Result{}, corecontrollers.UpdateStatusStorageDeleteError(&catalog.Status, err) - } - return crfinalizer.Result{}, nil - })); err != nil { - setupLog.Error(err, "unable to register finalizer", "finalizerKey", corecontrollers.FbcDeletionFinalizer) + clusterCatalogFinalizers, err := corecontrollers.NewFinalizers(localStorage, unpacker) + if err != nil { + setupLog.Error(err, "unable to configure finalizers") os.Exit(1) } @@ -301,9 +286,3 @@ func podNamespace() string { } return string(namespace) } - -type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) - -func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { - return f(ctx, obj) -} diff --git a/internal/controllers/core/clustercatalog_controller.go b/internal/controllers/core/clustercatalog_controller.go index 65ccd5f1..50eeb113 100644 --- a/internal/controllers/core/clustercatalog_controller.go +++ b/internal/controllers/core/clustercatalog_controller.go @@ -129,8 +129,8 @@ func (r *ClusterCatalogReconciler) reconcile(ctx context.Context, catalog *v1alp // This is not ideal, and we should consider a more nuanced approach that resolves // as much status as possible before returning, or at least keeps previous state if // it is properly labeled with its observed generation. - updateStatusStorageError(&catalog.Status, err) - updateStatusUnpackFailing(&catalog.Status, err) + _ = updateStatusStorageError(&catalog.Status, err) + _ = updateStatusUnpackFailing(&catalog.Status, err) return ctrl.Result{}, err } if finalizeResult.Updated || finalizeResult.StatusUpdated { @@ -245,7 +245,7 @@ func updateStatusStorageError(status *v1alpha1.ClusterCatalogStatus, err error) return err } -func UpdateStatusStorageDeleteError(status *v1alpha1.ClusterCatalogStatus, err error) error { +func updateStatusStorageDeleteError(status *v1alpha1.ClusterCatalogStatus, err error) error { meta.SetStatusCondition(&status.Conditions, metav1.Condition{ Type: v1alpha1.TypeDelete, Status: metav1.ConditionFalse, @@ -293,3 +293,30 @@ func checkForUnexpectedFieldChange(a, b v1alpha1.ClusterCatalog) bool { a.Finalizers, b.Finalizers = []string{}, []string{} return !equality.Semantic.DeepEqual(a, b) } + +type finalizerFunc func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) + +func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + return f(ctx, obj) +} + +func NewFinalizers(localStorage storage.Instance, unpacker source.Unpacker) (crfinalizer.Finalizers, error) { + f := crfinalizer.NewFinalizers() + err := f.Register(FbcDeletionFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + catalog, ok := obj.(*v1alpha1.ClusterCatalog) + if !ok { + panic("could not convert object to clusterCatalog") + } + if err := localStorage.Delete(catalog.Name); err != nil { + return crfinalizer.Result{}, updateStatusStorageDeleteError(&catalog.Status, err) + } + if err := unpacker.Cleanup(ctx, catalog); err != nil { + return crfinalizer.Result{}, updateStatusStorageDeleteError(&catalog.Status, err) + } + return crfinalizer.Result{}, nil + })) + if err != nil { + return f, err + } + return f, nil +} diff --git a/internal/controllers/core/clustercatalog_controller_test.go b/internal/controllers/core/clustercatalog_controller_test.go index 286a1244..7d48aac3 100644 --- a/internal/controllers/core/clustercatalog_controller_test.go +++ b/internal/controllers/core/clustercatalog_controller_test.go @@ -403,9 +403,14 @@ func TestCatalogdControllerReconcile(t *testing.T) { }, }, { - name: "storage finalizer not set, storage finalizer gets set", - source: &MockSource{}, - store: &MockStore{}, + name: "storage finalizer not set, storage finalizer gets set", + source: &MockSource{ + result: &source.Result{ + State: source.StateUnpacked, + FS: &fstest.MapFS{}, + }, + }, + store: &MockStore{}, catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", @@ -435,9 +440,14 @@ func TestCatalogdControllerReconcile(t *testing.T) { }, }, { - name: "storage finalizer set, catalog deletion timestamp is not zero (or nil), finalizer removed", - source: &MockSource{}, - store: &MockStore{}, + name: "storage finalizer set, catalog deletion timestamp is not zero (or nil), finalizer removed", + source: &MockSource{ + result: &source.Result{ + State: source.StateUnpacked, + FS: &fstest.MapFS{}, + }, + }, + store: &MockStore{}, catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", @@ -472,7 +482,12 @@ func TestCatalogdControllerReconcile(t *testing.T) { { name: "storage finalizer set, catalog deletion timestamp is not zero (or nil), storage delete failed, error returned and finalizer not removed", shouldErr: true, - source: &MockSource{}, + source: &MockSource{ + result: &source.Result{ + State: source.StateUnpacked, + FS: &fstest.MapFS{}, + }, + }, store: &MockStore{ shouldError: true, }, @@ -512,6 +527,11 @@ func TestCatalogdControllerReconcile(t *testing.T) { Status: metav1.ConditionFalse, Reason: catalogdv1alpha1.ReasonStorageDeleteFailed, }, + { + Type: catalogdv1alpha1.TypeUnpacked, + Status: metav1.ConditionFalse, + Reason: catalogdv1alpha1.ReasonUnpackFailed, + }, }, }, }, @@ -523,6 +543,11 @@ func TestCatalogdControllerReconcile(t *testing.T) { Unpacker: tt.source, Storage: tt.store, } + var err error + reconciler.Finalizers, err = NewFinalizers(reconciler.Storage, reconciler.Unpacker) + if err != nil { + panic("unable to initialize finalizers") + } ctx := context.Background() if tt.shouldPanic { @@ -594,6 +619,11 @@ func TestPollingRequeue(t *testing.T) { }}, Storage: &MockStore{}, } + var err error + reconciler.Finalizers, err = NewFinalizers(reconciler.Storage, reconciler.Unpacker) + if err != nil { + panic("unable to initialize finalizers") + } res, _ := reconciler.reconcile(context.Background(), tc.catalog) assert.GreaterOrEqual(t, res.RequeueAfter, tc.expectedRequeueAfter) // wait.Jitter used to calculate requeueAfter by the reconciler @@ -784,7 +814,12 @@ func TestPollingReconcilerUnpack(t *testing.T) { Unpacker: &MockSource{shouldError: true}, Storage: &MockStore{}, } - _, err := reconciler.reconcile(context.Background(), tc.catalog) + var err error + reconciler.Finalizers, err = NewFinalizers(reconciler.Storage, reconciler.Unpacker) + if err != nil { + panic("unable to initialize finalizers") + } + _, err = reconciler.reconcile(context.Background(), tc.catalog) if tc.expectedUnpackRun { assert.Error(t, err) } else { From 9082ae8d2fcd4803ed1408fdbca55e7528dfc9f1 Mon Sep 17 00:00:00 2001 From: Todd Short Date: Thu, 19 Sep 2024 14:22:06 -0400 Subject: [PATCH 3/3] Undo rename of fbcDeletionFinalizer Signed-off-by: Todd Short --- .../core/clustercatalog_controller.go | 4 +- .../core/clustercatalog_controller_test.go | 50 +++++++++---------- 2 files changed, 27 insertions(+), 27 deletions(-) diff --git a/internal/controllers/core/clustercatalog_controller.go b/internal/controllers/core/clustercatalog_controller.go index 50eeb113..ffae9ac1 100644 --- a/internal/controllers/core/clustercatalog_controller.go +++ b/internal/controllers/core/clustercatalog_controller.go @@ -37,7 +37,7 @@ import ( ) const ( - FbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" + fbcDeletionFinalizer = "olm.operatorframework.io/delete-server-cache" // CatalogSources are polled if PollInterval is mentioned, in intervals of wait.Jitter(pollDuration, maxFactor) // wait.Jitter returns a time.Duration between pollDuration and pollDuration + maxFactor * pollDuration. requeueJitterMaxFactor = 0.01 @@ -302,7 +302,7 @@ func (f finalizerFunc) Finalize(ctx context.Context, obj client.Object) (crfinal func NewFinalizers(localStorage storage.Instance, unpacker source.Unpacker) (crfinalizer.Finalizers, error) { f := crfinalizer.NewFinalizers() - err := f.Register(FbcDeletionFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { + err := f.Register(fbcDeletionFinalizer, finalizerFunc(func(ctx context.Context, obj client.Object) (crfinalizer.Result, error) { catalog, ok := obj.(*v1alpha1.ClusterCatalog) if !ok { panic("could not convert object to clusterCatalog") diff --git a/internal/controllers/core/clustercatalog_controller_test.go b/internal/controllers/core/clustercatalog_controller_test.go index 7d48aac3..4c145519 100644 --- a/internal/controllers/core/clustercatalog_controller_test.go +++ b/internal/controllers/core/clustercatalog_controller_test.go @@ -96,7 +96,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -108,7 +108,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -135,7 +135,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -149,7 +149,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -179,7 +179,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -193,7 +193,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -224,7 +224,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -238,7 +238,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -269,7 +269,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -283,7 +283,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -316,7 +316,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -330,7 +330,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -367,7 +367,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -381,7 +381,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -427,7 +427,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -451,7 +451,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -494,7 +494,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -509,7 +509,7 @@ func TestCatalogdControllerReconcile(t *testing.T) { expectedCatalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, DeletionTimestamp: &metav1.Time{Time: time.Date(2023, time.October, 10, 4, 19, 0, 0, time.UTC)}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -578,7 +578,7 @@ func TestPollingRequeue(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -595,7 +595,7 @@ func TestPollingRequeue(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -642,7 +642,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ Source: catalogdv1alpha1.CatalogSource{ @@ -660,7 +660,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, Generation: 2, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -697,7 +697,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, Generation: 2, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -735,7 +735,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, Generation: 2, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{ @@ -773,7 +773,7 @@ func TestPollingReconcilerUnpack(t *testing.T) { catalog: &catalogdv1alpha1.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: "test-catalog", - Finalizers: []string{FbcDeletionFinalizer}, + Finalizers: []string{fbcDeletionFinalizer}, Generation: 3, }, Spec: catalogdv1alpha1.ClusterCatalogSpec{