Fix CRDs deletion in operator deletion (#2129)

* Fix CRDs deletion in operator deletion

Also check DataImportCron CRD has no DeletionTimestamp before adding a
finalizer

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

* CR fixes

Signed-off-by: Arnon Gilboa <agilboa@redhat.com>

pkg/controller/BUILD.bazel

@@ -46,6 +46,7 @@ go_library(
         "//vendor/k8s.io/api/core/v1:go_default_library",
         "//vendor/k8s.io/api/networking/v1:go_default_library",
         "//vendor/k8s.io/api/storage/v1:go_default_library",
+        "//vendor/k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1:go_default_library",
         "//vendor/k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",

pkg/controller/dataimportcron-controller.go

@@ -35,6 +35,7 @@ import (
 	batchv1 "k8s.io/api/batch/v1"
 	v1beta1 "k8s.io/api/batch/v1beta1"
 	corev1 "k8s.io/api/core/v1"
+	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -121,7 +122,16 @@ func (r *DataImportCronReconciler) Reconcile(ctx context.Context, req reconcile.
 }
 
 func (r *DataImportCronReconciler) initCron(ctx context.Context, dataImportCron *cdiv1.DataImportCron) error {
-	AddFinalizer(dataImportCron, dataImportCronFinalizer)
+	if !HasFinalizer(dataImportCron, dataImportCronFinalizer) {
+		crd := &extv1.CustomResourceDefinition{}
+		if err := r.client.Get(context.TODO(), types.NamespacedName{Name: "dataimportcrons.cdi.kubevirt.io"}, crd); err != nil {
+			return err
+		}
+		if crd.DeletionTimestamp != nil {
+			return errors.Errorf("CRD has DeletionTimestamp")
+		}
+		AddFinalizer(dataImportCron, dataImportCronFinalizer)
+	}
 	if isURLSource(dataImportCron) && !r.cronJobExists(ctx, dataImportCron) {
 		cronJob, err := r.newCronJob(dataImportCron)
 		if err != nil {
@@ -551,6 +561,9 @@ func addDataImportCronControllerWatches(mgr manager.Manager, c controller.Contro
 	if err := imagev1.AddToScheme(mgr.GetScheme()); err != nil {
 		return err
 	}
+	if err := extv1.AddToScheme(mgr.GetScheme()); err != nil {
+		return err
+	}
 	if err := c.Watch(&source.Kind{Type: &cdiv1.DataImportCron{}}, &handler.EnqueueRequestForObject{}); err != nil {
 		return err
 	}

pkg/controller/dataimportcron-controller_test.go

@@ -29,6 +29,7 @@ import (
 
 	v1beta1 "k8s.io/api/batch/v1beta1"
 	corev1 "k8s.io/api/core/v1"
+	extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
@@ -353,13 +354,14 @@ var _ = Describe("untagURL", func() {
 
 func createDataImportCronReconciler(objects ...runtime.Object) *DataImportCronReconciler {
 	cdiConfig := MakeEmptyCDIConfigSpec(common.ConfigName)
-	objs := []runtime.Object{}
-	objs = append(objs, cdiConfig)
+	crd := &extv1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: "dataimportcrons.cdi.kubevirt.io"}}
+	objs := []runtime.Object{cdiConfig, crd}
 	objs = append(objs, objects...)
 
 	s := scheme.Scheme
 	cdiv1.AddToScheme(s)
 	imagev1.AddToScheme(s)
+	extv1.AddToScheme(s)
 
 	cl := fake.NewFakeClientWithScheme(s, objs...)
 	rec := record.NewFakeRecorder(1)

pkg/operator/controller/callbacks.go

@@ -30,6 +30,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	sdk "kubevirt.io/controller-lifecycle-operator-sdk/pkg/sdk"
@@ -79,38 +80,12 @@ func reconcileDeleteControllerDeployment(args *callbacks.ReconcileCallbackArgs)
 		return nil
 	}
 
-	args.Logger.Info("Deleting CRDs and verifing no finalizers")
-
-	crd := &extv1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: "dataimportcrons.cdi.kubevirt.io"}}
-	if err := args.Client.Delete(context.TODO(), crd, &client.DeleteOptions{}); cdicontroller.IgnoreNotFound(err) != nil {
-		return err
-	}
-	dics := &cdiv1.DataImportCronList{}
-	if err := args.Client.List(context.TODO(), dics, &client.ListOptions{}); cdicontroller.IgnoreIsNoMatchError(err) != nil {
-		return err
-	}
-	for _, dic := range dics.Items {
-		if len(dic.Finalizers) > 0 {
-			return fmt.Errorf("DataImportCron %s has Finalizers %v", dic.Name, dic.Finalizers)
-		}
-	}
-
-	crd = &extv1.CustomResourceDefinition{ObjectMeta: metav1.ObjectMeta{Name: "datavolumes.cdi.kubevirt.io"}}
-	if err := args.Client.Delete(context.TODO(), crd, &client.DeleteOptions{}); cdicontroller.IgnoreNotFound(err) != nil {
-		return err
-	}
-	dvs := &cdiv1.DataVolumeList{}
-	if err := args.Client.List(context.TODO(), dvs, &client.ListOptions{}); cdicontroller.IgnoreIsNoMatchError(err) != nil {
+	args.Logger.Info("Deleting CRDs")
+	if err := deleteCRDs(args); err != nil {
 		return err
 	}
-	for _, dv := range dvs.Items {
-		if len(dv.Finalizers) > 0 {
-			return fmt.Errorf("DataVolume %s has Finalizers %v", dv.Name, dv.Finalizers)
-		}
-	}
 
 	args.Logger.Info("Deleting CDI deployment and all import/upload/clone pods/services")
-
 	err := args.Client.Delete(context.TODO(), deployment, &client.DeleteOptions{
 		PropagationPolicy: &[]metav1.DeletionPropagation{metav1.DeletePropagationForeground}[0],
 	})
@@ -132,6 +107,44 @@ func reconcileDeleteControllerDeployment(args *callbacks.ReconcileCallbackArgs)
 	return nil
 }
 
+func deleteCRDs(args *callbacks.ReconcileCallbackArgs) error {
+	crdNames := []string{
+		"dataimportcrons.cdi.kubevirt.io",
+		"datavolumes.cdi.kubevirt.io",
+		"objecttransfers.cdi.kubevirt.io",
+	}
+	crdsExist := false
+	for _, crdName := range crdNames {
+		crd := &extv1.CustomResourceDefinition{}
+		if err := args.Client.Get(context.TODO(), types.NamespacedName{Name: crdName}, crd); err != nil {
+			if errors.IsNotFound(err) {
+				continue
+			}
+			return err
+		}
+		if crd.DeletionTimestamp == nil {
+			if err := args.Client.Delete(context.TODO(), crd, &client.DeleteOptions{}); err != nil {
+				if errors.IsNotFound(err) {
+					continue
+				}
+				return err
+			}
+			if err := args.Client.Get(context.TODO(), types.NamespacedName{Name: crdName}, crd); err != nil {
+				if errors.IsNotFound(err) {
+					continue
+				}
+				return err
+			}
+		}
+		args.Logger.Info("CRD is not deleted yet", "crdName", crdName)
+		crdsExist = true
+	}
+	if crdsExist {
+		return fmt.Errorf("CRDs are not deleted yet")
+	}
+	return nil
+}
+
 func reconcileCreateRoute(args *callbacks.ReconcileCallbackArgs) error {
 	if args.State != callbacks.ReconcileStatePostRead {
 		return nil

tests/operator_test.go

@@ -471,14 +471,33 @@ var _ = Describe("ALL Operator tests", func() {
 				Expect(err).ToNot(HaveOccurred())
 				Expect(dv.Status.Phase).To(Equal(cdiv1.Succeeded))
 
+				By("Start goroutine creating DataImportCrons")
+				go func() {
+					for i := 0; i < 100; i++ {
+						cron := NewDataImportCron(fmt.Sprintf("cron-test-%d", i), "5Gi", scheduleEveryMinute, "ds", cdiv1.DataVolumeSourceRegistry{URL: &url, PullMethod: &registryPullNode}, cdiv1.DataImportCronRetainAll)
+						cron, err := f.CdiClient.CdiV1beta1().DataImportCrons(f.Namespace.Name).Create(context.TODO(), cron, metav1.CreateOptions{})
+						if err != nil {
+							By(fmt.Sprintf("DataImportCron %d error %s", i, err.Error()))
+							break
+						}
+					}
+				}()
+
 				By("Delete CDI CR")
 				err = f.CdiClient.CdiV1beta1().CDIs().Delete(context.TODO(), cr.Name, metav1.DeleteOptions{})
 				Expect(err).ToNot(HaveOccurred())
 				By("Wait util CDI CR is gone")
-				Eventually(func() bool { return crGone(f, cr) }, 1*time.Minute, 2*time.Second).Should(BeTrue())
+				Eventually(func() bool {
+					dics, err := f.CdiClient.CdiV1beta1().DataImportCrons(f.Namespace.Name).List(context.TODO(), metav1.ListOptions{})
+					if err == nil {
+						By(fmt.Sprintf("DataImportCrons %d", len(dics.Items)))
+					}
+					return crGone(f, cr)
+				}, 1*time.Minute, 2*time.Second).Should(BeTrue())
 
-				By("Verify no DataImportCrons")
+				By("Verify no DataImportCrons are found as CRDs are gone")
 				_, err = f.CdiClient.CdiV1beta1().DataImportCrons(f.Namespace.Name).List(context.TODO(), metav1.ListOptions{})
+				Expect(err).To(HaveOccurred())
 				Expect(errors.IsNotFound(err)).To(BeTrue())
 			})
 		})