Merge pull request #124942 from AxeZhan/getFinishTimeFromContainers

[Sidecar Containers] Sidecar containers finish time needs to be accounted for in job controller

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
+	finishTime := latestFinishTime(nil, p.Status.ContainerStatuses, nil)
-	for _, containerState := range p.Status.ContainerStatuses {
+	if utilfeature.DefaultFeatureGate.Enabled(features.SidecarContainers) {
-		if containerState.State.Terminated == nil {
+		// We need to check InitContainerStatuses here also,
-			return nil
+		// 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)
 			}
-		if containerState.State.Terminated.FinishedAt.Time.IsZero() {
+		}
+		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 == nil ||
+			containerState.State.Terminated.FinishedAt.Time.IsZero() {
 			return nil
 		}
 		if finishTime == nil || finishTime.Before(containerState.State.Terminated.FinishedAt.Time) {

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 {