Merge pull request #299 from kerthcet/feat/add-webhook-to-resourceFlavor

Add the finalizer via webhook when creating resourceFlavor

apis/kueue/v1alpha1/resourceflavor_webhook.go

@@ -0,0 +1,73 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1
+
+import (
+	"k8s.io/apimachinery/pkg/apis/meta/v1/validation"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+	"k8s.io/klog/v2"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+)
+
+// log is for logging in this package.
+var resourceFlavorLog = ctrl.Log.WithName("resource-flavor-webhook")
+
+func (rf *ResourceFlavor) SetupWebhookWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewWebhookManagedBy(mgr).
+		For(rf).
+		Complete()
+}
+
+// +kubebuilder:webhook:path=/mutate-kueue-x-k8s-io-v1alpha1-resourceflavor,mutating=true,failurePolicy=fail,sideEffects=None,groups=kueue.x-k8s.io,resources=resourceflavors,verbs=create,versions=v1alpha1,name=mresourceflavor.kb.io,admissionReviewVersions=v1
+
+var _ webhook.Defaulter = &ResourceFlavor{}
+
+// Default implements webhook.Defaulter so a webhook will be registered for the type
+func (rf *ResourceFlavor) Default() {
+	resourceFlavorLog.Info("defaulter", "resourceFlavor", klog.KObj(rf))
+	if !controllerutil.ContainsFinalizer(rf, ResourceInUseFinalizerName) {
+		controllerutil.AddFinalizer(rf, ResourceInUseFinalizerName)
+	}
+}
+
+// +kubebuilder:webhook:path=/validate-kueue-x-k8s-io-v1alpha1-resourceflavor,mutating=false,failurePolicy=fail,sideEffects=None,groups=kueue.x-k8s.io,resources=resourceflavors,verbs=create;update,versions=v1alpha1,name=vresourceflavor.kb.io,admissionReviewVersions=v1
+
+var _ webhook.Validator = &ResourceFlavor{}
+
+// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
+func (rf *ResourceFlavor) ValidateCreate() error {
+	return ValidateResourceFlavorLabels(rf).ToAggregate()
+}
+
+// ValidateUpdate implements webhook.Validator so a webhook will be registered for the type
+func (rf *ResourceFlavor) ValidateUpdate(old runtime.Object) error {
+	return ValidateResourceFlavorLabels(rf).ToAggregate()
+}
+
+// ValidateDelete implements webhook.Validator so a webhook will be registered for the type
+func (rf *ResourceFlavor) ValidateDelete() error {
+	return nil
+}
+
+func ValidateResourceFlavorLabels(rf *ResourceFlavor) field.ErrorList {
+	field := field.NewPath("labels")
+	return validation.ValidateLabels(rf.Labels, field)
+
+}

apis/kueue/v1alpha1/resourceflavor_webhook_test.go

@@ -0,0 +1,72 @@
+/*
+Copyright 2022 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1alpha1_test
+
+// Rename the package to avoid circular dependencies which is caused by "sigs.k8s.io/kueue/pkg/util/testing".
+// See also: https://github.com/golang/go/wiki/CodeReviewComments#import-dot
+
+import (
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	"k8s.io/apimachinery/pkg/util/validation/field"
+
+	. "sigs.k8s.io/kueue/apis/kueue/v1alpha1"
+	utiltesting "sigs.k8s.io/kueue/pkg/util/testing"
+)
+
+func TestValidateResourceFlavorLabels(t *testing.T) {
+	testcases := []struct {
+		name    string
+		labels  map[string]string
+		wantErr field.ErrorList
+	}{
+		{
+			name:    "empty labels",
+			wantErr: field.ErrorList{},
+		},
+		{
+			name: "invalid label name",
+			wantErr: field.ErrorList{
+				field.Invalid(field.NewPath("labels"), nil, ""),
+			},
+			labels: map[string]string{
+				"foo@bar": "",
+			},
+		},
+		{
+			name: "invalid label value",
+			wantErr: field.ErrorList{
+				field.Invalid(field.NewPath("labels"), nil, ""),
+			},
+			labels: map[string]string{
+				"foo": "@abcdefg",
+			},
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			rf := utiltesting.MakeResourceFlavor("resource-flavor").MultiLabels(tc.labels).Obj()
+			gotErr := ValidateResourceFlavorLabels(rf)
+			if diff := cmp.Diff(tc.wantErr, gotErr, cmpopts.IgnoreFields(field.Error{}, "Detail", "BadValue")); diff != "" {
+				t.Errorf("validateResourceFlavorLabels() mismatch (-want +got):\n%s", diff)
+			}
+		})
+	}
+}

config/webhook/manifests.yaml

@@ -5,6 +5,25 @@ metadata:
   creationTimestamp: null
   name: mutating-webhook-configuration
 webhooks:
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /mutate-kueue-x-k8s-io-v1alpha1-resourceflavor
+  failurePolicy: Fail
+  name: mresourceflavor.kb.io
+  rules:
+  - apiGroups:
+    - kueue.x-k8s.io
+    apiVersions:
+    - v1alpha1
+    operations:
+    - CREATE
+    resources:
+    - resourceflavors
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
@@ -32,6 +51,26 @@ metadata:
   creationTimestamp: null
   name: validating-webhook-configuration
 webhooks:
+- admissionReviewVersions:
+  - v1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /validate-kueue-x-k8s-io-v1alpha1-resourceflavor
+  failurePolicy: Fail
+  name: vresourceflavor.kb.io
+  rules:
+  - apiGroups:
+    - kueue.x-k8s.io
+    apiVersions:
+    - v1alpha1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - resourceflavors
+  sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:

main.go

@@ -157,8 +157,8 @@ func setupControllers(mgr ctrl.Manager, cCache *cache.Cache, queues *queue.Manag
 		setupLog.Error(err, "unable to create controller", "controller", "Job")
 		os.Exit(1)
 	}
-	if err := (&kueuev1alpha1.Workload{}).SetupWebhookWithManager(mgr); err != nil {
-		setupLog.Error(err, "unable to create webhook", "webhook", "Workload")
+	if failedWebhook, err := core.SetupWebhooks(mgr); err != nil {
+		setupLog.Error(err, "Unable to create webhook", "webhook", failedWebhook)
 		os.Exit(1)
 	}
 	//+kubebuilder:scaffold:builder

pkg/controller/core/core.go

@@ -19,6 +19,7 @@ package core
 import (
 	ctrl "sigs.k8s.io/controller-runtime"
 
+	kueuev1alpha1 "sigs.k8s.io/kueue/apis/kueue/v1alpha1"
 	"sigs.k8s.io/kueue/pkg/cache"
 	"sigs.k8s.io/kueue/pkg/queue"
 )
@@ -45,3 +46,16 @@ func SetupControllers(mgr ctrl.Manager, qManager *queue.Manager, cc *cache.Cache
 	}
 	return "", nil
 }
+
+// SetupWebhooks sets up the webhooks for core controllers. It returns the name of the
+// webhook that failed to create and an error, if any.
+func SetupWebhooks(mgr ctrl.Manager) (string, error) {
+	if err := (&kueuev1alpha1.Workload{}).SetupWebhookWithManager(mgr); err != nil {
+		return "Workload", err
+	}
+
+	if err := (&kueuev1alpha1.ResourceFlavor{}).SetupWebhookWithManager(mgr); err != nil {
+		return "ResourceFlavor", err
+	}
+	return "", nil
+}

pkg/controller/core/resourceflavor_controller.go

@@ -69,6 +69,8 @@ func (r *ResourceFlavorReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 	log.V(2).Info("Reconciling ResourceFlavor")
 
 	if flavor.ObjectMeta.DeletionTimestamp.IsZero() {
+		// Although we'll add the finalizer via webhook mutation now, this is still useful
+		// as a fallback.
 		if !controllerutil.ContainsFinalizer(&flavor, kueue.ResourceInUseFinalizerName) {
 			controllerutil.AddFinalizer(&flavor, kueue.ResourceInUseFinalizerName)
 			if err := r.client.Update(ctx, &flavor); err != nil {

pkg/util/testing/wrappers.go

@@ -371,6 +371,14 @@ func (rf *ResourceFlavorWrapper) Obj() *kueue.ResourceFlavor {
 	return &rf.ResourceFlavor
 }
 
+// MultiLabels adds multi labels to the ResourceFlavor.
+func (rf *ResourceFlavorWrapper) MultiLabels(kv map[string]string) *ResourceFlavorWrapper {
+	for k, v := range kv {
+		rf.Labels[k] = v
+	}
+	return rf
+}
+
 // Label adds a label to the ResourceFlavor.
 func (rf *ResourceFlavorWrapper) Label(k, v string) *ResourceFlavorWrapper {
 	rf.Labels[k] = v

test/integration/controller/core/resourceflavor_controller_test.go

@@ -54,7 +54,7 @@ var _ = ginkgo.Describe("ResourceFlavor controller", func() {
 		})
 
 		ginkgo.AfterEach(func() {
-			gomega.Expect(framework.DeleteResourceFlavor(ctx, k8sClient, resourceFlavor)).To(gomega.Succeed())
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, resourceFlavor, true)
 		})
 
 		ginkgo.It("Should have finalizer in new created resourceFlavor", func() {

test/integration/controller/core/suite_test.go

@@ -53,6 +53,7 @@ var _ = ginkgo.BeforeSuite(func() {
 	fwk = &framework.Framework{
 		ManagerSetup: managerSetup,
 		CRDPath:      filepath.Join("..", "..", "..", "..", "config", "crd", "bases"),
+		WebhookPath:  filepath.Join("..", "..", "..", "..", "config", "webhook"),
 	}
 	ctx, cfg, k8sClient = fwk.Setup()
 })
@@ -68,6 +69,9 @@ func managerSetup(mgr manager.Manager, ctx context.Context) {
 	err = cache.SetupIndexes(mgr.GetFieldIndexer())
 	gomega.Expect(err).NotTo(gomega.HaveOccurred())
 
+	failedWebhook, err := core.SetupWebhooks(mgr)
+	gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook)
+
 	cCache := cache.New(mgr.GetClient())
 	queues := queue.NewManager(mgr.GetClient(), cCache)
 

test/integration/framework/framework.go

@@ -293,7 +293,7 @@ func ExpectClusterQueueToBeDeleted(ctx context.Context, k8sClient client.Client,
 	}, Timeout, Interval).Should(testing.BeNotFoundError())
 }
 
-func ExpectedResourceFlavorToBeDeleted(ctx context.Context, k8sClient client.Client, rf *kueue.ResourceFlavor, deleteRf bool) {
+func ExpectResourceFlavorToBeDeleted(ctx context.Context, k8sClient client.Client, rf *kueue.ResourceFlavor, deleteRf bool) {
 	if deleteRf {
 		gomega.Expect(DeleteResourceFlavor(ctx, k8sClient, rf)).ToNot(gomega.HaveOccurred())
 	}

test/integration/scheduler/scheduler_test.go

@@ -115,7 +115,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, prodClusterQ, true)
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, devClusterQ, true)
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 			gomega.Expect(framework.DeleteResourceFlavor(ctx, k8sClient, spotTaintedFlavor)).To(gomega.Succeed())
 			gomega.Expect(framework.DeleteResourceFlavor(ctx, k8sClient, spotUntaintedFlavor)).To(gomega.Succeed())
 		})
@@ -259,7 +259,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 		ginkgo.AfterEach(func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
 			gomega.Expect(framework.DeleteClusterQueue(ctx, k8sClient, cq)).To(gomega.Succeed())
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 			gomega.Expect(framework.DeleteResourceFlavor(ctx, k8sClient, spotTaintedFlavor)).To(gomega.Succeed())
 		})
 
@@ -491,9 +491,8 @@ var _ = ginkgo.Describe("Scheduler", func() {
 
 		ginkgo.AfterEach(func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
-			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, nsFoo)).To(gomega.Succeed())
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, cq, true)
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 		})
 
 		ginkgo.It("Should schedule jobs from the selected namespaces after label update", func() {
@@ -598,7 +597,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 		ginkgo.AfterEach(func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, cq, true)
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 			gomega.Expect(framework.DeleteResourceFlavor(ctx, k8sClient, spotTaintedFlavor)).To(gomega.Succeed())
 		})
 
@@ -662,7 +661,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 		ginkgo.AfterEach(func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, cq, true)
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 			gomega.Expect(framework.DeleteResourceFlavor(ctx, k8sClient, spotUntaintedFlavor)).To(gomega.Succeed())
 		})
 
@@ -713,7 +712,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, prodBEClusterQ, true)
 			gomega.Expect(framework.DeleteClusterQueue(ctx, k8sClient, devBEClusterQ)).ToNot(gomega.HaveOccurred())
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 			gomega.Expect(framework.DeleteResourceFlavor(ctx, k8sClient, spotTaintedFlavor)).To(gomega.Succeed())
 		})
 
@@ -844,7 +843,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 		ginkgo.AfterEach(func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, cq, true)
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 		})
 
 		ginkgo.It("Should admit two small workloads after a big one finishes", func() {
@@ -897,7 +896,7 @@ var _ = ginkgo.Describe("Scheduler", func() {
 		ginkgo.AfterEach(func() {
 			gomega.Expect(framework.DeleteNamespace(ctx, k8sClient, ns)).To(gomega.Succeed())
 			framework.ExpectClusterQueueToBeDeleted(ctx, k8sClient, strictFIFOClusterQ, true)
-			framework.ExpectedResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
+			framework.ExpectResourceFlavorToBeDeleted(ctx, k8sClient, onDemandFlavor, true)
 		})
 
 		ginkgo.It("Should schedule workloads by their priority strictly in StrictFIFO", func() {

test/integration/scheduler/suite_test.go

@@ -56,6 +56,7 @@ var _ = ginkgo.BeforeSuite(func() {
 	fwk = &framework.Framework{
 		ManagerSetup: managerAndSchedulerSetup,
 		CRDPath:      filepath.Join("..", "..", "..", "config", "crd", "bases"),
+		WebhookPath:  filepath.Join("..", "..", "..", "config", "webhook"),
 	}
 	ctx, cfg, k8sClient = fwk.Setup()
 })
@@ -77,6 +78,9 @@ func managerAndSchedulerSetup(mgr manager.Manager, ctx context.Context) {
 	failedCtrl, err := core.SetupControllers(mgr, queues, cCache)
 	gomega.Expect(err).ToNot(gomega.HaveOccurred(), "controller", failedCtrl)
 
+	failedWebhook, err := core.SetupWebhooks(mgr)
+	gomega.Expect(err).ToNot(gomega.HaveOccurred(), "webhook", failedWebhook)
+
 	err = workloadjob.SetupIndexes(mgr.GetFieldIndexer())
 	gomega.Expect(err).NotTo(gomega.HaveOccurred())
 	err = workloadjob.NewReconciler(mgr.GetScheme(), mgr.GetClient(),