Skip to content

Commit 9a9331a

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#124952 from AxeZhan/maxContainerRestarts
[Sidecar Containers] Pods comparison by maxContainerRestarts should account for sidecar containers
2 parents 09e5e62 + 7533123 commit 9a9331a

File tree

4 files changed

+201
-39
lines changed

4 files changed

+201
-39
lines changed

‎pkg/controller/controller_utils.go‎

Lines changed: 39 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -713,8 +713,8 @@ func (s ByLogging) Less(i, j int) bool {
713713
}
714714
}
715715
// 5. Pods with containers with higher restart counts < lower restart counts
716-
if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) {
717-
return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j])
716+
if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
717+
return *res
718718
}
719719
// 6. older pods < newer pods < empty timestamp pods
720720
if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -756,8 +756,8 @@ func (s ActivePods) Less(i, j int) bool {
756756
}
757757
}
758758
// 5. Pods with containers with higher restart counts < lower restart counts
759-
if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) {
760-
return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j])
759+
if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
760+
return *res
761761
}
762762
// 6. Empty creation time pods < newer pods < older pods
763763
if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -878,8 +878,8 @@ func (s ActivePodsWithRanks) Less(i, j int) bool {
878878
}
879879
}
880880
// 7. Pods with containers with higher restart counts < lower restart counts
881-
if maxContainerRestarts(s.Pods[i]) != maxContainerRestarts(s.Pods[j]) {
882-
return maxContainerRestarts(s.Pods[i]) > maxContainerRestarts(s.Pods[j])
881+
if res := compareMaxContainerRestarts(s.Pods[i], s.Pods[j]); res != nil {
882+
return *res
883883
}
884884
// 8. Empty creation time pods < newer pods < older pods
885885
if !s.Pods[i].CreationTimestamp.Equal(&s.Pods[j].CreationTimestamp) {
@@ -936,12 +936,41 @@ func podReadyTime(pod *v1.Pod) *metav1.Time {
936936
return &metav1.Time{}
937937
}
938938

939-
func maxContainerRestarts(pod *v1.Pod) int {
940-
maxRestarts := 0
939+
func maxContainerRestarts(pod *v1.Pod) (regularRestarts, sidecarRestarts int) {
941940
for _, c := range pod.Status.ContainerStatuses {
942-
maxRestarts = max(maxRestarts, int(c.RestartCount))
941+
regularRestarts = max(regularRestarts, int(c.RestartCount))
943942
}
944-
return maxRestarts
943+
names := sets.New[string]()
944+
for _, c := range pod.Spec.InitContainers {
945+
if c.RestartPolicy != nil && *c.RestartPolicy == v1.ContainerRestartPolicyAlways {
946+
names.Insert(c.Name)
947+
}
948+
}
949+
for _, c := range pod.Status.InitContainerStatuses {
950+
if names.Has(c.Name) {
951+
sidecarRestarts = max(sidecarRestarts, int(c.RestartCount))
952+
}
953+
}
954+
return
955+
}
956+
957+
// We use *bool here to determine equality:
958+
// true: pi has a higher container restart count.
959+
// false: pj has a higher container restart count.
960+
// nil: Both have the same container restart count.
961+
func compareMaxContainerRestarts(pi *v1.Pod, pj *v1.Pod) *bool {
962+
regularRestartsI, sidecarRestartsI := maxContainerRestarts(pi)
963+
regularRestartsJ, sidecarRestartsJ := maxContainerRestarts(pj)
964+
if regularRestartsI != regularRestartsJ {
965+
res := regularRestartsI > regularRestartsJ
966+
return &res
967+
}
968+
// If pods have the same restart count, an attempt is made to compare the restart counts of sidecar containers.
969+
if sidecarRestartsI != sidecarRestartsJ {
970+
res := sidecarRestartsI > sidecarRestartsJ
971+
return &res
972+
}
973+
return nil
945974
}
946975

947976
// FilterActivePods returns pods that have not terminated.

‎pkg/controller/controller_utils_test.go‎

Lines changed: 48 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,7 @@ func TestSortingActivePods(t *testing.T) {
491491
now := metav1.Now()
492492
then := metav1.Time{Time: now.AddDate(0, -1, 0)}
493493

494+
restartAlways := v1.ContainerRestartPolicyAlways
494495
tests := []struct {
495496
name string
496497
pods []v1.Pod
@@ -546,6 +547,22 @@ func TestSortingActivePods(t *testing.T) {
546547
ContainerStatuses: []v1.ContainerStatus{{RestartCount: 3}, {RestartCount: 0}},
547548
},
548549
},
550+
{
551+
ObjectMeta: metav1.ObjectMeta{Name: "lowerSidecarContainerRestartCount", CreationTimestamp: now},
552+
Spec: v1.PodSpec{
553+
NodeName: "foo",
554+
InitContainers: []v1.Container{{
555+
Name: "sidecar",
556+
RestartPolicy: &restartAlways,
557+
}},
558+
},
559+
Status: v1.PodStatus{
560+
Phase: v1.PodRunning,
561+
Conditions: []v1.PodCondition{{Type: v1.PodReady, Status: v1.ConditionTrue, LastTransitionTime: then}},
562+
ContainerStatuses: []v1.ContainerStatus{{RestartCount: 2}, {RestartCount: 1}},
563+
InitContainerStatuses: []v1.ContainerStatus{{Name: "sidecar", RestartCount: 2}},
564+
},
565+
},
549566
{
550567
ObjectMeta: metav1.ObjectMeta{Name: "lowerContainerRestartCount", CreationTimestamp: now},
551568
Spec: v1.PodSpec{NodeName: "foo"},
@@ -573,6 +590,7 @@ func TestSortingActivePods(t *testing.T) {
573590
"runningNoLastTransitionTime",
574591
"runningWithLastTransitionTime",
575592
"runningLongerTime",
593+
"lowerSidecarContainerRestartCount",
576594
"lowerContainerRestartCount",
577595
"oldest",
578596
},
@@ -611,44 +629,52 @@ func TestSortingActivePodsWithRanks(t *testing.T) {
611629
then5Hours := metav1.Time{Time: now.Add(-5 * time.Hour)}
612630
then8Hours := metav1.Time{Time: now.Add(-8 * time.Hour)}
613631
zeroTime := metav1.Time{}
614-
pod := func(podName, nodeName string, phase v1.PodPhase, ready bool, restarts int32, readySince metav1.Time, created metav1.Time, annotations map[string]string) *v1.Pod {
632+
restartAlways := v1.ContainerRestartPolicyAlways
633+
pod := func(podName, nodeName string, phase v1.PodPhase, ready bool, restarts int32, sideRestarts int32, readySince metav1.Time, created metav1.Time, annotations map[string]string) *v1.Pod {
615634
var conditions []v1.PodCondition
616635
var containerStatuses []v1.ContainerStatus
636+
var initContainerStatuses []v1.ContainerStatus
617637
if ready {
618638
conditions = []v1.PodCondition{{Type: v1.PodReady, Status: v1.ConditionTrue, LastTransitionTime: readySince}}
619639
containerStatuses = []v1.ContainerStatus{{RestartCount: restarts}}
640+
initContainerStatuses = []v1.ContainerStatus{{Name: "sidecar", RestartCount: sideRestarts}}
620641
}
621642
return &v1.Pod{
622643
ObjectMeta: metav1.ObjectMeta{
623644
CreationTimestamp: created,
624645
Name: podName,
625646
Annotations: annotations,
626647
},
627-
Spec: v1.PodSpec{NodeName: nodeName},
648+
Spec: v1.PodSpec{
649+
NodeName: nodeName,
650+
InitContainers: []v1.Container{{Name: "sidecar", RestartPolicy: &restartAlways}},
651+
},
628652
Status: v1.PodStatus{
629-
Conditions: conditions,
630-
ContainerStatuses: containerStatuses,
631-
Phase: phase,
653+
Conditions: conditions,
654+
ContainerStatuses: containerStatuses,
655+
InitContainerStatuses: initContainerStatuses,
656+
Phase: phase,
632657
},
633658
}
634659
}
635660
var (
636-
unscheduledPod = pod("unscheduled", "", v1.PodPending, false, 0, zeroTime, zeroTime, nil)
637-
scheduledPendingPod = pod("pending", "node", v1.PodPending, false, 0, zeroTime, zeroTime, nil)
638-
unknownPhasePod = pod("unknown-phase", "node", v1.PodUnknown, false, 0, zeroTime, zeroTime, nil)
639-
runningNotReadyPod = pod("not-ready", "node", v1.PodRunning, false, 0, zeroTime, zeroTime, nil)
640-
runningReadyNoLastTransitionTimePod = pod("ready-no-last-transition-time", "node", v1.PodRunning, true, 0, zeroTime, zeroTime, nil)
641-
runningReadyNow = pod("ready-now", "node", v1.PodRunning, true, 0, now, now, nil)
642-
runningReadyThen = pod("ready-then", "node", v1.PodRunning, true, 0, then1Month, then1Month, nil)
643-
runningReadyNowHighRestarts = pod("ready-high-restarts", "node", v1.PodRunning, true, 9001, now, now, nil)
644-
runningReadyNowCreatedThen = pod("ready-now-created-then", "node", v1.PodRunning, true, 0, now, then1Month, nil)
645-
lowPodDeletionCost = pod("low-deletion-cost", "node", v1.PodRunning, true, 0, now, then1Month, map[string]string{core.PodDeletionCost: "10"})
646-
highPodDeletionCost = pod("high-deletion-cost", "node", v1.PodRunning, true, 0, now, then1Month, map[string]string{core.PodDeletionCost: "100"})
647-
unscheduled5Hours = pod("unscheduled-5-hours", "", v1.PodPending, false, 0, then5Hours, then5Hours, nil)
648-
unscheduled8Hours = pod("unscheduled-10-hours", "", v1.PodPending, false, 0, then8Hours, then8Hours, nil)
649-
ready2Hours = pod("ready-2-hours", "", v1.PodRunning, true, 0, then2Hours, then1Month, nil)
650-
ready5Hours = pod("ready-5-hours", "", v1.PodRunning, true, 0, then5Hours, then1Month, nil)
651-
ready10Hours = pod("ready-10-hours", "", v1.PodRunning, true, 0, then8Hours, then1Month, nil)
661+
unscheduledPod = pod("unscheduled", "", v1.PodPending, false, 0, 0, zeroTime, zeroTime, nil)
662+
scheduledPendingPod = pod("pending", "node", v1.PodPending, false, 0, 0, zeroTime, zeroTime, nil)
663+
unknownPhasePod = pod("unknown-phase", "node", v1.PodUnknown, false, 0, 0, zeroTime, zeroTime, nil)
664+
runningNotReadyPod = pod("not-ready", "node", v1.PodRunning, false, 0, 0, zeroTime, zeroTime, nil)
665+
runningReadyNoLastTransitionTimePod = pod("ready-no-last-transition-time", "node", v1.PodRunning, true, 0, 0, zeroTime, zeroTime, nil)
666+
runningReadyNow = pod("ready-now", "node", v1.PodRunning, true, 0, 0, now, now, nil)
667+
runningReadyThen = pod("ready-then", "node", v1.PodRunning, true, 0, 0, then1Month, then1Month, nil)
668+
runningReadyNowHighRestarts = pod("ready-high-restarts", "node", v1.PodRunning, true, 9001, 0, now, now, nil)
669+
runningReadyNowHighSideRestarts = pod("ready-high-restarts", "node", v1.PodRunning, true, 9001, 9001, now, now, nil)
670+
runningReadyNowCreatedThen = pod("ready-now-created-then", "node", v1.PodRunning, true, 0, 0, now, then1Month, nil)
671+
lowPodDeletionCost = pod("low-deletion-cost", "node", v1.PodRunning, true, 0, 0, now, then1Month, map[string]string{core.PodDeletionCost: "10"})
672+
highPodDeletionCost = pod("high-deletion-cost", "node", v1.PodRunning, true, 0, 0, now, then1Month, map[string]string{core.PodDeletionCost: "100"})
673+
unscheduled5Hours = pod("unscheduled-5-hours", "", v1.PodPending, false, 0, 0, then5Hours, then5Hours, nil)
674+
unscheduled8Hours = pod("unscheduled-10-hours", "", v1.PodPending, false, 0, 0, then8Hours, then8Hours, nil)
675+
ready2Hours = pod("ready-2-hours", "", v1.PodRunning, true, 0, 0, then2Hours, then1Month, nil)
676+
ready5Hours = pod("ready-5-hours", "", v1.PodRunning, true, 0, 0, then5Hours, then1Month, nil)
677+
ready10Hours = pod("ready-10-hours", "", v1.PodRunning, true, 0, 0, then8Hours, then1Month, nil)
652678
)
653679
equalityTests := []struct {
654680
p1 *v1.Pod
@@ -703,6 +729,7 @@ func TestSortingActivePodsWithRanks(t *testing.T) {
703729
{lesser: podWithRank{runningReadyNow, 1}, greater: podWithRank{runningReadyThen, 1}},
704730
{lesser: podWithRank{runningReadyNow, 2}, greater: podWithRank{runningReadyThen, 1}},
705731
{lesser: podWithRank{runningReadyNowHighRestarts, 1}, greater: podWithRank{runningReadyNow, 1}},
732+
{lesser: podWithRank{runningReadyNowHighSideRestarts, 1}, greater: podWithRank{runningReadyNowHighRestarts, 1}},
706733
{lesser: podWithRank{runningReadyNow, 2}, greater: podWithRank{runningReadyNowHighRestarts, 1}},
707734
{lesser: podWithRank{runningReadyNow, 1}, greater: podWithRank{runningReadyNowCreatedThen, 1}},
708735
{lesser: podWithRank{runningReadyNowCreatedThen, 2}, greater: podWithRank{runningReadyNow, 1}},

‎staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go‎

Lines changed: 38 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import (
2121

2222
corev1 "k8s.io/api/core/v1"
2323
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
24+
"k8s.io/apimachinery/pkg/util/sets"
2425
)
2526

2627
// IsPodAvailable returns true if a pod is available; false otherwise.
@@ -113,8 +114,8 @@ func (s ByLogging) Less(i, j int) bool {
113114
return afterOrZero(podReadyTime(s[j]), podReadyTime(s[i]))
114115
}
115116
// 5. Pods with containers with higher restart counts < lower restart counts
116-
if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) {
117-
return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j])
117+
if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
118+
return *res
118119
}
119120
// 6. older pods < newer pods < empty timestamp pods
120121
if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -161,8 +162,8 @@ func (s ActivePods) Less(i, j int) bool {
161162
return afterOrZero(podReadyTime(s[i]), podReadyTime(s[j]))
162163
}
163164
// 7. Pods with containers with higher restart counts < lower restart counts
164-
if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) {
165-
return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j])
165+
if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
166+
return *res
166167
}
167168
// 8. Empty creation time pods < newer pods < older pods
168169
if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -190,12 +191,41 @@ func podReadyTime(pod *corev1.Pod) *metav1.Time {
190191
return &metav1.Time{}
191192
}
192193

193-
func maxContainerRestarts(pod *corev1.Pod) int {
194-
maxRestarts := 0
194+
func maxContainerRestarts(pod *corev1.Pod) (regularRestarts, sidecarRestarts int) {
195195
for _, c := range pod.Status.ContainerStatuses {
196-
maxRestarts = max(maxRestarts, int(c.RestartCount))
196+
regularRestarts = max(regularRestarts, int(c.RestartCount))
197197
}
198-
return maxRestarts
198+
names := sets.New[string]()
199+
for _, c := range pod.Spec.InitContainers {
200+
if c.RestartPolicy != nil && *c.RestartPolicy == corev1.ContainerRestartPolicyAlways {
201+
names.Insert(c.Name)
202+
}
203+
}
204+
for _, c := range pod.Status.InitContainerStatuses {
205+
if names.Has(c.Name) {
206+
sidecarRestarts = max(sidecarRestarts, int(c.RestartCount))
207+
}
208+
}
209+
return
210+
}
211+
212+
// We use *bool here to determine equality:
213+
// true: pi has a higher container restart count.
214+
// false: pj has a higher container restart count.
215+
// nil: Both have the same container restart count.
216+
func compareMaxContainerRestarts(pi *corev1.Pod, pj *corev1.Pod) *bool {
217+
regularRestartsI, sidecarRestartsI := maxContainerRestarts(pi)
218+
regularRestartsJ, sidecarRestartsJ := maxContainerRestarts(pj)
219+
if regularRestartsI != regularRestartsJ {
220+
res := regularRestartsI > regularRestartsJ
221+
return &res
222+
}
223+
// If pods have the same restart count, an attempt is made to compare the restart counts of sidecar containers.
224+
if sidecarRestartsI != sidecarRestartsJ {
225+
res := sidecarRestartsI > sidecarRestartsJ
226+
return &res
227+
}
228+
return nil
199229
}
200230

201231
// ContainerType and VisitContainers are taken from

‎staging/src/k8s.io/kubectl/pkg/util/podutils/podutils_test.go‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ func TestActivePods(t *testing.T) {
2929
time1 := metav1.Now()
3030
time2 := metav1.NewTime(time1.Add(1 * time.Second))
3131
time3 := metav1.NewTime(time1.Add(2 * time.Second))
32+
restartAlways := corev1.ContainerRestartPolicyAlways
3233

3334
tests := []struct {
3435
name string
@@ -364,6 +365,81 @@ func TestActivePods(t *testing.T) {
364365
},
365366
},
366367
},
368+
{
369+
name: "higher sidecar restart count should sort before lower restart count",
370+
pod1: &corev1.Pod{
371+
ObjectMeta: metav1.ObjectMeta{
372+
Name: "podWithMoreRestarts",
373+
Namespace: "default",
374+
},
375+
Spec: corev1.PodSpec{
376+
NodeName: "node1",
377+
InitContainers: []corev1.Container{
378+
{
379+
Name: "sidecar",
380+
RestartPolicy: &restartAlways,
381+
},
382+
},
383+
},
384+
Status: corev1.PodStatus{
385+
Phase: corev1.PodRunning,
386+
Conditions: []corev1.PodCondition{
387+
{
388+
Type: corev1.PodReady,
389+
Status: corev1.ConditionTrue,
390+
LastTransitionTime: time1,
391+
},
392+
},
393+
ContainerStatuses: []corev1.ContainerStatus{
394+
{
395+
RestartCount: 3,
396+
},
397+
},
398+
InitContainerStatuses: []corev1.ContainerStatus{
399+
{
400+
Name: "sidecar",
401+
RestartCount: 3,
402+
},
403+
},
404+
},
405+
},
406+
pod2: &corev1.Pod{
407+
ObjectMeta: metav1.ObjectMeta{
408+
Name: "podWithLessRestarts",
409+
Namespace: "default",
410+
},
411+
Spec: corev1.PodSpec{
412+
NodeName: "node1",
413+
InitContainers: []corev1.Container{
414+
{
415+
Name: "sidecar",
416+
RestartPolicy: &restartAlways,
417+
},
418+
},
419+
},
420+
Status: corev1.PodStatus{
421+
Phase: corev1.PodRunning,
422+
Conditions: []corev1.PodCondition{
423+
{
424+
Type: corev1.PodReady,
425+
Status: corev1.ConditionTrue,
426+
LastTransitionTime: time1,
427+
},
428+
},
429+
ContainerStatuses: []corev1.ContainerStatus{
430+
{
431+
RestartCount: 3,
432+
},
433+
},
434+
InitContainerStatuses: []corev1.ContainerStatus{
435+
{
436+
Name: "sidecar",
437+
RestartCount: 2,
438+
},
439+
},
440+
},
441+
},
442+
},
367443
}
368444
for _, tt := range tests {
369445
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)