From e4348a1210ad876a1bbd71523104215258d68f57 Mon Sep 17 00:00:00 2001 From: AxeZhan Date: Sat, 18 May 2024 22:08:26 +0800 Subject: [PATCH 1/3] consider sidecar containers in getFinishTimeFromContainers --- pkg/controller/job/backoff_utils.go | 18 ++++++++++++------ pkg/controller/job/backoff_utils_test.go | 24 ++++++++++++++++++++++++ 2 files changed, 36 insertions(+), 6 deletions(-) diff --git a/pkg/controller/job/backoff_utils.go b/pkg/controller/job/backoff_utils.go index ecd86dc3113..fd03dfe4670 100644 --- a/pkg/controller/job/backoff_utils.go +++ b/pkg/controller/job/backoff_utils.go @@ -183,12 +183,18 @@ 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 - } - if containerState.State.Terminated.FinishedAt.Time.IsZero() { + finishTime := latestFinishTime(nil, p.Status.ContainerStatuses) + // We need to check InitContainerStatuses here also, + // because with the sidecar (restartable init) containers, + // sidecar containers will always finish later than regular containers. + return latestFinishTime(finishTime, p.Status.InitContainerStatuses) +} + +func latestFinishTime(prevFinishTime *time.Time, cs []v1.ContainerStatus) *time.Time { + var finishTime = prevFinishTime + for _, containerState := range cs { + if containerState.State.Terminated == nil || + containerState.State.Terminated.FinishedAt.Time.IsZero() { return nil } if finishTime == nil || finishTime.Before(containerState.State.Terminated.FinishedAt.Time) { diff --git a/pkg/controller/job/backoff_utils_test.go b/pkg/controller/job/backoff_utils_test.go index e300982c0b8..f402ab681b8 100644 --- a/pkg/controller/job/backoff_utils_test.go +++ b/pkg/controller/job/backoff_utils_test.go @@ -355,6 +355,30 @@ 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 init container and all containers terminated": { + pod: v1.Pod{ + Status: v1.PodStatus{ + ContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(defaultTestTime.Add(-1 * time.Second))}, + }, + }, + }, + InitContainerStatuses: []v1.ContainerStatus{ + { + State: v1.ContainerState{ + Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(defaultTestTime)}, + }, + }, + }, + }, + }, + wantFinishTime: defaultTestTime, + }, } for name, tc := range testCases { From 3a2a50018200413a742c201dd747425cfdbdb64a Mon Sep 17 00:00:00 2001 From: AxeZhan Date: Tue, 21 May 2024 11:11:00 +0800 Subject: [PATCH 2/3] check restartpolicy in getFinishTimeFromContainers --- pkg/controller/job/backoff_utils.go | 18 +++++++++++++++--- pkg/controller/job/backoff_utils_test.go | 12 +++++++++++- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/pkg/controller/job/backoff_utils.go b/pkg/controller/job/backoff_utils.go index fd03dfe4670..ea388562e72 100644 --- a/pkg/controller/job/backoff_utils.go +++ b/pkg/controller/job/backoff_utils.go @@ -22,6 +22,7 @@ import ( "time" v1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2" apipod "k8s.io/kubernetes/pkg/api/v1/pod" @@ -183,16 +184,27 @@ func getFinishedTime(p *v1.Pod) time.Time { } func getFinishTimeFromContainers(p *v1.Pod) *time.Time { - finishTime := latestFinishTime(nil, p.Status.ContainerStatuses) + finishTime := latestFinishTime(nil, p.Status.ContainerStatuses, nil) // We need to check InitContainerStatuses here also, // because with the sidecar (restartable init) containers, // sidecar containers will always finish later than regular containers. - return latestFinishTime(finishTime, p.Status.InitContainerStatuses) + names := sets.New[string]() + for _, c := range p.Spec.InitContainers { + if c.RestartPolicy != nil && *c.RestartPolicy == v1.ContainerRestartPolicyAlways { + names.Insert(c.Name) + } + } + return latestFinishTime(finishTime, p.Status.InitContainerStatuses, func(status v1.ContainerStatus) bool { + return names.Has(status.Name) + }) } -func latestFinishTime(prevFinishTime *time.Time, cs []v1.ContainerStatus) *time.Time { +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 diff --git a/pkg/controller/job/backoff_utils_test.go b/pkg/controller/job/backoff_utils_test.go index f402ab681b8..d686e3bc66e 100644 --- a/pkg/controller/job/backoff_utils_test.go +++ b/pkg/controller/job/backoff_utils_test.go @@ -202,6 +202,7 @@ func TestNewBackoffRecord(t *testing.T) { func TestGetFinishedTime(t *testing.T) { 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 @@ -358,8 +359,16 @@ func TestGetFinishedTime(t *testing.T) { // 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 init container and all containers terminated": { + "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{ { @@ -370,6 +379,7 @@ func TestGetFinishedTime(t *testing.T) { }, InitContainerStatuses: []v1.ContainerStatus{ { + Name: "sidecar", State: v1.ContainerState{ Terminated: &v1.ContainerStateTerminated{FinishedAt: metav1.NewTime(defaultTestTime)}, }, From d97282052ef89d1946fef90368f8159f2805ece4 Mon Sep 17 00:00:00 2001 From: AxeZhan Date: Thu, 30 May 2024 10:27:42 +0800 Subject: [PATCH 3/3] check sidecar featuregate in getFinishedTime --- pkg/controller/job/backoff_utils.go | 25 ++++++++++++++---------- pkg/controller/job/backoff_utils_test.go | 4 ++++ 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/pkg/controller/job/backoff_utils.go b/pkg/controller/job/backoff_utils.go index ea388562e72..3bef68117b8 100644 --- a/pkg/controller/job/backoff_utils.go +++ b/pkg/controller/job/backoff_utils.go @@ -23,9 +23,11 @@ import ( 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" ) @@ -185,18 +187,21 @@ func getFinishedTime(p *v1.Pod) time.Time { func getFinishTimeFromContainers(p *v1.Pod) *time.Time { finishTime := latestFinishTime(nil, p.Status.ContainerStatuses, nil) - // 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) + 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 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 { diff --git a/pkg/controller/job/backoff_utils_test.go b/pkg/controller/job/backoff_utils_test.go index d686e3bc66e..677d8251cb8 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,6 +203,7 @@ 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