From fa31df5e96bf715499d2521f4c5b1f7fe37a16c1 Mon Sep 17 00:00:00 2001 From: shaofan-hs Date: Thu, 19 Sep 2024 20:39:24 +0800 Subject: [PATCH] fix: remove label service-available when pod is not ready --- .../podopslifecycle_controller.go | 48 +++++++++-- .../podopslifecycle_controller_test.go | 83 +++++++++++++++---- pkg/controllers/podopslifecycle/utils.go | 8 +- pkg/controllers/podopslifecycle/utils_test.go | 2 +- .../generic/pod/opslifecycle/mutating.go | 4 +- 5 files changed, 115 insertions(+), 30 deletions(-) diff --git a/pkg/controllers/podopslifecycle/podopslifecycle_controller.go b/pkg/controllers/podopslifecycle/podopslifecycle_controller.go index fa9c91d4..f166c49f 100644 --- a/pkg/controllers/podopslifecycle/podopslifecycle_controller.go +++ b/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 } @@ -152,6 +156,12 @@ func (r *ReconcilePodOpsLifecycle) Reconcile(ctx context.Context, request reconc } } + // Remove label service-available if pod is not ready + if !IsPodReadyFunc(pod) { + err := r.removeLabels(ctx, pod, []string{v1alpha1.PodServiceAvailableLabel}) + return reconcile.Result{}, err + } + // Get the state of pod managed by TransitionRule state, err := r.podTransitionRuleManager.GetState(ctx, r.Client, pod) if err != nil { @@ -217,7 +227,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 +243,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 +270,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 +472,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 +508,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 }) } diff --git a/pkg/controllers/podopslifecycle/podopslifecycle_controller_test.go b/pkg/controllers/podopslifecycle/podopslifecycle_controller_test.go index 65e62103..45d0d6b9 100644 --- a/pkg/controllers/podopslifecycle/podopslifecycle_controller_test.go +++ b/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,20 +582,41 @@ 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, + }, }, }, }, @@ -598,7 +624,7 @@ var _ = Describe("Label service-available processing", func() { fakeClient := fake.NewClientBuilder(). WithScheme(scheme). - WithObjects(pod). + WithObjects(pods...). Build() podOpsLifecycle := &ReconcilePodOpsLifecycle{ @@ -609,7 +635,26 @@ var _ = Describe("Label service-available processing", func() { 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 +668,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 + } }) }) diff --git a/pkg/controllers/podopslifecycle/utils.go b/pkg/controllers/podopslifecycle/utils.go index 3541b319..635c2cee 100644 --- a/pkg/controllers/podopslifecycle/utils.go +++ b/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 } diff --git a/pkg/controllers/podopslifecycle/utils_test.go b/pkg/controllers/podopslifecycle/utils_test.go index 9497abd6..4b0f0ee2 100644 --- a/pkg/controllers/podopslifecycle/utils_test.go +++ b/pkg/controllers/podopslifecycle/utils_test.go @@ -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) diff --git a/pkg/webhook/server/generic/pod/opslifecycle/mutating.go b/pkg/webhook/server/generic/pod/opslifecycle/mutating.go index e27e0e7f..208573a4 100644 --- a/pkg/webhook/server/generic/pod/opslifecycle/mutating.go +++ b/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 }