From dd3df217be481caea5de1a02360a44bf5721c175 Mon Sep 17 00:00:00 2001 From: Dhiraj Bokde Date: Fri, 13 Sep 2024 09:02:37 -0700 Subject: [PATCH] feat: added shared registries namespace property in model registry dsc component, fixes RHOAIENG-12335 (#1221) * feat: added shared registries namespace property in model registry dsc component, fixes RHOAIENG-12335 Signed-off-by: Dhiraj Bokde * fix: added registriesNamespace property in references to model registry component in docs, samples, etc. Signed-off-by: Dhiraj Bokde * fix: updated api-overview.md Signed-off-by: Dhiraj Bokde * fix: add missing field in setupDSCInstance() Signed-off-by: Dhiraj Bokde * fix: added debug log for failing e2e test * debug: added log messages in test dsc creation Signed-off-by: Dhiraj Bokde * debug: make registriesNamespace omitempty so apiserver will set the default value, clean testDSCCreation with more log messages Signed-off-by: Dhiraj Bokde * update: modelregistry with namespace - add namespace for rhoai case - remove omitemptry on namespace: if user does not set it, it will get default value to use. since its type is string Signed-off-by: Wen Zhou * fix: test need to update for modelreg namespace Signed-off-by: Wen Zhou * update: cleanup debug info in test cases Signed-off-by: Wen Zhou * fix: make registriesNamespace immutability check dynamic in cel validation rule * fix: refactor validation rule, set registriesNamespace to optional, add patch func in modelregistry to set default value * fix: add defaulting webhook including envtest, remove patch from modelregistry component * fix: add namespace name pattern for registriesNamespace, allow empty string to replace with default value * fix: use unique webhook names * fix: don't change existing validating webhook name to avoid upgrade issues * feat: add model registry registriesNamespace in DSC status * update: - move ModelRegistryStatus from ModelReg into status - update validation rule for namespace: - when update is to removed/unmanaged modelreg: regardless previous state, can change name and set to removed - upgrade 2.13 to 2.14 when no NS set in 2.13 - when update is from remove/unmanaged: e.g change from removed to managed and change name at the same tie - keep same namespace, regardless state - reduce print on passed allowed webhook request - set to omitemptry so required is not add in the CSV - fix review comments: - move update status after reconcile is successful - set to use const for caching on ModeReg namespace Signed-off-by: Wen Zhou --------- Signed-off-by: Dhiraj Bokde Signed-off-by: Wen Zhou Co-authored-by: Wen Zhou --- PROJECT | 1 + README.md | 1 + .../v1/datasciencecluster_types.go | 11 +++ .../v1/zz_generated.deepcopy.go | 22 ++++++ ...er.opendatahub.io_datascienceclusters.yaml | 24 +++++++ ...atahub-operator.clusterserviceversion.yaml | 23 ++++++- components/modelregistry/modelregistry.go | 23 +++++-- ...er.opendatahub.io_datascienceclusters.yaml | 24 +++++++ ...asciencecluster_v1_datasciencecluster.yaml | 1 + config/webhook/kustomizeconfig.yaml | 7 ++ config/webhook/manifests.yaml | 26 +++++++ .../datasciencecluster_controller.go | 12 ++++ controllers/status/status.go | 5 ++ controllers/webhook/webhook.go | 55 +++++++++++++-- controllers/webhook/webhook_suite_test.go | 16 ++++- docs/DESIGN.md | 1 + docs/api-overview.md | 20 +++++- docs/upgrade-testing.md | 1 + main.go | 13 ++-- tests/e2e/creation_test.go | 68 ++++++++++++++++--- tests/e2e/helper_test.go | 2 - 21 files changed, 328 insertions(+), 28 deletions(-) diff --git a/PROJECT b/PROJECT index 8619f2be939..e321d617330 100644 --- a/PROJECT +++ b/PROJECT @@ -34,6 +34,7 @@ resources: path: github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1 version: v1 webhooks: + defaulting: true validation: true webhookVersion: v1 version: "3" diff --git a/README.md b/README.md index 294b406a009..4496f1bee72 100644 --- a/README.md +++ b/README.md @@ -327,6 +327,7 @@ spec: managementState: Managed modelregistry: managementState: Managed + registriesNamespace: "odh-model-registries" ray: managementState: Managed trainingoperator: diff --git a/apis/datasciencecluster/v1/datasciencecluster_types.go b/apis/datasciencecluster/v1/datasciencecluster_types.go index 58d680e70c9..39166651ac1 100644 --- a/apis/datasciencecluster/v1/datasciencecluster_types.go +++ b/apis/datasciencecluster/v1/datasciencecluster_types.go @@ -36,6 +36,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/components/trainingoperator" "github.com/opendatahub-io/opendatahub-operator/v2/components/trustyai" "github.com/opendatahub-io/opendatahub-operator/v2/components/workbenches" + "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" ) @@ -86,6 +87,12 @@ type Components struct { TrainingOperator trainingoperator.TrainingOperator `json:"trainingoperator,omitempty"` } +// ComponentsStatus defines the custom status of DataScienceCluster components. +type ComponentsStatus struct { + // ModelRegistry component status + ModelRegistry *status.ModelRegistryStatus `json:"modelregistry,omitempty"` +} + // DataScienceClusterStatus defines the observed state of DataScienceCluster. type DataScienceClusterStatus struct { // Phase describes the Phase of DataScienceCluster reconciliation state @@ -105,6 +112,10 @@ type DataScienceClusterStatus struct { // List of components with status if installed or not InstalledComponents map[string]bool `json:"installedComponents,omitempty"` + // Expose component's specific status + // +optional + Components ComponentsStatus `json:"components,omitempty"` + // Version and release type Release cluster.Release `json:"release,omitempty"` } diff --git a/apis/datasciencecluster/v1/zz_generated.deepcopy.go b/apis/datasciencecluster/v1/zz_generated.deepcopy.go index cf315a2ee0a..f089d70e9ee 100644 --- a/apis/datasciencecluster/v1/zz_generated.deepcopy.go +++ b/apis/datasciencecluster/v1/zz_generated.deepcopy.go @@ -21,6 +21,7 @@ limitations under the License. package v1 import ( + "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" corev1 "k8s.io/api/core/v1" runtime "k8s.io/apimachinery/pkg/runtime" @@ -52,6 +53,26 @@ func (in *Components) DeepCopy() *Components { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ComponentsStatus) DeepCopyInto(out *ComponentsStatus) { + *out = *in + if in.ModelRegistry != nil { + in, out := &in.ModelRegistry, &out.ModelRegistry + *out = new(status.ModelRegistryStatus) + **out = **in + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ComponentsStatus. +func (in *ComponentsStatus) DeepCopy() *ComponentsStatus { + if in == nil { + return nil + } + out := new(ComponentsStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DataScienceCluster) DeepCopyInto(out *DataScienceCluster) { *out = *in @@ -149,6 +170,7 @@ func (in *DataScienceClusterStatus) DeepCopyInto(out *DataScienceClusterStatus) (*out)[key] = val } } + in.Components.DeepCopyInto(&out.Components) in.Release.DeepCopyInto(&out.Release) } diff --git a/bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml b/bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml index 761d550a5dc..da6d20509c6 100644 --- a/bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml +++ b/bundle/manifests/datasciencecluster.opendatahub.io_datascienceclusters.yaml @@ -443,7 +443,21 @@ spec: - Removed pattern: ^(Managed|Unmanaged|Force|Removed)$ type: string + registriesNamespace: + default: odh-model-registries + description: Namespace for model registries to be installed, + configurable only once when model registry is enabled, defaults + to "odh-model-registries" + maxLength: 63 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$ + type: string type: object + x-kubernetes-validations: + - message: RegistriesNamespace is immutable when model registry + is Managed + rule: (self.managementState != 'Managed') || (oldSelf.registriesNamespace + == '') || (oldSelf.managementState != 'Managed')|| (self.registriesNamespace + == oldSelf.registriesNamespace) ray: description: Ray component configuration. properties: @@ -633,6 +647,16 @@ spec: status: description: DataScienceClusterStatus defines the observed state of DataScienceCluster. properties: + components: + description: Expose component's specific status + properties: + modelregistry: + description: ModelRegistry component status + properties: + registriesNamespace: + type: string + type: object + type: object conditions: description: Conditions describes the state of the DataScienceCluster resource. diff --git a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml index d255f894420..e7311a196f6 100644 --- a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml @@ -47,7 +47,8 @@ metadata: "managementState": "Managed" }, "modelregistry": { - "managementState": "Managed" + "managementState": "Managed", + "registriesNamespace": "odh-model-registries" }, "ray": { "managementState": "Managed" @@ -1217,6 +1218,26 @@ spec: component: opendatahub-operator version: 2.17.0 webhookdefinitions: + - admissionReviewVersions: + - v1 + containerPort: 443 + deploymentName: opendatahub-operator-controller-manager + failurePolicy: Fail + generateName: mutate.operator.opendatahub.io + rules: + - apiGroups: + - datasciencecluster.opendatahub.io + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - datascienceclusters + sideEffects: None + targetPort: 9443 + type: MutatingAdmissionWebhook + webhookPath: /mutate-opendatahub-io-v1 - admissionReviewVersions: - v1 containerPort: 443 diff --git a/components/modelregistry/modelregistry.go b/components/modelregistry/modelregistry.go index 5d0610019e5..f05cbdd734a 100644 --- a/components/modelregistry/modelregistry.go +++ b/components/modelregistry/modelregistry.go @@ -29,22 +29,33 @@ import ( const DefaultModelRegistryCert = "default-modelregistry-cert" var ( - ComponentName = "model-registry-operator" - Path = deploy.DefaultManifestPath + "/" + ComponentName + "/overlays/odh" + ComponentName = "model-registry-operator" + DefaultModelRegistriesNamespace = "odh-model-registries" + Path = deploy.DefaultManifestPath + "/" + ComponentName + "/overlays/odh" // we should not apply this label to the namespace, as it triggered namspace deletion during operator uninstall // modelRegistryLabels = cluster.WithLabels( // labels.ODH.OwnedNamespace, "true", // ). - ModelRegistriesNamespace = "odh-model-registries" ) // Verifies that ModelRegistry implements ComponentInterface. var _ components.ComponentInterface = (*ModelRegistry)(nil) // ModelRegistry struct holds the configuration for the ModelRegistry component. +// The property `registriesNamespace` is immutable when `managementState` is `Managed` + // +kubebuilder:object:generate=true +// +kubebuilder:validation:XValidation:rule="(self.managementState != 'Managed') || (oldSelf.registriesNamespace == '') || (oldSelf.managementState != 'Managed')|| (self.registriesNamespace == oldSelf.registriesNamespace)",message="RegistriesNamespace is immutable when model registry is Managed" +//nolint:lll + type ModelRegistry struct { components.Component `json:""` + + // Namespace for model registries to be installed, configurable only once when model registry is enabled, defaults to "odh-model-registries" + // +kubebuilder:default="odh-model-registries" + // +kubebuilder:validation:Pattern="^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$" + // +kubebuilder:validation:MaxLength=63 + RegistriesNamespace string `json:"registriesNamespace,omitempty"` } func (m *ModelRegistry) OverrideManifests(ctx context.Context, _ cluster.Platform) error { @@ -109,17 +120,17 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien // Create model registries namespace // We do not delete this namespace even when ModelRegistry is Removed or when operator is uninstalled. - ns, err := cluster.CreateNamespace(ctx, cli, ModelRegistriesNamespace) + ns, err := cluster.CreateNamespace(ctx, cli, m.RegistriesNamespace) if err != nil { return err } - l.Info("created model registry namespace", "namespace", ModelRegistriesNamespace) + l.Info("created model registry namespace", "namespace", m.RegistriesNamespace) // create servicemeshmember here, for now until post MVP solution err = enrollToServiceMesh(ctx, cli, dscispec, ns) if err != nil { return err } - l.Info("created model registry servicemesh member", "namespace", ModelRegistriesNamespace) + l.Info("created model registry servicemesh member", "namespace", m.RegistriesNamespace) } else { err := m.removeDependencies(ctx, cli, dscispec) if err != nil { diff --git a/config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml b/config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml index 327c95af25a..41d1c76d658 100644 --- a/config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml +++ b/config/crd/bases/datasciencecluster.opendatahub.io_datascienceclusters.yaml @@ -443,7 +443,21 @@ spec: - Removed pattern: ^(Managed|Unmanaged|Force|Removed)$ type: string + registriesNamespace: + default: odh-model-registries + description: Namespace for model registries to be installed, + configurable only once when model registry is enabled, defaults + to "odh-model-registries" + maxLength: 63 + pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$ + type: string type: object + x-kubernetes-validations: + - message: RegistriesNamespace is immutable when model registry + is Managed + rule: (self.managementState != 'Managed') || (oldSelf.registriesNamespace + == '') || (oldSelf.managementState != 'Managed')|| (self.registriesNamespace + == oldSelf.registriesNamespace) ray: description: Ray component configuration. properties: @@ -633,6 +647,16 @@ spec: status: description: DataScienceClusterStatus defines the observed state of DataScienceCluster. properties: + components: + description: Expose component's specific status + properties: + modelregistry: + description: ModelRegistry component status + properties: + registriesNamespace: + type: string + type: object + type: object conditions: description: Conditions describes the state of the DataScienceCluster resource. diff --git a/config/samples/datasciencecluster_v1_datasciencecluster.yaml b/config/samples/datasciencecluster_v1_datasciencecluster.yaml index ed5971d37e6..626fc5442d1 100644 --- a/config/samples/datasciencecluster_v1_datasciencecluster.yaml +++ b/config/samples/datasciencecluster_v1_datasciencecluster.yaml @@ -42,3 +42,4 @@ spec: managementState: "Managed" modelregistry: managementState: "Managed" + registriesNamespace: "odh-model-registries" diff --git a/config/webhook/kustomizeconfig.yaml b/config/webhook/kustomizeconfig.yaml index e809f78208e..25e21e3c963 100644 --- a/config/webhook/kustomizeconfig.yaml +++ b/config/webhook/kustomizeconfig.yaml @@ -4,11 +4,18 @@ nameReference: - kind: Service version: v1 fieldSpecs: + - kind: MutatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/name - kind: ValidatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/name namespace: +- kind: MutatingWebhookConfiguration + group: admissionregistration.k8s.io + path: webhooks/clientConfig/service/namespace + create: true - kind: ValidatingWebhookConfiguration group: admissionregistration.k8s.io path: webhooks/clientConfig/service/namespace diff --git a/config/webhook/manifests.yaml b/config/webhook/manifests.yaml index 3dcfb31f3e3..9c1174d5173 100644 --- a/config/webhook/manifests.yaml +++ b/config/webhook/manifests.yaml @@ -1,5 +1,31 @@ --- apiVersion: admissionregistration.k8s.io/v1 +kind: MutatingWebhookConfiguration +metadata: + name: mutating-webhook-configuration +webhooks: +- admissionReviewVersions: + - v1 + clientConfig: + service: + name: webhook-service + namespace: system + path: /mutate-opendatahub-io-v1 + failurePolicy: Fail + name: mutate.operator.opendatahub.io + rules: + - apiGroups: + - datasciencecluster.opendatahub.io + apiVersions: + - v1 + operations: + - CREATE + - UPDATE + resources: + - datascienceclusters + sideEffects: None +--- +apiVersion: admissionregistration.k8s.io/v1 kind: ValidatingWebhookConfiguration metadata: name: validating-webhook-configuration diff --git a/controllers/datasciencecluster/datasciencecluster_controller.go b/controllers/datasciencecluster/datasciencecluster_controller.go index 223251e3fee..f968117dbff 100644 --- a/controllers/datasciencecluster/datasciencecluster_controller.go +++ b/controllers/datasciencecluster/datasciencecluster_controller.go @@ -53,6 +53,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/components" "github.com/opendatahub-io/opendatahub-operator/v2/components/datasciencepipelines" + "github.com/opendatahub-io/opendatahub-operator/v2/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" annotations "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" @@ -316,6 +317,8 @@ func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context } err = component.ReconcileComponent(ctx, r.Client, r.Log, instance, r.DataScienceCluster.DSCISpec, platform, installedComponentValue) + // TODO: replace this hack with a full refactor of component status in the future + if err != nil { // reconciliation failed: log errors, raise event and update status accordingly instance = r.reportError(err, instance, "failed to reconcile "+componentName+" on DataScienceCluster") @@ -343,6 +346,15 @@ func (r *DataScienceClusterReconciler) reconcileSubComponent(ctx context.Context } else { status.RemoveComponentCondition(&saved.Status.Conditions, componentName) } + + // TODO: replace this hack with a full refactor of component status in the future + if mr, isMR := component.(*modelregistry.ModelRegistry); isMR { + if enabled { + saved.Status.Components.ModelRegistry = &status.ModelRegistryStatus{RegistriesNamespace: mr.RegistriesNamespace} + } else { + saved.Status.Components.ModelRegistry = nil + } + } }) if err != nil { instance = r.reportError(err, instance, "failed to update DataScienceCluster status after reconciling "+componentName) diff --git a/controllers/status/status.go b/controllers/status/status.go index 52e3d79145f..808cfee2f7b 100644 --- a/controllers/status/status.go +++ b/controllers/status/status.go @@ -209,3 +209,8 @@ func SetComponentCondition(conditions *[]conditionsv1.Condition, component strin func RemoveComponentCondition(conditions *[]conditionsv1.Condition, component string) { conditionsv1.RemoveStatusCondition(conditions, conditionsv1.ConditionType(component+ReadySuffix)) } + +// ModelRegistryStatus struct holds the status for the ModelRegistry component. +type ModelRegistryStatus struct { + RegistriesNamespace string `json:"registriesNamespace,omitempty"` +} diff --git a/controllers/webhook/webhook.go b/controllers/webhook/webhook.go index 26034d8b305..dfda6d0fa32 100644 --- a/controllers/webhook/webhook.go +++ b/controllers/webhook/webhook.go @@ -29,6 +29,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/webhook" "sigs.k8s.io/controller-runtime/pkg/webhook/admission" + dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" ) @@ -37,12 +39,12 @@ var log = ctrl.Log.WithName("odh-controller-webhook") //+kubebuilder:webhook:path=/validate-opendatahub-io-v1,mutating=false,failurePolicy=fail,sideEffects=None,groups=datasciencecluster.opendatahub.io;dscinitialization.opendatahub.io,resources=datascienceclusters;dscinitializations,verbs=create;delete,versions=v1,name=operator.opendatahub.io,admissionReviewVersions=v1 //nolint:lll -type OpenDataHubWebhook struct { +type OpenDataHubValidatingWebhook struct { Client client.Client Decoder *admission.Decoder } -func (w *OpenDataHubWebhook) SetupWithManager(mgr ctrl.Manager) { +func (w *OpenDataHubValidatingWebhook) SetupWithManager(mgr ctrl.Manager) { hookServer := mgr.GetWebhookServer() odhWebhook := &webhook.Admission{ Handler: w, @@ -74,7 +76,7 @@ func denyCountGtZero(ctx context.Context, cli client.Client, gvk schema.GroupVer return admission.Allowed("") } -func (w *OpenDataHubWebhook) checkDupCreation(ctx context.Context, req admission.Request) admission.Response { +func (w *OpenDataHubValidatingWebhook) checkDupCreation(ctx context.Context, req admission.Request) admission.Response { switch req.Kind.Kind { case "DataScienceCluster", "DSCInitialization": default: @@ -93,7 +95,7 @@ func (w *OpenDataHubWebhook) checkDupCreation(ctx context.Context, req admission fmt.Sprintf("Only one instance of %s object is allowed", req.Kind.Kind)) } -func (w *OpenDataHubWebhook) checkDeletion(ctx context.Context, req admission.Request) admission.Response { +func (w *OpenDataHubValidatingWebhook) checkDeletion(ctx context.Context, req admission.Request) admission.Response { if req.Kind.Kind == "DataScienceCluster" { return admission.Allowed("") } @@ -103,7 +105,7 @@ func (w *OpenDataHubWebhook) checkDeletion(ctx context.Context, req admission.Re fmt.Sprintln("Cannot delete DSCI object when DSC object still exists")) } -func (w *OpenDataHubWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { +func (w *OpenDataHubValidatingWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { var resp admission.Response resp.Allowed = true // initialize Allowed to be true in case Operation falls into "default" case @@ -122,3 +124,46 @@ func (w *OpenDataHubWebhook) Handle(ctx context.Context, req admission.Request) return admission.Allowed(fmt.Sprintf("Operation %s on %s allowed", req.Operation, req.Kind.Kind)) } + +//+kubebuilder:webhook:path=/mutate-opendatahub-io-v1,mutating=true,failurePolicy=fail,sideEffects=None,groups=datasciencecluster.opendatahub.io,resources=datascienceclusters,verbs=create;update,versions=v1,name=mutate.operator.opendatahub.io,admissionReviewVersions=v1 +//nolint:lll + +// OpenDataHubMutatingWebhook is a mutating webhook +// It currently only sets defaults for modelregiestry in datascienceclusters. +type OpenDataHubMutatingWebhook struct { + Client client.Client + Decoder *admission.Decoder +} + +func (w *OpenDataHubMutatingWebhook) SetupWithManager(mgr ctrl.Manager) { + hookServer := mgr.GetWebhookServer() + odhWebhook := &webhook.Admission{ + Handler: w, + } + hookServer.Register("/mutate-opendatahub-io-v1", odhWebhook) +} + +func (w *OpenDataHubMutatingWebhook) Handle(ctx context.Context, req admission.Request) admission.Response { + var resp admission.Response + resp.Allowed = true // initialize Allowed to be true in case Operation falls into "default" case + + dsc := &dscv1.DataScienceCluster{} + err := w.Decoder.Decode(req, dsc) + if err != nil { + return admission.Denied(fmt.Sprintf("failed to decode request body: %s", err)) + } + resp = w.setDSCDefaults(ctx, dsc) + return resp +} + +func (w *OpenDataHubMutatingWebhook) setDSCDefaults(_ context.Context, dsc *dscv1.DataScienceCluster) admission.Response { + // set default registriesNamespace if empty + if len(dsc.Spec.Components.ModelRegistry.RegistriesNamespace) == 0 { + return admission.Patched("Property registriesNamespace set to default value", webhook.JSONPatchOp{ + Operation: "replace", + Path: "spec.components.modelregistry.registriesNamespace", + Value: modelregistry.DefaultModelRegistriesNamespace, + }) + } + return admission.Allowed("") +} diff --git a/controllers/webhook/webhook_suite_test.go b/controllers/webhook/webhook_suite_test.go index 9558c7e8cf0..529ee3f5313 100644 --- a/controllers/webhook/webhook_suite_test.go +++ b/controllers/webhook/webhook_suite_test.go @@ -48,6 +48,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/components/datasciencepipelines" "github.com/opendatahub-io/opendatahub-operator/v2/components/kserve" "github.com/opendatahub-io/opendatahub-operator/v2/components/modelmeshserving" + "github.com/opendatahub-io/opendatahub-operator/v2/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/components/ray" "github.com/opendatahub-io/opendatahub-operator/v2/components/trustyai" "github.com/opendatahub-io/opendatahub-operator/v2/components/workbenches" @@ -132,7 +133,12 @@ var _ = BeforeSuite(func() { }) Expect(err).NotTo(HaveOccurred()) - (&webhook.OpenDataHubWebhook{ + (&webhook.OpenDataHubValidatingWebhook{ + Client: mgr.GetClient(), + Decoder: admission.NewDecoder(mgr.GetScheme()), + }).SetupWithManager(mgr) + + (&webhook.OpenDataHubMutatingWebhook{ Client: mgr.GetClient(), Decoder: admission.NewDecoder(mgr.GetScheme()), }).SetupWithManager(mgr) @@ -202,6 +208,14 @@ var _ = Describe("DSC/DSCI webhook", func() { Expect(k8sClient.Delete(ctx, dscInstance)).Should(Succeed()) Expect(k8sClient.Delete(ctx, dsciInstance)).Should(Succeed()) }) + + It("Should set defaults in DSC instance", func(ctx context.Context) { + dscInstance := newDSC(nameBase+"-dsc-1", namespace) + Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed()) + Expect(dscInstance.Spec.Components.ModelRegistry.RegistriesNamespace). + Should(Equal(modelregistry.DefaultModelRegistriesNamespace)) + Expect(clearInstance(ctx, dscInstance)).Should(Succeed()) + }) }) func clearInstance(ctx context.Context, instance client.Object) error { diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 0e82ac0acb4..148517713b8 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -64,6 +64,7 @@ To deploy ODH components seamlessly, ODH operator will watch two CRDs: managementState: Managed modelregistry: managementState: Managed + registriesNamespace: "odh-model-registries" ray: managementState: Managed kueue: diff --git a/docs/api-overview.md b/docs/api-overview.md index c909cee2b11..0f0f7030595 100644 --- a/docs/api-overview.md +++ b/docs/api-overview.md @@ -241,7 +241,7 @@ Package modelregistry provides utility functions to config ModelRegistry, an ML -ModelRegistry struct holds the configuration for the ModelRegistry component. + @@ -251,6 +251,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | | `Component` _[Component](#component)_ | | | | +| `registriesNamespace` _string_ | Namespace for model registries to be installed, configurable only once when model registry is enabled, defaults to "odh-model-registries" | odh-model-registries | MaxLength: 63
Pattern: `^([a-z0-9]([-a-z0-9]*[a-z0-9])?)?$`
| @@ -412,6 +413,22 @@ _Appears in:_ | `trainingoperator` _[TrainingOperator](#trainingoperator)_ | Training Operator component configuration. | | | +#### ComponentsStatus + + + +ComponentsStatus defines the custom status of DataScienceCluster components. + + + +_Appears in:_ +- [DataScienceClusterStatus](#datascienceclusterstatus) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `modelregistry` _[ModelRegistryStatus](#modelregistrystatus)_ | ModelRegistry component status | | | + + #### ControlPlaneSpec @@ -485,6 +502,7 @@ _Appears in:_ | `relatedObjects` _[ObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.25/#objectreference-v1-core) array_ | RelatedObjects is a list of objects created and maintained by this operator.
Object references will be added to this list after they have been created AND found in the cluster. | | | | `errorMessage` _string_ | | | | | `installedComponents` _object (keys:string, values:boolean)_ | List of components with status if installed or not | | | +| `components` _[ComponentsStatus](#componentsstatus)_ | Expose component's specific status | | | | `release` _[Release](#release)_ | Version and release type | | | diff --git a/docs/upgrade-testing.md b/docs/upgrade-testing.md index ce9bd1ef0e4..d40d38f9418 100644 --- a/docs/upgrade-testing.md +++ b/docs/upgrade-testing.md @@ -138,6 +138,7 @@ spec: managementState: Managed modelregistry: managementState: Managed + registriesNamespace: "odh-model-registries" ray: managementState: Managed kueue: diff --git a/main.go b/main.go index 073723e4336..44966275e1b 100644 --- a/main.go +++ b/main.go @@ -57,6 +57,7 @@ import ( dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" featurev1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/features/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/certconfigmapgenerator" dscctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/datasciencecluster" dscictrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/dscinitialization" @@ -207,7 +208,12 @@ func main() { //nolint:funlen,maintidx os.Exit(1) } - (&webhook.OpenDataHubWebhook{ + (&webhook.OpenDataHubValidatingWebhook{ + Client: mgr.GetClient(), + Decoder: admission.NewDecoder(mgr.GetScheme()), + }).SetupWithManager(mgr) + + (&webhook.OpenDataHubMutatingWebhook{ Client: mgr.GetClient(), Decoder: admission.NewDecoder(mgr.GetScheme()), }).SetupWithManager(mgr) @@ -345,13 +351,12 @@ func createDeploymentCacheConfig(platform cluster.Platform) map[string]cache.Con case cluster.ManagedRhods: // no need workbench NS, only SFS no Deployment namespaceConfigs["redhat-ods-monitoring"] = cache.Config{} namespaceConfigs["redhat-ods-applications"] = cache.Config{} - //TODO: if ModelReg has a RHOAI NS case cluster.SelfManagedRhods: namespaceConfigs["redhat-ods-applications"] = cache.Config{} - //TODO: if ModelReg has a RHOAI NS default: namespaceConfigs["opendatahub"] = cache.Config{} - namespaceConfigs["odh-model-registries"] = cache.Config{} } + // for modelregistry namespace + namespaceConfigs[modelregistry.DefaultModelRegistriesNamespace] = cache.Config{} return namespaceConfigs } diff --git a/tests/e2e/creation_test.go b/tests/e2e/creation_test.go index b4406b8ee27..2a2abc3cd2b 100644 --- a/tests/e2e/creation_test.go +++ b/tests/e2e/creation_test.go @@ -59,7 +59,7 @@ func creationTestSuite(t *testing.T) { // DSC t.Run("Creation of DataScienceCluster instance", func(t *testing.T) { - err = testCtx.testDSCCreation() + err = testCtx.testDSCCreation(t) require.NoError(t, err, "error creating DataScienceCluster instance") }) t.Run("Creation of more than one of DataScienceCluster instance", func(t *testing.T) { @@ -91,6 +91,10 @@ func creationTestSuite(t *testing.T) { }) // ModelReg + t.Run("Validate model registry cert config", func(t *testing.T) { + err = testCtx.validateModelRegistryConfig() + require.NoError(t, err, "error validating ModelRegistry config") + }) t.Run("Validate default model registry cert available", func(t *testing.T) { err = testCtx.testDefaultModelRegistryCertAvailable() require.NoError(t, err, "error getting default cert secret for ModelRegistry") @@ -150,22 +154,22 @@ func (tc *testContext) testDSCICreation() error { return nil } -func (tc *testContext) testDSCCreation() error { +func (tc *testContext) testDSCCreation(t *testing.T) error { + t.Helper() // Create DataScienceCluster resource if not already created - dscLookupKey := types.NamespacedName{Name: tc.testDsc.Name} - createdDSC := &dscv1.DataScienceCluster{} existingDSCList := &dscv1.DataScienceClusterList{} - err := tc.customClient.List(tc.ctx, existingDSCList) if err == nil { if len(existingDSCList.Items) > 0 { // Use DSC instance if it already exists tc.testDsc = &existingDSCList.Items[0] - return nil } } + + dscLookupKey := types.NamespacedName{Name: tc.testDsc.Name} + createdDSC := &dscv1.DataScienceCluster{} err = tc.customClient.Get(tc.ctx, dscLookupKey, createdDSC) if err != nil { if k8serr.IsNotFound(err) { @@ -291,7 +295,7 @@ func (tc *testContext) testAllComponentCreation(t *testing.T) error { //nolint:f t.Run("Validate "+name, func(t *testing.T) { t.Parallel() err = tc.testComponentCreation(c) - require.NoError(t, err, "error validating component %v when "+c.GetManagementState()) + require.NoError(t, err, "error validating component %s when %v", name, c.GetManagementState()) }) } return nil @@ -491,7 +495,7 @@ func (tc *testContext) testMRServiceMeshMember() error { smm.SetAPIVersion("maistra.io/v1") smm.SetKind("ServiceMeshMember") err := tc.customClient.Get(tc.ctx, - client.ObjectKey{Namespace: modelregistry.ModelRegistriesNamespace, Name: "default"}, &smm) + client.ObjectKey{Namespace: modelregistry.DefaultModelRegistriesNamespace, Name: "default"}, &smm) if err != nil { return fmt.Errorf("failed to get servicemesh member: %w", err) } @@ -607,3 +611,51 @@ func (tc *testContext) testUpdateDSCComponentEnabled() error { dashboardDeploymentName, tc.applicationsNamespace) } + +const testNs = "test-model-registries" + +func (tc *testContext) validateModelRegistryConfig() error { + // check immutable property registriesNamespace + if tc.testDsc.Spec.Components.ModelRegistry.ManagementState != operatorv1.Managed { + // allowed to set registriesNamespace to non-default + err := patchRegistriesNamespace(tc, testNs, testNs, false) + if err != nil { + return err + } + // allowed to set registriesNamespace back to default value + err = patchRegistriesNamespace(tc, modelregistry.DefaultModelRegistriesNamespace, + modelregistry.DefaultModelRegistriesNamespace, false) + if err != nil { + return err + } + } else { + // not allowed to change registriesNamespace + err := patchRegistriesNamespace(tc, testNs, modelregistry.DefaultModelRegistriesNamespace, true) + if err != nil { + return err + } + } + return nil +} + +func patchRegistriesNamespace(tc *testContext, namespace string, expected string, expectErr bool) error { + patchStr := fmt.Sprintf("{\"spec\":{\"components\":{\"modelregistry\":{\"registriesNamespace\":\"%s\"}}}}", namespace) + err := tc.customClient.Patch(tc.ctx, tc.testDsc, client.RawPatch(types.MergePatchType, []byte(patchStr))) + if err != nil { + if !expectErr { + return fmt.Errorf("unexpected error when setting registriesNamespace in DSC %s to %s: %w", + tc.testDsc.Name, namespace, err) + } + } else { + if expectErr { + return fmt.Errorf("unexpected success when setting registriesNamespace in DSC %s to %s", + tc.testDsc.Name, namespace) + } + } + // compare expected against returned registriesNamespace + if tc.testDsc.Spec.Components.ModelRegistry.RegistriesNamespace != expected { + return fmt.Errorf("expected registriesNamespace %s, got %s", + expected, tc.testDsc.Spec.Components.ModelRegistry.RegistriesNamespace) + } + return nil +} diff --git a/tests/e2e/helper_test.go b/tests/e2e/helper_test.go index 7ea367207b5..83c222cde19 100644 --- a/tests/e2e/helper_test.go +++ b/tests/e2e/helper_test.go @@ -74,9 +74,7 @@ func (tc *testContext) waitForOperatorDeployment(name string, replicas int32) er } } } - log.Printf("Error in %s deployment", name) - return false, nil })