From d5dbb51970d57197b493693295ec323b8e9598b9 Mon Sep 17 00:00:00 2001 From: Daniel Vega-Myhre Date: Wed, 20 Mar 2024 00:13:30 +0000 Subject: [PATCH] move jobset webhook to webhooks package --- Makefile | 6 +- api/jobset/v1alpha2/zz_generated.deepcopy.go | 2 +- go.mod | 4 +- go.sum | 8 +- main.go | 11 +- .../webhooks}/jobset_webhook.go | 84 +++- .../webhooks}/jobset_webhook_test.go | 450 +++++++++++------- pkg/webhooks/pod_mutating_webhook.go | 4 +- test/integration/webhook/suite_test.go | 7 +- 9 files changed, 356 insertions(+), 220 deletions(-) rename {api/jobset/v1alpha2 => pkg/webhooks}/jobset_webhook.go (79%) rename {api/jobset/v1alpha2 => pkg/webhooks}/jobset_webhook_test.go (60%) diff --git a/Makefile b/Makefile index ad5bc913..22364a3d 100644 --- a/Makefile +++ b/Makefile @@ -82,9 +82,11 @@ help: ## Display this help. manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and CustomResourceDefinition objects. $(CONTROLLER_GEN) \ rbac:roleName=manager-role output:rbac:artifacts:config=config/components/rbac\ - crd:generateEmbeddedObjectMeta=true output:crd:artifacts:config=config/components/crd/bases\ + paths="./api/..." + $(CONTROLLER_GEN) \ + rbac:roleName=manager-role output:rbac:artifacts:config=config/components/rbac\ webhook output:webhook:artifacts:config=config/components/webhook\ - paths="{./api/..., ./pkg/...}" + paths="./pkg/..." .PHONY: generate generate: controller-gen code-generator openapi-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations and client-go libraries. diff --git a/api/jobset/v1alpha2/zz_generated.deepcopy.go b/api/jobset/v1alpha2/zz_generated.deepcopy.go index a8ce765f..857350d1 100644 --- a/api/jobset/v1alpha2/zz_generated.deepcopy.go +++ b/api/jobset/v1alpha2/zz_generated.deepcopy.go @@ -19,7 +19,7 @@ package v1alpha2 import ( "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" + runtime "k8s.io/apimachinery/pkg/runtime" ) // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. diff --git a/go.mod b/go.mod index 22fe2020..2148ab99 100644 --- a/go.mod +++ b/go.mod @@ -40,7 +40,7 @@ require ( github.com/google/gnostic-models v0.6.8 // indirect github.com/google/gofuzz v1.2.0 // indirect github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 // indirect - github.com/google/uuid v1.3.0 // indirect + github.com/google/uuid v1.3.1 // indirect github.com/imdario/mergo v0.3.16 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/json-iterator/go v1.1.12 // indirect @@ -59,7 +59,7 @@ require ( go.uber.org/atomic v1.11.0 // indirect go.uber.org/multierr v1.11.0 // indirect go.uber.org/zap v1.26.0 // indirect - golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e // indirect + golang.org/x/exp v0.0.0-20230905200255-921286631fa9 // indirect golang.org/x/mod v0.14.0 // indirect golang.org/x/net v0.20.0 // indirect golang.org/x/oauth2 v0.12.0 // indirect diff --git a/go.sum b/go.sum index bad5a53b..4d3811c3 100644 --- a/go.sum +++ b/go.sum @@ -51,8 +51,8 @@ github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0= github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg= github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1 h1:K6RDEckDVWvDI9JAJYCmNdQXq6neHJOYx3V6jnqNEec= github.com/google/pprof v0.0.0-20210720184732-4bb14d4b1be1/go.mod h1:kpwsk12EmLew5upagYY7GY0pfYCcupk39gWOCRROcvE= -github.com/google/uuid v1.3.0 h1:t6JiXgmwXMjEs8VusXIJk2BXHsn+wx8BZdTaoZ5fu7I= -github.com/google/uuid v1.3.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= +github.com/google/uuid v1.3.1 h1:KjJaJ9iWZ3jOFZIf1Lqf4laDRCasjl0BCmnEGxkdLb4= +github.com/google/uuid v1.3.1/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo= github.com/ianlancetaylor/demangle v0.0.0-20200824232613-28f6c0f3b639/go.mod h1:aSSvb/t6k1mPoxDqO4vJh6VOCGPwU4O0C2/Eqndh1Sc= github.com/imdario/mergo v0.3.16 h1:wwQJbIsHYGMUyLSPrEq1CT16AhnhNJQ51+4fdHUnCl4= github.com/imdario/mergo v0.3.16/go.mod h1:WBLT9ZmE3lPoWsEzCh9LPo3TiwVN+ZKEjmz+hD27ysY= @@ -128,8 +128,8 @@ go.uber.org/zap v1.26.0/go.mod h1:dtElttAiwGvoJ/vj4IwHBS/gXsEu/pZ50mUIRWuG0so= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI= golang.org/x/crypto v0.0.0-20200622213623-75b288015ac9/go.mod h1:LzIPMQfyMNhhGPhUkYOs5KpL4U8rLKemX1yGLhDgUto= -golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e h1:+WEEuIdZHnUeJJmEUjyYC2gfUMj69yZXw17EnHg/otA= -golang.org/x/exp v0.0.0-20220722155223-a9213eeb770e/go.mod h1:Kr81I6Kryrl9sr8s2FK3vxD90NdsKWRuOIl2O4CvYbA= +golang.org/x/exp v0.0.0-20230905200255-921286631fa9 h1:GoHiUyI/Tp2nVkLI2mCxVkOjsbSXD66ic0XW0js0R9g= +golang.org/x/exp v0.0.0-20230905200255-921286631fa9/go.mod h1:S2oDrQGGwySpoQPVqRShND87VCbxmc6bL1Yd2oYrm6k= golang.org/x/mod v0.2.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.3.0/go.mod h1:s0Qsj1ACt9ePp/hMypM3fl4fZqREWJwdYDEqhRiZZUA= golang.org/x/mod v0.14.0 h1:dGoOF9QVLYng8IHTm7BAyWqCqSheQ5pYWGhzW00YJr0= diff --git a/main.go b/main.go index c79f8403..dc54b0e3 100644 --- a/main.go +++ b/main.go @@ -158,15 +158,20 @@ func setupControllers(mgr ctrl.Manager, certsReady chan struct{}) { } // Set up JobSet validating/defaulting webhook. - if err := (&jobset.JobSet{}).SetupWebhookWithManager(mgr); err != nil { + jobSetWebHook, err := webhooks.NewJobSetWebhook(mgr.GetClient()) + if err != nil { setupLog.Error(err, "unable to create webhook", "webhook", "JobSet") os.Exit(1) } + if err := jobSetWebHook.SetupWebhookWithManager(mgr); err != nil { + setupLog.Error(err, "unable to set up webhook", "webhook", "JobSet") + os.Exit(1) + } // Set up pod mutating and admission webhook. - podWebhook := webhooks.NewPodWebhook(mgr) + podWebhook := webhooks.NewPodWebhook(mgr.GetClient()) if err := podWebhook.SetupWebhookWithManager(mgr); err != nil { - setupLog.Error(err, "unable to create webhook", "webhook", "Pod") + setupLog.Error(err, "unable to set up webhook", "webhook", "Pod") os.Exit(1) } //+kubebuilder:scaffold:builder diff --git a/api/jobset/v1alpha2/jobset_webhook.go b/pkg/webhooks/jobset_webhook.go similarity index 79% rename from api/jobset/v1alpha2/jobset_webhook.go rename to pkg/webhooks/jobset_webhook.go index 1fb478e0..7eab1472 100644 --- a/api/jobset/v1alpha2/jobset_webhook.go +++ b/pkg/webhooks/jobset_webhook.go @@ -11,30 +11,33 @@ See the License for the specific language governing permissions and limitations under the License. */ -package v1alpha2 +package webhooks import ( + "context" "errors" "fmt" "math" "strconv" "strings" + batchv1 "k8s.io/api/batch/v1" + corev1 "k8s.io/api/core/v1" apivalidation "k8s.io/apimachinery/pkg/api/validation" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/webhook" + "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" "sigs.k8s.io/jobset/pkg/util/collections" "sigs.k8s.io/jobset/pkg/util/placement" - batchv1 "k8s.io/api/batch/v1" - corev1 "k8s.io/api/core/v1" + jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2" ) // maximum lnegth of the value of the managedBy field @@ -58,24 +61,43 @@ const ( subdomainTooLongErrMsg = ".spec.network.subdomain is too long, must be less than 63 characters" ) -func (js *JobSet) SetupWebhookWithManager(mgr ctrl.Manager) error { - return ctrl.NewWebhookManagedBy(mgr). - For(js). - Complete() +//+kubebuilder:webhook:path=/mutate-jobset-x-k8s-io-v1alpha2-jobset,mutating=true,failurePolicy=fail,sideEffects=None,groups=jobset.x-k8s.io,resources=jobsets,verbs=create;update,versions=v1alpha2,name=mjobset.kb.io,admissionReviewVersions=v1 + +// jobSetWebhook for defaulting and admission. +type jobSetWebhook struct { + client client.Client + decoder *admission.Decoder } -//+kubebuilder:webhook:path=/mutate-jobset-x-k8s-io-v1alpha2-jobset,mutating=true,failurePolicy=fail,sideEffects=None,groups=jobset.x-k8s.io,resources=jobsets,verbs=create;update,versions=v1alpha2,name=mjobset.kb.io,admissionReviewVersions=v1 +func NewJobSetWebhook(mgrClient client.Client) (*jobSetWebhook, error) { + return &jobSetWebhook{client: mgrClient}, nil +} + +// InjectDecoder injects the decoder into the jobSetWebhook. +func (j *jobSetWebhook) InjectDecoder(d *admission.Decoder) error { + j.decoder = d + return nil +} -var _ webhook.Defaulter = &JobSet{} +func (j *jobSetWebhook) SetupWebhookWithManager(mgr ctrl.Manager) error { + return ctrl.NewWebhookManagedBy(mgr). + For(&jobset.JobSet{}). + WithDefaulter(j). + WithValidator(j). + Complete() +} -// Default implements webhook.Defaulter so a webhook will be registered for the type -func (js *JobSet) Default() { +func (j *jobSetWebhook) Default(ctx context.Context, obj runtime.Object) error { + js, ok := obj.(*jobset.JobSet) + if !ok { + return nil + } // Default success policy to operator "All" targeting all replicatedJobs. if js.Spec.SuccessPolicy == nil { - js.Spec.SuccessPolicy = &SuccessPolicy{Operator: OperatorAll} + js.Spec.SuccessPolicy = &jobset.SuccessPolicy{Operator: jobset.OperatorAll} } if js.Spec.StartupPolicy == nil { - js.Spec.StartupPolicy = &StartupPolicy{StartupPolicyOrder: AnyOrder} + js.Spec.StartupPolicy = &jobset.StartupPolicy{StartupPolicyOrder: jobset.AnyOrder} } for i := range js.Spec.ReplicatedJobs { // Default job completion mode to indexed. @@ -84,7 +106,7 @@ func (js *JobSet) Default() { } // Enable DNS hostnames by default. if js.Spec.Network == nil { - js.Spec.Network = &Network{} + js.Spec.Network = &jobset.Network{} } if js.Spec.Network.EnableDNSHostnames == nil { js.Spec.Network.EnableDNSHostnames = ptr.To(true) @@ -97,16 +119,20 @@ func (js *JobSet) Default() { } if js.Spec.ManagedBy == nil { - js.Spec.ManagedBy = ptr.To(JobSetControllerName) + js.Spec.ManagedBy = ptr.To(jobset.JobSetControllerName) } + return nil } //+kubebuilder:webhook:path=/validate-jobset-x-k8s-io-v1alpha2-jobset,mutating=false,failurePolicy=fail,sideEffects=None,groups=jobset.x-k8s.io,resources=jobsets,verbs=create;update,versions=v1alpha2,name=vjobset.kb.io,admissionReviewVersions=v1 -var _ webhook.Validator = &JobSet{} - // ValidateCreate implements webhook.Validator so a webhook will be registered for the type -func (js *JobSet) ValidateCreate() (admission.Warnings, error) { +func (j *jobSetWebhook) ValidateCreate(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { + js, ok := obj.(*jobset.JobSet) + if !ok { + return nil, fmt.Errorf("expected a JobSet but got a %T", obj) + } + var allErrs []error // Validate that replicatedJobs listed in success policy are part of this JobSet. validReplicatedJobs := replicatedJobNamesFromSpec(js) @@ -128,6 +154,7 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) { } } + // Validate the managedBy field used for multi-kueue support. if js.Spec.ManagedBy != nil { manager := *js.Spec.ManagedBy fieldPath := field.NewPath("spec", "managedBy") @@ -139,6 +166,7 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) { } } + // Validate each replicatedJob. for _, rjob := range js.Spec.ReplicatedJobs { var parallelism int32 = 1 if rjob.Template.Spec.Parallelism != nil { @@ -147,6 +175,7 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) { if int64(parallelism)*int64(rjob.Replicas) > math.MaxInt32 { allErrs = append(allErrs, fmt.Errorf("the product of replicas and parallelism must not exceed %d for replicatedJob '%s'", math.MaxInt32, rjob.Name)) } + // Check that the generated job names for this replicated job will be DNS 1035 compliant. // Use the largest job index as it will have the longest name. longestJobName := placement.GenJobName(js.Name, rjob.Name, int(rjob.Replicas-1)) @@ -171,6 +200,8 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) { } } } + + // Validate the success policy's target replicated jobs are valid. for _, rjobName := range js.Spec.SuccessPolicy.TargetReplicatedJobs { if !collections.Contains(validReplicatedJobs, rjobName) { allErrs = append(allErrs, fmt.Errorf("invalid replicatedJob name '%s' does not appear in .spec.ReplicatedJobs", rjobName)) @@ -180,9 +211,16 @@ func (js *JobSet) ValidateCreate() (admission.Warnings, error) { } // ValidateUpdate implements webhook.Validator so a webhook will be registered for the type -func (js *JobSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) { +func (j *jobSetWebhook) ValidateUpdate(ctx context.Context, old, newObj runtime.Object) (admission.Warnings, error) { + js, ok := newObj.(*jobset.JobSet) + if !ok { + return nil, fmt.Errorf("expected a JobSet but got a %T", newObj) + } + oldJS, ok := old.(*jobset.JobSet) + if !ok { + return nil, fmt.Errorf("expected a JobSet from old object but got a %T", old) + } mungedSpec := js.Spec.DeepCopy() - oldJS := old.(*JobSet) if ptr.Deref(oldJS.Spec.Suspend, false) { for index := range js.Spec.ReplicatedJobs { mungedSpec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector = oldJS.Spec.ReplicatedJobs[index].Template.Spec.Template.Spec.NodeSelector @@ -195,7 +233,7 @@ func (js *JobSet) ValidateUpdate(old runtime.Object) (admission.Warnings, error) } // ValidateDelete implements webhook.Validator so a webhook will be registered for the type -func (js *JobSet) ValidateDelete() (admission.Warnings, error) { +func (j *jobSetWebhook) ValidateDelete(ctx context.Context, obj runtime.Object) (admission.Warnings, error) { return nil, nil } @@ -203,7 +241,7 @@ func completionModePtr(mode batchv1.CompletionMode) *batchv1.CompletionMode { return &mode } -func replicatedJobNamesFromSpec(js *JobSet) []string { +func replicatedJobNamesFromSpec(js *jobset.JobSet) []string { names := []string{} for _, rjob := range js.Spec.ReplicatedJobs { names = append(names, rjob.Name) diff --git a/api/jobset/v1alpha2/jobset_webhook_test.go b/pkg/webhooks/jobset_webhook_test.go similarity index 60% rename from api/jobset/v1alpha2/jobset_webhook_test.go rename to pkg/webhooks/jobset_webhook_test.go index 6d66275d..0a0627ca 100644 --- a/api/jobset/v1alpha2/jobset_webhook_test.go +++ b/pkg/webhooks/jobset_webhook_test.go @@ -1,6 +1,7 @@ -package v1alpha2 +package webhooks import ( + "context" "errors" "fmt" "math" @@ -15,6 +16,10 @@ import ( "k8s.io/apimachinery/pkg/util/validation" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/utils/ptr" + + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + jobset "sigs.k8s.io/jobset/api/jobset/v1alpha2" ) // TestPodTemplate is the default pod template spec used for testing. @@ -31,21 +36,21 @@ var TestPodTemplate = corev1.PodTemplateSpec{ } func TestJobSetDefaulting(t *testing.T) { - defaultSuccessPolicy := &SuccessPolicy{Operator: OperatorAll} - defaultStartupPolicy := &StartupPolicy{StartupPolicyOrder: AnyOrder} + defaultSuccessPolicy := &jobset.SuccessPolicy{Operator: jobset.OperatorAll} + defaultStartupPolicy := &jobset.StartupPolicy{StartupPolicyOrder: jobset.AnyOrder} testCases := []struct { name string - js *JobSet - want *JobSet + js *jobset.JobSet + want *jobset.JobSet }{ { name: "job completion mode is unset", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -54,15 +59,15 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -72,17 +77,17 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "job completion mode is set to non-indexed", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -92,15 +97,15 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -110,17 +115,17 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "enableDNSHostnames is unset", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - ReplicatedJobs: []ReplicatedJob{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -130,15 +135,15 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -148,18 +153,18 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "enableDNSHostnames is false", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(false)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(false)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -169,15 +174,15 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(false)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(false)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -187,18 +192,18 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "pod restart policy unset", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -210,15 +215,15 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -232,18 +237,18 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "pod restart policy set", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -257,15 +262,15 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -279,17 +284,17 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "success policy unset", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -303,15 +308,15 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ StartupPolicy: defaultStartupPolicy, SuccessPolicy: defaultSuccessPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -325,20 +330,20 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "success policy operator set, replicatedJobNames unset", - js: &JobSet{ - Spec: JobSetSpec{ - SuccessPolicy: &SuccessPolicy{ - Operator: OperatorAny, + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAny, }, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, StartupPolicy: defaultStartupPolicy, - ReplicatedJobs: []ReplicatedJob{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -352,17 +357,17 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, - want: &JobSet{ - Spec: JobSetSpec{ - SuccessPolicy: &SuccessPolicy{ - Operator: OperatorAny, + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAny, }, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -376,22 +381,22 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "startup policy order InOrder set", - js: &JobSet{ - Spec: JobSetSpec{ - SuccessPolicy: &SuccessPolicy{ - Operator: OperatorAny, + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAny, }, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - StartupPolicy: &StartupPolicy{ - StartupPolicyOrder: InOrder, + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + StartupPolicy: &jobset.StartupPolicy{ + StartupPolicyOrder: jobset.InOrder, }, - ReplicatedJobs: []ReplicatedJob{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -407,16 +412,16 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - want: &JobSet{ - Spec: JobSetSpec{ - SuccessPolicy: &SuccessPolicy{ - Operator: OperatorAny, + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAny, }, - StartupPolicy: &StartupPolicy{ - StartupPolicyOrder: InOrder, + StartupPolicy: &jobset.StartupPolicy{ + StartupPolicyOrder: jobset.InOrder, }, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -430,17 +435,17 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "managed-by label is unset", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -452,12 +457,12 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -467,17 +472,17 @@ func TestJobSetDefaulting(t *testing.T) { }, }, }, - ManagedBy: ptr.To(JobSetControllerName), + ManagedBy: ptr.To(jobset.JobSetControllerName), }, }, }, { name: "when provided, managed-by label is preserved", - js: &JobSet{ - Spec: JobSetSpec{ + js: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -490,12 +495,12 @@ func TestJobSetDefaulting(t *testing.T) { ManagedBy: ptr.To("other-controller"), }, }, - want: &JobSet{ - Spec: JobSetSpec{ + want: &jobset.JobSet{ + Spec: jobset.JobSetSpec{ SuccessPolicy: defaultSuccessPolicy, StartupPolicy: defaultStartupPolicy, - Network: &Network{EnableDNSHostnames: ptr.To(true)}, - ReplicatedJobs: []ReplicatedJob{ + Network: &jobset.Network{EnableDNSHostnames: ptr.To(true)}, + ReplicatedJobs: []jobset.ReplicatedJob{ { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ @@ -510,10 +515,18 @@ func TestJobSetDefaulting(t *testing.T) { }, }, } + fakeClient := fake.NewFakeClient() + webhook, err := NewJobSetWebhook(fakeClient) + if err != nil { + t.Fatalf("error creating jobset webhook: %v", err) + } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - tc.js.Default() - if diff := cmp.Diff(tc.want, tc.js); diff != "" { + obj := tc.js.DeepCopyObject() + if err := webhook.Default(context.TODO(), obj); err != nil { + t.Errorf("unexpected error defaulting jobset: %v", err) + } + if diff := cmp.Diff(tc.want, obj.(*jobset.JobSet)); diff != "" { t.Errorf("unexpected jobset defaulting: (-want/+got): %s", diff) } }) @@ -537,23 +550,39 @@ func TestValidateCreate(t *testing.T) { Name: "js", } + validPodTemplateSpec := corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "test", + Image: "bash:latest", + }, + }, + }, + } + testCases := []struct { name string - js *JobSet + js *jobset.JobSet want error }{ { name: "number of pods exceeds the limit", - js: &JobSet{ + js: &jobset.JobSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "JobSet", + APIVersion: "jobset.x-k8s.io/v1alpha2", + }, ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "test-jobset-replicated-job-0", Replicas: math.MaxInt32, Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ Parallelism: ptr.To[int32](math.MaxInt32), + Template: validPodTemplateSpec, }, }, }, @@ -563,11 +592,14 @@ func TestValidateCreate(t *testing.T) { Template: batchv1.JobTemplateSpec{ Spec: batchv1.JobSpec{ Parallelism: ptr.To[int32](math.MinInt32), + Template: validPodTemplateSpec, }, }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAll, + }, }, }, want: errors.Join( @@ -577,65 +609,88 @@ func TestValidateCreate(t *testing.T) { }, { name: "number of pods within the limit", - js: &JobSet{ + js: &jobset.JobSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "JobSet", + APIVersion: "jobset.x-k8s.io/v1alpha2", + }, ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "test-jobset-replicated-job-0", Replicas: 1, Template: batchv1.JobTemplateSpec{ - Spec: batchv1.JobSpec{}, + Spec: batchv1.JobSpec{ + Template: validPodTemplateSpec, + }, }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAll, + }, }, }, want: errors.Join(), }, { name: "success policy has non matching replicated job", - js: &JobSet{ + js: &jobset.JobSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "JobSet", + APIVersion: "jobset.x-k8s.io/v1alpha2", + }, ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "test-jobset-replicated-job-0", Replicas: 1, Template: batchv1.JobTemplateSpec{ - Spec: batchv1.JobSpec{}, + Spec: batchv1.JobSpec{ + Template: validPodTemplateSpec, + }, }, }, }, - SuccessPolicy: &SuccessPolicy{ - TargetReplicatedJobs: []string{"do not exist"}, + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAll, + TargetReplicatedJobs: []string{"does not exist"}, }, }, }, want: errors.Join( - fmt.Errorf("invalid replicatedJob name 'do not exist' does not appear in .spec.ReplicatedJobs"), + fmt.Errorf("invalid replicatedJob name 'does not exist' does not appear in .spec.ReplicatedJobs"), ), }, { name: "network has invalid dns name", - js: &JobSet{ + js: &jobset.JobSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "JobSet", + APIVersion: "jobset.x-k8s.io/v1alpha2", + }, ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ - Network: &Network{ + Spec: jobset.JobSetSpec{ + Network: &jobset.Network{ EnableDNSHostnames: ptr.To(true), Subdomain: strings.Repeat("a", 64), }, - ReplicatedJobs: []ReplicatedJob{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "test-jobset-replicated-job-0", Replicas: 1, Template: batchv1.JobTemplateSpec{ - Spec: batchv1.JobSpec{}, + Spec: batchv1.JobSpec{ + Template: validPodTemplateSpec, + }, }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAll, + }, }, }, want: errors.Join( @@ -644,19 +699,27 @@ func TestValidateCreate(t *testing.T) { }, { name: "jobset name with invalid character", - js: &JobSet{ + js: &jobset.JobSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "JobSet", + APIVersion: "jobset.x-k8s.io/v1alpha2", + }, ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "username.llama65b", Replicas: 1, Template: batchv1.JobTemplateSpec{ - Spec: batchv1.JobSpec{}, + Spec: batchv1.JobSpec{ + Template: validPodTemplateSpec, + }, }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAll, + }, }, }, want: errors.Join( @@ -665,21 +728,29 @@ func TestValidateCreate(t *testing.T) { }, { name: "jobset name will result in job name being too long", - js: &JobSet{ + js: &jobset.JobSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "JobSet", + APIVersion: "jobset.x-k8s.io/v1alpha2", + }, ObjectMeta: metav1.ObjectMeta{ Name: strings.Repeat("a", 62), }, - Spec: JobSetSpec{ - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "rj", Replicas: 101, Template: batchv1.JobTemplateSpec{ - Spec: batchv1.JobSpec{}, + Spec: batchv1.JobSpec{ + Template: validPodTemplateSpec, + }, }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAll, + }, }, }, want: errors.Join( @@ -688,12 +759,16 @@ func TestValidateCreate(t *testing.T) { }, { name: "jobset name will result in a pod name being too long", - js: &JobSet{ + js: &jobset.JobSet{ + TypeMeta: metav1.TypeMeta{ + Kind: "JobSet", + APIVersion: "jobset.x-k8s.io/v1alpha2", + }, ObjectMeta: metav1.ObjectMeta{ Name: strings.Repeat("a", 56), }, - Spec: JobSetSpec{ - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "rj", Replicas: 1, @@ -702,11 +777,14 @@ func TestValidateCreate(t *testing.T) { CompletionMode: ptr.To(batchv1.IndexedCompletion), Completions: ptr.To(int32(1)), Parallelism: ptr.To(int32(1)), + Template: validPodTemplateSpec, }, }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{ + Operator: jobset.OperatorAll, + }, }, }, want: errors.Join( @@ -715,11 +793,11 @@ func TestValidateCreate(t *testing.T) { }, { name: "jobset controller name is not a domain-prefixed path", - js: &JobSet{ + js: &jobset.JobSet{ ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ + Spec: jobset.JobSetSpec{ ManagedBy: ptr.To(notDomainPrefixedPathControllerName), - ReplicatedJobs: []ReplicatedJob{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "rj", Replicas: 1, @@ -732,7 +810,7 @@ func TestValidateCreate(t *testing.T) { }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{}, }, }, want: errors.Join( @@ -741,11 +819,11 @@ func TestValidateCreate(t *testing.T) { }, { name: "jobset controller name is too long", - js: &JobSet{ + js: &jobset.JobSet{ ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ + Spec: jobset.JobSetSpec{ ManagedBy: ptr.To(tooLongControllerName), - ReplicatedJobs: []ReplicatedJob{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "rj", Replicas: 1, @@ -758,7 +836,7 @@ func TestValidateCreate(t *testing.T) { }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{}, }, }, want: errors.Join( @@ -767,11 +845,11 @@ func TestValidateCreate(t *testing.T) { }, { name: "jobset controller name is set and valid", - js: &JobSet{ + js: &jobset.JobSet{ ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ - ManagedBy: ptr.To(JobSetControllerName), - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ManagedBy: ptr.To(jobset.JobSetControllerName), + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "rj", Replicas: 1, @@ -784,17 +862,17 @@ func TestValidateCreate(t *testing.T) { }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{}, }, }, want: errors.Join(), }, { name: "jobset controller name is unset", - js: &JobSet{ + js: &jobset.JobSet{ ObjectMeta: validObjectMeta, - Spec: JobSetSpec{ - ReplicatedJobs: []ReplicatedJob{ + Spec: jobset.JobSetSpec{ + ReplicatedJobs: []jobset.ReplicatedJob{ { Name: "rj", Replicas: 1, @@ -807,17 +885,27 @@ func TestValidateCreate(t *testing.T) { }, }, }, - SuccessPolicy: &SuccessPolicy{}, + SuccessPolicy: &jobset.SuccessPolicy{}, }, }, want: errors.Join(), }, } - + fakeClient := fake.NewFakeClient() + webhook, err := NewJobSetWebhook(fakeClient) + if err != nil { + t.Fatalf("error creating jobset webhook: %v", err) + } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - _, err := tc.js.ValidateCreate() - assert.Equal(t, err, tc.want) + _, err := webhook.ValidateCreate(context.TODO(), tc.js.DeepCopyObject()) + if err != nil && tc.want != nil { + assert.Contains(t, err.Error(), tc.want.Error()) + } else if err != nil && tc.want == nil { + t.Errorf("unexpected error: %v", err) + } else if err == nil && tc.want != nil { + t.Errorf("missing expected error: %v", tc.want) + } }) } } diff --git a/pkg/webhooks/pod_mutating_webhook.go b/pkg/webhooks/pod_mutating_webhook.go index 4a7a0671..e6bbb95f 100644 --- a/pkg/webhooks/pod_mutating_webhook.go +++ b/pkg/webhooks/pod_mutating_webhook.go @@ -37,8 +37,8 @@ type podWebhook struct { decoder *admission.Decoder } -func NewPodWebhook(mgr ctrl.Manager) *podWebhook { - return &podWebhook{client: mgr.GetClient()} +func NewPodWebhook(client client.Client) *podWebhook { + return &podWebhook{client: client} } // SetupWebhookWithManager configures the mutating webhook for pods. diff --git a/test/integration/webhook/suite_test.go b/test/integration/webhook/suite_test.go index 4d3cc888..c57f3bd0 100644 --- a/test/integration/webhook/suite_test.go +++ b/test/integration/webhook/suite_test.go @@ -112,11 +112,14 @@ var _ = BeforeSuite(func() { err = controllers.SetupJobSetIndexes(ctx, mgr.GetFieldIndexer()) Expect(err).NotTo(HaveOccurred()) - err = (&jobset.JobSet{}).SetupWebhookWithManager(mgr) + jobSetWebhook, err := webhooks.NewJobSetWebhook(mgr.GetClient()) + Expect(err).NotTo(HaveOccurred()) + + err = jobSetWebhook.SetupWebhookWithManager(mgr) Expect(err).NotTo(HaveOccurred()) // Set up pod webhook and indexes. - podWebhook := webhooks.NewPodWebhook(mgr) + podWebhook := webhooks.NewPodWebhook(mgr.GetClient()) err = controllers.SetupPodIndexes(ctx, mgr.GetFieldIndexer()) Expect(err).NotTo(HaveOccurred())