From 927f7f00c747d7ad0929ac495816391ad9c6df9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 27 Apr 2020 16:27:33 +0200 Subject: [PATCH 1/3] Remove obsolete finalizer from KnativeServing resources. The genreconciler machinery adds a finalizer automatically. There's no need to define our own and do our own management of it. We need to make sure to remove it from old resources though. --- .../knativeserving/knativeserving.go | 60 +++++++++++-------- 1 file changed, 36 insertions(+), 24 deletions(-) diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go index 166e3aa2d9..4122550577 100644 --- a/pkg/reconciler/knativeserving/knativeserving.go +++ b/pkg/reconciler/knativeserving/knativeserving.go @@ -18,6 +18,7 @@ package knativeserving import ( "context" + "encoding/json" "fmt" mf "github.com/manifestival/manifestival" @@ -29,6 +30,8 @@ import ( "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" servingv1alpha1 "knative.dev/operator/pkg/apis/operator/v1alpha1" @@ -43,10 +46,10 @@ import ( ) const ( - finalizerName = "delete-knative-serving-manifest" - creationChange = "creation" - editChange = "edit" - deletionChange = "deletion" + oldFinalizerName = "delete-knative-serving-manifest" + creationChange = "creation" + editChange = "edit" + deletionChange = "deletion" ) var ( @@ -135,7 +138,7 @@ func (r *Reconciler) reconcile(ctx context.Context, ks *servingv1alpha1.KnativeS reqLogger.Infow("Reconciling KnativeServing", "status", ks.Status) stages := []func(context.Context, *mf.Manifest, *servingv1alpha1.KnativeServing) error{ - r.ensureFinalizer, + r.ensureFinalizerRemoval, r.initStatus, r.install, r.checkDeployments, @@ -252,24 +255,40 @@ func (r *Reconciler) checkDeployments(ctx context.Context, manifest *mf.Manifest } // ensureFinalizer attaches a "delete manifest" finalizer to the instance -func (r *Reconciler) ensureFinalizer(ctx context.Context, manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error { - for _, finalizer := range instance.GetFinalizers() { - if finalizer == finalizerName { - return nil - } +func (r *Reconciler) ensureFinalizerRemoval(_ context.Context, _ *mf.Manifest, instance *servingv1alpha1.KnativeServing) error { + finalizers := sets.NewString(instance.Finalizers...) + + if !finalizers.Has(oldFinalizerName) { + // Nothing to do. + return nil + } + + // Remove the finalizer + finalizers.Delete(oldFinalizerName) + + mergePatch := map[string]interface{}{ + "metadata": map[string]interface{}{ + "finalizers": finalizers.List(), + "resourceVersion": instance.ResourceVersion, + }, + } + + patch, err := json.Marshal(mergePatch) + if err != nil { + return fmt.Errorf("failed to construct finalizer patch: %w", err) } - instance.SetFinalizers(append(instance.GetFinalizers(), finalizerName)) - instance, err := r.knativeServingClientSet.OperatorV1alpha1().KnativeServings(instance.Namespace).Update(instance) - return err + + patcher := r.knativeServingClientSet.OperatorV1alpha1().KnativeServings(instance.Namespace) + if _, err := patcher.Patch(instance.Name, types.MergePatchType, patch); err != nil { + return fmt.Errorf("failed to patch finalizer away: %w", err) + } + return nil } // delete all the resources in the release manifest func (r *Reconciler) delete(ctx context.Context, instance *servingv1alpha1.KnativeServing) error { logger := logging.FromContext(ctx) - if len(instance.GetFinalizers()) == 0 || instance.GetFinalizers()[0] != finalizerName { - return nil - } logger.Info("Deleting resources") var RBAC = mf.Any(mf.ByKind("Role"), mf.ByKind("ClusterRole"), mf.ByKind("RoleBinding"), mf.ByKind("ClusterRoleBinding")) if len(r.servings) == 0 { @@ -284,14 +303,7 @@ func (r *Reconciler) delete(ctx context.Context, instance *servingv1alpha1.Knati return err } } - // The deletionTimestamp might've changed. Fetch the resource again. - refetched, err := r.knativeServingLister.KnativeServings(instance.Namespace).Get(instance.Name) - if err != nil { - return err - } - refetched.SetFinalizers(refetched.GetFinalizers()[1:]) - _, err = r.knativeServingClientSet.OperatorV1alpha1().KnativeServings(refetched.Namespace).Update(refetched) - return err + return nil } // Delete obsolete resources from previous versions From b53192facb97fc533a38a9afdb4f1f61f17ceb3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 27 Apr 2020 17:28:17 +0200 Subject: [PATCH 2/3] Adjust comment. --- pkg/reconciler/knativeserving/knativeserving.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go index 4122550577..b22c410392 100644 --- a/pkg/reconciler/knativeserving/knativeserving.go +++ b/pkg/reconciler/knativeserving/knativeserving.go @@ -254,7 +254,7 @@ func (r *Reconciler) checkDeployments(ctx context.Context, manifest *mf.Manifest return nil } -// ensureFinalizer attaches a "delete manifest" finalizer to the instance +// ensureFinalizerRemoval ensures that the obsolete "delete-knative-serving-manifest" is removed from the resource. func (r *Reconciler) ensureFinalizerRemoval(_ context.Context, _ *mf.Manifest, instance *servingv1alpha1.KnativeServing) error { finalizers := sets.NewString(instance.Finalizers...) From b301cf56e6b30e316158ad938efeaa20affc2301 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Markus=20Th=C3=B6mmes?= Date: Mon, 27 Apr 2020 17:44:05 +0200 Subject: [PATCH 3/3] Move function to FinalizeKind. --- .../knativeserving/knativeserving.go | 38 ++++++++----------- 1 file changed, 16 insertions(+), 22 deletions(-) diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go index b22c410392..70db39ab49 100644 --- a/pkg/reconciler/knativeserving/knativeserving.go +++ b/pkg/reconciler/knativeserving/knativeserving.go @@ -90,7 +90,22 @@ func (r *Reconciler) FinalizeKind(ctx context.Context, original *servingv1alpha1 if _, ok := r.servings[key]; ok { delete(r.servings, key) } - return r.delete(ctx, original) + + logger.Info("Deleting resources") + var RBAC = mf.Any(mf.ByKind("Role"), mf.ByKind("ClusterRole"), mf.ByKind("RoleBinding"), mf.ByKind("ClusterRoleBinding")) + if len(r.servings) == 0 { + if err := r.config.Filter(mf.ByKind("Deployment")).Delete(); err != nil { + return err + } + if err := r.config.Filter(mf.NoCRDs, mf.None(RBAC)).Delete(); err != nil { + return err + } + // Delete Roles last, as they may be useful for human operators to clean up. + if err := r.config.Filter(RBAC).Delete(); err != nil { + return err + } + } + return nil } // ReconcileKind compares the actual state with the desired, and attempts to @@ -285,27 +300,6 @@ func (r *Reconciler) ensureFinalizerRemoval(_ context.Context, _ *mf.Manifest, i return nil } -// delete all the resources in the release manifest -func (r *Reconciler) delete(ctx context.Context, instance *servingv1alpha1.KnativeServing) error { - logger := logging.FromContext(ctx) - - logger.Info("Deleting resources") - var RBAC = mf.Any(mf.ByKind("Role"), mf.ByKind("ClusterRole"), mf.ByKind("RoleBinding"), mf.ByKind("ClusterRoleBinding")) - if len(r.servings) == 0 { - if err := r.config.Filter(mf.ByKind("Deployment")).Delete(); err != nil { - return err - } - if err := r.config.Filter(mf.NoCRDs, mf.None(RBAC)).Delete(); err != nil { - return err - } - // Delete Roles last, as they may be useful for human operators to clean up. - if err := r.config.Filter(RBAC).Delete(); err != nil { - return err - } - } - return nil -} - // Delete obsolete resources from previous versions func (r *Reconciler) deleteObsoleteResources(ctx context.Context, manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error { // istio-system resources from 0.3