Merge pull request #1938 from joelanford/fix/zombie-operator

Bug 1899588: Only re-create operator resource if it has existing components

Author: openshift-merge-robot

pkg/controller/operators/operator_controller.go

@@ -11,6 +11,7 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -119,6 +120,12 @@ func (r *OperatorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 	in := &operatorsv1.Operator{}
 	if err := r.Get(ctx, req.NamespacedName, in); err != nil {
 		if apierrors.IsNotFound(err) {
+			// If the Operator instance is not found, we're likely reconciling because
+			// of a DELETE event. Only recreate the Operator if any of its components
+			// still exist.
+			if exists, err := r.hasExistingComponents(ctx, name); err != nil || !exists {
+				return reconcile.Result{}, err
+			}
 			create = true
 			in.SetName(name)
 		} else {
@@ -219,6 +226,33 @@ func (r *OperatorReconciler) listComponents(ctx context.Context, selector labels
 	return componentLists, nil
 }
 
+func (r *OperatorReconciler) hasExistingComponents(ctx context.Context, name string) (bool, error) {
+	op := &operatorsv1.Operator{}
+	op.SetName(name)
+	operator := decorators.Operator{Operator: op}
+
+	selector, err := operator.ComponentSelector()
+	if err != nil {
+		return false, err
+	}
+
+	components, err := r.listComponents(ctx, selector)
+	if err != nil {
+		return false, err
+	}
+
+	for _, list := range components {
+		items, err := meta.ExtractList(list)
+		if err != nil {
+			return false, fmt.Errorf("Unable to extract list from runtime.Object")
+		}
+		if len(items) > 0 {
+			return true, nil
+		}
+	}
+	return false, nil
+}
+
 func (r *OperatorReconciler) getLastResourceVersion(name types.NamespacedName) (string, bool) {
 	r.mu.RLock()
 	defer r.mu.RUnlock()

pkg/controller/operators/operator_controller_test.go

@@ -7,6 +7,7 @@ import (
 	. "github.com/onsi/gomega"
 	corev1 "k8s.io/api/core/v1"
 	rbacv1 "k8s.io/api/rbac/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -43,9 +44,52 @@ var _ = Describe("Operator Controller", func() {
 		Expect(k8sClient.Create(ctx, operator)).To(Succeed())
 	})
 
-	AfterEach(func() {
-		Expect(k8sClient.Get(ctx, name, operator)).To(Succeed())
-		Expect(k8sClient.Delete(ctx, operator, deleteOpts)).To(Succeed())
+	Describe("operator deletion", func() {
+		JustBeforeEach(func() {
+			Expect(k8sClient.Delete(ctx, operator)).To(Succeed())
+			Eventually(func() bool {
+				err := k8sClient.Get(ctx, name, operator)
+				return apierrors.IsNotFound(err)
+			}, timeout, interval).Should(BeTrue())
+		})
+		Context("with components bearing its label", func() {
+			var (
+				objs      []runtime.Object
+				namespace string
+			)
+
+			BeforeEach(func() {
+				namespace = genName("ns-")
+				objs = testobj.WithLabel(expectedKey, "",
+					testobj.WithName(namespace, &corev1.Namespace{}),
+				)
+
+				for _, obj := range objs {
+					Expect(k8sClient.Create(ctx, obj.(client.Object))).To(Succeed())
+				}
+			})
+
+			AfterEach(func() {
+				for _, obj := range objs {
+					Expect(k8sClient.Delete(ctx, obj.(client.Object), deleteOpts)).To(Succeed())
+				}
+				Expect(k8sClient.Delete(ctx, operator, deleteOpts)).To(Succeed())
+			})
+
+			It("should re-create it", func() {
+				Eventually(func() error {
+					return k8sClient.Get(ctx, name, operator)
+				}).Should(Succeed())
+			})
+		})
+		Context("with no components bearing its label", func() {
+			It("should not re-create it", func() {
+				Consistently(func() bool {
+					err := k8sClient.Get(ctx, name, operator)
+					return apierrors.IsNotFound(err)
+				}, timeout, interval).Should(BeTrue())
+			})
+		})
 	})
 
 	Describe("component selection", func() {
@@ -61,6 +105,10 @@ var _ = Describe("Operator Controller", func() {
 			}, timeout, interval).Should(Equal(expectedComponentLabelSelector))
 		})
 
+		AfterEach(func() {
+			Expect(k8sClient.Delete(ctx, operator, deleteOpts)).To(Succeed())
+		})
+
 		Context("with no components bearing its label", func() {
 			Specify("a status containing no component references", func() {
 				Consistently(func() ([]operatorsv1.RichReference, error) {
@@ -92,7 +140,6 @@ var _ = Describe("Operator Controller", func() {
 
 			AfterEach(func() {
 				for _, obj := range objs {
-					Expect(k8sClient.Get(ctx, testobj.NamespacedName(obj), obj.(client.Object))).To(Succeed())
 					Expect(k8sClient.Delete(ctx, obj.(client.Object), deleteOpts)).To(Succeed())
 				}
 			})

test/e2e/operator_test.go

@@ -64,8 +64,12 @@ var _ = Describe("Operator", func() {
 	// 8. Ensure o's status.components.refs field eventually contains references to sa-a and sa-b
 	// 9. Remove the component label from sa-b
 	// 10. Ensure the reference to sa-b is eventually removed from o's status.components.refs field
-	// 11. Delete ns-a
-	// 12. Ensure the reference to ns-a is eventually removed from o's status.components.refs field
+	// 11. Delete o
+	// 12. Ensure o is re-created
+	// 13. Delete ns-a
+	// 14. Ensure the reference to ns-a is eventually removed from o's status.components.refs field
+	// 15. Delete o
+	// 16. Ensure o is not re-created
 	It("should surface components in its status", func() {
 		o := &operatorsv1.Operator{}
 		o.SetName(genName("o-"))
@@ -184,6 +188,21 @@ var _ = Describe("Operator", func() {
 		By("removing a component's reference when it no longer bears the component label")
 		componentRefEventuallyExists(w, false, getReference(scheme, saB))
 
+		// Delete o
+		Eventually(func() error {
+			err := client.Delete(clientCtx, o)
+			if err != nil && !apierrors.IsNotFound(err) {
+				return err
+			}
+			return nil
+		}).Should(Succeed())
+
+		// Ensure that o is eventually recreated (because some of its components still exist).
+		By("recreating the Operator when any components still exist")
+		Eventually(func() error {
+			return client.Get(clientCtx, types.NamespacedName{Name: o.GetName()}, o)
+		}).Should(Succeed())
+
 		// Delete ns-a
 		Eventually(func() error {
 			err := client.Delete(clientCtx, nsA)
@@ -196,6 +215,25 @@ var _ = Describe("Operator", func() {
 		// Ensure the reference to ns-a is eventually removed from o's status.components.refs field
 		By("removing a component's reference when it no longer exists")
 		componentRefEventuallyExists(w, false, getReference(scheme, nsA))
+
+		// Delete o
+		Eventually(func() error {
+			err := client.Delete(clientCtx, o)
+			if apierrors.IsNotFound(err) {
+				return nil
+			}
+			return err
+		}).Should(Succeed())
+
+		// Ensure that o is consistently not found
+		By("verifying the Operator is permanently deleted if it has no components")
+		Consistently(func() error {
+			err := client.Get(clientCtx, types.NamespacedName{Name: o.GetName()}, o)
+			if apierrors.IsNotFound(err) {
+				return nil
+			}
+			return err
+		}).Should(Succeed())
 	})
 
 	Context("when a subscription to a package exists", func() {