Skip to content

Commit

Permalink
Only default Job fields when feature gates are enabled
Browse files Browse the repository at this point in the history 
Also use pointer for completionMode enum
  • Loading branch information
@alculquicondor
alculquicondor committed Mar 12, 2021
1 parent fcee7a0 commit e6c3d7b
Show file tree
Hide file tree
Showing 38 changed files with 302 additions and 248 deletions.
8 changes: 4 additions & 4 deletions pkg/apis/batch/fuzzer/fuzzer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,11 @@ var Funcs = func(codecs runtimeserializer.CodecFactory) []interface{} {
} else {
j.ManualSelector = nil
}
if c.Rand.Int31()%2 == 0 {
j.CompletionMode = batch.NonIndexedCompletion
} else {
j.CompletionMode = batch.IndexedCompletion
mode := batch.NonIndexedCompletion
if c.RandBool() {
mode = batch.IndexedCompletion
}
j.CompletionMode = &mode
// We're fuzzing the internal JobSpec type, not the v1 type, so we don't
// need to fuzz the nil value.
j.Suspend = pointer.BoolPtr(c.RandBool())
Expand Down
2 changes: 1 addition & 1 deletion pkg/apis/batch/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ type JobSpec struct {
// If the Job controller observes a mode that it doesn't recognize, the
// controller skips updates for the Job.
// +optional
CompletionMode CompletionMode
CompletionMode *CompletionMode

// Suspend specifies whether the Job controller should create Pods or not. If
// a Job is created with suspend set to true, no Pods are created by the Job
Expand Down
9 changes: 6 additions & 3 deletions pkg/apis/batch/v1/defaults.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package v1
import (
batchv1 "k8s.io/api/batch/v1"
"k8s.io/apimachinery/pkg/runtime"
utilfeature "k8s.io/apiserver/pkg/util/feature"
"k8s.io/kubernetes/pkg/features"
utilpointer "k8s.io/utils/pointer"
)

Expand All @@ -43,10 +45,11 @@ func SetDefaults_Job(obj *batchv1.Job) {
if labels != nil && len(obj.Labels) == 0 {
obj.Labels = labels
}
if len(obj.Spec.CompletionMode) == 0 {
obj.Spec.CompletionMode = batchv1.NonIndexedCompletion
if utilfeature.DefaultFeatureGate.Enabled(features.IndexedJob) && obj.Spec.CompletionMode == nil {
mode := batchv1.NonIndexedCompletion
obj.Spec.CompletionMode = &mode
}
if obj.Spec.Suspend == nil {
if utilfeature.DefaultFeatureGate.Enabled(features.SuspendJob) && obj.Spec.Suspend == nil {
obj.Spec.Suspend = utilpointer.BoolPtr(false)
}
}
Expand Down
129 changes: 80 additions & 49 deletions pkg/apis/batch/v1/defaults_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,12 @@ import (
"k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apiserver/pkg/util/feature"
featuregatetesting "k8s.io/component-base/featuregate/testing"
"k8s.io/kubernetes/pkg/api/legacyscheme"
_ "k8s.io/kubernetes/pkg/apis/batch/install"
_ "k8s.io/kubernetes/pkg/apis/core/install"
"k8s.io/kubernetes/pkg/features"
"k8s.io/utils/pointer"

. "k8s.io/kubernetes/pkg/apis/batch/v1"
Expand All @@ -36,11 +39,31 @@ import (
func TestSetDefaultJob(t *testing.T) {
defaultLabels := map[string]string{"default": "default"}
tests := map[string]struct {
original *batchv1.Job
expected *batchv1.Job
expectLabels bool
indexedJobEnabled bool
suspendJobEnabled bool
original *batchv1.Job
expected *batchv1.Job
expectLabels bool
}{
"All unspecified -> sets all to default values": {
original: &batchv1.Job{
Spec: batchv1.JobSpec{
Template: v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{Labels: defaultLabels},
},
},
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
},
},
expectLabels: true,
},
"All unspecified, indexed job enabled -> sets all to default values": {
indexedJobEnabled: true,
original: &batchv1.Job{
Spec: batchv1.JobSpec{
Template: v1.PodTemplateSpec{
Expand All @@ -53,13 +76,32 @@ func TestSetDefaultJob(t *testing.T) {
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(false),
CompletionMode: completionModePtr(batchv1.NonIndexedCompletion),
},
},
expectLabels: true,
},
"All unspecified, suspend job enabled -> sets all to default values": {
suspendJobEnabled: true,
original: &batchv1.Job{
Spec: batchv1.JobSpec{
Template: v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{Labels: defaultLabels},
},
},
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
Suspend: pointer.BoolPtr(false),
},
},
expectLabels: true,
},
"suspend set, everything else is defaulted": {
suspendJobEnabled: true,
original: &batchv1.Job{
Spec: batchv1.JobSpec{
Suspend: pointer.BoolPtr(true),
Expand All @@ -70,16 +112,15 @@ func TestSetDefaultJob(t *testing.T) {
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(true),
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
Suspend: pointer.BoolPtr(true),
},
},
expectLabels: true,
},
"All unspecified -> all pointers, CompletionMode are defaulted and no default labels": {
"All unspecified -> all pointers are defaulted and no default labels": {
original: &batchv1.Job{
ObjectMeta: metav1.ObjectMeta{
Labels: map[string]string{"mylabel": "myvalue"},
Expand All @@ -92,11 +133,9 @@ func TestSetDefaultJob(t *testing.T) {
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(false),
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
},
},
},
Expand All @@ -111,10 +150,8 @@ func TestSetDefaultJob(t *testing.T) {
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Parallelism: pointer.Int32Ptr(0),
BackoffLimit: pointer.Int32Ptr(6),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(false),
Parallelism: pointer.Int32Ptr(0),
BackoffLimit: pointer.Int32Ptr(6),
},
},
expectLabels: true,
Expand All @@ -130,10 +167,8 @@ func TestSetDefaultJob(t *testing.T) {
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Parallelism: pointer.Int32Ptr(2),
BackoffLimit: pointer.Int32Ptr(6),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(false),
Parallelism: pointer.Int32Ptr(2),
BackoffLimit: pointer.Int32Ptr(6),
},
},
expectLabels: true,
Expand All @@ -149,11 +184,9 @@ func TestSetDefaultJob(t *testing.T) {
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Completions: pointer.Int32Ptr(2),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(false),
Completions: pointer.Int32Ptr(2),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(6),
},
},
expectLabels: true,
Expand All @@ -169,11 +202,9 @@ func TestSetDefaultJob(t *testing.T) {
},
expected: &batchv1.Job{
Spec: batchv1.JobSpec{
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(5),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(false),
Completions: pointer.Int32Ptr(1),
Parallelism: pointer.Int32Ptr(1),
BackoffLimit: pointer.Int32Ptr(5),
},
},
expectLabels: true,
Expand All @@ -184,8 +215,8 @@ func TestSetDefaultJob(t *testing.T) {
Completions: pointer.Int32Ptr(8),
Parallelism: pointer.Int32Ptr(9),
BackoffLimit: pointer.Int32Ptr(10),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(true),
CompletionMode: completionModePtr(batchv1.NonIndexedCompletion),
Suspend: pointer.BoolPtr(false),
Template: v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{Labels: defaultLabels},
},
Expand All @@ -196,8 +227,8 @@ func TestSetDefaultJob(t *testing.T) {
Completions: pointer.Int32Ptr(8),
Parallelism: pointer.Int32Ptr(9),
BackoffLimit: pointer.Int32Ptr(10),
CompletionMode: batchv1.NonIndexedCompletion,
Suspend: pointer.BoolPtr(true),
CompletionMode: completionModePtr(batchv1.NonIndexedCompletion),
Suspend: pointer.BoolPtr(false),
Template: v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{Labels: defaultLabels},
},
Expand All @@ -211,7 +242,7 @@ func TestSetDefaultJob(t *testing.T) {
Completions: pointer.Int32Ptr(11),
Parallelism: pointer.Int32Ptr(10),
BackoffLimit: pointer.Int32Ptr(9),
CompletionMode: batchv1.IndexedCompletion,
CompletionMode: completionModePtr(batchv1.IndexedCompletion),
Suspend: pointer.BoolPtr(true),
Template: v1.PodTemplateSpec{
ObjectMeta: metav1.ObjectMeta{Labels: defaultLabels},
Expand All @@ -223,7 +254,7 @@ func TestSetDefaultJob(t *testing.T) {
Completions: pointer.Int32Ptr(11),
Parallelism: pointer.Int32Ptr(10),
BackoffLimit: pointer.Int32Ptr(9),
CompletionMode: batchv1.IndexedCompletion,
CompletionMode: completionModePtr(batchv1.IndexedCompletion),
Suspend: pointer.BoolPtr(true),
},
},
Expand All @@ -233,6 +264,8 @@ func TestSetDefaultJob(t *testing.T) {

for name, test := range tests {
t.Run(name, func(t *testing.T) {
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.IndexedJob, test.indexedJobEnabled)()
defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.SuspendJob, test.suspendJobEnabled)()

original := test.original
expected := test.expected
Expand All @@ -256,8 +289,8 @@ func TestSetDefaultJob(t *testing.T) {
t.Errorf("Unexpected equality: %v", actual.Labels)
}
}
if actual.Spec.CompletionMode != expected.Spec.CompletionMode {
t.Errorf("Got CompletionMode: %v, want: %v", actual.Spec.CompletionMode, expected.Spec.CompletionMode)
if diff := cmp.Diff(expected.Spec.CompletionMode, actual.Spec.CompletionMode); diff != "" {
t.Errorf("Unexpected CompletionMode (-want,+got):\n%s", diff)
}
})
}
Expand Down Expand Up @@ -304,7 +337,7 @@ func TestSetDefaultCronJob(t *testing.T) {
expected: &batchv1.CronJob{
Spec: batchv1.CronJobSpec{
ConcurrencyPolicy: batchv1.AllowConcurrent,
Suspend: newBool(false),
Suspend: pointer.BoolPtr(false),
SuccessfulJobsHistoryLimit: pointer.Int32Ptr(3),
FailedJobsHistoryLimit: pointer.Int32Ptr(1),
},
Expand All @@ -314,15 +347,15 @@ func TestSetDefaultCronJob(t *testing.T) {
original: &batchv1.CronJob{
Spec: batchv1.CronJobSpec{
ConcurrencyPolicy: batchv1.ForbidConcurrent,
Suspend: newBool(true),
Suspend: pointer.BoolPtr(true),
SuccessfulJobsHistoryLimit: pointer.Int32Ptr(5),
FailedJobsHistoryLimit: pointer.Int32Ptr(5),
},
},
expected: &batchv1.CronJob{
Spec: batchv1.CronJobSpec{
ConcurrencyPolicy: batchv1.ForbidConcurrent,
Suspend: newBool(true),
Suspend: pointer.BoolPtr(true),
SuccessfulJobsHistoryLimit: pointer.Int32Ptr(5),
FailedJobsHistoryLimit: pointer.Int32Ptr(5),
},
Expand Down Expand Up @@ -354,8 +387,6 @@ func TestSetDefaultCronJob(t *testing.T) {
}
}

func newBool(val bool) *bool {
p := new(bool)
*p = val
return p
func completionModePtr(m batchv1.CompletionMode) *batchv1.CompletionMode {
return &m
}
4 changes: 2 additions & 2 deletions pkg/apis/batch/v1/zz_generated.conversion.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 4 additions & 4 deletions pkg/apis/batch/validation/validation.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,12 +129,12 @@ func validateJobSpec(spec *batch.JobSpec, fldPath *field.Path, opts apivalidatio
if spec.TTLSecondsAfterFinished != nil {
allErrs = append(allErrs, apivalidation.ValidateNonnegativeField(int64(*spec.TTLSecondsAfterFinished), fldPath.Child("ttlSecondsAfterFinished"))...)
}
// CompletionMode might be empty when IndexedJob feature gate is disabled.
if spec.CompletionMode != "" {
if spec.CompletionMode != batch.NonIndexedCompletion && spec.CompletionMode != batch.IndexedCompletion {
// CompletionMode might be nil when IndexedJob feature gate is disabled.
if spec.CompletionMode != nil {
if *spec.CompletionMode != batch.NonIndexedCompletion && *spec.CompletionMode != batch.IndexedCompletion {
allErrs = append(allErrs, field.NotSupported(fldPath.Child("completionMode"), spec.CompletionMode, []string{string(batch.NonIndexedCompletion), string(batch.IndexedCompletion)}))
}
if spec.CompletionMode == batch.IndexedCompletion {
if *spec.CompletionMode == batch.IndexedCompletion {
if spec.Completions == nil {
allErrs = append(allErrs, field.Required(fldPath.Child("completions"), fmt.Sprintf("when completion mode is %s", batch.IndexedCompletion)))
}
Expand Down

0 comments on commit e6c3d7b

Please sign in to comment.