Merge pull request #125429 from tenzen-y/gurad-success-policy-by-feature-gate

Job: Fix a bug that the SuccessCriteriaMet could be added to the Job with successPolicy regardless of the featureGate enabling

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) {

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
 	}
 

pkg/controller/job/success_policy_test.go

@@ -21,7 +21,10 @@ 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"
 )
 
@@ -32,17 +35,25 @@ func TestMatchSuccessPolicy(t *testing.T) {
 		succeededIndexes       orderedIntervals
 		wantMessage            string
 		wantMetSuccessPolicy   bool
+		enableJobSuccessPolicy bool
 	}{
+		"JobSuccessPolicy is disabled": {
+			completions:      10,
+			succeededIndexes: orderedIntervals{{0, 0}},
+		},
 		"successPolicy is null": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 0}},
 		},
 		"any rules are nothing": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 0}},
 			successPolicy:          &batch.SuccessPolicy{Rules: []batch.SuccessPolicyRule{}},
 		},
 		"rules.succeededIndexes is invalid format": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 0}},
 			successPolicy: &batch.SuccessPolicy{
@@ -52,6 +63,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes is specified; succeededIndexes matched rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}, {4, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -63,6 +75,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMetSuccessPolicy: true,
 		},
 		"rules.succeededIndexes is specified; succeededIndexes didn't match rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}},
 			successPolicy: &batch.SuccessPolicy{
@@ -72,6 +85,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededCount is specified; succeededIndexes matched rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}},
 			successPolicy: &batch.SuccessPolicy{
@@ -83,6 +97,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMetSuccessPolicy: true,
 		},
 		"rules.succeededCount is specified; succeededIndexes didn't match rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}},
 			successPolicy: &batch.SuccessPolicy{
@@ -92,6 +107,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"multiple rules; rules.succeededIndexes is specified; succeededIndexes met one of rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}, {4, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -108,6 +124,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMetSuccessPolicy: true,
 		},
 		"multiple rules; rules.succeededIndexes is specified; succeededIndexes met all rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}, {4, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -124,6 +141,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMetSuccessPolicy: true,
 		},
 		"rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes met all rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}, {4, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -136,6 +154,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMessage:          "Matched rules at index 0",
 		},
 		"rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes didn't match rules": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{0, 2}, {6, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -156,6 +175,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 		},
 
 		"rules.succeededIndexes is specified; succeededIndexes matched rules; rules is proper subset of succeededIndexes": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 5}, {6, 9}},
 			successPolicy: &batch.SuccessPolicy{
@@ -167,6 +187,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMessage:          "Matched rules at index 0",
 		},
 		"rules.succeededIndexes is specified; succeededIndexes matched rules; rules equals succeededIndexes": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{2, 4}, {6, 9}},
 			successPolicy: &batch.SuccessPolicy{
@@ -178,6 +199,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMessage:          "Matched rules at index 0",
 		},
 		"rules.succeededIndexes is specified; succeededIndexes matched rules; rules is subset of succeededIndexes": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{2, 5}, {7, 15}},
 			successPolicy: &batch.SuccessPolicy{
@@ -189,6 +211,7 @@ 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": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}},
 			successPolicy: &batch.SuccessPolicy{
@@ -198,6 +221,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes is specified; succeededIndexes didn't match rules; succeededIndexes is an empty set": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{},
 			successPolicy: &batch.SuccessPolicy{
@@ -207,6 +231,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules and succeededIndexes are empty set": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{},
 			successPolicy: &batch.SuccessPolicy{
@@ -216,6 +241,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes is specified; succeededIndexes didn't match rules; all elements of rules.succeededIndexes aren't included in succeededIndexes": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}, {5, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -225,6 +251,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules overlaps succeededIndexes at first": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{2, 4}, {6, 8}},
 			successPolicy: &batch.SuccessPolicy{
@@ -234,6 +261,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules overlaps succeededIndexes at first": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{2, 4}, {6, 8}},
 			successPolicy: &batch.SuccessPolicy{
@@ -246,6 +274,7 @@ 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": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}, {5, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -255,6 +284,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules overlaps succeededIndexes at last": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}, {5, 7}},
 			successPolicy: &batch.SuccessPolicy{
@@ -267,6 +297,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			wantMessage:          "Matched rules at index 0",
 		},
 		"rules.succeededIndexes is specified; succeededIndexes didn't match rules; rules completely overlaps succeededIndexes": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}, {7, 8}},
 			successPolicy: &batch.SuccessPolicy{
@@ -276,6 +307,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules completely overlaps succeededIndexes": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}, {7, 8}},
 			successPolicy: &batch.SuccessPolicy{
@@ -288,6 +320,7 @@ 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": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}, {5, 9}},
 			successPolicy: &batch.SuccessPolicy{
@@ -297,6 +330,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; rules overlaps multiple succeededIndexes at last": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 3}, {5, 9}},
 			successPolicy: &batch.SuccessPolicy{
@@ -309,6 +343,7 @@ 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": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 5}, {7, 10}},
 			successPolicy: &batch.SuccessPolicy{
@@ -318,6 +353,7 @@ 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": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 5}, {7, 10}},
 			successPolicy: &batch.SuccessPolicy{
@@ -330,6 +366,7 @@ 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": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 10}},
 			successPolicy: &batch.SuccessPolicy{
@@ -339,6 +376,7 @@ func TestMatchSuccessPolicy(t *testing.T) {
 			},
 		},
 		"rules.succeededIndexes and rules.succeededCount are specified; succeededIndexes matched rules; the first rules overlaps succeededIndexes at first": {
+			enableJobSuccessPolicy: true,
 			completions:            10,
 			succeededIndexes:       orderedIntervals{{1, 10}},
 			successPolicy: &batch.SuccessPolicy{
@@ -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),