Repository: kubernetes/kubernetes (language: go)

Issue:
[Sidecar Containers] Pods comparison by maxContainerRestarts should account for sidecar containers
### What happened?

Today, there are a few uses of the function `maxContainerRestarts` - mostly to compare pods to decide which one is
better to delete or which logs to get. This is not a huge issue, mostly a quality of life improvement.

The code only look at Container Statuses, but likely need to look at init container statuses as well.
Especially in case of sidecar containers that may behave exactly as regular containers.

There are 2 implementations and 5 comparison interfaces.

Implementatons in:

- pkg/controller/controller_utils.go
- staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go

```
func maxContainerRestarts(pod *v1.Pod) int {
	maxRestarts := 0
	for _, c := range pod.Status.ContainerStatuses {
		maxRestarts = max(maxRestarts, int(c.RestartCount))
	}
	return maxRestarts
}
```

We may need to be careful including all init container statuses. If a Pod was failing to start for a while
because of Init container failures and now it is running OK, it is likely not important. However, including
the restartable containers (sidecars) restart count is important.

I think the desireable behavior will be to check regular containers max restart count first. And compare this.
Then compare max restart count for restarteable init containers.

/kind bug

### What did you expect to happen?

Comparison of max container restarts account for init containers as well as regular containers.

### How can we reproduce it (as minimally and precisely as possible)?

Two pods - one with the sidecar container in constant restart loop and one is running sucessfully. Random one will be picked to get logs from.

### Anything else we need to know?

/sig apps
/priority backlog

KEP: https://github.com/kubernetes/enhancements/issues/753


Discussion / hints:

Fix patch:
diff --git a/pkg/controller/controller_utils.go b/pkg/controller/controller_utils.go
index 143a3d361fe5c..ffd99ed12360b 100644
--- a/pkg/controller/controller_utils.go
+++ b/pkg/controller/controller_utils.go
@@ -713,8 +713,8 @@ func (s ByLogging) Less(i, j int) bool {
 		}
 	}
 	// 5. Pods with containers with higher restart counts < lower restart counts
-	if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) {
-		return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j])
+	if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
+		return *res
 	}
 	// 6. older pods < newer pods < empty timestamp pods
 	if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -756,8 +756,8 @@ func (s ActivePods) Less(i, j int) bool {
 		}
 	}
 	// 5. Pods with containers with higher restart counts < lower restart counts
-	if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) {
-		return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j])
+	if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
+		return *res
 	}
 	// 6. Empty creation time pods < newer pods < older pods
 	if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -878,8 +878,8 @@ func (s ActivePodsWithRanks) Less(i, j int) bool {
 		}
 	}
 	// 7. Pods with containers with higher restart counts < lower restart counts
-	if maxContainerRestarts(s.Pods[i]) != maxContainerRestarts(s.Pods[j]) {
-		return maxContainerRestarts(s.Pods[i]) > maxContainerRestarts(s.Pods[j])
+	if res := compareMaxContainerRestarts(s.Pods[i], s.Pods[j]); res != nil {
+		return *res
 	}
 	// 8. Empty creation time pods < newer pods < older pods
 	if !s.Pods[i].CreationTimestamp.Equal(&s.Pods[j].CreationTimestamp) {
@@ -936,12 +936,41 @@ func podReadyTime(pod *v1.Pod) *metav1.Time {
 	return &metav1.Time{}
 }
 
-func maxContainerRestarts(pod *v1.Pod) int {
-	maxRestarts := 0
+func maxContainerRestarts(pod *v1.Pod) (regularRestarts, sidecarRestarts int) {
 	for _, c := range pod.Status.ContainerStatuses {
-		maxRestarts = max(maxRestarts, int(c.RestartCount))
+		regularRestarts = max(regularRestarts, int(c.RestartCount))
 	}
-	return maxRestarts
+	names := sets.New[string]()
+	for _, c := range pod.Spec.InitContainers {
+		if c.RestartPolicy != nil && *c.RestartPolicy == v1.ContainerRestartPolicyAlways {
+			names.Insert(c.Name)
+		}
+	}
+	for _, c := range pod.Status.InitContainerStatuses {
+		if names.Has(c.Name) {
+			sidecarRestarts = max(sidecarRestarts, int(c.RestartCount))
+		}
+	}
+	return
+}
+
+// We use *bool here to determine equality:
+// true: pi has a higher container restart count.
+// false: pj has a higher container restart count.
+// nil: Both have the same container restart count.
+func compareMaxContainerRestarts(pi *v1.Pod, pj *v1.Pod) *bool {
+	regularRestartsI, sidecarRestartsI := maxContainerRestarts(pi)
+	regularRestartsJ, sidecarRestartsJ := maxContainerRestarts(pj)
+	if regularRestartsI != regularRestartsJ {
+		res := regularRestartsI > regularRestartsJ
+		return &res
+	}
+	// If pods have the same restart count, an attempt is made to compare the restart counts of sidecar containers.
+	if sidecarRestartsI != sidecarRestartsJ {
+		res := sidecarRestartsI > sidecarRestartsJ
+		return &res
+	}
+	return nil
 }
 
 // FilterActivePods returns pods that have not terminated.
diff --git a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go
index 642a6d47a7a98..dfefdef4567c2 100644
--- a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go
+++ b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils.go
@@ -21,6 +21,7 @@ import (
 
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 )
 
 // IsPodAvailable returns true if a pod is available; false otherwise.
@@ -113,8 +114,8 @@ func (s ByLogging) Less(i, j int) bool {
 		return afterOrZero(podReadyTime(s[j]), podReadyTime(s[i]))
 	}
 	// 5. Pods with containers with higher restart counts < lower restart counts
-	if maxContainerRestarts(s[i]) != maxContainerRestarts(s[j]) {
-		return maxContainerRestarts(s[i]) > maxContainerRestarts(s[j])
+	if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
+		return *res
 	}
 	// 6. older pods < newer pods < empty timestamp pods
 	if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -161,8 +162,8 @@ func (s ActivePods) Less(i, j int) bool {
 		return afterOrZero(podReadyTime(s[i]), podReadyTime(s[j]))
 	}
 	// 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])
+	if res := compareMaxContainerRestarts(s[i], s[j]); res != nil {
+		return *res
 	}
 	// 8. Empty creation time pods < newer pods < older pods
 	if !s[i].CreationTimestamp.Equal(&s[j].CreationTimestamp) {
@@ -190,12 +191,41 @@ func podReadyTime(pod *corev1.Pod) *metav1.Time {
 	return &metav1.Time{}
 }
 
-func maxContainerRestarts(pod *corev1.Pod) int {
-	maxRestarts := 0
+func maxContainerRestarts(pod *corev1.Pod) (regularRestarts, sidecarRestarts int) {
 	for _, c := range pod.Status.ContainerStatuses {
-		maxRestarts = max(maxRestarts, int(c.RestartCount))
+		regularRestarts = max(regularRestarts, int(c.RestartCount))
 	}
-	return maxRestarts
+	names := sets.New[string]()
+	for _, c := range pod.Spec.InitContainers {
+		if c.RestartPolicy != nil && *c.RestartPolicy == corev1.ContainerRestartPolicyAlways {
+			names.Insert(c.Name)
+		}
+	}
+	for _, c := range pod.Status.InitContainerStatuses {
+		if names.Has(c.Name) {
+			sidecarRestarts = max(sidecarRestarts, int(c.RestartCount))
+		}
+	}
+	return
+}
+
+// We use *bool here to determine equality:
+// true: pi has a higher container restart count.
+// false: pj has a higher container restart count.
+// nil: Both have the same container restart count.
+func compareMaxContainerRestarts(pi *corev1.Pod, pj *corev1.Pod) *bool {
+	regularRestartsI, sidecarRestartsI := maxContainerRestarts(pi)
+	regularRestartsJ, sidecarRestartsJ := maxContainerRestarts(pj)
+	if regularRestartsI != regularRestartsJ {
+		res := regularRestartsI > regularRestartsJ
+		return &res
+	}
+	// If pods have the same restart count, an attempt is made to compare the restart counts of sidecar containers.
+	if sidecarRestartsI != sidecarRestartsJ {
+		res := sidecarRestartsI > sidecarRestartsJ
+		return &res
+	}
+	return nil
 }
 
 // ContainerType and VisitContainers are taken from


Test patch:
diff --git a/pkg/controller/controller_utils_test.go b/pkg/controller/controller_utils_test.go
index a71bee6d28f34..a85ddac6089fe 100644
--- a/pkg/controller/controller_utils_test.go
+++ b/pkg/controller/controller_utils_test.go
@@ -490,6 +490,7 @@ func TestSortingActivePods(t *testing.T) {
 	now := metav1.Now()
 	then := metav1.Time{Time: now.AddDate(0, -1, 0)}
 
+	restartAlways := v1.ContainerRestartPolicyAlways
 	tests := []struct {
 		name      string
 		pods      []v1.Pod
@@ -545,6 +546,22 @@ func TestSortingActivePods(t *testing.T) {
 						ContainerStatuses: []v1.ContainerStatus{{RestartCount: 3}, {RestartCount: 0}},
 					},
 				},
+				{
+					ObjectMeta: metav1.ObjectMeta{Name: "lowerSidecarContainerRestartCount", CreationTimestamp: now},
+					Spec: v1.PodSpec{
+						NodeName: "foo",
+						InitContainers: []v1.Container{{
+							Name:          "sidecar",
+							RestartPolicy: &restartAlways,
+						}},
+					},
+					Status: v1.PodStatus{
+						Phase:                 v1.PodRunning,
+						Conditions:            []v1.PodCondition{{Type: v1.PodReady, Status: v1.ConditionTrue, LastTransitionTime: then}},
+						ContainerStatuses:     []v1.ContainerStatus{{RestartCount: 2}, {RestartCount: 1}},
+						InitContainerStatuses: []v1.ContainerStatus{{Name: "sidecar", RestartCount: 2}},
+					},
+				},
 				{
 					ObjectMeta: metav1.ObjectMeta{Name: "lowerContainerRestartCount", CreationTimestamp: now},
 					Spec:       v1.PodSpec{NodeName: "foo"},
@@ -572,6 +589,7 @@ func TestSortingActivePods(t *testing.T) {
 				"runningNoLastTransitionTime",
 				"runningWithLastTransitionTime",
 				"runningLongerTime",
+				"lowerSidecarContainerRestartCount",
 				"lowerContainerRestartCount",
 				"oldest",
 			},
@@ -610,12 +628,15 @@ func TestSortingActivePodsWithRanks(t *testing.T) {
 	then5Hours := metav1.Time{Time: now.Add(-5 * time.Hour)}
 	then8Hours := metav1.Time{Time: now.Add(-8 * time.Hour)}
 	zeroTime := metav1.Time{}
-	pod := func(podName, nodeName string, phase v1.PodPhase, ready bool, restarts int32, readySince metav1.Time, created metav1.Time, annotations map[string]string) *v1.Pod {
+	restartAlways := v1.ContainerRestartPolicyAlways
+	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 {
 		var conditions []v1.PodCondition
 		var containerStatuses []v1.ContainerStatus
+		var initContainerStatuses []v1.ContainerStatus
 		if ready {
 			conditions = []v1.PodCondition{{Type: v1.PodReady, Status: v1.ConditionTrue, LastTransitionTime: readySince}}
 			containerStatuses = []v1.ContainerStatus{{RestartCount: restarts}}
+			initContainerStatuses = []v1.ContainerStatus{{Name: "sidecar", RestartCount: sideRestarts}}
 		}
 		return &v1.Pod{
 			ObjectMeta: metav1.ObjectMeta{
@@ -623,31 +644,36 @@ func TestSortingActivePodsWithRanks(t *testing.T) {
 				Name:              podName,
 				Annotations:       annotations,
 			},
-			Spec: v1.PodSpec{NodeName: nodeName},
+			Spec: v1.PodSpec{
+				NodeName:       nodeName,
+				InitContainers: []v1.Container{{Name: "sidecar", RestartPolicy: &restartAlways}},
+			},
 			Status: v1.PodStatus{
-				Conditions:        conditions,
-				ContainerStatuses: containerStatuses,
-				Phase:             phase,
+				Conditions:            conditions,
+				ContainerStatuses:     containerStatuses,
+				InitContainerStatuses: initContainerStatuses,
+				Phase:                 phase,
 			},
 		}
 	}
 	var (
-		unscheduledPod                      = pod("unscheduled", "", v1.PodPending, false, 0, zeroTime, zeroTime, nil)
-		scheduledPendingPod                 = pod("pending", "node", v1.PodPending, false, 0, zeroTime, zeroTime, nil)
-		unknownPhasePod                     = pod("unknown-phase", "node", v1.PodUnknown, false, 0, zeroTime, zeroTime, nil)
-		runningNotReadyPod                  = pod("not-ready", "node", v1.PodRunning, false, 0, zeroTime, zeroTime, nil)
-		runningReadyNoLastTransitionTimePod = pod("ready-no-last-transition-time", "node", v1.PodRunning, true, 0, zeroTime, zeroTime, nil)
-		runningReadyNow                     = pod("ready-now", "node", v1.PodRunning, true, 0, now, now, nil)
-		runningReadyThen                    = pod("ready-then", "node", v1.PodRunning, true, 0, then1Month, then1Month, nil)
-		runningReadyNowHighRestarts         = pod("ready-high-restarts", "node", v1.PodRunning, true, 9001, now, now, nil)
-		runningReadyNowCreatedThen          = pod("ready-now-created-then", "node", v1.PodRunning, true, 0, now, then1Month, nil)
-		lowPodDeletionCost                  = pod("low-deletion-cost", "node", v1.PodRunning, true, 0, now, then1Month, map[string]string{core.PodDeletionCost: "10"})
-		highPodDeletionCost                 = pod("high-deletion-cost", "node", v1.PodRunning, true, 0, now, then1Month, map[string]string{core.PodDeletionCost: "100"})
-		unscheduled5Hours                   = pod("unscheduled-5-hours", "", v1.PodPending, false, 0, then5Hours, then5Hours, nil)
-		unscheduled8Hours                   = pod("unscheduled-10-hours", "", v1.PodPending, false, 0, then8Hours, then8Hours, nil)
-		ready2Hours                         = pod("ready-2-hours", "", v1.PodRunning, true, 0, then2Hours, then1Month, nil)
-		ready5Hours                         = pod("ready-5-hours", "", v1.PodRunning, true, 0, then5Hours, then1Month, nil)
-		ready10Hours                        = pod("ready-10-hours", "", v1.PodRunning, true, 0, then8Hours, then1Month, nil)
+		unscheduledPod                      = pod("unscheduled", "", v1.PodPending, false, 0, 0, zeroTime, zeroTime, nil)
+		scheduledPendingPod                 = pod("pending", "node", v1.PodPending, false, 0, 0, zeroTime, zeroTime, nil)
+		unknownPhasePod                     = pod("unknown-phase", "node", v1.PodUnknown, false, 0, 0, zeroTime, zeroTime, nil)
+		runningNotReadyPod                  = pod("not-ready", "node", v1.PodRunning, false, 0, 0, zeroTime, zeroTime, nil)
+		runningReadyNoLastTransitionTimePod = pod("ready-no-last-transition-time", "node", v1.PodRunning, true, 0, 0, zeroTime, zeroTime, nil)
+		runningReadyNow                     = pod("ready-now", "node", v1.PodRunning, true, 0, 0, now, now, nil)
+		runningReadyThen                    = pod("ready-then", "node", v1.PodRunning, true, 0, 0, then1Month, then1Month, nil)
+		runningReadyNowHighRestarts         = pod("ready-high-restarts", "node", v1.PodRunning, true, 9001, 0, now, now, nil)
+		runningReadyNowHighSideRestarts     = pod("ready-high-restarts", "node", v1.PodRunning, true, 9001, 9001, now, now, nil)
+		runningReadyNowCreatedThen          = pod("ready-now-created-then", "node", v1.PodRunning, true, 0, 0, now, then1Month, nil)
+		lowPodDeletionCost                  = pod("low-deletion-cost", "node", v1.PodRunning, true, 0, 0, now, then1Month, map[string]string{core.PodDeletionCost: "10"})
+		highPodDeletionCost                 = pod("high-deletion-cost", "node", v1.PodRunning, true, 0, 0, now, then1Month, map[string]string{core.PodDeletionCost: "100"})
+		unscheduled5Hours                   = pod("unscheduled-5-hours", "", v1.PodPending, false, 0, 0, then5Hours, then5Hours, nil)
+		unscheduled8Hours                   = pod("unscheduled-10-hours", "", v1.PodPending, false, 0, 0, then8Hours, then8Hours, nil)
+		ready2Hours                         = pod("ready-2-hours", "", v1.PodRunning, true, 0, 0, then2Hours, then1Month, nil)
+		ready5Hours                         = pod("ready-5-hours", "", v1.PodRunning, true, 0, 0, then5Hours, then1Month, nil)
+		ready10Hours                        = pod("ready-10-hours", "", v1.PodRunning, true, 0, 0, then8Hours, then1Month, nil)
 	)
 	equalityTests := []struct {
 		p1                          *v1.Pod
@@ -702,6 +728,7 @@ func TestSortingActivePodsWithRanks(t *testing.T) {
 		{lesser: podWithRank{runningReadyNow, 1}, greater: podWithRank{runningReadyThen, 1}},
 		{lesser: podWithRank{runningReadyNow, 2}, greater: podWithRank{runningReadyThen, 1}},
 		{lesser: podWithRank{runningReadyNowHighRestarts, 1}, greater: podWithRank{runningReadyNow, 1}},
+		{lesser: podWithRank{runningReadyNowHighSideRestarts, 1}, greater: podWithRank{runningReadyNowHighRestarts, 1}},
 		{lesser: podWithRank{runningReadyNow, 2}, greater: podWithRank{runningReadyNowHighRestarts, 1}},
 		{lesser: podWithRank{runningReadyNow, 1}, greater: podWithRank{runningReadyNowCreatedThen, 1}},
 		{lesser: podWithRank{runningReadyNowCreatedThen, 2}, greater: podWithRank{runningReadyNow, 1}},
diff --git a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils_test.go b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils_test.go
index b514a40342a84..1515823d6bfc9 100644
--- a/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils_test.go
+++ b/staging/src/k8s.io/kubectl/pkg/util/podutils/podutils_test.go
@@ -29,6 +29,7 @@ func TestActivePods(t *testing.T) {
 	time1 := metav1.Now()
 	time2 := metav1.NewTime(time1.Add(1 * time.Second))
 	time3 := metav1.NewTime(time1.Add(2 * time.Second))
+	restartAlways := corev1.ContainerRestartPolicyAlways
 
 	tests := []struct {
 		name string
@@ -364,6 +365,81 @@ func TestActivePods(t *testing.T) {
 				},
 			},
 		},
+		{
+			name: "higher sidecar restart count should sort before lower restart count",
+			pod1: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "podWithMoreRestarts",
+					Namespace: "default",
+				},
+				Spec: corev1.PodSpec{
+					NodeName: "node1",
+					InitContainers: []corev1.Container{
+						{
+							Name:          "sidecar",
+							RestartPolicy: &restartAlways,
+						},
+					},
+				},
+				Status: corev1.PodStatus{
+					Phase: corev1.PodRunning,
+					Conditions: []corev1.PodCondition{
+						{
+							Type:               corev1.PodReady,
+							Status:             corev1.ConditionTrue,
+							LastTransitionTime: time1,
+						},
+					},
+					ContainerStatuses: []corev1.ContainerStatus{
+						{
+							RestartCount: 3,
+						},
+					},
+					InitContainerStatuses: []corev1.ContainerStatus{
+						{
+							Name:         "sidecar",
+							RestartCount: 3,
+						},
+					},
+				},
+			},
+			pod2: &corev1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "podWithLessRestarts",
+					Namespace: "default",
+				},
+				Spec: corev1.PodSpec{
+					NodeName: "node1",
+					InitContainers: []corev1.Container{
+						{
+							Name:          "sidecar",
+							RestartPolicy: &restartAlways,
+						},
+					},
+				},
+				Status: corev1.PodStatus{
+					Phase: corev1.PodRunning,
+					Conditions: []corev1.PodCondition{
+						{
+							Type:               corev1.PodReady,
+							Status:             corev1.ConditionTrue,
+							LastTransitionTime: time1,
+						},
+					},
+					ContainerStatuses: []corev1.ContainerStatus{
+						{
+							RestartCount: 3,
+						},
+					},
+					InitContainerStatuses: []corev1.ContainerStatus{
+						{
+							Name:         "sidecar",
+							RestartCount: 2,
+						},
+					},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {