feat: added shared registries namespace property in model registry ds…

…c component, fixes RHOAIENG-12335 (opendatahub-io#1221) * feat: added shared registries namespace property in model registry dsc component, fixes RHOAIENG-12335 Signed-off-by: Dhiraj Bokde <[email protected]> * fix: added registriesNamespace property in references to model registry component in docs, samples, etc. Signed-off-by: Dhiraj Bokde <[email protected]> * fix: updated api-overview.md Signed-off-by: Dhiraj Bokde <[email protected]> * fix: add missing field in setupDSCInstance() Signed-off-by: Dhiraj Bokde <[email protected]> * fix: added debug log for failing e2e test * debug: added log messages in test dsc creation Signed-off-by: Dhiraj Bokde <[email protected]> * debug: make registriesNamespace omitempty so apiserver will set the default value, clean testDSCCreation with more log messages Signed-off-by: Dhiraj Bokde <[email protected]> * 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 <[email protected]> * fix: test need to update for modelreg namespace Signed-off-by: Wen Zhou <[email protected]> * update: cleanup debug info in test cases Signed-off-by: Wen Zhou <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Dhiraj Bokde <[email protected]> Signed-off-by: Wen Zhou <[email protected]> Co-authored-by: Wen Zhou <[email protected]>
ykaliuta · Sep 13, 2024 · dd3df21 · dd3df21
1 parent b4c880a
commit dd3df21
Show file tree

Hide file tree

Showing 21 changed files with 328 additions and 28 deletions.
diff --git 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
@@ -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
@@ -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
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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"`
+}