fix: remove label service-available when pod is not ready

pkg/controllers/podopslifecycle/podopslifecycle_controller.go

@@ -39,7 +39,7 @@ import (
 	"kusionstack.io/kube-api/apps/v1alpha1"
 
 	"kusionstack.io/kuperator/pkg/controllers/podtransitionrule"
-	controllerutils "kusionstack.io/kuperator/pkg/controllers/utils"
+	controllersutils "kusionstack.io/kuperator/pkg/controllers/utils"
 	"kusionstack.io/kuperator/pkg/controllers/utils/expectations"
 	"kusionstack.io/kuperator/pkg/utils"
 	"kusionstack.io/kuperator/pkg/utils/mixin"
@@ -49,6 +49,10 @@ const (
 	controllerName = "podopslifecycle-controller"
 )
 
+var (
+	IsPodReadyFunc = controllersutils.IsPodReady
+)
+
 func Add(mgr manager.Manager) error {
 	return AddToMgr(mgr, NewReconciler(mgr))
 }
@@ -126,7 +130,7 @@ func (r *ReconcilePodOpsLifecycle) Reconcile(ctx context.Context, request reconc
 		return reconcile.Result{}, nil
 	}
 
-	idToLabelsMap, _, err := PodIDAndTypesMap(pod)
+	idToLabelsMap, _, err := IDToLabelsMap(pod)
 	if err != nil {
 		return reconcile.Result{}, err
 	}
@@ -204,6 +208,13 @@ func (r *ReconcilePodOpsLifecycle) Reconcile(ctx context.Context, request reconc
 			break
 		}
 	}
+
+	// Remove label service-available if pod is not ready
+	if !IsPodReadyFunc(pod) {
+		err := r.removeLabels(ctx, pod, []string{v1alpha1.PodServiceAvailableLabel})
+		return reconcile.Result{}, err
+	}
+
 	return reconcile.Result{}, nil
 }
 
@@ -217,7 +228,7 @@ func (r *ReconcilePodOpsLifecycle) addServiceAvailable(pod *corev1.Pod) (bool, e
 	}
 
 	// Whether all expected finalizers are satisfied
-	satisfied, notSatisfiedFinalizers, err := controllerutils.IsExpectedFinalizerSatisfied(pod)
+	satisfied, notSatisfiedFinalizers, err := controllersutils.IsExpectedFinalizerSatisfied(pod)
 	if err != nil {
 		return false, err
 	}
@@ -233,7 +244,7 @@ func (r *ReconcilePodOpsLifecycle) addServiceAvailable(pod *corev1.Pod) (bool, e
 		// All not satisfied finalizers are dirty, so actually the pod satisfied expected finalizers now
 	}
 
-	if !controllerutils.IsPodReady(pod) {
+	if !controllersutils.IsPodReady(pod) {
 		return false, nil
 	}
 
@@ -260,7 +271,7 @@ func (r *ReconcilePodOpsLifecycle) removeDirtyExpectedFinalizer(pod *corev1.Pod,
 	}
 
 	if len(dirtyExpectedFinalizer) > 0 {
-		podAvailableConditions, err := controllerutils.PodAvailableConditions(pod)
+		podAvailableConditions, err := controllersutils.PodAvailableConditions(pod)
 		if err != nil {
 			return allDirty, err
 		}
@@ -462,6 +473,32 @@ func (r *ReconcilePodOpsLifecycle) addLabels(ctx context.Context, pod *corev1.Po
 	return err
 }
 
+func (r *ReconcilePodOpsLifecycle) removeLabels(ctx context.Context, pod *corev1.Pod, labels []string) error {
+	if len(labels) == 0 {
+		return nil
+	}
+
+	key := controllerKey(pod)
+	r.expectation.ExpectUpdate(key, pod.ResourceVersion)
+	err := retry.RetryOnConflict(retry.DefaultRetry, func() error {
+		newPod := &corev1.Pod{}
+		err := r.Client.Get(ctx, types.NamespacedName{Namespace: pod.Namespace, Name: pod.Name}, newPod)
+		if err != nil {
+			return err
+		}
+		for _, label := range labels {
+			delete(newPod.Labels, label)
+		}
+		return r.Client.Update(ctx, newPod)
+	})
+	if err != nil {
+		r.Logger.Error(err, "failed to remove pod labels", "pod", utils.ObjectKeyString(pod), "labels", labels)
+		r.expectation.DeleteExpectations(key)
+	}
+
+	return err
+}
+
 func (r *ReconcilePodOpsLifecycle) initPodTransitionRuleManager() {
 	r.podTransitionRuleManager.RegisterStage(v1alpha1.PodOpsLifecyclePreCheckStage, func(po client.Object) bool {
 		labels := po.GetLabels()
@@ -472,7 +509,7 @@ func (r *ReconcilePodOpsLifecycle) initPodTransitionRuleManager() {
 		return labels != nil && labelHasPrefix(labels, v1alpha1.PodPostCheckLabelPrefix)
 	})
 	podtransitionrule.AddUnAvailableFunc(func(po *corev1.Pod) (bool, *int64) {
-		return !controllerutils.IsPodServiceAvailable(po), nil
+		return !controllersutils.IsPodServiceAvailable(po), nil
 	})
 }
 

pkg/controllers/podopslifecycle/podopslifecycle_controller_test.go

@@ -43,6 +43,7 @@ import (
 
 	"kusionstack.io/kuperator/pkg/controllers/podtransitionrule"
 	"kusionstack.io/kuperator/pkg/controllers/podtransitionrule/checker"
+	controllersutils "kusionstack.io/kuperator/pkg/controllers/utils"
 	"kusionstack.io/kuperator/pkg/controllers/utils/expectations"
 	"kusionstack.io/kuperator/pkg/utils/mixin"
 )
@@ -87,6 +88,10 @@ var _ = BeforeSuite(func() {
 	err = AddToMgr(mgr, r)
 	Expect(err).NotTo(HaveOccurred())
 
+	IsPodReadyFunc = func(pod *corev1.Pod) bool {
+		return true
+	}
+
 	go func() {
 		err = mgr.Start(ctx)
 		Expect(err).NotTo(HaveOccurred())
@@ -423,7 +428,7 @@ var _ = Describe("Stage processing", func() {
 			Spec: podSpec,
 		}
 
-		idToLabelsMap, _, err := PodIDAndTypesMap(pod)
+		idToLabelsMap, _, err := IDToLabelsMap(pod)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(len(idToLabelsMap)).To(Equal(2))
 		fmt.Println(idToLabelsMap)
@@ -459,7 +464,7 @@ var _ = Describe("Stage processing", func() {
 			Spec: podSpec,
 		}
 
-		idToLabelsMap, _, err := PodIDAndTypesMap(pod)
+		idToLabelsMap, _, err := IDToLabelsMap(pod)
 		Expect(err).NotTo(HaveOccurred())
 		Expect(len(idToLabelsMap)).To(Equal(2))
 		fmt.Println(idToLabelsMap)
@@ -577,39 +582,80 @@ var _ = Describe("Label service-available processing", func() {
 	err := corev1.AddToScheme(scheme)
 	Expect(err).NotTo(HaveOccurred())
 
-	pod := &corev1.Pod{
-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "test1",
-			Namespace: "default",
-			Labels: map[string]string{
-				v1alpha1.ControlledByKusionStackLabelKey: "true",
+	pods := []client.Object{
+		&corev1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test0",
+				Namespace: "default",
+				Labels: map[string]string{
+					v1alpha1.ControlledByKusionStackLabelKey: "true",
+				},
+			},
+			Status: corev1.PodStatus{
+				Phase: corev1.PodRunning,
+				Conditions: []corev1.PodCondition{
+					{
+						Type:   corev1.PodReady,
+						Status: corev1.ConditionTrue,
+					},
+				},
 			},
 		},
-		Status: corev1.PodStatus{
-			Phase: corev1.PodRunning,
-			Conditions: []corev1.PodCondition{
-				{
-					Type:   corev1.PodReady,
-					Status: corev1.ConditionTrue,
+		&corev1.Pod{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "test1",
+				Namespace: "default",
+				Labels: map[string]string{
+					v1alpha1.ControlledByKusionStackLabelKey: "true",
+					v1alpha1.PodServiceAvailableLabel:        "true",
+				},
+			},
+			Status: corev1.PodStatus{
+				Phase: corev1.PodFailed,
+				Conditions: []corev1.PodCondition{
+					{
+						Type:   corev1.PodReady,
+						Status: corev1.ConditionFalse,
+					},
 				},
 			},
 		},
 	}
 
 	fakeClient := fake.NewClientBuilder().
 		WithScheme(scheme).
-		WithObjects(pod).
+		WithObjects(pods...).
 		Build()
 
 	podOpsLifecycle := &ReconcilePodOpsLifecycle{
 		ReconcilerMixin: &mixin.ReconcilerMixin{
 			Client: fakeClient,
 			Logger: klogr.New().WithName(controllerName),
 		},
-		expectation: expectations.NewResourceVersionExpectation(),
+		podTransitionRuleManager: &mockPodTransitionRuleManager{},
+		expectation:              expectations.NewResourceVersionExpectation(),
 	}
 
-	It("Service is available", func() {
+	It("Pod is ready", func() {
+		_, err := podOpsLifecycle.Reconcile(context.Background(), reconcile.Request{
+			NamespacedName: types.NamespacedName{
+				Name:      "test0",
+				Namespace: "default",
+			},
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		pod1 := &corev1.Pod{}
+		fakeClient.Get(context.Background(), client.ObjectKey{
+			Name:      "test0",
+			Namespace: "default",
+		}, pod1)
+		Expect(pod1.Labels[v1alpha1.PodServiceAvailableLabel]).NotTo(BeEmpty())
+	})
+
+	It("Pod is not ready", func() {
+		IsPodReadyFunc = controllersutils.IsPodReady
+
 		_, err := podOpsLifecycle.Reconcile(context.Background(), reconcile.Request{
 			NamespacedName: types.NamespacedName{
 				Name:      "test1",
@@ -623,7 +669,11 @@ var _ = Describe("Label service-available processing", func() {
 			Name:      "test1",
 			Namespace: "default",
 		}, pod1)
-		Expect(pod1.Labels[v1alpha1.PodServiceAvailableLabel]).NotTo(BeEmpty())
+		Expect(pod1.Labels[v1alpha1.PodServiceAvailableLabel]).To(BeEmpty())
+
+		IsPodReadyFunc = func(pod *corev1.Pod) bool {
+			return true
+		}
 	})
 })
 

pkg/controllers/podopslifecycle/utils.go

@@ -26,8 +26,8 @@ import (
 	"kusionstack.io/kube-api/apps/v1alpha1"
 )
 
-// PodIDAndTypesMap returns a map of pod id to labels map and a map of operation type to number of pods.
-func PodIDAndTypesMap(pod *corev1.Pod) (map[string]map[string]string, map[string]int, error) {
+// IDToLabelsMap returns a map of pod id to labels map and a map of operation type to number of pods.
+func IDToLabelsMap(pod *corev1.Pod) (map[string]map[string]string, map[string]int, error) {
 	idToLabelsMap := map[string]map[string]string{}
 	typeToNumsMap := map[string]int{}
 
@@ -80,7 +80,7 @@ func IsLifecycleOnPod(lifecycleId string, pod *corev1.Pod) (bool, error) {
 	if pod == nil {
 		return false, nil
 	}
-	newIDToLabelsMap, _, err := PodIDAndTypesMap(pod)
+	newIDToLabelsMap, _, err := IDToLabelsMap(pod)
 	_, exist := newIDToLabelsMap[lifecycleId]
 	return exist, err
 }
@@ -90,6 +90,6 @@ func NumOfLifecycleOnPod(pod *corev1.Pod) (int, error) {
 	if pod == nil {
 		return 0, nil
 	}
-	newIDToLabelsMap, _, err := PodIDAndTypesMap(pod)
+	newIDToLabelsMap, _, err := IDToLabelsMap(pod)
 	return len(newIDToLabelsMap), err
 }

pkg/controllers/podopslifecycle/utils_test.go

@@ -31,7 +31,7 @@ var (
 	namespace = "default"
 )
 
-func TestPodIDAndTypesMap(t *testing.T) {
+func TestIDToLabelsMap(t *testing.T) {
 	casee := []struct {
 		keyWords string
 
@@ -162,7 +162,7 @@ func TestPodIDAndTypesMap(t *testing.T) {
 			},
 		}
 
-		idToLabelsMap, typeToNumsMap, err := PodIDAndTypesMap(pod)
+		idToLabelsMap, typeToNumsMap, err := IDToLabelsMap(pod)
 		if c.err != nil {
 			if err == nil {
 				t.Errorf("%s, expect err %v, got nil", c.keyWords, c.err)

pkg/webhook/server/generic/pod/opslifecycle/mutating.go

@@ -42,7 +42,7 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 		addReadinessGates(newPod, v1alpha1.ReadinessGatePodServiceReady)
 	}
 
-	newIDToLabelsMap, typeToNumsMap, err := podopslifecycle.PodIDAndTypesMap(newPod)
+	newIDToLabelsMap, typeToNumsMap, err := podopslifecycle.IDToLabelsMap(newPod)
 	if err != nil {
 		return err
 	}
@@ -146,7 +146,7 @@ func (lc *OpsLifecycle) Mutating(ctx context.Context, c client.Client, oldPod, n
 	}
 
 	if operateCount == numOfIDs { // All operations are going to be done
-		oldIdToLabelsMap, _, err := podopslifecycle.PodIDAndTypesMap(oldPod)
+		oldIdToLabelsMap, _, err := podopslifecycle.IDToLabelsMap(oldPod)
 		if err != nil {
 			return err
 		}