From cfd8c0de2d6a4308aa206092c6c5f7748ba25368 Mon Sep 17 00:00:00 2001 From: chenpeicheng Date: Sun, 21 Jan 2024 10:48:35 +0800 Subject: [PATCH] the pod lifecycle can be transformed from PreparingNormal to Normal only if ContainerReady is true(#1485) --- apis/apps/pub/lifecycle.go | 2 +- .../cloneset/sync/cloneset_update.go | 10 +++-- .../cloneset/sync/cloneset_update_test.go | 41 +++++++++++++++++++ pkg/util/lifecycle/lifecycle_utils.go | 17 ++++++++ pkg/util/pods.go | 4 ++ 5 files changed, 70 insertions(+), 4 deletions(-) diff --git a/apis/apps/pub/lifecycle.go b/apis/apps/pub/lifecycle.go index ec6cb49869..44aec77572 100644 --- a/apis/apps/pub/lifecycle.go +++ b/apis/apps/pub/lifecycle.go @@ -21,7 +21,7 @@ const ( LifecycleTimestampKey = "lifecycle.apps.kruise.io/timestamp" // LifecycleStatePreparingNormal means the Pod is created but unavailable. - // It will translate to Normal state if Lifecycle.PreNormal is hooked. + // It will translate to Normal state if Lifecycle.PreNormal is hooked and pod condition ContainerReady is true. LifecycleStatePreparingNormal LifecycleStateType = "PreparingNormal" // LifecycleStateNormal is a necessary condition for Pod to be available. LifecycleStateNormal LifecycleStateType = "Normal" diff --git a/pkg/controller/cloneset/sync/cloneset_update.go b/pkg/controller/cloneset/sync/cloneset_update.go index 40089214ec..a3e1cc7343 100644 --- a/pkg/controller/cloneset/sync/cloneset_update.go +++ b/pkg/controller/cloneset/sync/cloneset_update.go @@ -169,9 +169,13 @@ func (c *realControl) refreshPodState(cs *appsv1alpha1.CloneSet, coreControl clo var state appspub.LifecycleStateType switch lifecycle.GetPodLifecycleState(pod) { case appspub.LifecycleStatePreparingNormal: - if cs.Spec.Lifecycle == nil || - cs.Spec.Lifecycle.PreNormal == nil || - lifecycle.IsPodAllHooked(cs.Spec.Lifecycle.PreNormal, pod) { + // 1.the pod lifecycle can be transformed from PreparingNormal to Normal only if ContainerReady is true: + // because: https://github.com/openkruise/kruise/issues/1485 + // 2.the semantic of this hook can be a bit confusing, but let's leave it as it is: + // PreNormalHook: only if all hooks are hooked (or no hook), the pod can be transformed to Normal + // PreDeleteHook: if any hook is hooked, we can NOT delete the pod. + // InPlaceUpdateHook: if any hook is hooked, we can NOT inplaceUpdate the pod. + if util.IsRunningAndContainerReady(pod) && lifecycle.IsPreNormalHookNilOrAllHooked(cs.Spec.Lifecycle, pod) { state = appspub.LifecycleStateNormal } case appspub.LifecycleStatePreparingUpdate: diff --git a/pkg/controller/cloneset/sync/cloneset_update_test.go b/pkg/controller/cloneset/sync/cloneset_update_test.go index 17add8342a..6f3e6f843f 100644 --- a/pkg/controller/cloneset/sync/cloneset_update_test.go +++ b/pkg/controller/cloneset/sync/cloneset_update_test.go @@ -592,6 +592,7 @@ func TestUpdate(t *testing.T) { Spec: v1.PodSpec{ReadinessGates: []v1.PodReadinessGate{{ConditionType: appspub.InPlaceUpdateReady}}}, Status: v1.PodStatus{Phase: v1.PodRunning, Conditions: []v1.PodCondition{ {Type: v1.PodReady, Status: v1.ConditionFalse}, + {Type: v1.ContainersReady, Status: v1.ConditionTrue}, {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue}, }}, }, @@ -606,6 +607,7 @@ func TestUpdate(t *testing.T) { Spec: v1.PodSpec{ReadinessGates: []v1.PodReadinessGate{{ConditionType: appspub.InPlaceUpdateReady}}}, Status: v1.PodStatus{Phase: v1.PodRunning, Conditions: []v1.PodCondition{ {Type: v1.PodReady, Status: v1.ConditionFalse}, + {Type: v1.ContainersReady, Status: v1.ConditionTrue}, {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue}, }}, }, @@ -669,6 +671,7 @@ func TestUpdate(t *testing.T) { Spec: v1.PodSpec{ReadinessGates: []v1.PodReadinessGate{{ConditionType: appspub.InPlaceUpdateReady}}}, Status: v1.PodStatus{Phase: v1.PodRunning, Conditions: []v1.PodCondition{ {Type: v1.PodReady, Status: v1.ConditionFalse}, + {Type: v1.ContainersReady, Status: v1.ConditionTrue}, {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue}, }}, }, @@ -684,6 +687,7 @@ func TestUpdate(t *testing.T) { Spec: v1.PodSpec{ReadinessGates: []v1.PodReadinessGate{{ConditionType: appspub.InPlaceUpdateReady}}}, Status: v1.PodStatus{Phase: v1.PodRunning, Conditions: []v1.PodCondition{ {Type: v1.PodReady, Status: v1.ConditionFalse}, + {Type: v1.ContainersReady, Status: v1.ConditionTrue}, {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue}, }}, }, @@ -843,6 +847,7 @@ func TestUpdate(t *testing.T) { Phase: v1.PodRunning, Conditions: []v1.PodCondition{ {Type: v1.PodReady, Status: v1.ConditionTrue}, + {Type: v1.ContainersReady, Status: v1.ConditionTrue}, {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue, LastTransitionTime: now}, }, ContainerStatuses: []v1.ContainerStatus{{Name: "c1", ImageID: "image-id-xyz"}}, @@ -867,6 +872,7 @@ func TestUpdate(t *testing.T) { Phase: v1.PodRunning, Conditions: []v1.PodCondition{ {Type: v1.PodReady, Status: v1.ConditionTrue}, + {Type: v1.ContainersReady, Status: v1.ConditionTrue}, {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue, LastTransitionTime: now}, }, ContainerStatuses: []v1.ContainerStatus{{Name: "c1", ImageID: "image-id-xyz"}}, @@ -945,6 +951,41 @@ func TestUpdate(t *testing.T) { }, }, }, + { + name: "create: preparingNormal->Normal without hook, pod not ready", + cs: &appsv1alpha1.CloneSet{Spec: appsv1alpha1.CloneSetSpec{Replicas: getInt32Pointer(1)}}, + updateRevision: &apps.ControllerRevision{ObjectMeta: metav1.ObjectMeta{Name: "rev_new"}}, + pods: []*v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{Name: "pod-0", Labels: map[string]string{ + apps.ControllerRevisionHashLabelKey: "rev_new", + apps.DefaultDeploymentUniqueLabelKey: "rev_new", + appspub.LifecycleStateKey: string(appspub.LifecycleStatePreparingNormal), + }}, + Spec: v1.PodSpec{ReadinessGates: []v1.PodReadinessGate{{ConditionType: appspub.InPlaceUpdateReady}}}, + Status: v1.PodStatus{Phase: v1.PodRunning, Conditions: []v1.PodCondition{ + {Type: v1.PodReady, Status: v1.ConditionFalse}, + {Type: v1.ContainersReady, Status: v1.ConditionFalse}, + {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue}, + }}, + }, + }, + expectedPods: []*v1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{Name: "pod-0", Labels: map[string]string{ + apps.ControllerRevisionHashLabelKey: "rev_new", + apps.DefaultDeploymentUniqueLabelKey: "rev_new", + appspub.LifecycleStateKey: string(appspub.LifecycleStatePreparingNormal), + }}, + Spec: v1.PodSpec{ReadinessGates: []v1.PodReadinessGate{{ConditionType: appspub.InPlaceUpdateReady}}}, + Status: v1.PodStatus{Phase: v1.PodRunning, Conditions: []v1.PodCondition{ + {Type: v1.PodReady, Status: v1.ConditionFalse}, + {Type: v1.ContainersReady, Status: v1.ConditionFalse}, + {Type: appspub.InPlaceUpdateReady, Status: v1.ConditionTrue}, + }}, + }, + }, + }, } inplaceupdate.Clock = clock.NewFakeClock(now.Time) diff --git a/pkg/util/lifecycle/lifecycle_utils.go b/pkg/util/lifecycle/lifecycle_utils.go index ea819ace2c..1c14f302bf 100644 --- a/pkg/util/lifecycle/lifecycle_utils.go +++ b/pkg/util/lifecycle/lifecycle_utils.go @@ -237,6 +237,23 @@ func IsPodAllHooked(hook *appspub.LifecycleHook, pod *v1.Pod) bool { return true } +// IsPreNormalHookNilOrAllHooked +// pod lifecycle can be transformed to Normal only if no hook or all hooks are hooked +func IsPreNormalHookNilOrAllHooked(hooks *appspub.Lifecycle, pod *v1.Pod) bool { + // nothing changed, but avoiding excessive complexity in if conditions + return hooks == nil || + hooks.PreNormal == nil || + IsPodAllHooked(hooks.PreNormal, pod) +} + +// IsInPlaceUpdateHookNilOrAllHooked +// after updating, we need to wait for all InPlaceUpdateHook hooks to be hooked back +func IsInPlaceUpdateHookNilOrAllHooked(hooks *appspub.Lifecycle, pod *v1.Pod) bool { + return hooks == nil || + hooks.InPlaceUpdate == nil || + IsPodAllHooked(hooks.InPlaceUpdate, pod) +} + func getReadinessMessage(key string) podreadiness.Message { return podreadiness.Message{UserAgent: "Lifecycle", Key: key} } diff --git a/pkg/util/pods.go b/pkg/util/pods.go index 7dae961a2c..f46acd1ac0 100644 --- a/pkg/util/pods.go +++ b/pkg/util/pods.go @@ -204,6 +204,10 @@ func IsRunningAndReady(pod *v1.Pod) bool { return pod.Status.Phase == v1.PodRunning && podutil.IsPodReady(pod) && pod.DeletionTimestamp.IsZero() } +func IsRunningAndContainerReady(pod *v1.Pod) bool { + return pod.Status.Phase == v1.PodRunning && podutil.IsContainersReadyConditionTrue(pod.Status) && pod.DeletionTimestamp.IsZero() +} + func GetPodContainerImageIDs(pod *v1.Pod) map[string]string { cImageIDs := make(map[string]string, len(pod.Status.ContainerStatuses)) for i := range pod.Status.ContainerStatuses {