Fix background deletion blocked (#51)

Signed-off-by: shaoyue.chen <[email protected]>
zilliztech · Nov 29, 2023 · c77fd02 · c77fd02
1 parent 4be02a4
commit c77fd02
Show file tree

Hide file tree

Showing 4 changed files with 61 additions and 17 deletions.
diff --git a/pkg/controllers/milvus.go b/pkg/controllers/milvus.go
@@ -84,7 +84,7 @@ func (r *MilvusReconciler) batchDeletePVC(ctx context.Context, namespace, labelK
 	return nil
 }
 
-func (r *MilvusReconciler) Finalize(ctx context.Context, mc v1beta1.Milvus) error {
+var Finalize = func(ctx context.Context, r *MilvusReconciler, mc v1beta1.Milvus) error {
 	deletingReleases := map[string]bool{}
 	if !mc.Spec.Dep.Etcd.External && mc.Spec.Dep.Etcd.InCluster.DeletionPolicy == v1beta1.DeletionPolicyDelete {
 		deletingReleases[mc.Name+"-etcd"] = mc.Spec.Dep.Etcd.InCluster.PVCDeletion

diff --git a/pkg/controllers/milvus_controller.go b/pkg/controllers/milvus_controller.go
@@ -37,9 +37,10 @@ import (
 )
 
 const (
-	MilvusFinalizerName      = "milvus.milvus.io/finalizer"
-	PauseReconcileAnnotation = "milvus.io/pause-reconcile"
-	MaintainingAnnotation    = "milvus.io/maintaining"
+	MilvusFinalizerName         = "milvus.milvus.io/finalizer"
+	ForegroundDeletionFinalizer = "foregroundDeletion"
+	PauseReconcileAnnotation    = "milvus.io/pause-reconcile"
+	MaintainingAnnotation       = "milvus.io/maintaining"
 )
 
 // MilvusReconciler reconciles a Milvus object
@@ -117,13 +118,15 @@ func (r *MilvusReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 			}
 		}
 
-		stopped, err := CheckMilvusStopped(ctx, r.Client, *milvus)
-		if !stopped || err != nil {
-			return ctrl.Result{RequeueAfter: unhealthySyncInterval}, err
+		if controllerutil.ContainsFinalizer(milvus, ForegroundDeletionFinalizer) {
+			stopped, err := CheckMilvusStopped(ctx, r.Client, *milvus)
+			if !stopped || err != nil {
+				return ctrl.Result{RequeueAfter: unhealthySyncInterval}, err
+			}
 		}
 
 		if controllerutil.ContainsFinalizer(milvus, MilvusFinalizerName) {
-			if err := r.Finalize(ctx, *milvus); err != nil {
+			if err := Finalize(ctx, r, *milvus); err != nil {
 				return ctrl.Result{}, err
 			}
 			// metrics

diff --git a/pkg/controllers/milvus_controller_test.go b/pkg/controllers/milvus_controller_test.go
@@ -24,13 +24,24 @@ var mockCheckMilvusStop = func(ctx context.Context, cli client.Client, mc v1beta
 	return mockCheckMilvusStopRet, mockCheckMilvusStopErr
 }
 
+var mockFinalizeRet error
+var mockFinalize = func(ctx context.Context, r *MilvusReconciler, mc v1beta1.Milvus) error {
+	return mockFinalizeRet
+}
+
 func TestClusterReconciler(t *testing.T) {
 	bak := CheckMilvusStopped
 	defer func() {
 		CheckMilvusStopped = bak
 	}()
 	CheckMilvusStopped = mockCheckMilvusStop
 
+	bakFinalize := Finalize
+	defer func() {
+		Finalize = bakFinalize
+	}()
+	Finalize = mockFinalize
+
 	config.Init(util.GetGitRepoRootDir())
 
 	ctrl := gomock.NewController(t)
@@ -76,9 +87,38 @@ func TestClusterReconciler(t *testing.T) {
 		assert.Error(t, err)
 	})
 
-	t.Run("case delete remove finalizer", func(t *testing.T) {
+	t.Run("case delete background", func(t *testing.T) {
+		defer ctrl.Finish()
+		m.Finalizers = []string{MilvusFinalizerName}
+		mockCheckMilvusStopRet = false
+		m.ObjectMeta.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+
+		mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
+			Do(func(ctx, key, obj interface{}) {
+				o := obj.(*v1beta1.Milvus)
+				*o = m
+			}).
+			Return(nil)
+
+		mockClient.EXPECT().Status().Return(mockClient)
+		mockClient.EXPECT().Update(gomock.Any(), gomock.Any()).Times(1)
+
+		mockClient.EXPECT().Update(gomock.Any(), gomock.Any()).Do(
+			func(ctx, obj interface{}, opts ...interface{}) {
+				// finalizer should be removed
+				u := obj.(*v1beta1.Milvus)
+				assert.Equal(t, []string{}, u.Finalizers)
+			},
+		).Return(nil)
+
+		_, err := r.Reconcile(ctx, reconcile.Request{})
+		assert.NoError(t, err)
+	})
+
+	t.Run("delete foreground deletion", func(t *testing.T) {
 		defer ctrl.Finish()
 		mockCheckMilvusStopRet = true
+		m.Finalizers = []string{ForegroundDeletionFinalizer, MilvusFinalizerName}
 		m.ObjectMeta.DeletionTimestamp = &metav1.Time{Time: time.Now()}
 		mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
 			Do(func(ctx, key, obj interface{}) {
@@ -94,7 +134,7 @@ func TestClusterReconciler(t *testing.T) {
 			func(ctx, obj interface{}, opts ...interface{}) {
 				// finalizer should be removed
 				u := obj.(*v1beta1.Milvus)
-				assert.Equal(t, u.Finalizers, []string{})
+				assert.Equal(t, []string{ForegroundDeletionFinalizer}, u.Finalizers)
 			},
 		).Return(nil)
 
@@ -104,6 +144,7 @@ func TestClusterReconciler(t *testing.T) {
 
 	t.Run("milvus not stopped or check failed", func(t *testing.T) {
 		defer ctrl.Finish()
+		m.Finalizers = []string{ForegroundDeletionFinalizer, MilvusFinalizerName}
 		mockClient.EXPECT().Get(gomock.Any(), gomock.Any(), gomock.Any()).
 			Do(func(ctx, key, obj interface{}) {
 				o := obj.(*v1beta1.Milvus)

diff --git a/pkg/controllers/milvus_test.go b/pkg/controllers/milvus_test.go
@@ -29,7 +29,7 @@ func TestCluster_Finalize(t *testing.T) {
 	m.Default()
 
 	t.Run("no delete", func(t *testing.T) {
-		err := r.Finalize(ctx, m)
+		err := Finalize(ctx, r, m)
 		assert.NoError(t, err)
 	})
 
@@ -47,7 +47,7 @@ func TestCluster_Finalize(t *testing.T) {
 				}
 			})
 		mockClient.EXPECT().Delete(gomock.Any(), gomock.Any())
-		err := r.Finalize(ctx, m)
+		err := Finalize(ctx, r, m)
 		assert.NoError(t, err)
 	})
 
@@ -66,7 +66,7 @@ func TestCluster_Finalize(t *testing.T) {
 				}
 			})
 		mockClient.EXPECT().Delete(gomock.Any(), gomock.Any())
-		err := r.Finalize(ctx, m)
+		err := Finalize(ctx, r, m)
 		assert.NoError(t, err)
 	})
 
@@ -76,7 +76,7 @@ func TestCluster_Finalize(t *testing.T) {
 		m.Spec.Dep.Storage.InCluster.DeletionPolicy = v1beta1.DeletionPolicyDelete
 		m.Spec.Dep.Storage.InCluster.PVCDeletion = true
 		mockHelm.EXPECT().Uninstall(gomock.Any(), gomock.Any()).Return(errTest)
-		err := r.Finalize(ctx, m)
+		err := Finalize(ctx, r, m)
 		assert.Error(t, err)
 	})
 
@@ -90,7 +90,7 @@ func TestCluster_Finalize(t *testing.T) {
 					{},
 				}
 			}).Return(errTest)
-		err := r.Finalize(ctx, m)
+		err := Finalize(ctx, r, m)
 		assert.Error(t, err)
 	})
 
@@ -106,7 +106,7 @@ func TestCluster_Finalize(t *testing.T) {
 				}
 			})
 		mockClient.EXPECT().Delete(gomock.Any(), gomock.Any()).Return(errTest)
-		err := r.Finalize(ctx, m)
+		err := Finalize(ctx, r, m)
 		assert.Error(t, err)
 	})
 
@@ -117,7 +117,7 @@ func TestCluster_Finalize(t *testing.T) {
 		m.Spec.Dep.Etcd.InCluster = nil
 		m.Spec.Dep.Pulsar.InCluster = nil
 		m.Spec.Dep.Storage.InCluster = nil
-		err := r.Finalize(ctx, m)
+		err := Finalize(ctx, r, m)
 		assert.NoError(t, err)
 	})