Merge pull request #110954 from kerthcet/fix/deadline-should-be-nil

Fix pod's deadline to nil when cache ttl is set to zero
kubernetes · Jul 6, 2022 · 1ea6eb2 · 1ea6eb2
2 parents bd2776e + 72e8fc1
commit 1ea6eb2
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 37 deletions.
diff --git a/pkg/scheduler/internal/cache/cache.go b/pkg/scheduler/internal/cache/cache.go
@@ -77,6 +77,7 @@ type cacheImpl struct {
 type podState struct {
 	pod *v1.Pod
 	// Used by assumedPod to determinate expiration.
+	// If deadline is nil, assumedPod will never expire.
 	deadline *time.Time
 	// Used to block cache from expiring assumedPod if binding still runs
 	bindingFinished bool
@@ -401,9 +402,13 @@ func (cache *cacheImpl) finishBinding(pod *v1.Pod, now time.Time) error {
 	klog.V(5).InfoS("Finished binding for pod, can be expired", "pod", klog.KObj(pod))
 	currState, ok := cache.podStates[key]
 	if ok && cache.assumedPods.Has(key) {
-		dl := now.Add(cache.ttl)
+		if cache.ttl == time.Duration(0) {
+			currState.deadline = nil
+		} else {
+			dl := now.Add(cache.ttl)
+			currState.deadline = &dl
+		}
 		currState.bindingFinished = true
-		currState.deadline = &dl
 	}
 	return nil
 }

diff --git a/pkg/scheduler/internal/cache/cache_test.go b/pkg/scheduler/internal/cache/cache_test.go
@@ -253,46 +253,75 @@ func TestExpirePod(t *testing.T) {
 		makeBasePod(t, nodeName, "test-3", "200m", "1Ki", "", []v1.ContainerPort{{HostIP: "127.0.0.1", HostPort: 8080, Protocol: "TCP"}}),
 	}
 	now := time.Now()
-	ttl := 10 * time.Second
+	defaultTTL := 10 * time.Second
 	tests := []struct {
+		name        string
 		pods        []*testExpirePodStruct
 		cleanupTime time.Time
-
-		wNodeInfo *framework.NodeInfo
-	}{{ // assumed pod would expires
-		pods: []*testExpirePodStruct{
-			{pod: testPods[0], finishBind: true, assumedTime: now},
-		},
-		cleanupTime: now.Add(2 * ttl),
-		wNodeInfo:   nil,
-	}, { // first one would expire, second and third would not.
-		pods: []*testExpirePodStruct{
-			{pod: testPods[0], finishBind: true, assumedTime: now},
-			{pod: testPods[1], finishBind: true, assumedTime: now.Add(3 * ttl / 2)},
-			{pod: testPods[2]},
+		ttl         time.Duration
+		wNodeInfo   *framework.NodeInfo
+	}{
+		{
+			name: "assumed pod would expire",
+			pods: []*testExpirePodStruct{
+				{pod: testPods[0], finishBind: true, assumedTime: now},
+			},
+			cleanupTime: now.Add(2 * defaultTTL),
+			wNodeInfo:   nil,
+			ttl:         defaultTTL,
 		},
-		cleanupTime: now.Add(2 * ttl),
-		wNodeInfo: newNodeInfo(
-			&framework.Resource{
-				MilliCPU: 400,
-				Memory:   2048,
+		{
+			name: "first one would expire, second and third would not",
+			pods: []*testExpirePodStruct{
+				{pod: testPods[0], finishBind: true, assumedTime: now},
+				{pod: testPods[1], finishBind: true, assumedTime: now.Add(3 * defaultTTL / 2)},
+				{pod: testPods[2]},
 			},
-			&framework.Resource{
-				MilliCPU: 400,
-				Memory:   2048,
+			cleanupTime: now.Add(2 * defaultTTL),
+			wNodeInfo: newNodeInfo(
+				&framework.Resource{
+					MilliCPU: 400,
+					Memory:   2048,
+				},
+				&framework.Resource{
+					MilliCPU: 400,
+					Memory:   2048,
+				},
+				// Order gets altered when removing pods.
+				[]*v1.Pod{testPods[2], testPods[1]},
+				newHostPortInfoBuilder().add("TCP", "127.0.0.1", 8080).build(),
+				make(map[string]*framework.ImageStateSummary),
+			),
+			ttl: defaultTTL,
+		},
+		{
+			name: "assumed pod would never expire",
+			pods: []*testExpirePodStruct{
+				{pod: testPods[0], finishBind: true, assumedTime: now},
 			},
-			// Order gets altered when removing pods.
-			[]*v1.Pod{testPods[2], testPods[1]},
-			newHostPortInfoBuilder().add("TCP", "127.0.0.1", 8080).build(),
-			make(map[string]*framework.ImageStateSummary),
-		),
-	}}
+			cleanupTime: now.Add(3 * defaultTTL),
+			wNodeInfo: newNodeInfo(
+				&framework.Resource{
+					MilliCPU: 100,
+					Memory:   500,
+				},
+				&framework.Resource{
+					MilliCPU: 100,
+					Memory:   500,
+				},
+				[]*v1.Pod{testPods[0]},
+				newHostPortInfoBuilder().add("TCP", "127.0.0.1", 80).build(),
+				make(map[string]*framework.ImageStateSummary),
+			),
+			ttl: time.Duration(0),
+		},
+	}
 
-	for i, tt := range tests {
-		t.Run(fmt.Sprintf("case_%d", i), func(t *testing.T) {
-			cache := newCache(ttl, time.Second, nil)
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			cache := newCache(tc.ttl, time.Second, nil)
 
-			for _, pod := range tt.pods {
+			for _, pod := range tc.pods {
 				if err := cache.AssumePod(pod.pod); err != nil {
 					t.Fatal(err)
 				}
@@ -305,9 +334,9 @@ func TestExpirePod(t *testing.T) {
 			}
 			// pods that got bound and have assumedTime + ttl < cleanupTime will get
 			// expired and removed
-			cache.cleanupAssumedPods(tt.cleanupTime)
+			cache.cleanupAssumedPods(tc.cleanupTime)
 			n := cache.nodes[nodeName]
-			if err := deepEqualWithoutGeneration(n, tt.wNodeInfo); err != nil {
+			if err := deepEqualWithoutGeneration(n, tc.wNodeInfo); err != nil {
 				t.Error(err)
 			}
 		})

diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go
@@ -53,7 +53,7 @@ import (
 const (
 	// Duration the scheduler will wait before expiring an assumed pod.
 	// See issue #106361 for more details about this parameter and its value.
-	durationToExpireAssumedPod = 0 * time.Minute
+	durationToExpireAssumedPod time.Duration = 0
 )
 
 // ErrNoNodesAvailable is used to describe the error that no nodes available to schedule pods.