From 98bfd03c728515f04f89fc99f8bb576f0e1b9602 Mon Sep 17 00:00:00 2001
From: Harold Cheng <niuchangcun@gmail.com>
Date: Thu, 14 Nov 2024 17:14:30 +0800
Subject: [PATCH] make leaveMember4ScaleIn cleaner

---
 .../apps/transformer_component_workload.go    | 43 ++++++++++---------
 .../apps/cluster_instance_set_test_util.go    |  2 +-
 2 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/controllers/apps/transformer_component_workload.go b/controllers/apps/transformer_component_workload.go
index 0d301c1eb6d..7a27133b58a 100644
--- a/controllers/apps/transformer_component_workload.go
+++ b/controllers/apps/transformer_component_workload.go
@@ -619,12 +619,10 @@ func (r *componentWorkloadOps) leaveMember4ScaleIn() error {
 		return false
 	}
 
-	tryToSwitchover := func(lfa lifecycle.Lifecycle, pod *corev1.Pod) error {
-		// if pod is not leader/primary, no need to switchover
+	trySwitchover := func(lfa lifecycle.Lifecycle, pod *corev1.Pod) error {
 		if !needSwitchover(pod) {
 			return nil
 		}
-		// if HA functionality is not enabled, no need to switchover
 		err := lfa.Switchover(r.reqCtx.Ctx, r.cli, nil, "")
 		if err != nil && errors.Is(err, lifecycle.ErrActionNotDefined) {
 			return nil
@@ -635,6 +633,20 @@ func (r *componentWorkloadOps) leaveMember4ScaleIn() error {
 		return err
 	}
 
+	tryMemberLeave := func(lfa lifecycle.Lifecycle) error {
+		if r.synthesizeComp.LifecycleActions == nil || r.synthesizeComp.LifecycleActions.MemberLeave == nil {
+			return nil
+		}
+
+		if err := lfa.MemberLeave(r.reqCtx.Ctx, r.cli, nil); err != nil {
+			if !errors.Is(err, lifecycle.ErrActionNotDefined) {
+				return err
+			}
+		}
+
+		return nil
+	}
+
 	// TODO: Move memberLeave to the ITS controller. Instead of performing a switchover, we can directly scale down the non-leader nodes. This is because the pod ordinal is not guaranteed to be continuous.
 	podsToMemberLeave := make([]*corev1.Pod, 0)
 	for _, pod := range pods {
@@ -645,28 +657,17 @@ func (r *componentWorkloadOps) leaveMember4ScaleIn() error {
 		podsToMemberLeave = append(podsToMemberLeave, pod)
 	}
 	for _, pod := range podsToMemberLeave {
-		if !(needSwitchover(pod) || // if the pod is leader, it needs to call switchover
-			(r.synthesizeComp.LifecycleActions != nil && r.synthesizeComp.LifecycleActions.MemberLeave != nil)) { // if the memberLeave action is defined, it needs to call it
-			continue
-		}
-
-		lfa, err1 := lifecycle.New(r.synthesizeComp, pod, pods...)
-		if err1 != nil {
-			if err == nil {
-				err = err1
-			}
-			continue
+		lfa, err := lifecycle.New(r.synthesizeComp, pod, pods...)
+		if err != nil {
+			return err
 		}
 
-		// switchover if the leaving pod is leader
-		if switchoverErr := tryToSwitchover(lfa, pod); switchoverErr != nil {
-			return switchoverErr
+		if err := trySwitchover(lfa, pod); err != nil {
+			return err
 		}
 
-		if err2 := lfa.MemberLeave(r.reqCtx.Ctx, r.cli, nil); err2 != nil {
-			if !errors.Is(err2, lifecycle.ErrActionNotDefined) && err == nil {
-				err = err2
-			}
+		if err := tryMemberLeave(lfa); err != nil {
+			return err
 		}
 	}
 	return err // TODO: use requeue-after
diff --git a/pkg/testutil/apps/cluster_instance_set_test_util.go b/pkg/testutil/apps/cluster_instance_set_test_util.go
index 832a6d5374c..51e05ca087b 100644
--- a/pkg/testutil/apps/cluster_instance_set_test_util.go
+++ b/pkg/testutil/apps/cluster_instance_set_test_util.go
@@ -314,7 +314,7 @@ func MockInstanceSetStatus(testCtx testutil.TestContext, cluster *appsv1.Cluster
 		if _, ok := pod.Labels[constant.RoleLabelKey]; !ok {
 			continue
 		}
-		role := &workloads.ReplicaRole{}
+		var role *workloads.ReplicaRole
 		for _, r := range its.Spec.Roles {
 			if r.Name == pod.Labels[constant.RoleLabelKey] {
 				role = r.DeepCopy()