Skip to content

Commit

Permalink
fix: remove label service-available when pod is not ready (#281)
Browse files Browse the repository at this point in the history
  • Loading branch information
@shaofan-hs
shaofan-hs authored Sep 23, 2024
1 parent 3d660e9 commit 1c69095
Show file tree
Hide file tree
Showing 5 changed files with 119 additions and 32 deletions.
49 changes: 43 additions & 6 deletions pkg/controllers/podopslifecycle/podopslifecycle_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -49,6 +49,10 @@ const (
controllerName = "podopslifecycle-controller"
)

var (
IsPodReadyFunc = controllersutils.IsPodReady
)

func Add(mgr manager.Manager) error {
return AddToMgr(mgr, NewReconciler(mgr))
}
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}

Expand All @@ -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
}
Expand All @@ -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
}

Expand All @@ -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
}
Expand Down Expand Up @@ -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()
Expand All @@ -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
})
}

Expand Down
86 changes: 68 additions & 18 deletions pkg/controllers/podopslifecycle/podopslifecycle_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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())
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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",
Expand All @@ -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
}
})
})

Expand Down
8 changes: 4 additions & 4 deletions pkg/controllers/podopslifecycle/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{}

Expand Down Expand Up @@ -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
}
Expand All @@ -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
}
4 changes: 2 additions & 2 deletions pkg/controllers/podopslifecycle/utils_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ var (
namespace = "default"
)

func TestPodIDAndTypesMap(t *testing.T) {
func TestIDToLabelsMap(t *testing.T) {
casee := []struct {
keyWords string

Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions pkg/webhook/server/generic/pod/opslifecycle/mutating.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down

0 comments on commit 1c69095

Please sign in to comment.