From 4386bb096010e949bad0e363199a0d13a2e8228d Mon Sep 17 00:00:00 2001
From: Jim Crossley <jim@crossleys.org>
Date: Tue, 26 Nov 2019 13:31:57 -0500
Subject: [PATCH] Delete manifest resources in a finalizer

This should be more predictable and thread-safe, i.e. The Right Way.
---
 .../knativeserving_controller.go              | 46 +++++++++++++++++--
 test/e2e/knativeservingdeployment_test.go     | 24 +++++-----
 2 files changed, 54 insertions(+), 16 deletions(-)

diff --git a/pkg/reconciler/knativeserving/knativeserving_controller.go b/pkg/reconciler/knativeserving/knativeserving_controller.go
index bfe992f1..3092fa37 100644
--- a/pkg/reconciler/knativeserving/knativeserving_controller.go
+++ b/pkg/reconciler/knativeserving/knativeserving_controller.go
@@ -39,6 +39,10 @@ import (
 	"knative.dev/serving-operator/version"
 )
 
+const (
+	finalizerName = "delete-knative-serving-manifest"
+)
+
 var (
 	// Platform-specific behavior to affect the installation
 	platform common.Platforms
@@ -69,17 +73,16 @@ func (r *Reconciler) Reconcile(ctx context.Context, key string) error {
 	// Get the KnativeServing resource with this namespace/name.
 	original, err := r.knativeServingLister.KnativeServings(namespace).Get(name)
 	if apierrs.IsNotFound(err) {
-		// The resource was deleted
-		r.servings.Delete(key)
-		if r.servings.Len() == 0 {
-			r.config.DeleteAll(&metav1.DeleteOptions{})
-		}
 		return nil
 
 	} else if err != nil {
 		r.Logger.Error(err, "Error getting KnativeServing")
 		return err
 	}
+	if original.GetDeletionTimestamp() != nil {
+		r.servings.Delete(key)
+		return r.delete(original)
+	}
 	// Keep track of the number of KnativeServings in the cluster
 	r.servings.Insert(key)
 
@@ -112,6 +115,7 @@ func (r *Reconciler) reconcile(ctx context.Context, ks *servingv1alpha1.KnativeS
 	reqLogger.Infow("Reconciling KnativeServing", "status", ks.Status)
 
 	stages := []func(*mf.Manifest, *servingv1alpha1.KnativeServing) error{
+		r.ensureFinalizer,
 		r.initStatus,
 		r.install,
 		r.checkDeployments,
@@ -210,6 +214,38 @@ func (r *Reconciler) checkDeployments(manifest *mf.Manifest, instance *servingv1
 	return nil
 }
 
+// ensureFinalizer attaches a "delete manifest" finalizer to the instance
+func (r *Reconciler) ensureFinalizer(manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
+	for _, finalizer := range instance.GetFinalizers() {
+		if finalizer == finalizerName {
+			return nil
+		}
+	}
+	instance.SetFinalizers(append(instance.GetFinalizers(), finalizerName))
+	instance, err := r.KnativeServingClientSet.OperatorV1alpha1().KnativeServings(instance.Namespace).Update(instance)
+	return err
+}
+
+// delete all the resources in the release manifest
+func (r *Reconciler) delete(instance *servingv1alpha1.KnativeServing) error {
+	if len(instance.GetFinalizers()) == 0 || instance.GetFinalizers()[0] != finalizerName {
+		return nil
+	}
+	if r.servings.Len() == 0 {
+		if err := r.config.DeleteAll(&metav1.DeleteOptions{}); err != nil {
+			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
+}
+
 // Delete obsolete resources from previous versions
 func (r *Reconciler) deleteObsoleteResources(manifest *mf.Manifest, instance *servingv1alpha1.KnativeServing) error {
 	// istio-system resources from 0.3
diff --git a/test/e2e/knativeservingdeployment_test.go b/test/e2e/knativeservingdeployment_test.go
index ed985a1a..1eb1928d 100644
--- a/test/e2e/knativeservingdeployment_test.go
+++ b/test/e2e/knativeservingdeployment_test.go
@@ -215,6 +215,16 @@ func knativeServingDelete(t *testing.T, clients *test.Clients, names test.Resour
 	if err := clients.KnativeServing().Delete(names.KnativeServing, &metav1.DeleteOptions{}); err != nil {
 		t.Fatalf("KnativeServing %q failed to delete: %v", names.KnativeServing, err)
 	}
+	err := wait.PollImmediate(resources.Interval, resources.Timeout, func() (bool, error) {
+		_, err := clients.KnativeServing().Get(names.KnativeServing, metav1.GetOptions{})
+		if apierrs.IsNotFound(err) {
+			return true, nil
+		}
+		return false, err
+	})
+	if err != nil {
+		t.Fatal("Timed out waiting on KnativeServing to delete", err)
+	}
 	_, b, _, _ := runtime.Caller(0)
 	m, err := mf.NewManifest(filepath.Join((filepath.Dir(b)+"/.."), "config/"), false, clients.Config)
 	if err != nil {
@@ -229,18 +239,10 @@ func knativeServingDelete(t *testing.T, clients *test.Clients, names test.Resour
 			// been modified, since the namespace can be injected.
 			continue
 		}
-		waitErr := wait.PollImmediate(resources.Interval, resources.Timeout, func() (bool, error) {
-			gvrs, _ := meta.UnsafeGuessKindToResource(u.GroupVersionKind())
-			if _, err := clients.Dynamic.Resource(gvrs).Get(u.GetName(), metav1.GetOptions{}); apierrs.IsNotFound(err) {
-				return true, nil
-			}
-			return false, err
-		})
-
-		if waitErr != nil {
-			t.Fatalf("The %s %s failed to be deleted: %v", u.GetKind(), u.GetName(), waitErr)
+		gvrs, _ := meta.UnsafeGuessKindToResource(u.GroupVersionKind())
+		if _, err := clients.Dynamic.Resource(gvrs).Get(u.GetName(), metav1.GetOptions{}); !apierrs.IsNotFound(err) {
+			t.Fatalf("The %s %s failed to be deleted", u.GetKind(), u.GetName())
 		}
-		t.Logf("The %s %s has been deleted.", u.GetKind(), u.GetName())
 	}
 }