diff --git a/pkg/controller/modeladapter/modeladapter_controller.go b/pkg/controller/modeladapter/modeladapter_controller.go index bd0ac70c..e2c82f8b 100644 --- a/pkg/controller/modeladapter/modeladapter_controller.go +++ b/pkg/controller/modeladapter/modeladapter_controller.go @@ -335,20 +335,6 @@ func (r *ModelAdapterReconciler) DoReconcile(ctx context.Context, req ctrl.Reque } oldInstance := instance.DeepCopy() - // Step 0: Validate ModelAdapter configurations - if err := validateModelAdapter(instance); err != nil { - klog.Error(err, "Failed to validate the ModelAdapter") - instance.Status.Phase = modelv1alpha1.ModelAdapterFailed - condition := NewCondition(string(modelv1alpha1.ModelAdapterPending), metav1.ConditionFalse, - ValidationFailedReason, "ModelAdapter resource is not valid") - // TODO: no need to update the status if the status remain the same - if updateErr := r.updateStatus(ctx, instance, condition); updateErr != nil { - klog.ErrorS(err, "Failed to update ModelAdapter status", "modelAdapter", klog.KObj(instance)) - return reconcile.Result{}, updateErr - } - - return ctrl.Result{}, err - } // Step 1: Schedule Pod for ModelAdapter selectedPod := &corev1.Pod{} diff --git a/pkg/controller/modeladapter/utils.go b/pkg/controller/modeladapter/utils.go index 48ddfde9..a4f66674 100644 --- a/pkg/controller/modeladapter/utils.go +++ b/pkg/controller/modeladapter/utils.go @@ -23,46 +23,10 @@ import ( "os" "strings" - modelv1alpha1 "github.com/vllm-project/aibrix/api/model/v1alpha1" "github.com/vllm-project/aibrix/pkg/config" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -func validateModelAdapter(instance *modelv1alpha1.ModelAdapter) error { - if instance.Spec.ArtifactURL == "" { - return fmt.Errorf("artifactURL is required") - } - - if instance.Spec.PodSelector == nil { - return fmt.Errorf("podSelector is required") - } - - if _, err := url.ParseRequestURI(instance.Spec.ArtifactURL); err != nil { - return fmt.Errorf("artifactURL is not a valid URL: %v", err) - } - - if err := validateArtifactURL(instance.Spec.ArtifactURL); err != nil { - return err - } - - if instance.Spec.Replicas != nil && *instance.Spec.Replicas <= 0 { - return fmt.Errorf("replicas must be greater than 0") - } - - return nil -} - -// validateArtifactURL checks if the ArtifactURL has a valid schema (s3://, gcs://, huggingface://, https://, /) -func validateArtifactURL(artifactURL string) error { - allowedSchemes := []string{"s3://", "gcs://", "huggingface://", "hf://", "/"} - for _, scheme := range allowedSchemes { - if strings.HasPrefix(artifactURL, scheme) { - return nil - } - } - return fmt.Errorf("artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://") -} - func equalStringSlices(a, b []string) bool { if len(a) != len(b) { return false diff --git a/pkg/controller/modeladapter/utils_test.go b/pkg/controller/modeladapter/utils_test.go index 426d92e9..8c5afbba 100644 --- a/pkg/controller/modeladapter/utils_test.go +++ b/pkg/controller/modeladapter/utils_test.go @@ -22,119 +22,10 @@ import ( "testing" "github.com/stretchr/testify/assert" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/utils/ptr" - modelv1alpha1 "github.com/vllm-project/aibrix/api/model/v1alpha1" "github.com/vllm-project/aibrix/pkg/config" ) -// Test for validateModelAdapter function -func TestValidateModelAdapter(t *testing.T) { - // Case 1: All valid fields - t.Run("valid input", func(t *testing.T) { - instance := &modelv1alpha1.ModelAdapter{ - Spec: modelv1alpha1.ModelAdapterSpec{ - ArtifactURL: "s3://bucket/path/to/artifact", - PodSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"app": "test"}, - }, - Replicas: ptr.To(int32(1)), - }, - } - - err := validateModelAdapter(instance) - assert.NoError(t, err) - }) - - // Case 2: Missing ArtifactURL - t.Run("missing ArtifactURL", func(t *testing.T) { - instance := &modelv1alpha1.ModelAdapter{ - Spec: modelv1alpha1.ModelAdapterSpec{ - ArtifactURL: "", - PodSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"app": "test"}, - }, - }, - } - - err := validateModelAdapter(instance) - assert.EqualError(t, err, "artifactURL is required") - }) - - // Case 3: Invalid ArtifactURL format - t.Run("invalid ArtifactURL", func(t *testing.T) { - instance := &modelv1alpha1.ModelAdapter{ - Spec: modelv1alpha1.ModelAdapterSpec{ - ArtifactURL: "ftp://bucket/path/to/artifact", - PodSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"app": "test"}, - }, - }, - } - - err := validateModelAdapter(instance) - assert.EqualError(t, err, "artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://") - }) - - // Case 4: Missing PodSelector - t.Run("missing PodSelector", func(t *testing.T) { - instance := &modelv1alpha1.ModelAdapter{ - Spec: modelv1alpha1.ModelAdapterSpec{ - ArtifactURL: "s3://bucket/path/to/artifact", - PodSelector: nil, - }, - } - - err := validateModelAdapter(instance) - assert.EqualError(t, err, "podSelector is required") - }) - - // Case 5: Invalid Replicas - t.Run("invalid Replicas", func(t *testing.T) { - replicas := int32(0) - instance := &modelv1alpha1.ModelAdapter{ - Spec: modelv1alpha1.ModelAdapterSpec{ - ArtifactURL: "s3://bucket/path/to/artifact", - PodSelector: &metav1.LabelSelector{ - MatchLabels: map[string]string{"app": "test"}, - }, - Replicas: &replicas, - }, - } - - err := validateModelAdapter(instance) - assert.EqualError(t, err, "replicas must be greater than 0") - }) -} - -// Test for validateArtifactURL function -func TestValidateArtifactURL(t *testing.T) { - // Case 1: Valid s3 URL - t.Run("valid s3 URL", func(t *testing.T) { - err := validateArtifactURL("s3://bucket/path") - assert.NoError(t, err) - }) - - // Case 2: Valid gcs URL - t.Run("valid gcs URL", func(t *testing.T) { - err := validateArtifactURL("gcs://bucket/path") - assert.NoError(t, err) - }) - - // Case 3: Valid huggingface URL - t.Run("valid huggingface URL", func(t *testing.T) { - err := validateArtifactURL("huggingface://path/to/model") - assert.NoError(t, err) - }) - - // Case 4: Invalid scheme - t.Run("invalid scheme", func(t *testing.T) { - err := validateArtifactURL("ftp://bucket/path") - assert.EqualError(t, err, "artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://") - }) -} - // Test for equalStringSlices function func TestEqualStringSlices(t *testing.T) { // Case 1: Equal slices diff --git a/pkg/utils/modeladapter.go b/pkg/utils/modeladapter.go new file mode 100644 index 00000000..49634aba --- /dev/null +++ b/pkg/utils/modeladapter.go @@ -0,0 +1,34 @@ +/* +Copyright 2025 The Aibrix Team. + +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 utils + +import ( + "fmt" + "strings" +) + +var AllowedSchemas = []string{"s3://", "gcs://", "huggingface://", "hf://", "/"} + +// ValidateArtifactURL checks if the ArtifactURL has a valid schema (s3://, gcs://, huggingface://, https://, /) +func ValidateArtifactURL(artifactURL string) error { + for _, schema := range AllowedSchemas { + if strings.HasPrefix(artifactURL, schema) { + return nil + } + } + return fmt.Errorf("unsupported schema") +} diff --git a/pkg/utils/modeladapter_test.go b/pkg/utils/modeladapter_test.go new file mode 100644 index 00000000..1569a0f6 --- /dev/null +++ b/pkg/utils/modeladapter_test.go @@ -0,0 +1,34 @@ +package utils + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// Test for ValidateArtifactURL function +func TestValidateArtifactURL(t *testing.T) { + // Case 1: Valid s3 URL + t.Run("valid s3 URL", func(t *testing.T) { + err := ValidateArtifactURL("s3://bucket/path") + assert.NoError(t, err) + }) + + // Case 2: Valid gcs URL + t.Run("valid gcs URL", func(t *testing.T) { + err := ValidateArtifactURL("gcs://bucket/path") + assert.NoError(t, err) + }) + + // Case 3: Valid huggingface URL + t.Run("valid huggingface URL", func(t *testing.T) { + err := ValidateArtifactURL("huggingface://path/to/model") + assert.NoError(t, err) + }) + + // Case 4: Invalid scheme + t.Run("invalid scheme", func(t *testing.T) { + err := ValidateArtifactURL("ftp://bucket/path") + assert.EqualError(t, err, "artifactURL must start with one of the following schemes: s3://, gcs://, huggingface://") + }) +} diff --git a/pkg/webhook/modeladapter_webhook.go b/pkg/webhook/modeladapter_webhook.go index 97e74da4..e0a7f391 100644 --- a/pkg/webhook/modeladapter_webhook.go +++ b/pkg/webhook/modeladapter_webhook.go @@ -28,6 +28,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook/admission" modelapi "github.com/vllm-project/aibrix/api/model/v1alpha1" + "github.com/vllm-project/aibrix/pkg/utils" ) type ModelAdapterWebhook struct{} @@ -65,6 +66,14 @@ func (w *ModelAdapterWebhook) ValidateCreate(ctx context.Context, obj runtime.Ob allErrs = append(allErrs, field.Invalid(specPath.Child("artifactURL"), adapter.Spec.ArtifactURL, fmt.Sprintf("artifactURL is invalid: %v", err))) } + if adapter.Spec.Replicas != nil && *adapter.Spec.Replicas <= 0 { + allErrs = append(allErrs, field.Invalid(specPath.Child("replicas"), adapter.Spec.Replicas, "replicas must be greater than 0")) + } + + if err := utils.ValidateArtifactURL(adapter.Spec.ArtifactURL); err != nil { + allErrs = append(allErrs, field.NotSupported(specPath.Child("artifactURL"), adapter.Spec.ArtifactURL, utils.AllowedSchemas)) + } + return nil, allErrs.ToAggregate() } diff --git a/test/integration/webhook/modeladapter_test.go b/test/integration/webhook/modeladapter_test.go index 7e99abdc..7582f88c 100644 --- a/test/integration/webhook/modeladapter_test.go +++ b/test/integration/webhook/modeladapter_test.go @@ -21,6 +21,7 @@ import ( "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/ptr" modelapi "github.com/vllm-project/aibrix/api/model/v1alpha1" ) @@ -60,6 +61,22 @@ var _ = ginkgo.Describe("modelAdapter default and validation", func() { gomega.Expect(k8sClient.Create(ctx, tc.adapter())).To(gomega.Succeed()) } }, + ginkgo.Entry("normal creation", &testValidatingCase{ + adapter: func() *modelapi.ModelAdapter { + adapter := modelapi.ModelAdapter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-adapter", + Namespace: ns.Name, + }, + Spec: modelapi.ModelAdapterSpec{ + ArtifactURL: "s3://test-bucket/test-model", + PodSelector: &metav1.LabelSelector{}, + }, + } + return &adapter + }, + failed: false, + }), ginkgo.Entry("adapter creation with invalid artifactURL should be failed", &testValidatingCase{ adapter: func() *modelapi.ModelAdapter { adapter := modelapi.ModelAdapter{ @@ -76,5 +93,70 @@ var _ = ginkgo.Describe("modelAdapter default and validation", func() { }, failed: true, }), + ginkgo.Entry("adapter creation with empty artifactURL should be failed", &testValidatingCase{ + adapter: func() *modelapi.ModelAdapter { + adapter := modelapi.ModelAdapter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-adapter", + Namespace: ns.Name, + }, + Spec: modelapi.ModelAdapterSpec{ + ArtifactURL: "", + PodSelector: &metav1.LabelSelector{}, + }, + } + return &adapter + }, + failed: true, + }), + ginkgo.Entry("adapter creation with empty podSelector should be failed", &testValidatingCase{ + adapter: func() *modelapi.ModelAdapter { + adapter := modelapi.ModelAdapter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-adapter", + Namespace: ns.Name, + }, + Spec: modelapi.ModelAdapterSpec{ + ArtifactURL: "s3://test-bucket/test-model", + PodSelector: &metav1.LabelSelector{}, + Replicas: ptr.To[int32](0), + }, + } + return &adapter + }, + failed: true, + }), + ginkgo.Entry("adapter creation with replicas less than 0 should be failed", &testValidatingCase{ + adapter: func() *modelapi.ModelAdapter { + adapter := modelapi.ModelAdapter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-adapter", + Namespace: ns.Name, + }, + Spec: modelapi.ModelAdapterSpec{ + ArtifactURL: "s3://test-bucket/test-model", + PodSelector: nil, + }, + } + return &adapter + }, + failed: true, + }), + ginkgo.Entry("adapter creation with unsupported schema should be failed", &testValidatingCase{ + adapter: func() *modelapi.ModelAdapter { + adapter := modelapi.ModelAdapter{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-adapter", + Namespace: ns.Name, + }, + Spec: modelapi.ModelAdapterSpec{ + ArtifactURL: "ms://", + PodSelector: &metav1.LabelSelector{}, + }, + } + return &adapter + }, + failed: true, + }), ) })