From 70109f0921ee0eb591157d027009fa0913e3f746 Mon Sep 17 00:00:00 2001 From: Brian Pursley Date: Tue, 18 Jul 2023 08:31:37 -0400 Subject: [PATCH] Prefer pods without a deletion timestamp over pods with a deletion timestamp when getting an ordered list of active pods. This allows commands like kubectl port-forward, when used with deployments, to avoid selecting a pod that is terminating (due to a rollout, for example). Kubernetes-commit: d281daee335dc3b2aa67fb3128f00d7d2f5f7d91 --- pkg/util/podutils/podutils.go | 18 +- pkg/util/podutils/podutils_test.go | 385 +++++++++++++++++++++++++++++ 2 files changed, 400 insertions(+), 3 deletions(-) create mode 100644 pkg/util/podutils/podutils_test.go diff --git a/pkg/util/podutils/podutils.go b/pkg/util/podutils/podutils.go index ddd2b9dbe..e9cfdeba3 100644 --- a/pkg/util/podutils/podutils.go +++ b/pkg/util/podutils/podutils.go @@ -47,6 +47,10 @@ func IsPodReady(pod *corev1.Pod) bool { return isPodReadyConditionTrue(pod.Status) } +func isPodDeleting(pod *corev1.Pod) bool { + return pod.DeletionTimestamp != nil +} + // IsPodReadyConditionTrue returns true if a pod is ready; false otherwise. func isPodReadyConditionTrue(status corev1.PodStatus) bool { condition := getPodReadyCondition(status) @@ -142,18 +146,26 @@ func (s ActivePods) Less(i, j int) bool { if IsPodReady(s[i]) != IsPodReady(s[j]) { return !IsPodReady(s[i]) } + // 4. Deleting < Not deleting + if isPodDeleting(s[i]) != isPodDeleting(s[j]) { + return isPodDeleting(s[i]) + } + // 5. Older deletion timestamp < newer deletion timestamp + if isPodDeleting(s[i]) && isPodDeleting(s[j]) && !s[i].ObjectMeta.DeletionTimestamp.Equal(s[j].ObjectMeta.DeletionTimestamp) { + return s[i].ObjectMeta.DeletionTimestamp.Before(s[j].ObjectMeta.DeletionTimestamp) + } // TODO: take availability into account when we push minReadySeconds information from deployment into pods, // see https://github.com/kubernetes/kubernetes/issues/22065 - // 4. Been ready for empty time < less time < more time + // 6. Been ready for empty time < less time < more time // If both pods are ready, the latest ready one is smaller if IsPodReady(s[i]) && IsPodReady(s[j]) && !podReadyTime(s[i]).Equal(podReadyTime(s[j])) { return afterOrZero(podReadyTime(s[i]), podReadyTime(s[j])) } - // 5. Pods with containers with higher restart counts < lower restart counts + // 7. Pods with containers with higher restart counts < lower restart counts if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) { return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j]) } - // 6. Empty creation time pods < newer pods < older pods + // 8. Empty creation time pods < newer pods < older pods if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) { return afterOrZero(&s[i].CreationTimestamp, &s[j].CreationTimestamp) } diff --git a/pkg/util/podutils/podutils_test.go b/pkg/util/podutils/podutils_test.go new file mode 100644 index 000000000..b514a4034 --- /dev/null +++ b/pkg/util/podutils/podutils_test.go @@ -0,0 +1,385 @@ +/* +Copyright 2023 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package podutils + +import ( + "sort" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func TestActivePods(t *testing.T) { + time1 := metav1.Now() + time2 := metav1.NewTime(time1.Add(1 * time.Second)) + time3 := metav1.NewTime(time1.Add(2 * time.Second)) + + tests := []struct { + name string + pod1 *corev1.Pod + pod2 *corev1.Pod + }{ + { + name: "unassigned pod should sort before assigned pod", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unassignedPod", + Namespace: "default", + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "assignedPod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + }, + }, + { + name: "pending pod should sort before unknown pod", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pendingPod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodPending, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unknownPod", + Namespace: "default", + CreationTimestamp: time1, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodUnknown, + }, + }, + }, + { + name: "unknown pod should sort before running pod", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unknownPod", + Namespace: "default", + CreationTimestamp: time1, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodUnknown, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "runningPod", + Namespace: "default", + CreationTimestamp: time1, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + }, + }, + }, + { + name: "unready pod should sort before ready pod", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "unreadyPod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "readyPod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + }, + }, + }, + { + name: "pod with deletion timestamp should sort before pod without deletion timestamp", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "readyPodDeleting", + Namespace: "default", + DeletionTimestamp: &time2, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "readyPod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + }, + }, + }, + { + name: "older deletion timestamp should sort before newer deletion timestamp", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "readyPodDeletingOlder", + Namespace: "default", + DeletionTimestamp: &time2, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "readyPodDeletingNewer", + Namespace: "default", + DeletionTimestamp: &time3, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + }, + }, + }, + { + name: "newer ready timestamp should sort before older ready timestamp", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "newerReadyPod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time3, + }, + }, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "olderReadyPod", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time2, + }, + }, + }, + }, + }, + { + name: "higher restart count should sort before lower restart count", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podWithMoreRestarts", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + ContainerStatuses: []corev1.ContainerStatus{ + { + RestartCount: 3, + }, + }, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "podWithLessRestarts", + Namespace: "default", + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + ContainerStatuses: []corev1.ContainerStatus{ + { + RestartCount: 2, + }, + }, + }, + }, + }, + { + name: "newer creation timestamp should sort before older creation timestamp", + pod1: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "newerCreationPod", + Namespace: "default", + CreationTimestamp: time3, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + }, + }, + pod2: &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "olderCreationPod", + Namespace: "default", + CreationTimestamp: time2, + }, + Spec: corev1.PodSpec{ + NodeName: "node1", + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + Conditions: []corev1.PodCondition{ + { + Type: corev1.PodReady, + Status: corev1.ConditionTrue, + LastTransitionTime: time1, + }, + }, + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Test that the pods are sorted in the correct order when pod1 is first and pod2 is second. + pods := ActivePods{tt.pod1, tt.pod2} + sort.Sort(pods) + if pods[0] != tt.pod1 || pods[1] != tt.pod2 { + t.Errorf("Incorrect ActivePods sorting, expected pod1 to be first") + } + + // Test that the pods are sorted in the correct order when pod2 is first and pod1 is second. + pods = ActivePods{tt.pod2, tt.pod1} + sort.Sort(pods) + if pods[0] != tt.pod1 || pods[1] != tt.pod2 { + t.Errorf("Incorrect ActivePods sorting, expected pod1 to be first") + } + }) + } +}