Repository: kubernetes/kubernetes (language: go)

Issue:
[Sidecar Containers] Sidecar containers finish time needs to be accounted for in job controller
### What happened?

Today the function `getFinishTimeFromContainers` (pkg/controller/job/backoff_utils.go) only account for the regular containers finish time,
presumably assuming that init containers have completed before before.
However, with the sidecar (restartable init) containers, sidecar containers will always finish later than
regular containers. And those needs to be accounted for the calculation.

/kind bug

### What did you expect to happen?

The sidecar finish time will be accounted when calculating the job's finish time.

### How can we reproduce it (as minimally and precisely as possible)?

Sidecar container with prolonged termination logic on a job will result in incorrect finish time reporting.

### Anything else we need to know?

/sig apps
/priority backlog
KEP: https://github.com/kubernetes/enhancements/issues/753


Discussion / hints:
/assign


Fix patch:
diff --git a/pkg/controller/job/backoff_utils.go b/pkg/controller/job/backoff_utils.go
index ecd86dc31139c..3bef68117b8b0 100644
--- a/pkg/controller/job/backoff_utils.go
+++ b/pkg/controller/job/backoff_utils.go
@@ -22,9 +22,12 @@ import (
 	"time"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
 	apipod "k8s.io/kubernetes/pkg/api/v1/pod"
+	"k8s.io/kubernetes/pkg/features"
 	"k8s.io/utils/clock"
 	"k8s.io/utils/ptr"
 )
@@ -183,12 +186,32 @@ func getFinishedTime(p *v1.Pod) time.Time {
 }
 
 func getFinishTimeFromContainers(p *v1.Pod) *time.Time {
-	var finishTime *time.Time
-	for _, containerState := range p.Status.ContainerStatuses {
-		if containerState.State.Terminated == nil {
-			return nil
+	finishTime := latestFinishTime(nil, p.Status.ContainerStatuses, nil)
+	if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
+		// We need to check InitContainerStatuses here also,
+		// because with the sidecar (restartable init) containers,
+		// sidecar containers will always finish later than regular containers.
+		names := sets.New[string]()
+		for _, c := range p.Spec.InitContainers {
+			if c.RestartPolicy != nil && *c.RestartPolicy == v1.ContainerRestartPolicyAlways {
+				names.Insert(c.Name)
+			}
+		}
+		finishTime = latestFinishTime(finishTime, p.Status.InitContainerStatuses, func(status v1.ContainerStatus) bool {
+			return names.Has(status.Name)
+		})
+	}
+	return finishTime
+}
+
+func latestFinishTime(prevFinishTime *time.Time, cs []v1.ContainerStatus, check func(status v1.ContainerStatus) bool) *time.Time {
+	var finishTime = prevFinishTime
+	for _, containerState := range cs {
+		if check != nil && !check(containerState) {
+			continue
 		}
-		if containerState.State.Terminated.FinishedAt.Time.IsZero() {
+		if containerState.State.Terminated == nil ||
+			containerState.State.Terminated.FinishedAt.Time.IsZero() {
 			return nil
 		}
 		if finishTime == nil || finishTime.Before(containerState.State.Terminated.FinishedAt.Time) {


Test patch:
diff --git a/pkg/controller/job/backoff_utils_test.go b/pkg/controller/job/backoff_utils_test.go
index e300982c0b8c3..677d8251cb8da 100644
--- a/pkg/controller/job/backoff_utils_test.go
+++ b/pkg/controller/job/backoff_utils_test.go
@@ -23,7 +23,10 @@ import (
 	"github.com/google/go-cmp/cmp"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	utilfeature "k8s.io/apiserver/pkg/util/feature"
+	featuregatetesting "k8s.io/component-base/featuregate/testing"
 	"k8s.io/klog/v2/ktesting"
+	"k8s.io/kubernetes/pkg/features"
 	clocktesting "k8s.io/utils/clock/testing"
 	"k8s.io/utils/ptr"
 )
@@ -200,8 +203,10 @@ func TestNewBackoffRecord(t *testing.T) {
 }
 
 func TestGetFinishedTime(t *testing.T) {
+	featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.SidecarContainers, true)
 	defaultTestTime := time.Date(2009, time.November, 10, 23, 0, 0, 0, time.UTC)
 	defaultTestTimeMinus30s := defaultTestTime.Add(-30 * time.Second)
+	containerRestartPolicyAlways := v1.ContainerRestartPolicyAlways
 	testCases := map[string]struct {
 		pod            v1.Pod
 		wantFinishTime time.Time
@@ -355,6 +360,39 @@ func TestGetFinishedTime(t *testing.T) {
 			},
 			wantFinishTime: defaultTestTime,
 		},
+		// In this case, init container is stopped after the regular containers.
+		// This is because with the sidecar (restartable init) containers,
+		// sidecar containers will always finish later than regular containers.
+		"Pod with sidecar container and all containers terminated": {
+			pod: v1.Pod{
+				Spec: v1.PodSpec{
+					InitContainers: []v1.Container{
+						{
+							Name:          "sidecar",
+							RestartPolicy: &containerRestartPolicyAlways,
+						},
+					},
+				},
+				Status: v1.PodStatus{
+					ContainerStatuses: []v1.ContainerStatus{
+						{
+							State: v1.ContainerState{
+								Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(defaultTestTime.Add(-1 * time.Second))},
+							},
+						},
+					},
+					InitContainerStatuses: []v1.ContainerStatus{
+						{
+							Name: "sidecar",
+							State: v1.ContainerState{
+								Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(defaultTestTime)},
+							},
+						},
+					},
+				},
+			},
+			wantFinishTime: defaultTestTime,
+		},
 	}
 
 	for name, tc := range testCases {