Do not revert the pod condition if there might be running containers,…

… skip condition update instead.
yibozhuang · Nov 7, 2022 · 4e732e2 · 4e732e2
1 parent 52cd675
commit 4e732e2
Show file tree

Hide file tree

Showing 5 changed files with 170 additions and 34 deletions.
diff --git a/pkg/kubelet/kubelet_pods.go b/pkg/kubelet/kubelet_pods.go
@@ -57,6 +57,7 @@ import (
 	"k8s.io/kubernetes/pkg/kubelet/status"
 	kubetypes "k8s.io/kubernetes/pkg/kubelet/types"
 	"k8s.io/kubernetes/pkg/kubelet/util"
+	utilpod "k8s.io/kubernetes/pkg/util/pod"
 	volumeutil "k8s.io/kubernetes/pkg/volume/util"
 	"k8s.io/kubernetes/pkg/volume/util/hostutil"
 	"k8s.io/kubernetes/pkg/volume/util/subpath"
@@ -1518,11 +1519,7 @@ func (kl *Kubelet) generateAPIPodStatus(pod *v1.Pod, podStatus *kubecontainer.Po
 		// on the container statuses as they are added based on one-time events.
 		cType := v1.AlphaNoCompatGuaranteeDisruptionTarget
 		if _, condition := podutil.GetPodConditionFromList(oldPodStatus.Conditions, cType); condition != nil {
-			if i, _ := podutil.GetPodConditionFromList(s.Conditions, cType); i >= 0 {
-				s.Conditions[i] = *condition
-			} else {
-				s.Conditions = append(s.Conditions, *condition)
-			}
+			s.Conditions = utilpod.ReplaceOrAppendPodCondition(s.Conditions, condition)
 		}
 	}
 

diff --git a/pkg/kubelet/status/status_manager.go b/pkg/kubelet/status/status_manager.go
@@ -888,15 +888,24 @@ func mergePodStatus(oldPodStatus, newPodStatus v1.PodStatus, couldHaveRunningCon
 		}
 	}
 
+	transitioningToTerminalPhase := !podutil.IsPodPhaseTerminal(oldPodStatus.Phase) && podutil.IsPodPhaseTerminal(newPodStatus.Phase)
+
 	for _, c := range newPodStatus.Conditions {
 		if kubetypes.PodConditionByKubelet(c.Type) {
 			podConditions = append(podConditions, c)
 		} else if kubetypes.PodConditionSharedByKubelet(c.Type) {
-			// for shared conditions we update or append in podConditions
-			if i, _ := podutil.GetPodConditionFromList(podConditions, c.Type); i >= 0 {
-				podConditions[i] = c
-			} else {
-				podConditions = append(podConditions, c)
+			if c.Type == v1.AlphaNoCompatGuaranteeDisruptionTarget {
+				// update the pod disruption condition only if transitioning to terminal phase. In particular, check if
+				// there might still be running containers to avoid sending an unnecessary PATCH request if the
+				// actual transition is delayed (see below)
+				if transitioningToTerminalPhase && !couldHaveRunningContainers {
+					// update the LastTransitionTime
+					updateLastTransitionTime(&newPodStatus, &oldPodStatus, c.Type)
+					if _, c := podutil.GetPodConditionFromList(newPodStatus.Conditions, c.Type); c != nil {
+						// for shared conditions we update or append in podConditions
+						podConditions = statusutil.ReplaceOrAppendPodCondition(podConditions, c)
+					}
+				}
 			}
 		}
 	}
@@ -911,21 +920,11 @@ func mergePodStatus(oldPodStatus, newPodStatus v1.PodStatus, couldHaveRunningCon
 	// the Kubelet exclusively owns must be released prior to a pod being reported terminal,
 	// while resources that have participanting components above the API use the pod's
 	// transition to a terminal phase (or full deletion) to release those resources.
-	if !podutil.IsPodPhaseTerminal(oldPodStatus.Phase) && podutil.IsPodPhaseTerminal(newPodStatus.Phase) {
+	if transitioningToTerminalPhase {
 		if couldHaveRunningContainers {
 			newPodStatus.Phase = oldPodStatus.Phase
 			newPodStatus.Reason = oldPodStatus.Reason
 			newPodStatus.Message = oldPodStatus.Message
-			if utilfeature.DefaultFeatureGate.Enabled(features.PodDisruptionConditions) {
-				// revert setting of the pod disruption condition until the pod is terminal in order to do not issue
-				// an unnecessary PATCH request
-				revertPodCondition(&oldPodStatus, &newPodStatus, v1.AlphaNoCompatGuaranteeDisruptionTarget)
-			}
-		} else {
-			if utilfeature.DefaultFeatureGate.Enabled(features.PodDisruptionConditions) {
-				// update the LastTransitionTime when transitioning into the failed state
-				updateLastTransitionTime(&newPodStatus, &oldPodStatus, v1.AlphaNoCompatGuaranteeDisruptionTarget)
-			}
 		}
 	}
 
@@ -946,18 +945,6 @@ func mergePodStatus(oldPodStatus, newPodStatus v1.PodStatus, couldHaveRunningCon
 	return newPodStatus
 }
 
-func revertPodCondition(oldPodStatus, newPodStatus *v1.PodStatus, cType v1.PodConditionType) {
-	if newIndex, newCondition := podutil.GetPodConditionFromList(newPodStatus.Conditions, cType); newCondition != nil {
-		if _, oldCondition := podutil.GetPodConditionFromList(oldPodStatus.Conditions, cType); oldCondition != nil {
-			// revert the new condition to what was before
-			newPodStatus.Conditions[newIndex] = *oldCondition
-		} else {
-			// delete the new condition as it wasn't there before
-			newPodStatus.Conditions = append(newPodStatus.Conditions[:newIndex], newPodStatus.Conditions[newIndex+1:]...)
-		}
-	}
-}
-
 // NeedToReconcilePodReadiness returns if the pod "Ready" condition need to be reconcile
 func NeedToReconcilePodReadiness(pod *v1.Pod) bool {
 	if len(pod.Spec.ReadinessGates) == 0 {

diff --git a/pkg/kubelet/status/status_manager_test.go b/pkg/kubelet/status/status_manager_test.go
@@ -1569,6 +1569,53 @@ func TestMergePodStatus(t *testing.T) {
 				})
 				return input
 			},
+			func(input v1.PodStatus) v1.PodStatus {
+				input.Phase = v1.PodFailed
+				input.Conditions = append(input.Conditions, v1.PodCondition{
+					Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
+					Status: v1.ConditionTrue,
+					Reason: "TerminationByKubelet",
+				})
+				return input
+			},
+			v1.PodStatus{
+				Phase: v1.PodFailed,
+				Conditions: []v1.PodCondition{
+					{
+						Type:   v1.PodReady,
+						Status: v1.ConditionFalse,
+						Reason: "PodFailed",
+					},
+					{
+						Type:   v1.ContainersReady,
+						Status: v1.ConditionFalse,
+						Reason: "PodFailed",
+					},
+					{
+						Type:   v1.PodScheduled,
+						Status: v1.ConditionTrue,
+					},
+					{
+						Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
+						Status: v1.ConditionTrue,
+						Reason: "TerminationByKubelet",
+					},
+				},
+				Message: "Message",
+			},
+		},
+		{
+			"don't override DisruptionTarget condition when remaining in running phase; PodDisruptionConditions enabled",
+			true,
+			false,
+			func(input v1.PodStatus) v1.PodStatus {
+				input.Conditions = append(input.Conditions, v1.PodCondition{
+					Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
+					Status: v1.ConditionTrue,
+					Reason: "EvictedByEvictionAPI",
+				})
+				return input
+			},
 			func(input v1.PodStatus) v1.PodStatus {
 				input.Conditions = append(input.Conditions, v1.PodCondition{
 					Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
@@ -1580,6 +1627,11 @@ func TestMergePodStatus(t *testing.T) {
 			v1.PodStatus{
 				Phase: v1.PodRunning,
 				Conditions: []v1.PodCondition{
+					{
+						Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
+						Status: v1.ConditionTrue,
+						Reason: "EvictedByEvictionAPI",
+					},
 					{
 						Type:   v1.PodReady,
 						Status: v1.ConditionTrue,
@@ -1588,10 +1640,46 @@ func TestMergePodStatus(t *testing.T) {
 						Type:   v1.PodScheduled,
 						Status: v1.ConditionTrue,
 					},
+				},
+				Message: "Message",
+			},
+		},
+		{
+			"don't override DisruptionTarget condition when transitioning to failed phase but there might still be running containers; PodDisruptionConditions enabled",
+			true,
+			true,
+			func(input v1.PodStatus) v1.PodStatus {
+				input.Conditions = append(input.Conditions, v1.PodCondition{
+					Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
+					Status: v1.ConditionTrue,
+					Reason: "EvictedByEvictionAPI",
+				})
+				return input
+			},
+			func(input v1.PodStatus) v1.PodStatus {
+				input.Phase = v1.PodFailed
+				input.Conditions = append(input.Conditions, v1.PodCondition{
+					Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
+					Status: v1.ConditionTrue,
+					Reason: "TerminationByKubelet",
+				})
+				return input
+			},
+			v1.PodStatus{
+				Phase: v1.PodRunning,
+				Conditions: []v1.PodCondition{
 					{
 						Type:   v1.AlphaNoCompatGuaranteeDisruptionTarget,
 						Status: v1.ConditionTrue,
-						Reason: "TerminationByKubelet",
+						Reason: "EvictedByEvictionAPI",
+					},
+					{
+						Type:   v1.PodReady,
+						Status: v1.ConditionTrue,
+					},
+					{
+						Type:   v1.PodScheduled,
+						Status: v1.ConditionTrue,
 					},
 				},
 				Message: "Message",

diff --git a/pkg/util/pod/pod.go b/pkg/util/pod/pod.go
@@ -27,6 +27,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	clientset "k8s.io/client-go/kubernetes"
+	podutil "k8s.io/kubernetes/pkg/api/v1/pod"
 )
 
 // PatchPodStatus patches pod status. It returns true and avoids an update if the patch contains no changes.
@@ -68,3 +69,13 @@ func preparePatchBytesForPodStatus(namespace, name string, uid types.UID, oldPod
 	}
 	return patchBytes, bytes.Equal(patchBytes, []byte(fmt.Sprintf(`{"metadata":{"uid":%q}}`, uid))), nil
 }
+
+// ReplaceOrAppendPodCondition replaces the first pod condition with equal type or appends if there is none
+func ReplaceOrAppendPodCondition(conditions []v1.PodCondition, condition *v1.PodCondition) []v1.PodCondition {
+	if i, _ := podutil.GetPodConditionFromList(conditions, condition.Type); i >= 0 {
+		conditions[i] = *condition
+	} else {
+		conditions = append(conditions, *condition)
+	}
+	return conditions
+}
diff --git a/pkg/util/pod/pod_test.go b/pkg/util/pod/pod_test.go
@@ -23,6 +23,7 @@ import (
 
 	"reflect"
 
+	"github.com/google/go-cmp/cmp"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
@@ -128,3 +129,55 @@ func getPodStatus() v1.PodStatus {
 		Message: "Message",
 	}
 }
+
+func TestReplaceOrAppendPodCondition(t *testing.T) {
+	cType := v1.PodConditionType("ExampleType")
+	testCases := []struct {
+		description    string
+		conditions     []v1.PodCondition
+		condition      v1.PodCondition
+		wantConditions []v1.PodCondition
+	}{
+		{
+			description: "append",
+			conditions:  []v1.PodCondition{},
+			condition: v1.PodCondition{
+				Type:   cType,
+				Status: v1.ConditionTrue,
+			},
+			wantConditions: []v1.PodCondition{
+				{
+					Type:   cType,
+					Status: v1.ConditionTrue,
+				},
+			},
+		},
+		{
+			description: "replace",
+			conditions: []v1.PodCondition{
+				{
+					Type:   cType,
+					Status: v1.ConditionTrue,
+				},
+			},
+			condition: v1.PodCondition{
+				Type:   cType,
+				Status: v1.ConditionFalse,
+			},
+			wantConditions: []v1.PodCondition{
+				{
+					Type:   cType,
+					Status: v1.ConditionFalse,
+				},
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.description, func(t *testing.T) {
+			gotConditions := ReplaceOrAppendPodCondition(tc.conditions, &tc.condition)
+			if diff := cmp.Diff(tc.wantConditions, gotConditions); diff != "" {
+				t.Errorf("Unexpected conditions: %s", diff)
+			}
+		})
+	}
+}