From 6ae9696e2c4ce4a41171574cd3c3014c7200d541 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 27 Nov 2019 10:00:45 +0100
Subject: [PATCH 1/9] Update the 'cli-install-uninstall' E2E test to test a
 cleanup plan

---
 test/e2e/cli-install-uninstall/02-errors.yaml |  5 +++++
 .../03-check-events-for-cleanup.yaml          |  4 ++++
 .../first-operator/operator.yaml              | 14 ++++++++++++++
 .../first-operator/templates/cleanup.yaml     | 19 +++++++++++++++++++
 4 files changed, 42 insertions(+)
 create mode 100644 test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml
 create mode 100644 test/e2e/cli-install-uninstall/first-operator/templates/cleanup.yaml

diff --git a/test/e2e/cli-install-uninstall/02-errors.yaml b/test/e2e/cli-install-uninstall/02-errors.yaml
index 7b5331d6b..9d7835b32 100644
--- a/test/e2e/cli-install-uninstall/02-errors.yaml
+++ b/test/e2e/cli-install-uninstall/02-errors.yaml
@@ -10,3 +10,8 @@ apiVersion: apps/v1
 kind: Deployment
 metadata:
   name: nginx-deployment
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: cleanup-deployment
\ No newline at end of file
diff --git a/test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml b/test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml
new file mode 100644
index 000000000..d78dcb1c3
--- /dev/null
+++ b/test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml
@@ -0,0 +1,4 @@
+apiVersion: kudo.dev/v1beta1
+kind: TestStep
+commands:
+- command: bash -c 'kubectl get -A events | grep -q "Execution of plan cleanup finished with status COMPLETE"'
diff --git a/test/e2e/cli-install-uninstall/first-operator/operator.yaml b/test/e2e/cli-install-uninstall/first-operator/operator.yaml
index f7c6cd592..ee0795884 100644
--- a/test/e2e/cli-install-uninstall/first-operator/operator.yaml
+++ b/test/e2e/cli-install-uninstall/first-operator/operator.yaml
@@ -12,6 +12,11 @@ tasks:
     spec:
       resources:
         - deployment.yaml
+  - name: cleanup
+    kind: Apply
+    spec:
+      resources:
+        - cleanup.yaml
 plans:
   deploy:
     strategy: serial
@@ -22,3 +27,12 @@ plans:
           - name: everything
             tasks:
               - app
+  cleanup:
+    strategy: serial
+    phases:
+      - name: main
+        strategy: parallel
+        steps:
+          - name: everything
+            tasks:
+              - cleanup
diff --git a/test/e2e/cli-install-uninstall/first-operator/templates/cleanup.yaml b/test/e2e/cli-install-uninstall/first-operator/templates/cleanup.yaml
new file mode 100644
index 000000000..8f310842d
--- /dev/null
+++ b/test/e2e/cli-install-uninstall/first-operator/templates/cleanup.yaml
@@ -0,0 +1,19 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: cleanup-deployment
+spec:
+  selector:
+    matchLabels:
+      app: nginx
+  replicas: 1
+  template:
+    metadata:
+      labels:
+        app: nginx
+    spec:
+      containers:
+        - name: nginx
+          image: nginx:1.7.9
+          ports:
+            - containerPort: 80

From b3a688500c56a2d9e5c2ff837f6878b038feb772 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 27 Nov 2019 10:02:16 +0100
Subject: [PATCH 2/9] Run a cleanup plan when deleting instances

If an operator provides a 'cleanup' plan, this plan is run when the operator's instance is deleted from the cluster.
This is achieved by detecting an instance deletion in the controller and adding a finalizer.
If there is another plan currently in progress, this will be superseded by the cleanup plan.
This allows the deletion of instances where a plan fails to complete.
---
 pkg/apis/kudo/v1beta1/instance_types.go       |  3 +
 .../instance/instance_controller.go           | 98 +++++++++++++++++--
 2 files changed, 95 insertions(+), 6 deletions(-)

diff --git a/pkg/apis/kudo/v1beta1/instance_types.go b/pkg/apis/kudo/v1beta1/instance_types.go
index 494378aad..98ba3ffef 100644
--- a/pkg/apis/kudo/v1beta1/instance_types.go
+++ b/pkg/apis/kudo/v1beta1/instance_types.go
@@ -159,6 +159,9 @@ const (
 
 	// UpdatePlanName is the name of the update plan
 	UpdatePlanName = "update"
+
+	// CleanupPlanName is the name of the cleanup plan
+	CleanupPlanName = "cleanup"
 )
 
 // IsTerminal returns true if the status is terminal (either complete, or in a nonrecoverable error)
diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
index 7db2dcfd6..6a4636721 100644
--- a/pkg/controller/instance/instance_controller.go
+++ b/pkg/controller/instance/instance_controller.go
@@ -46,6 +46,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
+const instanceCleanupFinalizerName = "kudo.finalizers.instance.cleanup"
+
 // Reconciler reconciles an Instance object.
 type Reconciler struct {
 	client.Client
@@ -105,6 +107,12 @@ func (r *Reconciler) SetupWithManager(
 //                  |
 //                  v
 //   +-------------------------------+
+//   | Start cleanup plan if object  |
+//   | is being deleted              |
+//   +-------------------------------+
+//                  |
+//                  v
+//   +-------------------------------+
 //   | Start new plan if required    |
 //   | and none is running           |
 //   +-------------------------------+
@@ -141,12 +149,40 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 		return reconcile.Result{}, err // OV not found has to be retried because it can really have been created after Instance
 	}
 
-	// ---------- 2. First check if we should start execution of new plan ----------
+	// ---------- 2. Check if the object is being deleted, start cleanup plan ----------
 
-	planToBeExecuted, err := instance.GetPlanToBeExecuted(ov)
-	if err != nil {
-		return reconcile.Result{}, err
+	var planToBeExecuted *string
+
+	// A delete request is indicated by a non-zero 'metadata.deletionTimestamp',
+	// see https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
+	if instance.ObjectMeta.DeletionTimestamp.IsZero() {
+		if _, hasCleanupPlan := ov.Spec.Plans[kudov1beta1.CleanupPlanName]; hasCleanupPlan {
+			if err := r.addFinalizer(instance); err != nil {
+				return reconcile.Result{}, err
+			}
+		}
+	} else {
+		log.Printf("InstanceController: Instance %s/%s is being deleted", instance.Namespace, instance.Name)
+		if contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
+			// Only start the cleanup plan if it isn't currently running or if
+			// it isn't already completed.
+			lastExecutedPlanStatus := instance.GetLastExecutedPlanStatus()
+			if lastExecutedPlanStatus == nil || lastExecutedPlanStatus.Name != kudov1beta1.CleanupPlanName {
+				cleanupPlan := kudov1beta1.CleanupPlanName
+				planToBeExecuted = &cleanupPlan
+			}
+		}
+	}
+
+	// ---------- 3. First check if we should start execution of new plan ----------
+
+	if planToBeExecuted == nil {
+		planToBeExecuted, err = instance.GetPlanToBeExecuted(ov)
+		if err != nil {
+			return reconcile.Result{}, err
+		}
 	}
+
 	if planToBeExecuted != nil {
 		log.Printf("InstanceController: Going to start execution of plan %s on instance %s/%s", kudo.StringValue(planToBeExecuted), instance.Namespace, instance.Name)
 		err = instance.StartPlanExecution(kudo.StringValue(planToBeExecuted), ov)
@@ -156,7 +192,7 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 		r.Recorder.Event(instance, "Normal", "PlanStarted", fmt.Sprintf("Execution of plan %s started", kudo.StringValue(planToBeExecuted)))
 	}
 
-	// ---------- 3. If there's currently active plan, continue with the execution ----------
+	// ---------- 4. If there's currently active plan, continue with the execution ----------
 
 	activePlanStatus := instance.GetPlanInProgress()
 	if activePlanStatus == nil { // we have no plan in progress
@@ -182,7 +218,7 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 	log.Printf("InstanceController: Going to proceed in execution of active plan %s on instance %s/%s", activePlan.Name, instance.Namespace, instance.Name)
 	newStatus, err := workflow.Execute(activePlan, metadata, r.Client, &renderer.KustomizeEnhancer{Scheme: r.Scheme}, time.Now())
 
-	// ---------- 4. Update status of instance after the execution proceeded ----------
+	// ---------- 5. Update status of instance after the execution proceeded ----------
 	if newStatus != nil {
 		instance.UpdateInstanceStatus(newStatus)
 	}
@@ -199,6 +235,13 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 
 	if instance.Status.AggregatedStatus.Status.IsTerminal() {
 		r.Recorder.Event(instance, "Normal", "PlanFinished", fmt.Sprintf("Execution of plan %s finished with status %s", activePlanStatus.Name, instance.Status.AggregatedStatus.Status))
+
+		lastExecutedPlanStatus := instance.GetLastExecutedPlanStatus()
+		if lastExecutedPlanStatus != nil && lastExecutedPlanStatus.Name == kudov1beta1.CleanupPlanName {
+			if err := r.removeFinalizer(instance); err != nil {
+				return reconcile.Result{}, err
+			}
+		}
 	}
 
 	return reconcile.Result{}, nil
@@ -348,3 +391,46 @@ func parameterDifference(old, new map[string]string) map[string]string {
 
 	return diff
 }
+
+func contains(values []string, s string) bool {
+	for _, value := range values {
+		if value == s {
+			return true
+		}
+	}
+	return false
+}
+
+func (r *Reconciler) addFinalizer(instance *kudov1beta1.Instance) error {
+	if !contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
+		log.Printf("InstanceController: Adding finalizer on instance %s/%s", instance.Namespace, instance.Name)
+		instance.ObjectMeta.Finalizers = append(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
+		if err := r.Update(context.TODO(), instance); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+func (r *Reconciler) removeFinalizer(instance *kudov1beta1.Instance) error {
+	remove := func(values []string, s string) (result []string) {
+		for _, value := range values {
+			if value == s {
+				continue
+			}
+			result = append(result, value)
+		}
+		return
+	}
+
+	if contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
+		log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
+		instance.ObjectMeta.Finalizers = remove(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
+		if err := r.Update(context.TODO(), instance); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}

From d725947adac6ca695f24d88a52bb3773004d238d Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Tue, 3 Dec 2019 11:30:51 +0100
Subject: [PATCH 3/9] Address review issues.

---
 pkg/apis/kudo/v1beta1/instance_types.go       | 29 +++++-
 .../instance/instance_controller.go           | 92 +++++++++----------
 test/e2e/cli-install-uninstall/02-assert.yaml |  3 +
 .../03-check-events-for-cleanup.yaml          |  4 -
 4 files changed, 75 insertions(+), 53 deletions(-)
 create mode 100644 test/e2e/cli-install-uninstall/02-assert.yaml
 delete mode 100644 test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml

diff --git a/pkg/apis/kudo/v1beta1/instance_types.go b/pkg/apis/kudo/v1beta1/instance_types.go
index 98ba3ffef..9dd236ffd 100644
--- a/pkg/apis/kudo/v1beta1/instance_types.go
+++ b/pkg/apis/kudo/v1beta1/instance_types.go
@@ -390,8 +390,35 @@ func selectPlan(possiblePlans []string, ov *OperatorVersion) *string {
 	return nil
 }
 
+func (i *Instance) PlanStatus(plan string) *PlanStatus {
+	for _, planStatus := range i.Status.PlanStatus {
+		if planStatus.Name == plan {
+			return &planStatus
+		}
+	}
+
+	return nil
+}
+
 // GetPlanToBeExecuted returns name of the plan that should be executed
-func (i *Instance) GetPlanToBeExecuted(ov *OperatorVersion) (*string, error) {
+func (i *Instance) GetPlanToBeExecuted(ov *OperatorVersion, isDeleting bool) (*string, error) {
+	// the instance is being deleted
+	if isDeleting {
+		// we have a cleanup plan
+		plan := selectPlan([]string{CleanupPlanName}, ov)
+		if plan != nil {
+			if planStatus := i.PlanStatus(*plan); planStatus != nil {
+				if !planStatus.Status.IsRunning() {
+					if planStatus.Status.IsFinished() {
+						// we already finished the cleanup plan
+						return nil, nil
+					}
+					return plan, nil
+				}
+			}
+		}
+	}
+
 	if i.GetPlanInProgress() != nil { // we're already running some plan
 		return nil, nil
 	}
diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
index 6a4636721..a1d1f146e 100644
--- a/pkg/controller/instance/instance_controller.go
+++ b/pkg/controller/instance/instance_controller.go
@@ -37,6 +37,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 
+	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	kudov1beta1 "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	appsv1 "k8s.io/api/apps/v1"
 	batchv1 "k8s.io/api/batch/v1"
@@ -151,36 +152,24 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 
 	// ---------- 2. Check if the object is being deleted, start cleanup plan ----------
 
-	var planToBeExecuted *string
+	var isDeleting bool
 
-	// A delete request is indicated by a non-zero 'metadata.deletionTimestamp',
+	// a delete request is indicated by a non-zero 'metadata.deletionTimestamp',
 	// see https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
 	if instance.ObjectMeta.DeletionTimestamp.IsZero() {
 		if _, hasCleanupPlan := ov.Spec.Plans[kudov1beta1.CleanupPlanName]; hasCleanupPlan {
-			if err := r.addFinalizer(instance); err != nil {
-				return reconcile.Result{}, err
-			}
+			maybeAddFinalizer(instance)
 		}
 	} else {
 		log.Printf("InstanceController: Instance %s/%s is being deleted", instance.Namespace, instance.Name)
-		if contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
-			// Only start the cleanup plan if it isn't currently running or if
-			// it isn't already completed.
-			lastExecutedPlanStatus := instance.GetLastExecutedPlanStatus()
-			if lastExecutedPlanStatus == nil || lastExecutedPlanStatus.Name != kudov1beta1.CleanupPlanName {
-				cleanupPlan := kudov1beta1.CleanupPlanName
-				planToBeExecuted = &cleanupPlan
-			}
-		}
+		isDeleting = true
 	}
 
-	// ---------- 3. First check if we should start execution of new plan ----------
+	// ---------- 3. Check if we should start execution of new plan ----------
 
-	if planToBeExecuted == nil {
-		planToBeExecuted, err = instance.GetPlanToBeExecuted(ov)
-		if err != nil {
-			return reconcile.Result{}, err
-		}
+	planToBeExecuted, err := instance.GetPlanToBeExecuted(ov, isDeleting)
+	if err != nil {
+		return reconcile.Result{}, err
 	}
 
 	if planToBeExecuted != nil {
@@ -235,13 +224,6 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 
 	if instance.Status.AggregatedStatus.Status.IsTerminal() {
 		r.Recorder.Event(instance, "Normal", "PlanFinished", fmt.Sprintf("Execution of plan %s finished with status %s", activePlanStatus.Name, instance.Status.AggregatedStatus.Status))
-
-		lastExecutedPlanStatus := instance.GetLastExecutedPlanStatus()
-		if lastExecutedPlanStatus != nil && lastExecutedPlanStatus.Name == kudov1beta1.CleanupPlanName {
-			if err := r.removeFinalizer(instance); err != nil {
-				return reconcile.Result{}, err
-			}
-		}
 	}
 
 	return reconcile.Result{}, nil
@@ -265,6 +247,17 @@ func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Ins
 		log.Printf("InstanceController: Error when updating instance status. %v", err)
 		return err
 	}
+
+	// update instance metadata if finalizer is removed
+	// because Kubernetes will immediately delete the instance, this has to be the last instance update
+	maybeRemoveFinalizer(instance)
+	if !reflect.DeepEqual(instance.ObjectMeta.Finalizers, oldInstance.ObjectMeta.Finalizers) {
+		if err := client.Update(context.TODO(), instance); err != nil {
+			log.Printf("InstanceController: Error when removing instance finalizer. %v", err)
+			return err
+		}
+	}
+
 	return nil
 }
 
@@ -401,36 +394,39 @@ func contains(values []string, s string) bool {
 	return false
 }
 
-func (r *Reconciler) addFinalizer(instance *kudov1beta1.Instance) error {
-	if !contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
-		log.Printf("InstanceController: Adding finalizer on instance %s/%s", instance.Namespace, instance.Name)
-		instance.ObjectMeta.Finalizers = append(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
-		if err := r.Update(context.TODO(), instance); err != nil {
-			return err
+func remove(values []string, s string) (result []string) {
+	for _, value := range values {
+		if value == s {
+			continue
 		}
+		result = append(result, value)
 	}
-
-	return nil
+	return
 }
 
-func (r *Reconciler) removeFinalizer(instance *kudov1beta1.Instance) error {
-	remove := func(values []string, s string) (result []string) {
-		for _, value := range values {
-			if value == s {
-				continue
+// maybeAddFinalizer adds the cleanup finalizer to an instance if the finalizer
+// hasn't been added yet, the instance has a cleanup plan and the cleanup plan
+// did not complete yet.
+func maybeAddFinalizer(instance *kudov1beta1.Instance) {
+	if !contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
+		if planStatus := instance.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
+			if !planStatus.Status.IsFinished() {
+				log.Printf("InstanceController: Adding finalizer on instance %s/%s", instance.Namespace, instance.Name)
+				instance.ObjectMeta.Finalizers = append(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
 			}
-			result = append(result, value)
 		}
-		return
 	}
+}
 
+// maybeRemoveFinalizer removes the cleanup finalizer of an instance if it has
+// been added, the instance has a cleanup plan and the cleanup plan completed.
+func maybeRemoveFinalizer(instance *kudov1beta1.Instance) {
 	if contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
-		log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
-		instance.ObjectMeta.Finalizers = remove(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
-		if err := r.Update(context.TODO(), instance); err != nil {
-			return err
+		if planStatus := instance.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
+			if planStatus.Status.IsFinished() {
+				log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
+				instance.ObjectMeta.Finalizers = remove(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
+			}
 		}
 	}
-
-	return nil
 }
diff --git a/test/e2e/cli-install-uninstall/02-assert.yaml b/test/e2e/cli-install-uninstall/02-assert.yaml
new file mode 100644
index 000000000..f8c5e2d5d
--- /dev/null
+++ b/test/e2e/cli-install-uninstall/02-assert.yaml
@@ -0,0 +1,3 @@
+apiVersion: v1
+kind: Event
+message: 'Execution of plan cleanup finished with status COMPLETE'
diff --git a/test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml b/test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml
deleted file mode 100644
index d78dcb1c3..000000000
--- a/test/e2e/cli-install-uninstall/03-check-events-for-cleanup.yaml
+++ /dev/null
@@ -1,4 +0,0 @@
-apiVersion: kudo.dev/v1beta1
-kind: TestStep
-commands:
-- command: bash -c 'kubectl get -A events | grep -q "Execution of plan cleanup finished with status COMPLETE"'

From 61f6e9e8d6218cfdfcdb2b55a77a739f7e1f4b60 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 4 Dec 2019 10:04:23 +0100
Subject: [PATCH 4/9] Change finalizer name

---
 pkg/controller/instance/instance_controller.go | 2 +-
 test/e2e/cli-install-uninstall/02-errors.yaml  | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
index a1d1f146e..2e150e810 100644
--- a/pkg/controller/instance/instance_controller.go
+++ b/pkg/controller/instance/instance_controller.go
@@ -47,7 +47,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
-const instanceCleanupFinalizerName = "kudo.finalizers.instance.cleanup"
+const instanceCleanupFinalizerName = "kudo.dev.instance.cleanup"
 
 // Reconciler reconciles an Instance object.
 type Reconciler struct {
diff --git a/test/e2e/cli-install-uninstall/02-errors.yaml b/test/e2e/cli-install-uninstall/02-errors.yaml
index 9d7835b32..7c867a0c3 100644
--- a/test/e2e/cli-install-uninstall/02-errors.yaml
+++ b/test/e2e/cli-install-uninstall/02-errors.yaml
@@ -14,4 +14,4 @@ metadata:
 apiVersion: apps/v1
 kind: Deployment
 metadata:
-  name: cleanup-deployment
\ No newline at end of file
+  name: cleanup-deployment

From 705d29fc607ab4bda30a0df57052e68e4c0523d9 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 4 Dec 2019 11:34:57 +0100
Subject: [PATCH 5/9] Address more issues

---
 pkg/apis/kudo/v1beta1/instance_types.go        |  7 ++++---
 pkg/controller/instance/instance_controller.go | 11 ++++-------
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/pkg/apis/kudo/v1beta1/instance_types.go b/pkg/apis/kudo/v1beta1/instance_types.go
index 9dd236ffd..279f630a9 100644
--- a/pkg/apis/kudo/v1beta1/instance_types.go
+++ b/pkg/apis/kudo/v1beta1/instance_types.go
@@ -401,9 +401,10 @@ func (i *Instance) PlanStatus(plan string) *PlanStatus {
 }
 
 // GetPlanToBeExecuted returns name of the plan that should be executed
-func (i *Instance) GetPlanToBeExecuted(ov *OperatorVersion, isDeleting bool) (*string, error) {
-	// the instance is being deleted
-	if isDeleting {
+func (i *Instance) GetPlanToBeExecuted(ov *OperatorVersion) (*string, error) {
+	// a delete request is indicated by a non-zero 'metadata.deletionTimestamp',
+	// see https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
+	if !i.ObjectMeta.DeletionTimestamp.IsZero() {
 		// we have a cleanup plan
 		plan := selectPlan([]string{CleanupPlanName}, ov)
 		if plan != nil {
diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
index 2e150e810..9b74fa674 100644
--- a/pkg/controller/instance/instance_controller.go
+++ b/pkg/controller/instance/instance_controller.go
@@ -152,8 +152,6 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 
 	// ---------- 2. Check if the object is being deleted, start cleanup plan ----------
 
-	var isDeleting bool
-
 	// a delete request is indicated by a non-zero 'metadata.deletionTimestamp',
 	// see https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
 	if instance.ObjectMeta.DeletionTimestamp.IsZero() {
@@ -162,12 +160,11 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 		}
 	} else {
 		log.Printf("InstanceController: Instance %s/%s is being deleted", instance.Namespace, instance.Name)
-		isDeleting = true
 	}
 
 	// ---------- 3. Check if we should start execution of new plan ----------
 
-	planToBeExecuted, err := instance.GetPlanToBeExecuted(ov, isDeleting)
+	planToBeExecuted, err := instance.GetPlanToBeExecuted(ov)
 	if err != nil {
 		return reconcile.Result{}, err
 	}
@@ -406,11 +403,11 @@ func remove(values []string, s string) (result []string) {
 
 // maybeAddFinalizer adds the cleanup finalizer to an instance if the finalizer
 // hasn't been added yet, the instance has a cleanup plan and the cleanup plan
-// did not complete yet.
+// didn't run yet.
 func maybeAddFinalizer(instance *kudov1beta1.Instance) {
 	if !contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
 		if planStatus := instance.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
-			if !planStatus.Status.IsFinished() {
+			if planStatus.Status == v1beta1.ExecutionNeverRun {
 				log.Printf("InstanceController: Adding finalizer on instance %s/%s", instance.Namespace, instance.Name)
 				instance.ObjectMeta.Finalizers = append(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
 			}
@@ -423,7 +420,7 @@ func maybeAddFinalizer(instance *kudov1beta1.Instance) {
 func maybeRemoveFinalizer(instance *kudov1beta1.Instance) {
 	if contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
 		if planStatus := instance.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
-			if planStatus.Status.IsFinished() {
+			if planStatus.Status.IsTerminal() {
 				log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
 				instance.ObjectMeta.Finalizers = remove(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
 			}

From e3b7b00987e2665e41e1a3b55d8b611a10ee3790 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 4 Dec 2019 11:38:55 +0100
Subject: [PATCH 6/9] Comment finalizer handling

---
 pkg/controller/instance/instance_controller.go | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
index 9b74fa674..f8d29b872 100644
--- a/pkg/controller/instance/instance_controller.go
+++ b/pkg/controller/instance/instance_controller.go
@@ -407,6 +407,8 @@ func remove(values []string, s string) (result []string) {
 func maybeAddFinalizer(instance *kudov1beta1.Instance) {
 	if !contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
 		if planStatus := instance.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
+			// avoid adding a finalizer again if a reconciliation is requested
+			// after it has just been removed but the instance isn't deleted yet
 			if planStatus.Status == v1beta1.ExecutionNeverRun {
 				log.Printf("InstanceController: Adding finalizer on instance %s/%s", instance.Namespace, instance.Name)
 				instance.ObjectMeta.Finalizers = append(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)

From c3f4007d7b429c83d0ad4491c421277db0ef1ef7 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 4 Dec 2019 13:29:21 +0100
Subject: [PATCH 7/9] Refactor instance functions for readability

---
 pkg/apis/kudo/v1beta1/instance_types.go       | 66 +++++++++++++-
 .../instance/instance_controller.go           | 90 ++++---------------
 2 files changed, 80 insertions(+), 76 deletions(-)

diff --git a/pkg/apis/kudo/v1beta1/instance_types.go b/pkg/apis/kudo/v1beta1/instance_types.go
index 279f630a9..fa1bf0de6 100644
--- a/pkg/apis/kudo/v1beta1/instance_types.go
+++ b/pkg/apis/kudo/v1beta1/instance_types.go
@@ -31,6 +31,8 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
+const instanceCleanupFinalizerName = "kudo.dev.instance.cleanup"
+
 // InstanceSpec defines the desired state of Instance.
 type InstanceSpec struct {
 	// OperatorVersion specifies a reference to a specific Operator object.
@@ -402,9 +404,7 @@ func (i *Instance) PlanStatus(plan string) *PlanStatus {
 
 // GetPlanToBeExecuted returns name of the plan that should be executed
 func (i *Instance) GetPlanToBeExecuted(ov *OperatorVersion) (*string, error) {
-	// a delete request is indicated by a non-zero 'metadata.deletionTimestamp',
-	// see https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
-	if !i.ObjectMeta.DeletionTimestamp.IsZero() {
+	if i.IsDeleting() {
 		// we have a cleanup plan
 		plan := selectPlan([]string{CleanupPlanName}, ov)
 		if plan != nil {
@@ -507,6 +507,66 @@ func parameterDifference(old, new map[string]string) map[string]string {
 	return diff
 }
 
+func contains(values []string, s string) bool {
+	for _, value := range values {
+		if value == s {
+			return true
+		}
+	}
+	return false
+}
+
+func remove(values []string, s string) (result []string) {
+	for _, value := range values {
+		if value == s {
+			continue
+		}
+		result = append(result, value)
+	}
+	return
+}
+
+// TryAddFinalizer adds the cleanup finalizer to an instance if the finalizer
+// hasn't been added yet, the instance has a cleanup plan and the cleanup plan
+// didn't run yet. Returns true if the cleanup finalizer has been added.
+func (i *Instance) TryAddFinalizer() bool {
+	if !contains(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
+		if planStatus := i.PlanStatus(CleanupPlanName); planStatus != nil {
+			// avoid adding a finalizer again if a reconciliation is requested
+			// after it has just been removed but the instance isn't deleted yet
+			if planStatus.Status == ExecutionNeverRun {
+				i.ObjectMeta.Finalizers = append(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
+// TryRemoveFinalizer removes the cleanup finalizer of an instance if it has
+// been added, the instance has a cleanup plan and the cleanup plan completed.
+// Returns true if the cleanup finalizer has been removed.
+func (i *Instance) TryRemoveFinalizer() bool {
+	if contains(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
+		if planStatus := i.PlanStatus(CleanupPlanName); planStatus != nil {
+			if planStatus.Status.IsTerminal() {
+				i.ObjectMeta.Finalizers = remove(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
+				return true
+			}
+		}
+	}
+
+	return false
+}
+
+// IsDeleting returns true is the instance is being deleted.
+func (i *Instance) IsDeleting() bool {
+	// a delete request is indicated by a non-zero 'metadata.deletionTimestamp',
+	// see https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
+	return !i.ObjectMeta.DeletionTimestamp.IsZero()
+}
+
 // +genclient
 // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object
 
diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
index f8d29b872..fc9c19023 100644
--- a/pkg/controller/instance/instance_controller.go
+++ b/pkg/controller/instance/instance_controller.go
@@ -37,7 +37,6 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/tools/record"
 
-	"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	kudov1beta1 "github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1"
 	appsv1 "k8s.io/api/apps/v1"
 	batchv1 "k8s.io/api/batch/v1"
@@ -47,8 +46,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
-const instanceCleanupFinalizerName = "kudo.dev.instance.cleanup"
-
 // Reconciler reconciles an Instance object.
 type Reconciler struct {
 	client.Client
@@ -152,11 +149,11 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 
 	// ---------- 2. Check if the object is being deleted, start cleanup plan ----------
 
-	// a delete request is indicated by a non-zero 'metadata.deletionTimestamp',
-	// see https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#finalizers
-	if instance.ObjectMeta.DeletionTimestamp.IsZero() {
+	if !instance.IsDeleting() {
 		if _, hasCleanupPlan := ov.Spec.Plans[kudov1beta1.CleanupPlanName]; hasCleanupPlan {
-			maybeAddFinalizer(instance)
+			if instance.TryAddFinalizer() {
+				log.Printf("InstanceController: Adding finalizer on instance %s/%s", instance.Namespace, instance.Name)
+			}
 		}
 	} else {
 		log.Printf("InstanceController: Instance %s/%s is being deleted", instance.Namespace, instance.Name)
@@ -168,7 +165,6 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 	if err != nil {
 		return reconcile.Result{}, err
 	}
-
 	if planToBeExecuted != nil {
 		log.Printf("InstanceController: Going to start execution of plan %s on instance %s/%s", kudo.StringValue(planToBeExecuted), instance.Namespace, instance.Name)
 		err = instance.StartPlanExecution(kudo.StringValue(planToBeExecuted), ov)
@@ -227,17 +223,6 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 }
 
 func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Instance, client client.Client) error {
-	// update instance spec and metadata. this will not update Instance.Status field
-	if !reflect.DeepEqual(instance.Spec, oldInstance.Spec) || !reflect.DeepEqual(instance.ObjectMeta.Annotations, oldInstance.ObjectMeta.Annotations) {
-		instanceStatus := instance.Status.DeepCopy()
-		err := client.Update(context.TODO(), instance)
-		if err != nil {
-			log.Printf("InstanceController: Error when updating instance spec. %v", err)
-			return err
-		}
-		instance.Status = *instanceStatus
-	}
-
 	// update instance status
 	err := client.Status().Update(context.TODO(), instance)
 	if err != nil {
@@ -245,14 +230,21 @@ func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Ins
 		return err
 	}
 
-	// update instance metadata if finalizer is removed
-	// because Kubernetes will immediately delete the instance, this has to be the last instance update
-	maybeRemoveFinalizer(instance)
-	if !reflect.DeepEqual(instance.ObjectMeta.Finalizers, oldInstance.ObjectMeta.Finalizers) {
-		if err := client.Update(context.TODO(), instance); err != nil {
-			log.Printf("InstanceController: Error when removing instance finalizer. %v", err)
+	if instance.TryRemoveFinalizer() {
+		log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
+	}
+
+	// update instance spec and metadata. this will not update Instance.Status field
+	if !reflect.DeepEqual(instance.Spec, oldInstance.Spec) ||
+		!reflect.DeepEqual(instance.ObjectMeta.Annotations, oldInstance.ObjectMeta.Annotations) ||
+		!reflect.DeepEqual(instance.ObjectMeta.Finalizers, oldInstance.ObjectMeta.Finalizers) {
+		instanceStatus := instance.Status.DeepCopy()
+		err := client.Update(context.TODO(), instance)
+		if err != nil {
+			log.Printf("InstanceController: Error when updating instance spec. %v", err)
 			return err
 		}
+		instance.Status = *instanceStatus
 	}
 
 	return nil
@@ -381,51 +373,3 @@ func parameterDifference(old, new map[string]string) map[string]string {
 
 	return diff
 }
-
-func contains(values []string, s string) bool {
-	for _, value := range values {
-		if value == s {
-			return true
-		}
-	}
-	return false
-}
-
-func remove(values []string, s string) (result []string) {
-	for _, value := range values {
-		if value == s {
-			continue
-		}
-		result = append(result, value)
-	}
-	return
-}
-
-// maybeAddFinalizer adds the cleanup finalizer to an instance if the finalizer
-// hasn't been added yet, the instance has a cleanup plan and the cleanup plan
-// didn't run yet.
-func maybeAddFinalizer(instance *kudov1beta1.Instance) {
-	if !contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
-		if planStatus := instance.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
-			// avoid adding a finalizer again if a reconciliation is requested
-			// after it has just been removed but the instance isn't deleted yet
-			if planStatus.Status == v1beta1.ExecutionNeverRun {
-				log.Printf("InstanceController: Adding finalizer on instance %s/%s", instance.Namespace, instance.Name)
-				instance.ObjectMeta.Finalizers = append(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
-			}
-		}
-	}
-}
-
-// maybeRemoveFinalizer removes the cleanup finalizer of an instance if it has
-// been added, the instance has a cleanup plan and the cleanup plan completed.
-func maybeRemoveFinalizer(instance *kudov1beta1.Instance) {
-	if contains(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName) {
-		if planStatus := instance.PlanStatus(v1beta1.CleanupPlanName); planStatus != nil {
-			if planStatus.Status.IsTerminal() {
-				log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
-				instance.ObjectMeta.Finalizers = remove(instance.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
-			}
-		}
-	}
-}

From 8744db32bf73cb035354cd1e3c182bf8483abb82 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 4 Dec 2019 14:01:33 +0100
Subject: [PATCH 8/9] Remove finalizer is there isn't a cleanup plan

---
 pkg/apis/kudo/v1beta1/instance_types.go | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/pkg/apis/kudo/v1beta1/instance_types.go b/pkg/apis/kudo/v1beta1/instance_types.go
index fa1bf0de6..5fd3ea040 100644
--- a/pkg/apis/kudo/v1beta1/instance_types.go
+++ b/pkg/apis/kudo/v1beta1/instance_types.go
@@ -554,6 +554,11 @@ func (i *Instance) TryRemoveFinalizer() bool {
 				i.ObjectMeta.Finalizers = remove(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
 				return true
 			}
+		} else {
+			// We have a finalizer but no cleanup plan. This could be due to an updated instance.
+			// Let's remove the finalizer.
+			i.ObjectMeta.Finalizers = remove(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
+			return true
 		}
 	}
 

From cef9e6d01af27d81127af5a413557ece77739019 Mon Sep 17 00:00:00 2001
From: Jan Schlicht <jan@d2iq.com>
Date: Wed, 4 Dec 2019 14:09:55 +0100
Subject: [PATCH 9/9] restore instance updating order

---
 .../instance/instance_controller.go           | 28 +++++++++++--------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/pkg/controller/instance/instance_controller.go b/pkg/controller/instance/instance_controller.go
index fc9c19023..5f38be59f 100644
--- a/pkg/controller/instance/instance_controller.go
+++ b/pkg/controller/instance/instance_controller.go
@@ -223,17 +223,6 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) {
 }
 
 func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Instance, client client.Client) error {
-	// update instance status
-	err := client.Status().Update(context.TODO(), instance)
-	if err != nil {
-		log.Printf("InstanceController: Error when updating instance status. %v", err)
-		return err
-	}
-
-	if instance.TryRemoveFinalizer() {
-		log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
-	}
-
 	// update instance spec and metadata. this will not update Instance.Status field
 	if !reflect.DeepEqual(instance.Spec, oldInstance.Spec) ||
 		!reflect.DeepEqual(instance.ObjectMeta.Annotations, oldInstance.ObjectMeta.Annotations) ||
@@ -247,6 +236,23 @@ func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Ins
 		instance.Status = *instanceStatus
 	}
 
+	// update instance status
+	err := client.Status().Update(context.TODO(), instance)
+	if err != nil {
+		log.Printf("InstanceController: Error when updating instance status. %v", err)
+		return err
+	}
+
+	// update instance metadata if finalizer is removed
+	// because Kubernetes might immediately delete the instance, this has to be the last instance update
+	if instance.TryRemoveFinalizer() {
+		log.Printf("InstanceController: Removing finalizer on instance %s/%s", instance.Namespace, instance.Name)
+		if err := client.Update(context.TODO(), instance); err != nil {
+			log.Printf("InstanceController: Error when removing instance finalizer. %v", err)
+			return err
+		}
+	}
+
 	return nil
 }