the pod lifecycle can be transformed from PreparingNormal to Normal o…

…nly if ContainerReady is true(#1485)
openkruise · Jan 27, 2024 · cfd8c0d · cfd8c0d
1 parent 30a660b
commit cfd8c0d
Show file tree

Hide file tree

Showing 5 changed files with 70 additions and 4 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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 {