Merge pull request #61504 from Random-Liu/fix-pod-scheduled-bug

pkg/kubelet/kubelet_pods.go

@@ -1394,13 +1394,10 @@ func (kl *Kubelet) generateAPIPodStatus(pod *v1.Pod, podStatus *kubecontainer.Po
 	kl.probeManager.UpdatePodStatus(pod.UID, s)
 	s.Conditions = append(s.Conditions, status.GeneratePodInitializedCondition(spec, s.InitContainerStatuses, s.Phase))
 	s.Conditions = append(s.Conditions, status.GeneratePodReadyCondition(spec, s.ContainerStatuses, s.Phase))
-	// s (the PodStatus we are creating) will not have a PodScheduled condition yet, because converStatusToAPIStatus()
-	// does not create one. If the existing PodStatus has a PodScheduled condition, then copy it into s and make sure
-	// it is set to true. If the existing PodStatus does not have a PodScheduled condition, then create one that is set to true.
-	if _, oldPodScheduled := podutil.GetPodCondition(&pod.Status, v1.PodScheduled); oldPodScheduled != nil {
-		s.Conditions = append(s.Conditions, *oldPodScheduled)
-	}
-	podutil.UpdatePodCondition(s, &v1.PodCondition{
+	// Status manager will take care of the LastTransitionTimestamp, either preserve
+	// the timestamp from apiserver, or set a new one. When kubelet sees the pod,
+	// `PodScheduled` condition must be true.
+	s.Conditions = append(s.Conditions, v1.PodCondition{
 		Type:   v1.PodScheduled,
 		Status: v1.ConditionTrue,
 	})

pkg/kubelet/status/status_manager.go

@@ -316,26 +316,13 @@ func (m *manager) updateStatusInternal(pod *v1.Pod, status v1.PodStatus, forceUp
 	}
 
 	// Set ReadyCondition.LastTransitionTime.
-	if _, readyCondition := podutil.GetPodCondition(&status, v1.PodReady); readyCondition != nil {
-		// Need to set LastTransitionTime.
-		lastTransitionTime := metav1.Now()
-		_, oldReadyCondition := podutil.GetPodCondition(&oldStatus, v1.PodReady)
-		if oldReadyCondition != nil && readyCondition.Status == oldReadyCondition.Status {
-			lastTransitionTime = oldReadyCondition.LastTransitionTime
-		}
-		readyCondition.LastTransitionTime = lastTransitionTime
-	}
+	updateLastTransitionTime(&status, &oldStatus, v1.PodReady)
 
 	// Set InitializedCondition.LastTransitionTime.
-	if _, initCondition := podutil.GetPodCondition(&status, v1.PodInitialized); initCondition != nil {
-		// Need to set LastTransitionTime.
-		lastTransitionTime := metav1.Now()
-		_, oldInitCondition := podutil.GetPodCondition(&oldStatus, v1.PodInitialized)
-		if oldInitCondition != nil && initCondition.Status == oldInitCondition.Status {
-			lastTransitionTime = oldInitCondition.LastTransitionTime
-		}
-		initCondition.LastTransitionTime = lastTransitionTime
-	}
+	updateLastTransitionTime(&status, &oldStatus, v1.PodInitialized)
+
+	// Set PodScheduledCondition.LastTransitionTime.
+	updateLastTransitionTime(&status, &oldStatus, v1.PodScheduled)
 
 	// ensure that the start time does not change across updates.
 	if oldStatus.StartTime != nil && !oldStatus.StartTime.IsZero() {
@@ -376,6 +363,21 @@ func (m *manager) updateStatusInternal(pod *v1.Pod, status v1.PodStatus, forceUp
 	}
 }
 
+// updateLastTransitionTime updates the LastTransitionTime of a pod condition.
+func updateLastTransitionTime(status, oldStatus *v1.PodStatus, conditionType v1.PodConditionType) {
+	_, condition := podutil.GetPodCondition(status, conditionType)
+	if condition == nil {
+		return
+	}
+	// Need to set LastTransitionTime.
+	lastTransitionTime := metav1.Now()
+	_, oldCondition := podutil.GetPodCondition(oldStatus, conditionType)
+	if oldCondition != nil && condition.Status == oldCondition.Status {
+		lastTransitionTime = oldCondition.LastTransitionTime
+	}
+	condition.LastTransitionTime = lastTransitionTime
+}
+
 // deletePodStatus simply removes the given pod from the status cache.
 func (m *manager) deletePodStatus(uid types.UID) {
 	m.podStatusesLock.Lock()

pkg/kubelet/status/status_manager_test.go

@@ -798,6 +798,67 @@ func TestDoNotDeleteMirrorPods(t *testing.T) {
 	verifyActions(t, m, []core.Action{getAction(), updateAction()})
 }
 
+func TestUpdateLastTransitionTime(t *testing.T) {
+	old := metav1.Now()
+	for desc, test := range map[string]struct {
+		condition    *v1.PodCondition
+		oldCondition *v1.PodCondition
+		expectUpdate bool
+	}{
+		"should do nothing if no corresponding condition": {
+			expectUpdate: false,
+		},
+		"should update last transition time if no old condition": {
+			condition: &v1.PodCondition{
+				Type:   "test-type",
+				Status: v1.ConditionTrue,
+			},
+			oldCondition: nil,
+			expectUpdate: true,
+		},
+		"should update last transition time if condition is changed": {
+			condition: &v1.PodCondition{
+				Type:   "test-type",
+				Status: v1.ConditionTrue,
+			},
+			oldCondition: &v1.PodCondition{
+				Type:               "test-type",
+				Status:             v1.ConditionFalse,
+				LastTransitionTime: old,
+			},
+			expectUpdate: true,
+		},
+		"should keep last transition time if condition is not changed": {
+			condition: &v1.PodCondition{
+				Type:   "test-type",
+				Status: v1.ConditionFalse,
+			},
+			oldCondition: &v1.PodCondition{
+				Type:               "test-type",
+				Status:             v1.ConditionFalse,
+				LastTransitionTime: old,
+			},
+			expectUpdate: false,
+		},
+	} {
+		t.Logf("TestCase %q", desc)
+		status := &v1.PodStatus{}
+		oldStatus := &v1.PodStatus{}
+		if test.condition != nil {
+			status.Conditions = []v1.PodCondition{*test.condition}
+		}
+		if test.oldCondition != nil {
+			oldStatus.Conditions = []v1.PodCondition{*test.oldCondition}
+		}
+		updateLastTransitionTime(status, oldStatus, "test-type")
+		if test.expectUpdate {
+			assert.True(t, status.Conditions[0].LastTransitionTime.After(old.Time))
+		} else if test.condition != nil {
+			assert.Equal(t, old, status.Conditions[0].LastTransitionTime)
+		}
+	}
+}
+
 func getAction() core.GetAction {
 	return core.GetActionImpl{ActionImpl: core.ActionImpl{Verb: "get", Resource: schema.GroupVersionResource{Resource: "pods"}}}
 }