kubelet: Mark ready condition as false explicitly for terminal pods

Terminal pods may continue to report a ready condition of true because
there is a delay in reconciling the ready condition of the containers
from the runtime with the pod status. It should be invalid for kubelet
to report a terminal phase with a true ready condition. To fix the
issue, explicitly override the ready condition to false for terminal
pods during status updates.

Signed-off-by: David Porter <david@porter.me>

pkg/api/v1/pod/util.go

@@ -317,6 +317,12 @@ func IsPodReadyConditionTrue(status v1.PodStatus) bool {
 	return condition != nil && condition.Status == v1.ConditionTrue
 }
 
+// IsContainersReadyConditionTrue returns true if a pod is ready; false otherwise.
+func IsContainersReadyConditionTrue(status v1.PodStatus) bool {
+	condition := GetContainersReadyCondition(status)
+	return condition != nil && condition.Status == v1.ConditionTrue
+}
+
 // GetPodReadyCondition extracts the pod ready condition from the given status and returns that.
 // Returns nil if the condition is not present.
 func GetPodReadyCondition(status v1.PodStatus) *v1.PodCondition {
@@ -324,6 +330,13 @@ func GetPodReadyCondition(status v1.PodStatus) *v1.PodCondition {
 	return condition
 }
 
+// GetContainersReadyCondition extracts the containers ready condition from the given status and returns that.
+// Returns nil if the condition is not present.
+func GetContainersReadyCondition(status v1.PodStatus) *v1.PodCondition {
+	_, condition := GetPodCondition(&status, v1.ContainersReady)
+	return condition
+}
+
 // GetPodCondition extracts the provided condition from the given status and returns that.
 // Returns nil and -1 if the condition is not present, and the index of the located condition.
 func GetPodCondition(status *v1.PodStatus, conditionType v1.PodConditionType) (int, *v1.PodCondition) {

pkg/api/v1/pod/util_test.go

@@ -904,3 +904,97 @@ func TestUpdatePodCondition(t *testing.T) {
 		assert.Equal(t, test.expected, resultStatus, test.desc)
 	}
 }
+
+func TestGetContainersReadyCondition(t *testing.T) {
+	time := metav1.Now()
+
+	containersReadyCondition := v1.PodCondition{
+		Type:               v1.ContainersReady,
+		Status:             v1.ConditionTrue,
+		Reason:             "successfully",
+		Message:            "sync pod successfully",
+		LastProbeTime:      time,
+		LastTransitionTime: metav1.NewTime(time.Add(1000)),
+	}
+
+	tests := []struct {
+		desc              string
+		podStatus         v1.PodStatus
+		expectedCondition *v1.PodCondition
+	}{
+		{
+			desc: "containers ready condition exists",
+			podStatus: v1.PodStatus{
+				Conditions: []v1.PodCondition{containersReadyCondition},
+			},
+			expectedCondition: &containersReadyCondition,
+		},
+		{
+			desc: "containers ready condition does not exist",
+			podStatus: v1.PodStatus{
+				Conditions: []v1.PodCondition{},
+			},
+			expectedCondition: nil,
+		},
+	}
+
+	for _, test := range tests {
+		containersReadyCondition := GetContainersReadyCondition(test.podStatus)
+		assert.Equal(t, test.expectedCondition, containersReadyCondition, test.desc)
+	}
+}
+
+func TestIsContainersReadyConditionTrue(t *testing.T) {
+	time := metav1.Now()
+
+	tests := []struct {
+		desc      string
+		podStatus v1.PodStatus
+		expected  bool
+	}{
+		{
+			desc: "containers ready condition is true",
+			podStatus: v1.PodStatus{
+				Conditions: []v1.PodCondition{
+					{
+						Type:               v1.ContainersReady,
+						Status:             v1.ConditionTrue,
+						Reason:             "successfully",
+						Message:            "sync pod successfully",
+						LastProbeTime:      time,
+						LastTransitionTime: metav1.NewTime(time.Add(1000)),
+					},
+				},
+			},
+			expected: true,
+		},
+		{
+			desc: "containers ready condition is false",
+			podStatus: v1.PodStatus{
+				Conditions: []v1.PodCondition{
+					{
+						Type:               v1.ContainersReady,
+						Status:             v1.ConditionFalse,
+						Reason:             "successfully",
+						Message:            "sync pod successfully",
+						LastProbeTime:      time,
+						LastTransitionTime: metav1.NewTime(time.Add(1000)),
+					},
+				},
+			},
+			expected: false,
+		},
+		{
+			desc: "containers ready condition is empty",
+			podStatus: v1.PodStatus{
+				Conditions: []v1.PodCondition{},
+			},
+			expected: false,
+		},
+	}
+
+	for _, test := range tests {
+		isContainersReady := IsContainersReadyConditionTrue(test.podStatus)
+		assert.Equal(t, test.expected, isContainersReady, test.desc)
+	}
+}

pkg/kubelet/status/generate.go

@@ -20,7 +20,7 @@ import (
 	"fmt"
 	"strings"
 
-	"k8s.io/api/core/v1"
+	v1 "k8s.io/api/core/v1"
 	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
 )
 
@@ -29,6 +29,8 @@ const (
 	UnknownContainerStatuses = "UnknownContainerStatuses"
 	// PodCompleted says that all related containers have succeeded.
 	PodCompleted = "PodCompleted"
+	// PodFailed says that the pod has failed and as such the containers have failed.
+	PodFailed = "PodFailed"
 	// ContainersNotReady says that one or more containers are not ready.
 	ContainersNotReady = "ContainersNotReady"
 	// ContainersNotInitialized says that one or more init containers have not succeeded.
@@ -62,11 +64,12 @@ func GenerateContainersReadyCondition(spec *v1.PodSpec, containerStatuses []v1.C
 
 	// If all containers are known and succeeded, just return PodCompleted.
 	if podPhase == v1.PodSucceeded && len(unknownContainers) == 0 {
-		return v1.PodCondition{
-			Type:   v1.ContainersReady,
-			Status: v1.ConditionFalse,
-			Reason: PodCompleted,
-		}
+		return generateContainersReadyConditionForTerminalPhase(podPhase)
+	}
+
+	// If the pod phase is failed, explicitly set the ready condition to false for containers since they may be in progress of terminating.
+	if podPhase == v1.PodFailed {
+		return generateContainersReadyConditionForTerminalPhase(podPhase)
 	}
 
 	// Generate message for containers in unknown condition.
@@ -191,3 +194,33 @@ func GeneratePodInitializedCondition(spec *v1.PodSpec, containerStatuses []v1.Co
 		Status: v1.ConditionTrue,
 	}
 }
+
+func generateContainersReadyConditionForTerminalPhase(podPhase v1.PodPhase) v1.PodCondition {
+	condition := v1.PodCondition{
+		Type:   v1.ContainersReady,
+		Status: v1.ConditionFalse,
+	}
+
+	if podPhase == v1.PodFailed {
+		condition.Reason = PodFailed
+	} else if podPhase == v1.PodSucceeded {
+		condition.Reason = PodCompleted
+	}
+
+	return condition
+}
+
+func generatePodReadyConditionForTerminalPhase(podPhase v1.PodPhase) v1.PodCondition {
+	condition := v1.PodCondition{
+		Type:   v1.PodReady,
+		Status: v1.ConditionFalse,
+	}
+
+	if podPhase == v1.PodFailed {
+		condition.Reason = PodFailed
+	} else if podPhase == v1.PodSucceeded {
+		condition.Reason = PodCompleted
+	}
+
+	return condition
+}

pkg/kubelet/status/status_manager.go

@@ -867,6 +867,20 @@ func mergePodStatus(oldPodStatus, newPodStatus v1.PodStatus, couldHaveRunningCon
 		}
 	}
 
+	// If the new phase is terminal, explicitly set the ready condition to false for v1.PodReady and v1.ContainersReady.
+	// It may take some time for kubelet to reconcile the ready condition, so explicitly set ready conditions to false if the phase is terminal.
+	// This is done to ensure kubelet does not report a status update with terminal pod phase and ready=true.
+	// See https://issues.k8s.io/108594 for more details.
+	if podutil.IsPodPhaseTerminal(newPodStatus.Phase) {
+		if podutil.IsPodReadyConditionTrue(newPodStatus) || podutil.IsContainersReadyConditionTrue(newPodStatus) {
+			containersReadyCondition := generateContainersReadyConditionForTerminalPhase(newPodStatus.Phase)
+			podutil.UpdatePodCondition(&newPodStatus, &containersReadyCondition)
+
+			podReadyCondition := generatePodReadyConditionForTerminalPhase(newPodStatus.Phase)
+			podutil.UpdatePodCondition(&newPodStatus, &podReadyCondition)
+		}
+	}
+
 	return newPodStatus
 }
 

pkg/kubelet/status/status_manager_test.go

@@ -1548,7 +1548,11 @@ func TestMergePodStatus(t *testing.T) {
 				Conditions: []v1.PodCondition{
 					{
 						Type:   v1.PodReady,
-						Status: v1.ConditionTrue,
+						Status: v1.ConditionFalse,
+					},
+					{
+						Type:   v1.ContainersReady,
+						Status: v1.ConditionFalse,
 					},
 					{
 						Type:   v1.PodScheduled,