From be3316e2e1994614c4cb3e8e601c5a5f3868b106 Mon Sep 17 00:00:00 2001 From: Yuki Iwai Date: Tue, 11 Jun 2024 11:39:44 +0900 Subject: [PATCH] Job: Fix a bug that the SuccessCriteriaMet condition is added to the Job with successPolicy even if the JobSuccessPolicy featureGate is disabled Signed-off-by: Yuki Iwai --- pkg/controller/job/job_controller_test.go | 31 ++++ pkg/controller/job/success_policy.go | 2 +- pkg/controller/job/success_policy_test.go | 169 +++++++++++++--------- 3 files changed, 136 insertions(+), 66 deletions(-) diff --git a/pkg/controller/job/job_controller_test.go b/pkg/controller/job/job_controller_test.go index 9f5ceac0185..21bf17fc5bb 100644 --- a/pkg/controller/job/job_controller_test.go +++ b/pkg/controller/job/job_controller_test.go @@ -4531,6 +4531,37 @@ func TestSyncJobWithJobSuccessPolicy(t *testing.T) { }, }, }, + "when the JobSuccessPolicy is disabled, the Job never got SuccessCriteriaMet condition even if the Job has the successPolicy field": { + job: batch.Job{ + TypeMeta: validTypeMeta, + ObjectMeta: validObjectMeta, + Spec: batch.JobSpec{ + Selector: validSelector, + Template: validTemplate, + CompletionMode: completionModePtr(batch.IndexedCompletion), + Parallelism: ptr.To[int32](3), + Completions: ptr.To[int32](3), + BackoffLimit: ptr.To[int32](math.MaxInt32), + SuccessPolicy: &batch.SuccessPolicy{ + Rules: []batch.SuccessPolicyRule{{ + SucceededIndexes: ptr.To("0,1"), + SucceededCount: ptr.To[int32](1), + }}, + }, + }, + }, + pods: []v1.Pod{ + *buildPod().uid("a2").index("0").phase(v1.PodRunning).trackingFinalizer().Pod, + *buildPod().uid("b").index("1").phase(v1.PodSucceeded).trackingFinalizer().Pod, + *buildPod().uid("c").index("2").phase(v1.PodRunning).trackingFinalizer().Pod, + }, + wantStatus: batch.JobStatus{ + Active: 2, + Succeeded: 1, + CompletedIndexes: "1", + UncountedTerminatedPods: &batch.UncountedTerminatedPods{}, + }, + }, } for name, tc := range testCases { t.Run(name, func(t *testing.T) { diff --git a/pkg/controller/job/success_policy.go b/pkg/controller/job/success_policy.go index 9fe5ea83fe4..c70c1d3289d 100644 --- a/pkg/controller/job/success_policy.go +++ b/pkg/controller/job/success_policy.go @@ -27,7 +27,7 @@ import ( ) func matchSuccessPolicy(logger klog.Logger, successPolicy *batch.SuccessPolicy, completions int32, succeededIndexes orderedIntervals) (string, bool) { - if successPolicy == nil || len(succeededIndexes) == 0 { + if !feature.DefaultFeatureGate.Enabled(features.JobSuccessPolicy) || successPolicy == nil || len(succeededIndexes) == 0 { return "", false } diff --git a/pkg/controller/job/success_policy_test.go b/pkg/controller/job/success_policy_test.go index 265fc6e651d..6bf629e43b6 100644 --- a/pkg/controller/job/success_policy_test.go +++ b/pkg/controller/job/success_policy_test.go @@ -21,30 +21,41 @@ import ( "github.com/google/go-cmp/cmp" batch "k8s.io/api/batch/v1" + "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" "k8s.io/klog/v2/ktesting" + "k8s.io/kubernetes/pkg/features" "k8s.io/utils/ptr" ) func TestMatchSuccessPolicy(t *testing.T) { testCases := map[string]struct { - successPolicy *batch.SuccessPolicy - completions int32 - succeededIndexes orderedIntervals - wantMessage string - wantMetSuccessPolicy bool + successPolicy *batch.SuccessPolicy + completions int32 + succeededIndexes orderedIntervals + wantMessage string + wantMetSuccessPolicy bool + enableJobSuccessPolicy bool }{ - "successPolicy is null": { + "JobSuccessPolicy is disabled": { completions: 10, succeededIndexes: orderedIntervals{{0, 0}}, }, + "successPolicy is null": { + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 0}}, + }, "any rules are nothing": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 0}}, - successPolicy: &batch.SuccessPolicy{Rules: []batch.SuccessPolicyRule{}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 0}}, + successPolicy: &batch.SuccessPolicy{Rules: []batch.SuccessPolicyRule{}}, }, "rules.succeededIndexes is invalid format": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 0}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 0}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("invalid-form"), @@ -52,8 +63,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes is specified; succeededIndexes matched rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-2"), @@ -63,8 +75,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMetSuccessPolicy: true, }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("3"), @@ -72,8 +85,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededCount is specified; succeededIndexes matched rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededCount: ptr.To[int32](2), @@ -83,8 +97,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMetSuccessPolicy: true, }, "rules.succeededCount is specified; succeededIndexes didn't match rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededCount: ptr.To[int32](4), @@ -92,8 +107,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "multiple rules; rules.succeededIndexes is specified; succeededIndexes met one of rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{ { @@ -108,8 +124,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMetSuccessPolicy: true, }, "multiple rules; rules.succeededIndexes is specified; succeededIndexes met all rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{ { @@ -124,8 +141,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMetSuccessPolicy: true, }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes met all rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}, {4, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("3-6"), @@ -136,8 +154,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes didn't match rules": { - completions: 10, - succeededIndexes: orderedIntervals{{0, 2}, {6, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{0, 2}, {6, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("3-6"), @@ -156,8 +175,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, "rules.succeededIndexes is specified; succeededIndexes matched rules; rules is proper subset of succeededIndexes": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 5}, {6, 9}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 5}, {6, 9}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("2-4,6-8"), @@ -167,8 +187,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes matched rules; rules equals succeededIndexes": { - completions: 10, - succeededIndexes: orderedIntervals{{2, 4}, {6, 9}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{2, 4}, {6, 9}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("2-4,6-9"), @@ -178,8 +199,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes matched rules; rules is subset of succeededIndexes": { - completions: 10, - succeededIndexes: orderedIntervals{{2, 5}, {7, 15}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{2, 5}, {7, 15}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("2-4,8-12"), @@ -189,8 +211,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules is an empty set": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To(""), @@ -198,8 +221,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; succeededIndexes is an empty set": { - completions: 10, - succeededIndexes: orderedIntervals{}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To(""), @@ -207,8 +231,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules and succeededIndexes are empty set": { - completions: 10, - succeededIndexes: orderedIntervals{}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To(""), @@ -216,8 +241,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; all elements of rules.succeededIndexes aren't included in succeededIndexes": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}, {5, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}, {5, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("10-12,14-16"), @@ -225,8 +251,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules overlaps succeededIndexes at first": { - completions: 10, - succeededIndexes: orderedIntervals{{2, 4}, {6, 8}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{2, 4}, {6, 8}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("1-3,5-7"), @@ -234,8 +261,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules overlaps succeededIndexes at first": { - completions: 10, - succeededIndexes: orderedIntervals{{2, 4}, {6, 8}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{2, 4}, {6, 8}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("1-3,5-7"), @@ -246,8 +274,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules overlaps succeededIndexes at last": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}, {5, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}, {5, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("2-4,6-9"), @@ -255,8 +284,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules overlaps succeededIndexes at last": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}, {5, 7}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}, {5, 7}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("2-4,6-9"), @@ -267,8 +297,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules completely overlaps succeededIndexes": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}, {7, 8}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}, {7, 8}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-4,6-9"), @@ -276,8 +307,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules completely overlaps succeededIndexes": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}, {7, 8}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}, {7, 8}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-4,6-9"), @@ -288,8 +320,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules overlaps multiple succeededIndexes at last": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}, {5, 9}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}, {5, 9}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-6,8-9"), @@ -297,8 +330,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules overlaps multiple succeededIndexes at last": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 3}, {5, 9}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 3}, {5, 9}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-6,8-9"), @@ -309,8 +343,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules overlaps succeededIndexes at first, and rules equals succeededIndexes at last": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 5}, {7, 10}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 5}, {7, 10}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-5,7-9"), @@ -318,8 +353,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules overlaps succeededIndexes at first, and rules equals succeededIndexes at last": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 5}, {7, 10}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 5}, {7, 10}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-5,7-9"), @@ -330,8 +366,9 @@ func TestMatchSuccessPolicy(t *testing.T) { wantMessage: "Matched rules at index 0", }, "rules.succeededIndexes is specified; succeededIndexes didn't match rules; the first rules overlaps succeededIndexes at first": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 10}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 10}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-3,6-9"), @@ -339,8 +376,9 @@ func TestMatchSuccessPolicy(t *testing.T) { }, }, "rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; the first rules overlaps succeededIndexes at first": { - completions: 10, - succeededIndexes: orderedIntervals{{1, 10}}, + enableJobSuccessPolicy: true, + completions: 10, + succeededIndexes: orderedIntervals{{1, 10}}, successPolicy: &batch.SuccessPolicy{ Rules: []batch.SuccessPolicyRule{{ SucceededIndexes: ptr.To("0-3,6-9"), @@ -353,6 +391,7 @@ func TestMatchSuccessPolicy(t *testing.T) { } for name, tc := range testCases { t.Run(name, func(t *testing.T) { + featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.JobSuccessPolicy, tc.enableJobSuccessPolicy) logger := ktesting.NewLogger(t, ktesting.NewConfig( ktesting.BufferLogs(true),