From be4560cd384e7f96aaeba90baf69b9b9ca9248e0 Mon Sep 17 00:00:00 2001 From: Jesse Claven Date: Tue, 12 Sep 2023 14:13:52 +0100 Subject: [PATCH 1/2] refactor(crds): Use built-in OpenAPI validation Part of a progressive change to use the CRD validation that's available via kubebuilder [1]. This has the benefits of us: - Not having to write the code for the validation, since it's generated/handled via OpenAPI - It's structurally/programatically available in the spec - Less repetition [1] https://book.kubebuilder.io/reference/markers/crd-validation.html --- .../apis/mlops/v1alpha1/pipeline_types.go | 83 ++++--- .../crd/bases/mlops.seldon.io_pipelines.yaml | 40 ++-- operator/controllers/mlops/suite_test.go | 210 +++++++++++++++++- 3 files changed, 287 insertions(+), 46 deletions(-) diff --git a/operator/apis/mlops/v1alpha1/pipeline_types.go b/operator/apis/mlops/v1alpha1/pipeline_types.go index c5c66364ea..a12850febe 100644 --- a/operator/apis/mlops/v1alpha1/pipeline_types.go +++ b/operator/apis/mlops/v1alpha1/pipeline_types.go @@ -25,42 +25,77 @@ import ( "github.com/seldonio/seldon-core/apis/go/v2/mlops/scheduler" ) +//+kubebuilder:object:root=true +//+kubebuilder:subresource:status +//+kubebuilder:resource:shortName=mlp + +// Pipeline is the Schema for the pipelines API +type Pipeline struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Spec PipelineSpec `json:"spec,omitempty"` + Status PipelineStatus `json:"status,omitempty"` +} + +//+kubebuilder:object:root=true + +// PipelineList contains a list of Pipeline +type PipelineList struct { + metav1.TypeMeta `json:",inline"` + metav1.ListMeta `json:"metadata,omitempty"` + + Items []Pipeline `json:"items"` +} + // PipelineSpec defines the desired state of Pipeline type PipelineSpec struct { // External inputs to this pipeline, optional Input *PipelineInput `json:"input,omitempty"` + // The steps of this inference graph pipeline Steps []PipelineStep `json:"steps"` + // Synchronous output from this pipeline, optional Output *PipelineOutput `json:"output,omitempty"` } +// +kubebuilder:validation:Enum=inner;outer;any type JoinType string const ( + // data must be available from all inputs JoinTypeInner JoinType = "inner" + // data will include any data from any inputs at end of window JoinTypeOuter JoinType = "outer" - JoinTypeAny JoinType = "any" + // first data input that arrives will be forwarded + JoinTypeAny JoinType = "any" ) type PipelineStep struct { // Name of the step + // +kubebuilder:validation:MinLength=1 + // +kubebuilder:validation:MaxLength=253 + // +kubebuilder:validation:Pattern=`^[a-z0-9][a-z0-9\-\.]+[a-z0-9]+$` Name string `json:"name"` + // Previous step to receive data from Inputs []string `json:"inputs,omitempty"` + // msecs to wait for messages from multiple inputs to arrive before joining the inputs JoinWindowMs *uint32 `json:"joinWindowMs,omitempty"` + // Map of tensor name conversions to use e.g. output1 -> input1 TensorMap map[string]string `json:"tensorMap,omitempty"` + // Triggers required to activate step Triggers []string `json:"triggers,omitempty"` - // One of inner (default), outer, or any - // inner - do an inner join: data must be available from all inputs - // outer - do an outer join: data will include any data from any inputs at end of window - // any - first data input that arrives will be forwarded + + // +kubebuilder:default=inner InputsJoinType *JoinType `json:"inputsJoinType,omitempty"` - // One of inner (default), outer, or any (see above for details) + TriggersJoinType *JoinType `json:"triggersJoinType,omitempty"` + // Batch size of request required before data will be sent to this step Batch *PipelineBatch `json:"batch,omitempty"` } @@ -74,14 +109,19 @@ type PipelineBatch struct { type PipelineInput struct { // Previous external pipeline steps to receive data from ExternalInputs []string `json:"externalInputs,omitempty"` + // Triggers required to activate inputs ExternalTriggers []string `json:"externalTriggers,omitempty"` + // msecs to wait for messages from multiple inputs to arrive before joining the inputs JoinWindowMs *uint32 `json:"joinWindowMs,omitempty"` - // One of inner (default), outer, or any (see above for details) + + // +kubebuilder:default=inner JoinType *JoinType `json:"joinType,omitempty"` - // One of inner (default), outer, or any (see above for details) + + // +kubebuilder:default=inner TriggersJoinType *JoinType `json:"triggersJoinType,omitempty"` + // Map of tensor name conversions to use e.g. output1 -> input1 TensorMap map[string]string `json:"tensorMap,omitempty"` } @@ -89,10 +129,13 @@ type PipelineInput struct { type PipelineOutput struct { // Previous step to receive data from Steps []string `json:"steps,omitempty"` + // msecs to wait for messages from multiple inputs to arrive before joining the inputs JoinWindowMs uint32 `json:"joinWindowMs,omitempty"` - // One of inner (default), outer, or any (see above for details) + + // +kubebuilder:default=inner StepsJoin *JoinType `json:"stepsJoin,omitempty"` + // Map of tensor name conversions to use e.g. output1 -> input1 TensorMap map[string]string `json:"tensorMap,omitempty"` } @@ -102,28 +145,6 @@ type PipelineStatus struct { duckv1.Status `json:",inline"` } -//+kubebuilder:object:root=true -//+kubebuilder:subresource:status -//+kubebuilder:resource:shortName=mlp - -// Pipeline is the Schema for the pipelines API -type Pipeline struct { - metav1.TypeMeta `json:",inline"` - metav1.ObjectMeta `json:"metadata,omitempty"` - - Spec PipelineSpec `json:"spec,omitempty"` - Status PipelineStatus `json:"status,omitempty"` -} - -//+kubebuilder:object:root=true - -// PipelineList contains a list of Pipeline -type PipelineList struct { - metav1.TypeMeta `json:",inline"` - metav1.ListMeta `json:"metadata,omitempty"` - Items []Pipeline `json:"items"` -} - func init() { SchemeBuilder.Register(&Pipeline{}, &PipelineList{}) } diff --git a/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml b/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml index dbfa516cc3..773ac74aa6 100644 --- a/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml +++ b/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml @@ -52,8 +52,11 @@ spec: type: string type: array joinType: - description: One of inner (default), outer, or any (see above - for details) + default: inner + enum: + - inner + - outer + - any type: string joinWindowMs: description: msecs to wait for messages from multiple inputs to @@ -67,8 +70,11 @@ spec: -> input1 type: object triggersJoinType: - description: One of inner (default), outer, or any (see above - for details) + default: inner + enum: + - inner + - outer + - any type: string type: object output: @@ -85,8 +91,11 @@ spec: type: string type: array stepsJoin: - description: One of inner (default), outer, or any (see above - for details) + default: inner + enum: + - inner + - outer + - any type: string tensorMap: additionalProperties: @@ -118,11 +127,11 @@ spec: type: string type: array inputsJoinType: - description: 'One of inner (default), outer, or any inner - - do an inner join: data must be available from all inputs outer - - do an outer join: data will include any data from any inputs - at end of window any - first data input that arrives will - be forwarded' + default: inner + enum: + - inner + - outer + - any type: string joinWindowMs: description: msecs to wait for messages from multiple inputs @@ -131,6 +140,9 @@ spec: type: integer name: description: Name of the step + maxLength: 253 + minLength: 1 + pattern: ^[a-z0-9][a-z0-9\-\.]+[a-z0-9]+$ type: string tensorMap: additionalProperties: @@ -144,8 +156,10 @@ spec: type: string type: array triggersJoinType: - description: One of inner (default), outer, or any (see above - for details) + enum: + - inner + - outer + - any type: string required: - name diff --git a/operator/controllers/mlops/suite_test.go b/operator/controllers/mlops/suite_test.go index c85ce52acf..6daf53d0db 100644 --- a/operator/controllers/mlops/suite_test.go +++ b/operator/controllers/mlops/suite_test.go @@ -17,12 +17,18 @@ limitations under the License. package mlops import ( + "context" + "fmt" + "net/http" "path/filepath" + "strings" "testing" //+kubebuilder:scaffold:imports . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/client" @@ -44,9 +50,11 @@ var testEnv *envtest.Environment func TestAPIs(t *testing.T) { RegisterFailHandler(Fail) - RunSpecsWithDefaultAndCustomReporters(t, + RunSpecsWithDefaultAndCustomReporters( + t, "Controller Suite", - []Reporter{printer.NewlineReporter{}}) + []Reporter{printer.NewlineReporter{}}, + ) } var _ = BeforeSuite(func() { @@ -78,3 +86,201 @@ var _ = AfterSuite(func() { err := testEnv.Stop() Expect(err).NotTo(HaveOccurred()) }) + +var _ = Describe("Controller", func() { + const ( + Namespace = "default" + ) + + Context("When creating a Pipeline", func() { + It("Rejects an invalid Spec", func() { + By("By returning an error") + ctx := context.Background() + + pipelineName := "-test-pipeline" + strings.Repeat("b", 252) + + pipeline := &mlopsv1alpha1.Pipeline{ + TypeMeta: metav1.TypeMeta{APIVersion: "batch.tutorial.kubebuilder.io/v1", Kind: "Pipeline"}, + ObjectMeta: metav1.ObjectMeta{Name: pipelineName, Namespace: Namespace}, + Spec: mlopsv1alpha1.PipelineSpec{}, + Status: mlopsv1alpha1.PipelineStatus{}, + } + + expectedError := &apierrors.StatusError{ + ErrStatus: metav1.Status{ + TypeMeta: metav1.TypeMeta{Kind: "", APIVersion: ""}, + ListMeta: metav1.ListMeta{ + SelfLink: "", + ResourceVersion: "", + Continue: "", + RemainingItemCount: nil, + }, + Status: "Failure", + Message: fmt.Sprintf("Pipeline.mlops.seldon.io \"%s\" is invalid: [metadata.name: Invalid value: \"%s\": must be no more than 253 characters, metadata.name: Invalid value: \"%s\": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*'), spec.steps: Required value]", pipelineName, pipelineName, pipelineName), + Reason: metav1.StatusReasonInvalid, + Details: &metav1.StatusDetails{ + Name: pipelineName, + Group: "mlops.seldon.io", + Kind: "Pipeline", + UID: "", + Causes: []metav1.StatusCause{ + { + Type: metav1.CauseTypeFieldValueInvalid, + Message: fmt.Sprintf("Invalid value: \"%s\": must be no more than 253 characters", pipelineName), + Field: "metadata.name", + }, + { + Type: metav1.CauseTypeFieldValueInvalid, + Message: fmt.Sprintf("Invalid value: \"%s\": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')", pipelineName), + Field: "metadata.name", + }, + { + Type: metav1.CauseTypeFieldValueRequired, + Message: "Required value", + Field: "spec.steps", + }, + }, + RetryAfterSeconds: 0, + }, + Code: http.StatusUnprocessableEntity, + }, + } + + Expect(k8sClient.Create(ctx, pipeline)).Should(MatchError(expectedError)) + }) + + It("Accepts a valid Spec", func() { + By("By returning nil") + ctx := context.Background() + + pipeline := &mlopsv1alpha1.Pipeline{ + TypeMeta: metav1.TypeMeta{APIVersion: "batch.tutorial.kubebuilder.io/v1", Kind: "Pipeline"}, + ObjectMeta: metav1.ObjectMeta{Name: "test-pipeline-valid", Namespace: Namespace}, + Spec: mlopsv1alpha1.PipelineSpec{ + Steps: []mlopsv1alpha1.PipelineStep{ + { + Name: "step-1", + }, + }, + }, + Status: mlopsv1alpha1.PipelineStatus{}, + } + + Expect(k8sClient.Create(ctx, pipeline)).Should(Succeed()) + }) + + It("Retrieves a pipeline by name", func() { + By("By fetching the pipeline by name") + ctx := context.Background() + pipelineName := "test-pipeline-valid" + + retrievedPipeline := &mlopsv1alpha1.Pipeline{} + + // Default value, as per kubebuilder annotation + expectedInputsJoinType := mlopsv1alpha1.JoinTypeInner + + expectedPipeline := + mlopsv1alpha1.PipelineSpec{ + Steps: []mlopsv1alpha1.PipelineStep{ + { + Name: "step-1", + Inputs: nil, + JoinWindowMs: nil, + TensorMap: nil, + Triggers: nil, + InputsJoinType: &expectedInputsJoinType, + TriggersJoinType: nil, + Batch: nil, + }}, + } + + err := k8sClient.Get( + ctx, client.ObjectKey{Name: pipelineName, Namespace: Namespace}, + retrievedPipeline, + ) + Expect(err).To(BeNil()) + + Expect(retrievedPipeline.Spec).To(Equal(expectedPipeline)) + }) + }) + + Context("When creating a Model", func() { + It("Accepts a valid Spec", func() { + By("By returning nil") + ctx := context.Background() + + model := &mlopsv1alpha1.Model{ + TypeMeta: metav1.TypeMeta{APIVersion: "batch.tutorial.kubebuilder.io/v1", Kind: "Model"}, + ObjectMeta: metav1.ObjectMeta{Name: "test-model-valid", Namespace: Namespace}, + Spec: mlopsv1alpha1.ModelSpec{}, + Status: mlopsv1alpha1.ModelStatus{}, + } + + Expect(k8sClient.Create(ctx, model)).Should(Succeed()) + }) + + // This relies on the previous test having run + It("Rejects a Spec with the name of a model that already exists", func() { + By("By returning an error") + ctx := context.Background() + model := &mlopsv1alpha1.Model{ + TypeMeta: metav1.TypeMeta{APIVersion: "batch.tutorial.kubebuilder.io/v1", Kind: "Model"}, + ObjectMeta: metav1.ObjectMeta{Name: "test-model-valid", Namespace: Namespace}, + Spec: mlopsv1alpha1.ModelSpec{}, + Status: mlopsv1alpha1.ModelStatus{}, + } + + expectedError := &apierrors.StatusError{ + ErrStatus: metav1.Status{ + TypeMeta: metav1.TypeMeta{Kind: "", APIVersion: ""}, + ListMeta: metav1.ListMeta{ + SelfLink: "", + ResourceVersion: "", + Continue: "", + RemainingItemCount: nil, + }, + Status: "Failure", + Message: "models.mlops.seldon.io \"test-model-valid\" already exists", + Reason: metav1.StatusReasonAlreadyExists, + Details: &metav1.StatusDetails{ + Name: "test-model-valid", + Group: "mlops.seldon.io", + Kind: "models", + UID: "", + Causes: nil, + RetryAfterSeconds: 0, + }, + Code: http.StatusConflict, + }, + } + + Expect(k8sClient.Create(ctx, model)).Should(MatchError(expectedError)) + }) + + It("Retrieves a model by name", func() { + By("By fetching the model by name") + ctx := context.Background() + modelName := "test-model-valid" // Replace with the actual name you want to retrieve + + retrievedModel := &mlopsv1alpha1.Model{} + + expectedModel := + mlopsv1alpha1.ModelSpec{ + ScalingSpec: mlopsv1alpha1.ScalingSpec{ + Replicas: nil, + MinReplicas: nil, + MaxReplicas: nil, + }, + } + + // Fetch the model by name + err := k8sClient.Get( + ctx, client.ObjectKey{Name: modelName, Namespace: Namespace}, + retrievedModel, + ) + Expect(err).To(BeNil()) + + Expect(retrievedModel.Spec).To(Equal(expectedModel)) + }) + }) +}) From 898db26549e7c8d6974ef4bcc59fa9c5c35a4dd4 Mon Sep 17 00:00:00 2001 From: Jesse Claven Date: Mon, 18 Sep 2023 18:32:42 +0100 Subject: [PATCH 2/2] fixup! refactor(crds): Use built-in OpenAPI validation --- operator/apis/mlops/v1alpha1/pipeline_types.go | 3 --- operator/config/crd/bases/mlops.seldon.io_pipelines.yaml | 3 --- 2 files changed, 6 deletions(-) diff --git a/operator/apis/mlops/v1alpha1/pipeline_types.go b/operator/apis/mlops/v1alpha1/pipeline_types.go index a12850febe..6944203316 100644 --- a/operator/apis/mlops/v1alpha1/pipeline_types.go +++ b/operator/apis/mlops/v1alpha1/pipeline_types.go @@ -74,9 +74,6 @@ const ( type PipelineStep struct { // Name of the step - // +kubebuilder:validation:MinLength=1 - // +kubebuilder:validation:MaxLength=253 - // +kubebuilder:validation:Pattern=`^[a-z0-9][a-z0-9\-\.]+[a-z0-9]+$` Name string `json:"name"` // Previous step to receive data from diff --git a/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml b/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml index 773ac74aa6..ce6f5d7f4e 100644 --- a/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml +++ b/operator/config/crd/bases/mlops.seldon.io_pipelines.yaml @@ -140,9 +140,6 @@ spec: type: integer name: description: Name of the step - maxLength: 253 - minLength: 1 - pattern: ^[a-z0-9][a-z0-9\-\.]+[a-z0-9]+$ type: string tensorMap: additionalProperties: