From f48636589e4cfad5d42197f3abc68e34c689bd98 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Thu, 31 Oct 2024 15:21:40 +0100 Subject: [PATCH 1/5] Create ModelRegistry component API and reconciler --- apis/components/v1/modelregistry_types.go | 54 +++- apis/components/v1/zz_generated.deepcopy.go | 52 +++- .../v1/datasciencecluster_types.go | 18 +- .../v1/zz_generated.deepcopy.go | 4 +- components/modelregistry/modelregistry.go | 230 ------------------ .../modelregistry/zz_generated.deepcopy.go | 39 --- ...onents.opendatahub.io_modelregistries.yaml | 51 +++- .../components/modelregistry/modelregistry.go | 58 +++++ .../modelregistry/modelregistry_controller.go | 98 +++++--- .../modelregistry_controller_actions.go | 174 +++++++++++++ .../modelregistry/modelregistry_support.go | 116 +++++++++ .../resources/servicemesh-member.tmpl.yaml | 0 .../datasciencecluster_controller.go | 10 + .../datasciencecluster/kubebuilder_rbac.go | 16 +- controllers/webhook/webhook.go | 2 +- controllers/webhook/webhook_suite_test.go | 20 +- docs/api-overview.md | 90 ++++--- main.go | 12 +- pkg/cluster/cert.go | 27 +- pkg/cluster/gvk/gvk.go | 28 ++- pkg/controller/actions/errors/errors.go | 24 ++ .../actions/render/action_render_manifests.go | 2 +- pkg/controller/handlers/handlers.go | 14 ++ .../predicates/resources/resources.go | 15 ++ .../reconciler/component_reconciler.go | 21 +- pkg/controller/types/types.go | 31 ++- pkg/upgrade/upgrade.go | 5 +- tests/e2e/controller_test.go | 79 ++++++ tests/e2e/creation_test.go | 93 +------ tests/e2e/helper_test.go | 13 +- tests/e2e/modelregistry_test.go | 220 +++++++++++++++++ tests/e2e/odh_manager_test.go | 6 + 32 files changed, 1146 insertions(+), 476 deletions(-) delete mode 100644 components/modelregistry/modelregistry.go delete mode 100644 components/modelregistry/zz_generated.deepcopy.go create mode 100644 controllers/components/modelregistry/modelregistry.go create mode 100644 controllers/components/modelregistry/modelregistry_controller_actions.go create mode 100644 controllers/components/modelregistry/modelregistry_support.go rename {components => controllers/components}/modelregistry/resources/servicemesh-member.tmpl.yaml (100%) create mode 100644 pkg/controller/actions/errors/errors.go create mode 100644 tests/e2e/modelregistry_test.go diff --git a/apis/components/v1/modelregistry_types.go b/apis/components/v1/modelregistry_types.go index 49e7865d7d3..36df447744c 100644 --- a/apis/components/v1/modelregistry_types.go +++ b/apis/components/v1/modelregistry_types.go @@ -21,26 +21,45 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) -// EDIT THIS FILE! THIS IS SCAFFOLDING FOR YOU TO OWN! -// NOTE: json tags are required. Any new fields you add must have json tags for the fields to be serialized. +const ( + ModelRegistryComponentName = "model-registry-operator" + // ModelRegistryInstanceName the name of the ModelRegistry instance singleton. + // value should match what's set in the XValidation below + ModelRegistryInstanceName = "default-model-registry" + ModelRegistryKind = "ModelRegistry" +) + +// ModelRegistryCommonSpec spec defines the shared desired state of ModelRegistry +type ModelRegistryCommonSpec struct { + // model registry spec exposed to DSC api + components.DevFlagsSpec `json:",inline"` + + // 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"` +} // ModelRegistrySpec defines the desired state of ModelRegistry type ModelRegistrySpec struct { - // INSERT ADDITIONAL SPEC FIELDS - desired state of cluster - // Important: Run "make" to regenerate code after modifying this file - - // Foo is an example field of ModelRegistry. Edit modelregistry_types.go to remove/update - Foo string `json:"foo,omitempty"` + // model registry spec exposed to DSC api + ModelRegistryCommonSpec `json:",inline"` + // model registry spec exposed only to internal api } // ModelRegistryStatus defines the observed state of ModelRegistry type ModelRegistryStatus struct { - components.Status `json:",inline"` + components.Status `json:",inline"` + DSCModelRegistryStatus `json:",inline"` } // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster +// +kubebuilder:validation:XValidation:rule="self.metadata.name == 'default-model-registry'",message="ModelRegistry name must be default-model-registry" +// +kubebuilder:printcolumn:name="Ready",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].status`,description="Ready" +// +kubebuilder:printcolumn:name="Reason",type=string,JSONPath=`.status.conditions[?(@.type=="Ready")].reason`,description="Reason" // ModelRegistry is the Schema for the modelregistries API type ModelRegistry struct { @@ -52,7 +71,7 @@ type ModelRegistry struct { } func (c *ModelRegistry) GetDevFlags() *components.DevFlags { - return nil + return c.Spec.DevFlags } func (c *ModelRegistry) GetStatus() *components.Status { @@ -71,3 +90,20 @@ type ModelRegistryList struct { func init() { SchemeBuilder.Register(&ModelRegistry{}, &ModelRegistryList{}) } + +// +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 + +// DSCModelRegistry contains all the configuration exposed in DSC instance for ModelRegistry component +type DSCModelRegistry struct { + // configuration fields common across components + components.ManagementSpec `json:",inline"` + // model registry specific field + ModelRegistryCommonSpec `json:",inline"` +} + +// DSCModelRegistryStatus struct holds the status for the ModelRegistry component exposed in the DSC +type DSCModelRegistryStatus struct { + RegistriesNamespace string `json:"registriesNamespace,omitempty"` +} diff --git a/apis/components/v1/zz_generated.deepcopy.go b/apis/components/v1/zz_generated.deepcopy.go index f9264ae5659..d357c5076e6 100644 --- a/apis/components/v1/zz_generated.deepcopy.go +++ b/apis/components/v1/zz_generated.deepcopy.go @@ -131,6 +131,38 @@ func (in *DSCDashboard) DeepCopy() *DSCDashboard { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DSCModelRegistry) DeepCopyInto(out *DSCModelRegistry) { + *out = *in + out.ManagementSpec = in.ManagementSpec + in.ModelRegistryCommonSpec.DeepCopyInto(&out.ModelRegistryCommonSpec) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DSCModelRegistry. +func (in *DSCModelRegistry) DeepCopy() *DSCModelRegistry { + if in == nil { + return nil + } + out := new(DSCModelRegistry) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *DSCModelRegistryStatus) DeepCopyInto(out *DSCModelRegistryStatus) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new DSCModelRegistryStatus. +func (in *DSCModelRegistryStatus) DeepCopy() *DSCModelRegistryStatus { + if in == nil { + return nil + } + out := new(DSCModelRegistryStatus) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *DSCRay) DeepCopyInto(out *DSCRay) { *out = *in @@ -620,7 +652,7 @@ func (in *ModelRegistry) DeepCopyInto(out *ModelRegistry) { *out = *in out.TypeMeta = in.TypeMeta in.ObjectMeta.DeepCopyInto(&out.ObjectMeta) - out.Spec = in.Spec + in.Spec.DeepCopyInto(&out.Spec) in.Status.DeepCopyInto(&out.Status) } @@ -642,6 +674,22 @@ func (in *ModelRegistry) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ModelRegistryCommonSpec) DeepCopyInto(out *ModelRegistryCommonSpec) { + *out = *in + in.DevFlagsSpec.DeepCopyInto(&out.DevFlagsSpec) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ModelRegistryCommonSpec. +func (in *ModelRegistryCommonSpec) DeepCopy() *ModelRegistryCommonSpec { + if in == nil { + return nil + } + out := new(ModelRegistryCommonSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ModelRegistryList) DeepCopyInto(out *ModelRegistryList) { *out = *in @@ -677,6 +725,7 @@ func (in *ModelRegistryList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ModelRegistrySpec) DeepCopyInto(out *ModelRegistrySpec) { *out = *in + in.ModelRegistryCommonSpec.DeepCopyInto(&out.ModelRegistryCommonSpec) } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ModelRegistrySpec. @@ -693,6 +742,7 @@ func (in *ModelRegistrySpec) DeepCopy() *ModelRegistrySpec { func (in *ModelRegistryStatus) DeepCopyInto(out *ModelRegistryStatus) { *out = *in in.Status.DeepCopyInto(&out.Status) + out.DSCModelRegistryStatus = in.DSCModelRegistryStatus } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ModelRegistryStatus. diff --git a/apis/datasciencecluster/v1/datasciencecluster_types.go b/apis/datasciencecluster/v1/datasciencecluster_types.go index 0cac0fd8a9a..cd473164f24 100644 --- a/apis/datasciencecluster/v1/datasciencecluster_types.go +++ b/apis/datasciencecluster/v1/datasciencecluster_types.go @@ -31,11 +31,9 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/components/kserve" "github.com/opendatahub-io/opendatahub-operator/v2/components/kueue" "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/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" ) @@ -80,16 +78,16 @@ type Components struct { TrustyAI trustyai.TrustyAI `json:"trustyai,omitempty"` // ModelRegistry component configuration. - ModelRegistry modelregistry.ModelRegistry `json:"modelregistry,omitempty"` + ModelRegistry componentsv1.DSCModelRegistry `json:"modelregistry,omitempty"` - //Training Operator component configuration. + // Training Operator component configuration. 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"` + ModelRegistry *componentsv1.DSCModelRegistryStatus `json:"modelregistry,omitempty"` } // DataScienceClusterStatus defines the observed state of DataScienceCluster. @@ -119,10 +117,10 @@ type DataScienceClusterStatus struct { Release cluster.Release `json:"release,omitempty"` } -//+kubebuilder:object:root=true -//+kubebuilder:subresource:status -//+kubebuilder:resource:scope=Cluster,shortName=dsc -//+kubebuilder:storageversion +// +kubebuilder:object:root=true +// +kubebuilder:subresource:status +// +kubebuilder:resource:scope=Cluster,shortName=dsc +// +kubebuilder:storageversion // DataScienceCluster is the Schema for the datascienceclusters API. type DataScienceCluster struct { @@ -133,7 +131,7 @@ type DataScienceCluster struct { Status DataScienceClusterStatus `json:"status,omitempty"` } -//+kubebuilder:object:root=true +// +kubebuilder:object:root=true // DataScienceClusterList contains a list of DataScienceCluster. type DataScienceClusterList struct { diff --git a/apis/datasciencecluster/v1/zz_generated.deepcopy.go b/apis/datasciencecluster/v1/zz_generated.deepcopy.go index f089d70e9ee..26532140501 100644 --- a/apis/datasciencecluster/v1/zz_generated.deepcopy.go +++ b/apis/datasciencecluster/v1/zz_generated.deepcopy.go @@ -21,7 +21,7 @@ limitations under the License. package v1 import ( - "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" + componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1" corev1 "k8s.io/api/core/v1" runtime "k8s.io/apimachinery/pkg/runtime" @@ -58,7 +58,7 @@ func (in *ComponentsStatus) DeepCopyInto(out *ComponentsStatus) { *out = *in if in.ModelRegistry != nil { in, out := &in.ModelRegistry, &out.ModelRegistry - *out = new(status.ModelRegistryStatus) + *out = new(componentsv1.DSCModelRegistryStatus) **out = **in } } diff --git a/components/modelregistry/modelregistry.go b/components/modelregistry/modelregistry.go deleted file mode 100644 index dbf577ec8f8..00000000000 --- a/components/modelregistry/modelregistry.go +++ /dev/null @@ -1,230 +0,0 @@ -// Package modelregistry provides utility functions to config ModelRegistry, an ML Model metadata repository service -// +groupName=datasciencecluster.opendatahub.io -package modelregistry - -import ( - "context" - "errors" - "fmt" - "path/filepath" - "strings" - "text/template" - - "github.com/go-logr/logr" - operatorv1 "github.com/openshift/api/operator/v1" - corev1 "k8s.io/api/core/v1" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" - logf "sigs.k8s.io/controller-runtime/pkg/log" - - dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" - infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" - "github.com/opendatahub-io/opendatahub-operator/v2/components" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/conversion" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" - - _ "embed" -) - -const DefaultModelRegistryCert = "default-modelregistry-cert" - -var ( - 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", - // ). -) - -// 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) Init(ctx context.Context, _ cluster.Platform) error { - log := logf.FromContext(ctx).WithName(ComponentName) - - var imageParamMap = map[string]string{ - "IMAGES_MODELREGISTRY_OPERATOR": "RELATED_IMAGE_ODH_MODEL_REGISTRY_OPERATOR_IMAGE", - "IMAGES_GRPC_SERVICE": "RELATED_IMAGE_ODH_MLMD_GRPC_SERVER_IMAGE", - "IMAGES_REST_SERVICE": "RELATED_IMAGE_ODH_MODEL_REGISTRY_IMAGE", - } - - if err := deploy.ApplyParams(Path, imageParamMap); err != nil { - log.Error(err, "failed to update image", "path", Path) - } - - return nil -} - -func (m *ModelRegistry) OverrideManifests(ctx context.Context, _ cluster.Platform) error { - // If devflags are set, update default manifests path - if len(m.DevFlags.Manifests) != 0 { - manifestConfig := m.DevFlags.Manifests[0] - if err := deploy.DownloadManifests(ctx, ComponentName, manifestConfig); err != nil { - return err - } - // If overlay is defined, update paths - defaultKustomizePath := "overlays/odh" - if manifestConfig.SourcePath != "" { - defaultKustomizePath = manifestConfig.SourcePath - } - Path = filepath.Join(deploy.DefaultManifestPath, ComponentName, defaultKustomizePath) - } - - return nil -} - -func (m *ModelRegistry) GetComponentName() string { - return ComponentName -} - -func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Client, l logr.Logger, - owner metav1.Object, dscispec *dsciv1.DSCInitializationSpec, platform cluster.Platform, _ bool) error { - enabled := m.GetManagementState() == operatorv1.Managed - monitoringEnabled := dscispec.Monitoring.ManagementState == operatorv1.Managed - - if enabled { - // return error if ServiceMesh is not enabled, as it's a required feature - if dscispec.ServiceMesh == nil || dscispec.ServiceMesh.ManagementState != operatorv1.Managed { - return errors.New("ServiceMesh needs to be set to 'Managed' in DSCI CR, it is required by Model Registry") - } - - if err := m.createDependencies(ctx, cli, dscispec); err != nil { - return err - } - - if m.DevFlags != nil { - // Download manifests and update paths - if err := m.OverrideManifests(ctx, platform); err != nil { - return err - } - } - - extraParamsMap := map[string]string{ - "DEFAULT_CERT": DefaultModelRegistryCert, - } - if err := deploy.ApplyParams(Path, nil, extraParamsMap); err != nil { - return fmt.Errorf("failed to update image from %s : %w", Path, err) - } - - // 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, m.RegistriesNamespace) - if err != nil { - return err - } - 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", m.RegistriesNamespace) - } else { - err := m.removeDependencies(ctx, cli, dscispec) - if err != nil { - return err - } - } - - // Deploy ModelRegistry Operator - if err := deploy.DeployManifestsFromPath(ctx, cli, owner, Path, dscispec.ApplicationsNamespace, m.GetComponentName(), enabled); err != nil { - return err - } - l.Info("apply manifests done") - - // Create additional model registry resources, componentEnabled=true because these extras are never deleted! - if err := deploy.DeployManifestsFromPath(ctx, cli, owner, Path+"/extras", dscispec.ApplicationsNamespace, m.GetComponentName(), true); err != nil { - return err - } - l.Info("apply extra manifests done") - - if enabled { - if err := cluster.WaitForDeploymentAvailable(ctx, cli, m.GetComponentName(), dscispec.ApplicationsNamespace, 10, 1); err != nil { - return fmt.Errorf("deployment for %s is not ready to server: %w", ComponentName, err) - } - } - - // CloudService Monitoring handling - if platform == cluster.ManagedRhods { - if err := m.UpdatePrometheusConfig(cli, l, enabled && monitoringEnabled, ComponentName); err != nil { - return err - } - if err := deploy.DeployManifestsFromPath(ctx, cli, owner, - filepath.Join(deploy.DefaultManifestPath, "monitoring", "prometheus", "apps"), - dscispec.Monitoring.Namespace, - "prometheus", true); err != nil { - return err - } - l.Info("updating SRE monitoring done") - } - return nil -} - -func (m *ModelRegistry) createDependencies(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { - // create DefaultModelRegistryCert - if err := cluster.PropagateDefaultIngressCertificate(ctx, cli, DefaultModelRegistryCert, dscispec.ServiceMesh.ControlPlane.Namespace); err != nil { - return err - } - return nil -} - -func (m *ModelRegistry) removeDependencies(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { - // delete DefaultModelRegistryCert - certSecret := corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: DefaultModelRegistryCert, - Namespace: dscispec.ServiceMesh.ControlPlane.Namespace, - }, - } - // ignore error if the secret has already been removed - if err := cli.Delete(ctx, &certSecret); client.IgnoreNotFound(err) != nil { - return err - } - return nil -} - -//go:embed resources/servicemesh-member.tmpl.yaml -var smmTemplate string - -func enrollToServiceMesh(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec, namespace *corev1.Namespace) error { - tmpl, err := template.New("servicemeshmember").Parse(smmTemplate) - if err != nil { - return fmt.Errorf("error parsing servicemeshmember template: %w", err) - } - builder := strings.Builder{} - controlPlaneData := struct { - Namespace string - ControlPlane *infrav1.ControlPlaneSpec - }{Namespace: namespace.Name, ControlPlane: &dscispec.ServiceMesh.ControlPlane} - - if err = tmpl.Execute(&builder, controlPlaneData); err != nil { - return fmt.Errorf("error executing servicemeshmember template: %w", err) - } - - unstrObj, err := conversion.StrToUnstructured(builder.String()) - if err != nil || len(unstrObj) != 1 { - return fmt.Errorf("error converting servicemeshmember template: %w", err) - } - - return client.IgnoreAlreadyExists(cli.Create(ctx, unstrObj[0])) -} diff --git a/components/modelregistry/zz_generated.deepcopy.go b/components/modelregistry/zz_generated.deepcopy.go deleted file mode 100644 index 86c4a17e14c..00000000000 --- a/components/modelregistry/zz_generated.deepcopy.go +++ /dev/null @@ -1,39 +0,0 @@ -//go:build !ignore_autogenerated - -/* -Copyright 2023. - -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. -*/ - -// Code generated by controller-gen. DO NOT EDIT. - -package modelregistry - -import () - -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *ModelRegistry) DeepCopyInto(out *ModelRegistry) { - *out = *in - in.Component.DeepCopyInto(&out.Component) -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ModelRegistry. -func (in *ModelRegistry) DeepCopy() *ModelRegistry { - if in == nil { - return nil - } - out := new(ModelRegistry) - in.DeepCopyInto(out) - return out -} diff --git a/config/crd/bases/components.opendatahub.io_modelregistries.yaml b/config/crd/bases/components.opendatahub.io_modelregistries.yaml index b2ff3377b2b..df5899a41ca 100644 --- a/config/crd/bases/components.opendatahub.io_modelregistries.yaml +++ b/config/crd/bases/components.opendatahub.io_modelregistries.yaml @@ -14,7 +14,16 @@ spec: singular: modelregistry scope: Cluster versions: - - name: v1 + - additionalPrinterColumns: + - description: Ready + jsonPath: .status.conditions[?(@.type=="Ready")].status + name: Ready + type: string + - description: Reason + jsonPath: .status.conditions[?(@.type=="Ready")].reason + name: Reason + type: string + name: v1 schema: openAPIV3Schema: description: ModelRegistry is the Schema for the modelregistries API @@ -39,9 +48,38 @@ spec: spec: description: ModelRegistrySpec defines the desired state of ModelRegistry properties: - foo: - description: Foo is an example field of ModelRegistry. Edit modelregistry_types.go - to remove/update + devFlags: + description: Add developer fields + properties: + manifests: + description: List of custom manifests for the given component + items: + properties: + contextDir: + default: manifests + description: contextDir is the relative path to the folder + containing manifests in a repository, default value "manifests" + type: string + sourcePath: + default: "" + description: 'sourcePath is the subpath within contextDir + where kustomize builds start. Examples include any sub-folder + or path: `base`, `overlays/dev`, `default`, `odh` etc.' + type: string + uri: + default: "" + description: uri is the URI point to a git repo with tag/branch. + e.g. https://github.com/org/repo/tarball/ + type: string + type: object + type: array + type: object + 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 status: @@ -108,8 +146,13 @@ spec: type: integer phase: type: string + registriesNamespace: + type: string type: object type: object + x-kubernetes-validations: + - message: ModelRegistry name must be default-model-registry + rule: self.metadata.name == 'default-model-registry' served: true storage: true subresources: diff --git a/controllers/components/modelregistry/modelregistry.go b/controllers/components/modelregistry/modelregistry.go new file mode 100644 index 00000000000..3ff8d69bfd4 --- /dev/null +++ b/controllers/components/modelregistry/modelregistry.go @@ -0,0 +1,58 @@ +package modelregistry + +import ( + "fmt" + + operatorv1 "github.com/openshift/api/operator/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" + dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" +) + +func Init(platform cluster.Platform) error { + mi := baseManifestInfo(platform, BaseManifestsSourcePath) + + params := make(map[string]string) + for k, v := range imagesMap { + params[k] = v + } + for k, v := range extraParamsMap { + params[k] = v + } + + if err := odhdeploy.ApplyParams(mi.String(), params); err != nil { + return fmt.Errorf("failed to update images on path %s: %w", mi, err) + } + + return nil +} + +func GetComponentCR(dsc *dscv1.DataScienceCluster) *componentsv1.ModelRegistry { + componentAnnotations := make(map[string]string) + + switch dsc.Spec.Components.ModelRegistry.ManagementState { + case operatorv1.Managed, operatorv1.Removed: + componentAnnotations[annotations.ManagementStateAnnotation] = string(dsc.Spec.Components.ModelRegistry.ManagementState) + default: + // Force and Unmanaged case for unknown values, we do not support these yet + componentAnnotations[annotations.ManagementStateAnnotation] = "Unknown" + } + + return &componentsv1.ModelRegistry{ + TypeMeta: metav1.TypeMeta{ + Kind: componentsv1.ModelRegistryKind, + APIVersion: componentsv1.GroupVersion.String(), + }, + ObjectMeta: metav1.ObjectMeta{ + Name: componentsv1.ModelRegistryInstanceName, + Annotations: componentAnnotations, + }, + Spec: componentsv1.ModelRegistrySpec{ + ModelRegistryCommonSpec: dsc.Spec.Components.ModelRegistry.ModelRegistryCommonSpec, + }, + } +} diff --git a/controllers/components/modelregistry/modelregistry_controller.go b/controllers/components/modelregistry/modelregistry_controller.go index 6b1be9083e8..f80456bd256 100644 --- a/controllers/components/modelregistry/modelregistry_controller.go +++ b/controllers/components/modelregistry/modelregistry_controller.go @@ -18,41 +18,81 @@ package modelregistry import ( "context" + "fmt" - "k8s.io/apimachinery/pkg/runtime" + admissionregistrationv1 "k8s.io/api/admissionregistration/v1" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + rbacv1 "k8s.io/api/rbac/v1" + extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/builder" componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/render" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/updatestatus" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/handlers" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/predicates/resources" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/reconciler" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" ) -// ModelRegistryReconciler reconciles a ModelRegistry object. -type ModelRegistryReconciler struct { - client.Client - Scheme *runtime.Scheme -} +// NewComponentReconciler creates a ComponentReconciler for the Dashboard API. +func NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { + _, err := reconciler.ComponentReconcilerFor[*componentsv1.ModelRegistry]( + mgr, + componentsv1.ModelRegistryInstanceName, + &componentsv1.ModelRegistry{}, + ). + Owns(&corev1.ConfigMap{}). + Owns(&corev1.Secret{}). + Owns(&rbacv1.Role{}). + Owns(&rbacv1.RoleBinding{}). + Owns(&rbacv1.ClusterRole{}). + Owns(&rbacv1.ClusterRoleBinding{}). + Owns(&corev1.Service{}). + Owns(&corev1.ServiceAccount{}). + Owns(&appsv1.Deployment{}, builder.WithPredicates(resources.NewDeploymentPredicate())). + Owns(&admissionregistrationv1.MutatingWebhookConfiguration{}). + Owns(&admissionregistrationv1.ValidatingWebhookConfiguration{}). + Watches(&corev1.Namespace{}). + Watches(&extv1.CustomResourceDefinition{}). + // Some ClusterRoles are part of the component deployment, but not owned by + // the operator (overlays/odh/extras), so in order to properly keep them + // in sync with the manifests, we should also create an additional watcher + Watches(&rbacv1.ClusterRole{}). + // This component copies a secret referenced by the default IngressController + // to the Istio namespace, hence we need to watch for the related secret + // placed in the openshift-ingress namespace and with name defined in the + // IngressController on the .spec.defaultCertificate.name path + WatchesH( + &corev1.Secret{}, + handlers.ToNamed(componentsv1.ModelRegistryInstanceName), + builder.WithPredicates(ingressSecret(ctx, mgr.GetClient()))). + // actions + WithAction(gate). + WithAction(initialize). + WithAction(configureDependencies). + WithAction(render.NewAction( + render.WithCache(true, render.DefaultCachingKeyFn), + render.WithLabel(labels.ODH.Component(ComponentName), "true"), + render.WithLabel(labels.K8SCommon.PartOf, ComponentName), + )). + WithAction(customizeResources). + WithAction(deploy.NewAction( + deploy.WithFieldOwner(componentsv1.ModelRegistryInstanceName), + deploy.WithLabel(labels.ComponentManagedBy, componentsv1.ModelRegistryInstanceName), + )). + WithAction(updatestatus.NewAction( + updatestatus.WithSelectorLabel(labels.ComponentManagedBy, componentsv1.ModelRegistryInstanceName), + )). + WithAction(updateStatus). + Build(ctx) -// Reconcile is part of the main kubernetes reconciliation loop which aims to -// move the current state of the cluster closer to the desired state. -// TODO(user): Modify the Reconcile function to compare the state specified by -// the ModelRegistry object against the actual cluster state, and then -// perform operations to make the cluster state reflect the state specified by -// the user. -// -// For more details, check Reconcile and its Result here: -// - https://pkg.go.dev/sigs.k8s.io/controller-runtime@v0.12.2/pkg/reconcile -func (r *ModelRegistryReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { - _ = log.FromContext(ctx) - - // TODO(user): your logic here - - return ctrl.Result{}, nil -} + if err != nil { + return fmt.Errorf("could not create the model registry controller: %w", err) + } -// SetupWithManager sets up the controller with the Manager. -func (r *ModelRegistryReconciler) SetupWithManager(mgr ctrl.Manager) error { - return ctrl.NewControllerManagedBy(mgr). - For(&componentsv1.ModelRegistry{}). - Complete(r) + return nil } diff --git a/controllers/components/modelregistry/modelregistry_controller_actions.go b/controllers/components/modelregistry/modelregistry_controller_actions.go new file mode 100644 index 00000000000..9f0f848e307 --- /dev/null +++ b/controllers/components/modelregistry/modelregistry_controller_actions.go @@ -0,0 +1,174 @@ +package modelregistry + +import ( + "context" + "errors" + "fmt" + + operatorv1 "github.com/openshift/api/operator/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" + odherrors "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/errors" + odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" + odhdeploy "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" + + _ "embed" +) + +const ( + ServiceMeshNotConfiguredReason = "ServiceMeshNotConfigured" + ServiceMeshNotConfiguredMessage = "ServiceMesh needs to be set to 'Managed' in DSCI CR, it is required by Model Registry" +) + +func gate(_ context.Context, rr *odhtypes.ReconciliationRequest) error { + obj, ok := rr.Instance.(odhtypes.ResourceObject) + if !ok { + return fmt.Errorf("resource instance %v is not a ResourceObject", rr.Instance) + } + + if rr.DSCI.Spec.ServiceMesh != nil && rr.DSCI.Spec.ServiceMesh.ManagementState == operatorv1.Managed { + return nil + } + + s := obj.GetStatus() + s.Phase = "NotReady" + + meta.SetStatusCondition(&s.Conditions, metav1.Condition{ + Type: status.ConditionTypeReady, + Status: metav1.ConditionFalse, + Reason: ServiceMeshNotConfiguredReason, + Message: ServiceMeshNotConfiguredMessage, + ObservedGeneration: s.ObservedGeneration, + }) + + return odherrors.NewStopError(ServiceMeshNotConfiguredMessage) +} + +func initialize(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { + c, ok := rr.Instance.(*componentsv1.ModelRegistry) + if !ok { + return fmt.Errorf("resource instance %v is not a componentsv1.ModelRegistry)", rr.Instance) + } + + rr.Manifests = []odhtypes.ManifestInfo{ + baseManifestInfo(rr.Release.Name, BaseManifestsSourcePath), + extraManifestInfo(rr.Release.Name, BaseManifestsSourcePath), + } + + df := c.GetDevFlags() + + if df == nil { + return nil + } + if len(df.Manifests) == 0 { + return nil + } + if len(df.Manifests) > 1 { + return fmt.Errorf("unexpected number of manifests found: %d, expected 1)", len(df.Manifests)) + } + + if err := odhdeploy.DownloadManifests(ctx, ComponentName, df.Manifests[0]); err != nil { + return err + } + + if df.Manifests[0].SourcePath != "" { + rr.Manifests = []odhtypes.ManifestInfo{ + baseManifestInfo(rr.Release.Name, df.Manifests[0].SourcePath), + extraManifestInfo(rr.Release.Name, df.Manifests[0].SourcePath), + } + } + + return nil +} + +func configureDependencies(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { + c, ok := rr.Instance.(*componentsv1.ModelRegistry) + if !ok { + return fmt.Errorf("resource instance %v is not a componentsv1.ModelRegistry)", rr.Instance) + } + + // Namespace + + if err := rr.AddResource( + &corev1.Namespace{ + ObjectMeta: metav1.ObjectMeta{ + Name: c.Spec.RegistriesNamespace, + }, + }, + ); err != nil { + return fmt.Errorf("failed to add namespace %s to manifests", c.Spec.RegistriesNamespace) + } + + // Secret + + // TODO: this should be done by a dedicated controller + is, err := cluster.FindDefaultIngressSecret(ctx, rr.Client) + if err != nil { + return fmt.Errorf("failed to find default ingress secret for model registry: %w", err) + } + + if err := rr.AddResource( + &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: DefaultModelRegistryCert, + Namespace: rr.DSCI.Spec.ServiceMesh.ControlPlane.Namespace, + }, + Data: is.Data, + Type: is.Type, + }, + ); err != nil { + return fmt.Errorf("failed to add default ingress secret for model registry: %w", err) + } + + // Service Mesh + + smm, err := createServiceMeshMember(rr.DSCI, c.Spec.RegistriesNamespace) + if err != nil { + return fmt.Errorf("failed to create ServiceMesh Member: %w", err) + } + + if err := rr.AddResource(smm); err != nil { + return fmt.Errorf("failed to add ServiceMesh Member: %w", err) + } + + return nil +} + +func customizeResources(_ context.Context, rr *odhtypes.ReconciliationRequest) error { + // Some ClusterRoles are part of the component deployment, but not owned by the + // operator (overlays/odh/extras) and we expect them to be left on the cluster + // even if the component is removed, hence we should mark them a not managed by + // the operator. By doing so the deploy action won't set ownership and won't + // patch them, just recreate if missing + for i := range rr.Resources { + r := rr.Resources[i] + + switch { + case r.GroupVersionKind() == gvk.ClusterRole && r.GetName() == "modelregistry-editor-role": + resources.SetAnnotation(&rr.Resources[i], annotations.ManagedByODHOperator, "false") + case r.GroupVersionKind() == gvk.ClusterRole && r.GetName() == "modelregistry-viewer-role": + resources.SetAnnotation(&rr.Resources[i], annotations.ManagedByODHOperator, "false") + } + } + + return nil +} + +func updateStatus(_ context.Context, rr *odhtypes.ReconciliationRequest) error { + mr, ok := rr.Instance.(*componentsv1.ModelRegistry) + if !ok { + return errors.New("instance is not of type *odhTypes.ModelRegistry") + } + + mr.Status.RegistriesNamespace = mr.Spec.RegistriesNamespace + + return nil +} diff --git a/controllers/components/modelregistry/modelregistry_support.go b/controllers/components/modelregistry/modelregistry_support.go new file mode 100644 index 00000000000..06f24706db2 --- /dev/null +++ b/controllers/components/modelregistry/modelregistry_support.go @@ -0,0 +1,116 @@ +package modelregistry + +import ( + "context" + "fmt" + "path" + "strings" + "text/template" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/predicate" + + componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" + dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" + infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + odhtypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/conversion" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" + + _ "embed" +) + +const ( + ComponentName = componentsv1.ModelRegistryComponentName + DefaultModelRegistriesNamespace = "odh-model-registries" + DefaultModelRegistryCert = "default-modelregistry-cert" + BaseManifestsSourcePath = "overlays/odh" +) + +var ( + imagesMap = map[string]string{ + "IMAGES_MODELREGISTRY_OPERATOR": "RELATED_IMAGE_ODH_MODEL_REGISTRY_OPERATOR_IMAGE", + "IMAGES_GRPC_SERVICE": "RELATED_IMAGE_ODH_MLMD_GRPC_SERVER_IMAGE", + "IMAGES_REST_SERVICE": "RELATED_IMAGE_ODH_MODEL_REGISTRY_IMAGE", + } + + extraParamsMap = map[string]string{ + "DEFAULT_CERT": DefaultModelRegistryCert, + } +) + +//go:embed resources/servicemesh-member.tmpl.yaml +var smmTemplate string + +func baseManifestInfo(_ cluster.Platform, sourcePath string) odhtypes.ManifestInfo { + return odhtypes.ManifestInfo{ + Path: deploy.DefaultManifestPath, + ContextDir: ComponentName, + SourcePath: sourcePath, + } +} + +func extraManifestInfo(_ cluster.Platform, sourcePath string) odhtypes.ManifestInfo { + return odhtypes.ManifestInfo{ + Path: deploy.DefaultManifestPath, + ContextDir: ComponentName, + SourcePath: path.Join(sourcePath, "extras"), + } +} + +func createServiceMeshMember(dsci *dsciv1.DSCInitialization, namespace string) (*unstructured.Unstructured, error) { + tmpl, err := template.New("servicemeshmember").Parse(smmTemplate) + if err != nil { + return nil, fmt.Errorf("error parsing servicemeshmember template: %w", err) + } + + controlPlaneData := struct { + Namespace string + ControlPlane *infrav1.ControlPlaneSpec + }{ + Namespace: namespace, + ControlPlane: &dsci.Spec.ServiceMesh.ControlPlane, + } + + builder := strings.Builder{} + if err = tmpl.Execute(&builder, controlPlaneData); err != nil { + return nil, fmt.Errorf("error executing servicemeshmember template: %w", err) + } + + obj, err := conversion.StrToUnstructured(builder.String()) + if err != nil || len(obj) != 1 { + return nil, fmt.Errorf("error converting servicemeshmember template: %w", err) + } + + return obj[0], nil +} + +func ingressSecret(ctx context.Context, cli client.Client) predicate.Funcs { + f := func(obj client.Object) bool { + ic, err := cluster.FindAvailableIngressController(ctx, cli) + if err != nil { + return false + } + + return obj.GetName() == ic.Spec.DefaultCertificate.Name && + obj.GetNamespace() == cluster.IngressNamespace + } + + return predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return false + }, + GenericFunc: func(e event.GenericEvent) bool { + return false + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return f(e.Object) + }, + UpdateFunc: func(e event.UpdateEvent) bool { + return f(e.ObjectNew) + }, + } +} diff --git a/components/modelregistry/resources/servicemesh-member.tmpl.yaml b/controllers/components/modelregistry/resources/servicemesh-member.tmpl.yaml similarity index 100% rename from components/modelregistry/resources/servicemesh-member.tmpl.yaml rename to controllers/components/modelregistry/resources/servicemesh-member.tmpl.yaml diff --git a/controllers/datasciencecluster/datasciencecluster_controller.go b/controllers/datasciencecluster/datasciencecluster_controller.go index 775ca99e84c..7f00a7ec38c 100644 --- a/controllers/datasciencecluster/datasciencecluster_controller.go +++ b/controllers/datasciencecluster/datasciencecluster_controller.go @@ -54,6 +54,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/components/datasciencepipelines" dashboardctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/dashboard" + modelregistryctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/modelregistry" rayctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/ray" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/status" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" @@ -261,6 +262,14 @@ func (r *DataScienceClusterReconciler) Reconcile(ctx context.Context, req ctrl.R componentErrors = multierror.Append(componentErrors, err) } + // Deploy Model Registry + if instance, err = r.ReconcileComponent(ctx, instance, componentsv1.ModelRegistryComponentName, func() (error, bool) { + modelregistry := modelregistryctrl.GetComponentCR(instance) + return r.apply(ctx, instance, modelregistry), instance.Spec.Components.ModelRegistry.ManagementState == operatorv1.Managed + }); err != nil { + componentErrors = multierror.Append(componentErrors, err) + } + // Process errors for components if componentErrors != nil { log.Info("DataScienceCluster Deployment Incomplete.") @@ -526,6 +535,7 @@ func (r *DataScienceClusterReconciler) SetupWithManager(ctx context.Context, mgr // components CRs Owns(&componentsv1.Dashboard{}). Owns(&componentsv1.Ray{}). + Owns(&componentsv1.ModelRegistry{}). Owns( &corev1.ServiceAccount{}, builder.WithPredicates(saPredicates), diff --git a/controllers/datasciencecluster/kubebuilder_rbac.go b/controllers/datasciencecluster/kubebuilder_rbac.go index 5c8075c2902..6a91f3a588c 100644 --- a/controllers/datasciencecluster/kubebuilder_rbac.go +++ b/controllers/datasciencecluster/kubebuilder_rbac.go @@ -142,6 +142,14 @@ package datasciencecluster // +kubebuilder:rbac:groups="user.openshift.io",resources=groups,verbs=get;create;list;watch;patch;delete // +kubebuilder:rbac:groups="console.openshift.io",resources=consolelinks,verbs=create;get;patch;delete +// ModelRegistry +// +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelregistries,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelregistries/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelregistries/finalizers,verbs=update +// +kubebuilder:rbac:groups=modelregistry.opendatahub.io,resources=modelregistries,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups=modelregistry.opendatahub.io,resources=modelregistries/status,verbs=get;update;patch +// +kubebuilder:rbac:groups=modelregistry.opendatahub.io,resources=modelregistries/finalizers,verbs=update;get + // TODO: Kueue // +kubebuilder:rbac:groups=components.opendatahub.io,resources=kueues,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=components.opendatahub.io,resources=kueues/status,verbs=get;update;patch @@ -207,14 +215,6 @@ package datasciencecluster // +kubebuilder:rbac:groups=components.opendatahub.io,resources=trainingoperators/status,verbs=get;update;patch // +kubebuilder:rbac:groups=components.opendatahub.io,resources=trainingoperators/finalizers,verbs=update -// TODO: MR -// +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelregistries,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelregistries/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelregistries/finalizers,verbs=update -// +kubebuilder:rbac:groups=modelregistry.opendatahub.io,resources=modelregistries,verbs=get;list;watch;create;update;patch;delete -// +kubebuilder:rbac:groups=modelregistry.opendatahub.io,resources=modelregistries/status,verbs=get;update;patch -// +kubebuilder:rbac:groups=modelregistry.opendatahub.io,resources=modelregistries/finalizers,verbs=update;get - // TODO: ModelMesh // +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelmeshservings,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=components.opendatahub.io,resources=modelmeshservings/status,verbs=get;update;patch diff --git a/controllers/webhook/webhook.go b/controllers/webhook/webhook.go index 5a539667b91..3b339d535fe 100644 --- a/controllers/webhook/webhook.go +++ b/controllers/webhook/webhook.go @@ -34,7 +34,7 @@ import ( "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/controllers/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" ) diff --git a/controllers/webhook/webhook_suite_test.go b/controllers/webhook/webhook_suite_test.go index 48fec37e1bd..cb116af69ba 100644 --- a/controllers/webhook/webhook_suite_test.go +++ b/controllers/webhook/webhook_suite_test.go @@ -49,9 +49,9 @@ 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/trustyai" "github.com/opendatahub-io/opendatahub-operator/v2/components/workbenches" + modelregistry2 "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/controllers/webhook" . "github.com/onsi/ginkgo/v2" @@ -215,7 +215,7 @@ var _ = Describe("DSC mutating webhook", func() { dscInstance := newMRDSC1(nameBase+"-dsc-mr1", "", operatorv1.Managed) Expect(k8sClient.Create(ctx, dscInstance)).Should(Succeed()) Expect(dscInstance.Spec.Components.ModelRegistry.RegistriesNamespace). - Should(Equal(modelregistry.DefaultModelRegistriesNamespace)) + Should(Equal(modelregistry2.DefaultModelRegistriesNamespace)) Expect(clearInstance(ctx, dscInstance)).Should(Succeed()) }) @@ -301,8 +301,8 @@ func newDSC(name string, namespace string) *dscv1.DataScienceCluster { ManagementState: operatorv1.Removed, }, }, - ModelRegistry: modelregistry.ModelRegistry{ - Component: componentsold.Component{ + ModelRegistry: componentsv1.DSCModelRegistry{ + ManagementSpec: components.ManagementSpec{ ManagementState: operatorv1.Removed, }, }, @@ -311,7 +311,7 @@ func newDSC(name string, namespace string) *dscv1.DataScienceCluster { } } -func newMRDSC1(name string, mrNamespace string, state operatorv1.ManagementState) *dscv1.DataScienceCluster { +func newMRDSC1(name string, mrNamespace string, _ operatorv1.ManagementState) *dscv1.DataScienceCluster { return &dscv1.DataScienceCluster{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -319,11 +319,13 @@ func newMRDSC1(name string, mrNamespace string, state operatorv1.ManagementState }, Spec: dscv1.DataScienceClusterSpec{ Components: dscv1.Components{ - ModelRegistry: modelregistry.ModelRegistry{ - Component: componentsold.Component{ - ManagementState: state, + ModelRegistry: componentsv1.DSCModelRegistry{ + ManagementSpec: components.ManagementSpec{ + ManagementState: operatorv1.Removed, + }, + ModelRegistryCommonSpec: componentsv1.ModelRegistryCommonSpec{ + RegistriesNamespace: mrNamespace, }, - RegistriesNamespace: mrNamespace, }, }, }, diff --git a/docs/api-overview.md b/docs/api-overview.md index c626b1fbaf1..7e16979b22a 100644 --- a/docs/api-overview.md +++ b/docs/api-overview.md @@ -129,6 +129,41 @@ _Appears in:_ | `devFlags` _[DevFlags](#devflags)_ | Add developer fields | | | +#### DSCModelRegistry + + + +DSCModelRegistry contains all the configuration exposed in DSC instance for ModelRegistry component + + + +_Appears in:_ +- [Components](#components) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `managementState` _[ManagementState](#managementstate)_ | Set to one of the following values:

- "Managed" : the operator is actively managing the component and trying to keep it active.
It will only upgrade the component if it is safe to do so

- "Removed" : the operator is actively managing the component and will not install it,
or if it is installed, the operator will try to remove it | | Enum: [Managed Removed]
| +| `devFlags` _[DevFlags](#devflags)_ | Add developer fields | | | +| `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])?)?$`
| + + +#### DSCModelRegistryStatus + + + +DSCModelRegistryStatus struct holds the status for the ModelRegistry component exposed in the DSC + + + +_Appears in:_ +- [ComponentsStatus](#componentsstatus) +- [ModelRegistryStatus](#modelregistrystatus) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `registriesNamespace` _string_ | | | | + + #### DSCRay @@ -566,6 +601,24 @@ _Appears in:_ | `status` _[ModelRegistryStatus](#modelregistrystatus)_ | | | | +#### ModelRegistryCommonSpec + + + +ModelRegistryCommonSpec spec defines the shared desired state of ModelRegistry + + + +_Appears in:_ +- [DSCModelRegistry](#dscmodelregistry) +- [ModelRegistrySpec](#modelregistryspec) + +| Field | Description | Default | Validation | +| --- | --- | --- | --- | +| `devFlags` _[DevFlags](#devflags)_ | Add developer fields | | | +| `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])?)?$`
| + + #### ModelRegistryList @@ -599,7 +652,8 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `foo` _string_ | Foo is an example field of ModelRegistry. Edit modelregistry_types.go to remove/update | | | +| `devFlags` _[DevFlags](#devflags)_ | Add developer fields | | | +| `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])?)?$`
| #### ModelRegistryStatus @@ -618,6 +672,7 @@ _Appears in:_ | `phase` _string_ | | | | | `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.25/#condition-v1-meta) array_ | | | | | `observedGeneration` _integer_ | | | | +| `registriesNamespace` _string_ | | | | #### Ray @@ -985,7 +1040,6 @@ _Appears in:_ - [Kserve](#kserve) - [Kueue](#kueue) - [ModelMeshServing](#modelmeshserving) -- [ModelRegistry](#modelregistry) - [TrainingOperator](#trainingoperator) - [TrustyAI](#trustyai) - [Workbenches](#workbenches) @@ -1028,9 +1082,12 @@ DevFlagsSpec struct defines the component's dev flags configuration. _Appears in:_ - [Component](#component) - [DSCDashboard](#dscdashboard) +- [DSCModelRegistry](#dscmodelregistry) - [DSCRay](#dscray) - [DashboardCommonSpec](#dashboardcommonspec) - [DashboardSpec](#dashboardspec) +- [ModelRegistryCommonSpec](#modelregistrycommonspec) +- [ModelRegistrySpec](#modelregistryspec) - [RayCommonSpec](#raycommonspec) - [RaySpec](#rayspec) @@ -1050,6 +1107,7 @@ ManagementSpec struct defines the component's management configuration. _Appears in:_ - [Component](#component) - [DSCDashboard](#dscdashboard) +- [DSCModelRegistry](#dscmodelregistry) - [DSCRay](#dscray) | Field | Description | Default | Validation | @@ -1200,30 +1258,6 @@ _Appears in:_ -## datasciencecluster.opendatahub.io/modelregistry - -Package modelregistry provides utility functions to config ModelRegistry, an ML Model metadata repository service - - - -#### ModelRegistry - - - - - - - -_Appears in:_ -- [Components](#components) - -| 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])?)?$`
| - - - ## datasciencecluster.opendatahub.io/trainingoperator Package trainingoperator provides utility functions to config trainingoperator as part of the stack @@ -1354,7 +1388,7 @@ _Appears in:_ | `codeflare` _[CodeFlare](#codeflare)_ | CodeFlare component configuration.
If CodeFlare Operator has been installed in the cluster, it should be uninstalled first before enabled component. | | | | `ray` _[DSCRay](#dscray)_ | Ray component configuration. | | | | `trustyai` _[TrustyAI](#trustyai)_ | TrustyAI component configuration. | | | -| `modelregistry` _[ModelRegistry](#modelregistry)_ | ModelRegistry component configuration. | | | +| `modelregistry` _[DSCModelRegistry](#dscmodelregistry)_ | ModelRegistry component configuration. | | | | `trainingoperator` _[TrainingOperator](#trainingoperator)_ | Training Operator component configuration. | | | @@ -1371,7 +1405,7 @@ _Appears in:_ | Field | Description | Default | Validation | | --- | --- | --- | --- | -| `modelregistry` _[ModelRegistryStatus](#modelregistrystatus)_ | ModelRegistry component status | | | +| `modelregistry` _[DSCModelRegistryStatus](#dscmodelregistrystatus)_ | ModelRegistry component status | | | #### ControlPlaneSpec diff --git a/main.go b/main.go index bb1418f66c3..1c49ead8c82 100644 --- a/main.go +++ b/main.go @@ -62,9 +62,9 @@ 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" dashboardctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/dashboard" + modelregistryctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/modelregistry" rayctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/ray" dscctrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/datasciencecluster" dscictrl "github.com/opendatahub-io/opendatahub-operator/v2/controllers/dscinitialization" @@ -121,6 +121,10 @@ func initComponents(_ context.Context, p cluster.Platform) error { if err := rayctrl.Init(p); err != nil { multiErr = multierror.Append(multiErr, err) } + if err := modelregistryctrl.Init(p); err != nil { + return err + } + return multiErr.ErrorOrNil() } @@ -397,7 +401,7 @@ func createDeploymentCacheConfig(platform cluster.Platform) map[string]cache.Con namespaceConfigs["opendatahub"] = cache.Config{} } // for modelregistry namespace - namespaceConfigs[modelregistry.DefaultModelRegistriesNamespace] = cache.Config{} + namespaceConfigs[modelregistryctrl.DefaultModelRegistriesNamespace] = cache.Config{} return namespaceConfigs } @@ -411,6 +415,10 @@ func CreateComponentReconcilers(ctx context.Context, mgr manager.Manager) error setupLog.Error(err, "unable to create controller", "controller", "RayReconciler") return err } + if err := modelregistryctrl.NewComponentReconciler(ctx, mgr); err != nil { + setupLog.Error(err, "unable to create controller", "controller", "ModelRegistryReconciler") + return err + } return nil } diff --git a/pkg/cluster/cert.go b/pkg/cluster/cert.go index 57daf9c87ae..10eb557cd3d 100644 --- a/pkg/cluster/cert.go +++ b/pkg/cluster/cert.go @@ -19,9 +19,17 @@ import ( corev1 "k8s.io/api/core/v1" k8serr "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" ) +const IngressNamespace = "openshift-ingress" + +var IngressControllerName = types.NamespacedName{ + Namespace: "openshift-ingress-operator", + Name: "default", +} + func CreateSelfSignedCertificate(ctx context.Context, c client.Client, secretName, domain, namespace string, metaOptions ...MetaOptions) error { certSecret, err := GenerateSelfSignedCertificateAsSecret(secretName, domain, namespace) if err != nil { @@ -123,16 +131,25 @@ func generateCertificate(addr string) ([]byte, []byte, error) { return certBuffer.Bytes(), keyBuffer.Bytes(), nil } -// PropagateDefaultIngressCertificate copies ingress cert secrets from openshift-ingress ns to given namespace. -func PropagateDefaultIngressCertificate(ctx context.Context, c client.Client, secretName, namespace string) error { +func FindDefaultIngressSecret(ctx context.Context, c client.Client) (*corev1.Secret, error) { defaultIngressCtrl, err := FindAvailableIngressController(ctx, c) if err != nil { - return fmt.Errorf("failed to get ingress controller: %w", err) + return nil, fmt.Errorf("failed to get ingress controller: %w", err) } defaultIngressCertName := GetDefaultIngressCertSecretName(defaultIngressCtrl) - defaultIngressSecret, err := GetSecret(ctx, c, "openshift-ingress", defaultIngressCertName) + defaultIngressSecret, err := GetSecret(ctx, c, IngressNamespace, defaultIngressCertName) + if err != nil { + return nil, err + } + + return defaultIngressSecret, nil +} + +// PropagateDefaultIngressCertificate copies ingress cert secrets from openshift-ingress ns to given namespace. +func PropagateDefaultIngressCertificate(ctx context.Context, c client.Client, secretName, namespace string) error { + defaultIngressSecret, err := FindDefaultIngressSecret(ctx, c) if err != nil { return err } @@ -143,7 +160,7 @@ func PropagateDefaultIngressCertificate(ctx context.Context, c client.Client, se func FindAvailableIngressController(ctx context.Context, c client.Client) (*operatorv1.IngressController, error) { defaultIngressCtrl := &operatorv1.IngressController{} - err := c.Get(ctx, client.ObjectKey{Namespace: "openshift-ingress-operator", Name: "default"}, defaultIngressCtrl) + err := c.Get(ctx, IngressControllerName, defaultIngressCtrl) if err != nil { return nil, fmt.Errorf("error getting ingresscontroller resource :%w", err) } diff --git a/pkg/cluster/gvk/gvk.go b/pkg/cluster/gvk/gvk.go index 2563cdbfff0..44aad440a8f 100644 --- a/pkg/cluster/gvk/gvk.go +++ b/pkg/cluster/gvk/gvk.go @@ -1,6 +1,10 @@ package gvk -import "k8s.io/apimachinery/pkg/runtime/schema" +import ( + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/runtime/schema" +) var ( ClusterServiceVersion = schema.GroupVersionKind{ @@ -21,11 +25,23 @@ var ( } Deployment = schema.GroupVersionKind{ - Group: "apps", - Version: "v1", + Group: appsv1.SchemeGroupVersion.Group, + Version: appsv1.SchemeGroupVersion.Version, Kind: "Deployment", } + ClusterRole = schema.GroupVersionKind{ + Group: "rbac.authorization.k8s.io", + Version: "v1", + Kind: "ClusterRole", + } + + Secret = schema.GroupVersionKind{ + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Kind: "Secret", + } + KnativeServing = schema.GroupVersionKind{ Group: "operator.knative.dev", Version: "v1beta1", @@ -133,4 +149,10 @@ var ( Version: "v1", Kind: "CustomResourceDefinition", } + + ServiceMeshMember = schema.GroupVersionKind{ + Group: "maistra.io", + Version: "v1", + Kind: "ServiceMeshMember", + } ) diff --git a/pkg/controller/actions/errors/errors.go b/pkg/controller/actions/errors/errors.go new file mode 100644 index 00000000000..419984ef862 --- /dev/null +++ b/pkg/controller/actions/errors/errors.go @@ -0,0 +1,24 @@ +package errors + +import ( + "fmt" +) + +// StopError is a marker error that thew ComponentController uses +// to break out from the action execution loop. +type StopError struct { + reason error +} + +func (e StopError) Error() string { + return e.reason.Error() +} + +func NewStopErrorW(reason error) StopError { + return StopError{reason} +} +func NewStopError(format string, args ...any) StopError { + return StopError{ + fmt.Errorf(format, args...), + } +} diff --git a/pkg/controller/actions/render/action_render_manifests.go b/pkg/controller/actions/render/action_render_manifests.go index be5f27ba277..248d9a6ed33 100644 --- a/pkg/controller/actions/render/action_render_manifests.go +++ b/pkg/controller/actions/render/action_render_manifests.go @@ -119,7 +119,7 @@ func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error // deep copy object so changes done in the pipelines won't // alter them - rr.Resources = result.Clone() + rr.Resources = append(rr.Resources, result.Clone()...) return nil } diff --git a/pkg/controller/handlers/handlers.go b/pkg/controller/handlers/handlers.go index ae27a0fb55b..69c731195c7 100644 --- a/pkg/controller/handlers/handlers.go +++ b/pkg/controller/handlers/handlers.go @@ -30,3 +30,17 @@ func ToOwner() handler.EventHandler { }} }) } + +func Fn(fn func(ctx context.Context, a client.Object) []reconcile.Request) handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(fn) +} + +func ToNamed(name string) handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, a client.Object) []reconcile.Request { + return []reconcile.Request{{ + NamespacedName: types.NamespacedName{ + Name: name, + }, + }} + }) +} diff --git a/pkg/controller/predicates/resources/resources.go b/pkg/controller/predicates/resources/resources.go index 8a072af1c89..71435e399dd 100644 --- a/pkg/controller/predicates/resources/resources.go +++ b/pkg/controller/predicates/resources/resources.go @@ -2,6 +2,7 @@ package resources import ( appsv1 "k8s.io/api/apps/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -36,3 +37,17 @@ func (DeploymentPredicate) Update(e event.UpdateEvent) bool { func NewDeploymentPredicate() *DeploymentPredicate { return &DeploymentPredicate{} } + +func NamespacedNamed(nn types.NamespacedName) predicate.Funcs { + return predicate.Funcs{ + CreateFunc: func(e event.CreateEvent) bool { + return false + }, + DeleteFunc: func(e event.DeleteEvent) bool { + return e.Object.GetName() == nn.Name && e.Object.GetNamespace() == nn.Namespace + }, + UpdateFunc: func(e event.UpdateEvent) bool { + return e.ObjectNew.GetName() == nn.Name && e.ObjectNew.GetNamespace() == nn.Namespace + }, + } +} diff --git a/pkg/controller/reconciler/component_reconciler.go b/pkg/controller/reconciler/component_reconciler.go index ec1f68111c7..f1df83809d7 100644 --- a/pkg/controller/reconciler/component_reconciler.go +++ b/pkg/controller/reconciler/component_reconciler.go @@ -22,6 +22,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions" + odherrors "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/errors" odhClient "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client" odhManager "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/manager" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types" @@ -144,8 +145,14 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( ) if err := action(actx, &rr); err != nil { - l.Error(err, "Failed to execute finalizer", "action", action) - return ctrl.Result{}, err + se := odherrors.StopError{} + if !errors.As(err, &se) { + l.Error(err, "Failed to execute finalizer", "action", action) + return ctrl.Result{}, err + } + + l.Info("detected stop marker", "action", action) + break } } @@ -162,8 +169,14 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( ) if err := action(actx, &rr); err != nil { - l.Error(err, "Failed to execute action", "action", action) - return ctrl.Result{}, err + se := odherrors.StopError{} + if !errors.As(err, &se) { + l.Error(err, "Failed to execute action", "action", action) + return ctrl.Result{}, err + } + + l.Info("detected stop marker", "action", action) + break } } diff --git a/pkg/controller/types/types.go b/pkg/controller/types/types.go index a25d1e5b496..1d0385ed10d 100644 --- a/pkg/controller/types/types.go +++ b/pkg/controller/types/types.go @@ -1,6 +1,7 @@ package types import ( + "fmt" "path" "github.com/go-logr/logr" @@ -57,8 +58,15 @@ type ReconciliationRequest struct { Resources []unstructured.Unstructured } -func (rr *ReconciliationRequest) AddResource(obj interface{}) error { - u, err := machineryrt.DefaultUnstructuredConverter.ToUnstructured(obj) +func (rr *ReconciliationRequest) AddResource(in interface{}) error { + if obj, ok := in.(client.Object); ok { + err := rr.normalize(obj) + if err != nil { + return fmt.Errorf("cannot normalize object: %w", err) + } + } + + u, err := machineryrt.DefaultUnstructuredConverter.ToUnstructured(in) if err != nil { return err } @@ -67,3 +75,22 @@ func (rr *ReconciliationRequest) AddResource(obj interface{}) error { return nil } + +func (rr *ReconciliationRequest) normalize(obj client.Object) error { + if obj.GetObjectKind().GroupVersionKind().Kind != "" { + return nil + } + + kinds, _, err := rr.Client.Scheme().ObjectKinds(obj) + if err != nil { + return fmt.Errorf("cannot get kind of resource: %w", err) + } + + if len(kinds) != 1 { + return fmt.Errorf("expected to find a single GVK for %v, but got %d", obj, len(kinds)) + } + + obj.GetObjectKind().SetGroupVersionKind(kinds[0]) + + return nil +} diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index 24c6025b762..2a6963bdf00 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -37,7 +37,6 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/components/kserve" "github.com/opendatahub-io/opendatahub-operator/v2/components/kueue" "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/trainingoperator" "github.com/opendatahub-io/opendatahub-operator/v2/components/trustyai" "github.com/opendatahub-io/opendatahub-operator/v2/components/workbenches" @@ -95,8 +94,8 @@ func CreateDefaultDSC(ctx context.Context, cli client.Client) error { TrustyAI: trustyai.TrustyAI{ Component: componentsold.Component{ManagementState: operatorv1.Managed}, }, - ModelRegistry: modelregistry.ModelRegistry{ - Component: componentsold.Component{ManagementState: operatorv1.Managed}, + ModelRegistry: componentsv1.DSCModelRegistry{ + ManagementSpec: components.ManagementSpec{ManagementState: operatorv1.Managed}, }, TrainingOperator: trainingoperator.TrainingOperator{ Component: componentsold.Component{ManagementState: operatorv1.Managed}, diff --git a/tests/e2e/controller_test.go b/tests/e2e/controller_test.go index 98ca34746bd..5d45cbb90db 100644 --- a/tests/e2e/controller_test.go +++ b/tests/e2e/controller_test.go @@ -13,7 +13,10 @@ import ( monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" autoscalingv1 "k8s.io/api/autoscaling/v1" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" k8sclient "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -27,6 +30,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/pkg/resources" ) var ( @@ -56,6 +60,78 @@ type testContext struct { ctx context.Context } +func (tc *testContext) List( + gvk schema.GroupVersionKind, + option ...client.ListOption, +) func() ([]unstructured.Unstructured, error) { + return func() ([]unstructured.Unstructured, error) { + items := unstructured.UnstructuredList{} + items.SetGroupVersionKind(gvk) + + err := tc.customClient.List(tc.ctx, &items, option...) + if err != nil { + return nil, err + } + + return items.Items, nil + } +} + +func (tc *testContext) Get( + gvk schema.GroupVersionKind, + ns string, + name string, + option ...client.GetOption, +) func() (*unstructured.Unstructured, error) { + return func() (*unstructured.Unstructured, error) { + u := unstructured.Unstructured{} + u.SetGroupVersionKind(gvk) + + err := tc.customClient.Get(tc.ctx, client.ObjectKey{Namespace: ns, Name: name}, &u, option...) + if err != nil { + return nil, err + } + + return &u, nil + } +} +func (tc *testContext) MergePatch( + obj client.Object, + patch []byte, +) func() (*unstructured.Unstructured, error) { + return func() (*unstructured.Unstructured, error) { + u, err := resources.ToUnstructured(obj) + if err != nil { + return nil, err + } + + err = tc.customClient.Patch(tc.ctx, u, client.RawPatch(types.MergePatchType, patch)) + if err != nil { + return nil, err + } + + return u, nil + } +} + +func (tc *testContext) updateComponent(fn func(dsc *dscv1.Components)) func() error { + return func() error { + err := tc.customClient.Get(tc.ctx, types.NamespacedName{Name: tc.testDsc.Name}, tc.testDsc) + if err != nil { + return err + } + + fn(&tc.testDsc.Spec.Components) + + err = tc.customClient.Update(tc.ctx, tc.testDsc) + if err != nil { + return err + } + + return nil + } +} + func NewTestContext() (*testContext, error) { // GetConfig(): If KUBECONFIG env variable is set, it is used to create // the client, else the inClusterConfig() is used. @@ -113,11 +189,14 @@ func TestOdhOperator(t *testing.T) { if !t.Run("validate operator pod is running", testODHOperatorValidation) { return } + // Run create and delete tests for all the components t.Run("create DSCI and DSC CRs", creationTestSuite) + // Validate deployment of each component in separate test suite t.Run("validate installation of Dashboard Component", dashboardTestSuite) t.Run("validate installation of Ray Component", rayTestSuite) + t.Run("validate installation of ModelRegistry Component", modelRegistryTestSuite) // Run deletion if skipDeletion is not set if !skipDeletion { diff --git a/tests/e2e/creation_test.go b/tests/e2e/creation_test.go index c1920815a06..a6c127bb988 100644 --- a/tests/e2e/creation_test.go +++ b/tests/e2e/creation_test.go @@ -24,7 +24,7 @@ import ( dsciv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/dscinitialization/v1" infrav1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/infrastructure/v1" "github.com/opendatahub-io/opendatahub-operator/v2/components" - "github.com/opendatahub-io/opendatahub-operator/v2/components/modelregistry" + "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/modelregistry" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/feature/serverless" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" @@ -34,9 +34,8 @@ func creationTestSuite(t *testing.T) { testCtx, err := NewTestContext() require.NoError(t, err) - // TODO: Uncomment this when we have Kserve api implemented - // err = testCtx.setUp(t) - // require.NoError(t, err, "error setting up environment") + err = testCtx.setUp(t) + require.NoError(t, err, "error setting up environment") t.Run(testCtx.testDsc.Name, func(t *testing.T) { // DSCI @@ -48,10 +47,10 @@ func creationTestSuite(t *testing.T) { testCtx.testDSCIDuplication(t) }) // Validates Servicemesh fields - // t.Run("Validate DSCInitialization instance", func(t *testing.T) { - // err = testCtx.validateDSCI() - // require.NoError(t, err, "error validating DSCInitialization instance") - // }) + t.Run("Validate DSCInitialization instance", func(t *testing.T) { + err = testCtx.validateDSCI() + require.NoError(t, err, "error validating DSCInitialization instance") + }) t.Run("Check owned namespaces exist", func(t *testing.T) { err = testCtx.testOwnedNamespacesAllExist() require.NoError(t, err, "error owned namespace is missing") @@ -66,7 +65,7 @@ func creationTestSuite(t *testing.T) { testCtx.testDSCDuplication(t) }) - //// Kserve + // // Kserve // t.Run("Validate Knative resoruce", func(t *testing.T) { // err = testCtx.validateDSC() // require.NoError(t, err, "error getting Knatvie resrouce as part of DataScienceCluster validation") @@ -77,19 +76,11 @@ func creationTestSuite(t *testing.T) { // require.NoError(t, err, "error getting default cert secrets for Kserve") // }) // - //// 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") - // }) - // t.Run("Validate model registry servicemeshmember available", func(t *testing.T) { - // err = testCtx.testMRServiceMeshMember() - // require.NoError(t, err, "error getting servicemeshmember for Model Registry") - // }) + // ModelReg + t.Run("Validate model registry config", func(t *testing.T) { + err = testCtx.validateModelRegistryConfig() + require.NoError(t, err, "error validating ModelRegistry config") + }) }) } @@ -401,64 +392,6 @@ func (tc *testContext) testDefaultCertsAvailable() error { return nil } -func (tc *testContext) testDefaultModelRegistryCertAvailable() error { - // return if MR is not set to Managed - if tc.testDsc.Spec.Components.ModelRegistry.ManagementState != operatorv1.Managed { - return nil - } - - // Get expected cert secrets - defaultIngressCtrl, err := cluster.FindAvailableIngressController(tc.ctx, tc.customClient) - if err != nil { - return fmt.Errorf("failed to get ingress controller: %w", err) - } - - defaultIngressCertName := cluster.GetDefaultIngressCertSecretName(defaultIngressCtrl) - - defaultIngressSecret, err := cluster.GetSecret(tc.ctx, tc.customClient, "openshift-ingress", defaultIngressCertName) - if err != nil { - return err - } - - // Verify secret from Control Plane namespace matches the default MR cert secret - defaultMRSecretName := modelregistry.DefaultModelRegistryCert - defaultMRSecret, err := cluster.GetSecret(tc.ctx, tc.customClient, tc.testDSCI.Spec.ServiceMesh.ControlPlane.Namespace, - defaultMRSecretName) - if err != nil { - return err - } - - if defaultMRSecret.Type != defaultIngressSecret.Type { - return fmt.Errorf("wrong type of MR cert secret is created for %v. Expected %v, Got %v", defaultMRSecretName, defaultIngressSecret.Type, defaultMRSecret.Type) - } - - if string(defaultIngressSecret.Data["tls.crt"]) != string(defaultMRSecret.Data["tls.crt"]) { - return fmt.Errorf("default MR cert secret not expected. Epected %v, Got %v", defaultIngressSecret.Data["tls.crt"], defaultMRSecret.Data["tls.crt"]) - } - - if string(defaultIngressSecret.Data["tls.key"]) != string(defaultMRSecret.Data["tls.key"]) { - return fmt.Errorf("default MR cert secret not expected. Epected %v, Got %v", defaultIngressSecret.Data["tls.crt"], defaultMRSecret.Data["tls.crt"]) - } - return nil -} - -func (tc *testContext) testMRServiceMeshMember() error { - if tc.testDsc.Spec.Components.ModelRegistry.ManagementState != operatorv1.Managed { - return nil - } - - // Get unstructured ServiceMeshMember - smm := unstructured.Unstructured{} - smm.SetAPIVersion("maistra.io/v1") - smm.SetKind("ServiceMeshMember") - err := tc.customClient.Get(tc.ctx, - client.ObjectKey{Namespace: modelregistry.DefaultModelRegistriesNamespace, Name: "default"}, &smm) - if err != nil { - return fmt.Errorf("failed to get servicemesh member: %w", err) - } - return nil -} - const testNs = "test-model-registries" func (tc *testContext) validateModelRegistryConfig() error { diff --git a/tests/e2e/helper_test.go b/tests/e2e/helper_test.go index c4052135bfa..b9dc2159a96 100644 --- a/tests/e2e/helper_test.go +++ b/tests/e2e/helper_test.go @@ -1,4 +1,3 @@ -//nolint:unused package e2e_test import ( @@ -33,7 +32,6 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/components/kserve" "github.com/opendatahub-io/opendatahub-operator/v2/components/kueue" "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/trainingoperator" "github.com/opendatahub-io/opendatahub-operator/v2/components/trustyai" "github.com/opendatahub-io/opendatahub-operator/v2/components/workbenches" @@ -105,7 +103,7 @@ func setupDSCICR(name string) *dsciv1.DSCInitialization { Name: "data-science-smcp", Namespace: "istio-system", }, - ManagementState: "Removed", + ManagementState: "Managed", }, }, } @@ -168,9 +166,12 @@ func setupDSCInstance(name string) *dscv1.DataScienceCluster { ManagementState: operatorv1.Removed, }, }, - ModelRegistry: modelregistry.ModelRegistry{ - Component: componentsold.Component{ - ManagementState: operatorv1.Removed, + ModelRegistry: componentsv1.DSCModelRegistry{ + ManagementSpec: components.ManagementSpec{ + ManagementState: operatorv1.Managed, + }, + ModelRegistryCommonSpec: componentsv1.ModelRegistryCommonSpec{ + RegistriesNamespace: "odh-model-registries", }, }, TrainingOperator: trainingoperator.TrainingOperator{ diff --git a/tests/e2e/modelregistry_test.go b/tests/e2e/modelregistry_test.go new file mode 100644 index 00000000000..5d1d168142b --- /dev/null +++ b/tests/e2e/modelregistry_test.go @@ -0,0 +1,220 @@ +package e2e_test + +import ( + "testing" + "time" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/stretchr/testify/require" + autoscalingv1 "k8s.io/api/autoscaling/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + + componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" + dscv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/datasciencecluster/v1" + "github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/modelregistry" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq" + + . "github.com/onsi/gomega" +) + +type ModelRegistryTestCtx struct { + *testContext +} + +func modelRegistryTestSuite(t *testing.T) { + tc, err := NewTestContext() + require.NoError(t, err) + + componentCtx := ModelRegistryTestCtx{ + testContext: tc, + } + + t.Run(componentCtx.testDsc.Name, func(t *testing.T) { + t.Run("Validate ModelRegistry instance", componentCtx.validateModelRegistryInstance) + t.Run("Validate ModelRegistry operands OwnerReferences", componentCtx.validateOperandsOwnerReferences) + t.Run("Validate Update ModelRegistry operands resources", componentCtx.validateUpdateModelRegistryOperandsResources) + t.Run("Validate ModelRegistry Cert", componentCtx.validateModelRegistryCert) + t.Run("Validate ModelRegistry ServiceMeshMember", componentCtx.validateModelRegistryServiceMeshMember) + // must be the latest one + t.Run("Validate Disabling ModelRegistry Component", componentCtx.validateModelRegistryDisabled) + }) +} + +func (tc *ModelRegistryTestCtx) validateModelRegistryInstance(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(generalWaitTimeout) + g.SetDefaultEventuallyPollingInterval(1 * time.Second) + + g.Eventually( + tc.List(gvk.ModelRegistry), + ).Should(And( + HaveLen(1), + HaveEach(And( + jq.Match(`.metadata.ownerReferences[0].kind == "%s"`, gvk.DataScienceCluster.Kind), + jq.Match(`.spec.registriesNamespace == "%s"`, tc.testDsc.Spec.Components.ModelRegistry.RegistriesNamespace), + jq.Match(`.status.phase == "%s"`, readyStatus), + )), + )) +} + +func (tc *ModelRegistryTestCtx) validateOperandsOwnerReferences(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(generalWaitTimeout) + g.SetDefaultEventuallyPollingInterval(generalRetryInterval) + + g.Eventually( + tc.List( + gvk.Deployment, + client.InNamespace(tc.applicationsNamespace), + client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, + ), + ).Should(And( + HaveLen(1), + HaveEach( + jq.Match(`.metadata.ownerReferences[0].kind == "%s"`, componentsv1.ModelRegistryKind), + ), + )) +} + +func (tc *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(generalWaitTimeout) + g.SetDefaultEventuallyPollingInterval(generalRetryInterval) + + appDeployments, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).List( + tc.ctx, + metav1.ListOptions{ + LabelSelector: labels.ComponentManagedBy + "=" + componentsv1.ModelRegistryInstanceName, + }, + ) + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(appDeployments.Items).To(HaveLen(1)) + + const expectedReplica int32 = 2 // from 1 to 2 + + testDeployment := appDeployments.Items[0] + patchedReplica := &autoscalingv1.Scale{ + ObjectMeta: metav1.ObjectMeta{ + Name: testDeployment.Name, + Namespace: testDeployment.Namespace, + }, + Spec: autoscalingv1.ScaleSpec{ + Replicas: expectedReplica, + }, + Status: autoscalingv1.ScaleStatus{}, + } + + updatedDep, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).UpdateScale( + tc.ctx, + testDeployment.Name, + patchedReplica, + metav1.UpdateOptions{}, + ) + + g.Expect(err).ShouldNot(HaveOccurred()) + g.Expect(updatedDep.Spec.Replicas).Should(Equal(patchedReplica.Spec.Replicas)) + + g.Eventually( + tc.List( + gvk.Deployment, + client.InNamespace(tc.applicationsNamespace), + client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, + ), + ).Should(And( + HaveLen(1), + HaveEach( + jq.Match(`.spec.replicas == %d`, expectedReplica), + ), + )) + + g.Consistently( + tc.List( + gvk.Deployment, + client.InNamespace(tc.applicationsNamespace), + client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, + ), + ).WithTimeout(30 * time.Second).WithPolling(1 * time.Second).Should(And( + HaveLen(1), + HaveEach( + jq.Match(`.spec.replicas == %d`, expectedReplica), + ), + )) +} + +func (tc *ModelRegistryTestCtx) validateModelRegistryCert(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(generalWaitTimeout) + g.SetDefaultEventuallyPollingInterval(generalRetryInterval) + + is, err := cluster.FindDefaultIngressSecret(tc.ctx, tc.customClient) + g.Expect(err).ShouldNot(HaveOccurred()) + + g.Eventually( + tc.Get( + gvk.Secret, + tc.testDSCI.Spec.ServiceMesh.ControlPlane.Namespace, + modelregistry.DefaultModelRegistryCert, + ), + ).Should(And( + jq.Match(`.type == "%s"`, is.Type), + jq.Match(`(.data."tls.crt" | @base64d) == "%s"`, is.Data["tls.crt"]), + jq.Match(`(.data."tls.key" | @base64d) == "%s"`, is.Data["tls.key"]), + )) +} + +func (tc *ModelRegistryTestCtx) validateModelRegistryServiceMeshMember(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(generalWaitTimeout) + g.SetDefaultEventuallyPollingInterval(generalRetryInterval) + + g.Eventually( + tc.Get(gvk.ServiceMeshMember, modelregistry.DefaultModelRegistriesNamespace, "default"), + ).Should( + jq.Match(`.spec | has("controlPlaneRef")`), + ) +} + +func (tc *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(generalWaitTimeout) + g.SetDefaultEventuallyPollingInterval(generalRetryInterval) + + g.Eventually( + tc.List( + gvk.Deployment, + client.InNamespace(tc.applicationsNamespace), + client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, + ), + ).Should( + HaveLen(1), + ) + + g.Eventually( + tc.updateComponent(func(c *dscv1.Components) { + c.ModelRegistry.ManagementState = operatorv1.Removed + }), + ).ShouldNot( + HaveOccurred(), + ) + + g.Eventually( + tc.List( + gvk.Deployment, + client.InNamespace(tc.applicationsNamespace), + client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, + ), + ).Should( + BeEmpty(), + ) + + g.Eventually( + tc.List(gvk.ModelRegistry), + ).Should( + BeEmpty(), + ) +} diff --git a/tests/e2e/odh_manager_test.go b/tests/e2e/odh_manager_test.go index f9cbfe4a042..90fbba36943 100644 --- a/tests/e2e/odh_manager_test.go +++ b/tests/e2e/odh_manager_test.go @@ -50,4 +50,10 @@ func (tc *testContext) validateOwnedCRDs(t *testing.T) { require.NoErrorf(t, tc.validateCRD("rays.components.opendatahub.io"), "error in validating CRD : rays.components.opendatahub.io") }) + + t.Run("Validate ModelRegistry CRD", func(t *testing.T) { + t.Parallel() + require.NoErrorf(t, tc.validateCRD("modelregistries.components.opendatahub.io"), + "error in validating CRD : modelregistries.components.opendatahub.io") + }) } From c0338c920e57069a9cc0e1856f500cafdbbea1ff Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Mon, 4 Nov 2024 14:20:42 +0100 Subject: [PATCH 2/5] Create ModelRegistry component API and reconciler (ssa) --- ...onents.opendatahub.io_modelregistries.yaml | 51 +++++- ...atahub-operator.clusterserviceversion.yaml | 2 +- .../modelregistry/modelregistry_support.go | 3 + pkg/controller/client/client.go | 53 ++++-- .../reconciler/component_reconciler.go | 23 +-- .../component_reconciler_support.go | 10 +- tests/e2e/controller_test.go | 76 -------- tests/e2e/modelregistry_test.go | 166 +++++++++++++----- 8 files changed, 231 insertions(+), 153 deletions(-) diff --git a/bundle/manifests/components.opendatahub.io_modelregistries.yaml b/bundle/manifests/components.opendatahub.io_modelregistries.yaml index 9f69d7b20f9..889c7ff426e 100644 --- a/bundle/manifests/components.opendatahub.io_modelregistries.yaml +++ b/bundle/manifests/components.opendatahub.io_modelregistries.yaml @@ -14,7 +14,16 @@ spec: singular: modelregistry scope: Cluster versions: - - name: v1 + - additionalPrinterColumns: + - description: Ready + jsonPath: .status.conditions[?(@.type=="Ready")].status + name: Ready + type: string + - description: Reason + jsonPath: .status.conditions[?(@.type=="Ready")].reason + name: Reason + type: string + name: v1 schema: openAPIV3Schema: description: ModelRegistry is the Schema for the modelregistries API @@ -39,9 +48,38 @@ spec: spec: description: ModelRegistrySpec defines the desired state of ModelRegistry properties: - foo: - description: Foo is an example field of ModelRegistry. Edit modelregistry_types.go - to remove/update + devFlags: + description: Add developer fields + properties: + manifests: + description: List of custom manifests for the given component + items: + properties: + contextDir: + default: manifests + description: contextDir is the relative path to the folder + containing manifests in a repository, default value "manifests" + type: string + sourcePath: + default: "" + description: 'sourcePath is the subpath within contextDir + where kustomize builds start. Examples include any sub-folder + or path: `base`, `overlays/dev`, `default`, `odh` etc.' + type: string + uri: + default: "" + description: uri is the URI point to a git repo with tag/branch. + e.g. https://github.com/org/repo/tarball/ + type: string + type: object + type: array + type: object + 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 status: @@ -108,8 +146,13 @@ spec: type: integer phase: type: string + registriesNamespace: + type: string type: object type: object + x-kubernetes-validations: + - message: ModelRegistry name must be default-model-registry + rule: self.metadata.name == 'default-model-registry' served: true storage: true subresources: diff --git a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml index d55386df0e6..69a254b1301 100644 --- a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml @@ -1210,7 +1210,7 @@ spec: value: /opt/manifests - name: ODH_PLATFORM_TYPE value: OpenDataHub - image: REPLACE_IMAGE:latest + image: ttl.sh/882e644b-532e-4feb-95d4-2bd23fb2e6df:2h imagePullPolicy: Always livenessProbe: httpGet: diff --git a/controllers/components/modelregistry/modelregistry_support.go b/controllers/components/modelregistry/modelregistry_support.go index 06f24706db2..2ba98d3487b 100644 --- a/controllers/components/modelregistry/modelregistry_support.go +++ b/controllers/components/modelregistry/modelregistry_support.go @@ -94,6 +94,9 @@ func ingressSecret(ctx context.Context, cli client.Client) predicate.Funcs { if err != nil { return false } + if ic.Spec.DefaultCertificate == nil { + return false + } return obj.GetName() == ic.Spec.DefaultCertificate.Name && obj.GetNamespace() == cluster.IngressNamespace diff --git a/pkg/controller/client/client.go b/pkg/controller/client/client.go index 80d94e4de2c..dad42d5c904 100644 --- a/pkg/controller/client/client.go +++ b/pkg/controller/client/client.go @@ -4,9 +4,13 @@ import ( "context" "fmt" + "github.com/pkg/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/client-go/rest" ctrl "sigs.k8s.io/controller-runtime" ctrlCli "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" ) func NewFromManager(ctx context.Context, mgr ctrl.Manager) (*Client, error) { @@ -23,27 +27,56 @@ type Client struct { ctrlCli.Client } -func (c *Client) Apply(ctx context.Context, obj ctrlCli.Object, opts ...ctrlCli.PatchOption) error { +func (c *Client) Apply(ctx context.Context, in ctrlCli.Object, opts ...ctrlCli.PatchOption) error { + u, err := resources.ToUnstructured(in) + if err != nil { + return fmt.Errorf("failed to convert resource to unstructured: %w", err) + } + + // safe copy + u = u.DeepCopy() + // remove not required fields - obj.SetManagedFields(nil) - obj.SetResourceVersion("") + unstructured.RemoveNestedField(u.Object, "metadata", "managedFields") + unstructured.RemoveNestedField(u.Object, "metadata", "resourceVersion") + unstructured.RemoveNestedField(u.Object, "status") + + err = c.Client.Patch(ctx, u, ctrlCli.Apply, opts...) + if err != nil { + return fmt.Errorf("unable to pactch object %s: %w", u, err) + } - err := c.Client.Patch(ctx, obj, ctrlCli.Apply, opts...) + // Write back the modified object so callers can access the patched object. + err = c.Scheme().Convert(u, in, ctx) if err != nil { - return fmt.Errorf("unable to pactch object %s: %w", obj, err) + return errors.Wrapf(err, "failed to write modified object") } return nil } -func (c *Client) ApplyStatus(ctx context.Context, obj ctrlCli.Object, opts ...ctrlCli.SubResourcePatchOption) error { +func (c *Client) ApplyStatus(ctx context.Context, in ctrlCli.Object, opts ...ctrlCli.SubResourcePatchOption) error { + u, err := resources.ToUnstructured(in) + if err != nil { + return fmt.Errorf("failed to convert resource to unstructured: %w", err) + } + + // safe copy + u = u.DeepCopy() + // remove not required fields - obj.SetManagedFields(nil) - obj.SetResourceVersion("") + unstructured.RemoveNestedField(u.Object, "metadata", "managedFields") + unstructured.RemoveNestedField(u.Object, "metadata", "resourceVersion") + + err = c.Client.Status().Patch(ctx, u, ctrlCli.Apply, opts...) + if err != nil { + return fmt.Errorf("unable to patch object status %s: %w", u, err) + } - err := c.Client.Status().Patch(ctx, obj, ctrlCli.Apply, opts...) + // Write back the modified object so callers can access the patched object. + err = c.Scheme().Convert(u, in, ctx) if err != nil { - return fmt.Errorf("unable to patch object status %s: %w", obj, err) + return errors.Wrapf(err, "failed to write modified object") } return nil diff --git a/pkg/controller/reconciler/component_reconciler.go b/pkg/controller/reconciler/component_reconciler.go index f1df83809d7..383d01af021 100644 --- a/pkg/controller/reconciler/component_reconciler.go +++ b/pkg/controller/reconciler/component_reconciler.go @@ -7,7 +7,6 @@ import ( "reflect" "github.com/go-logr/logr" - k8serr "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/tools/record" @@ -38,6 +37,7 @@ type ComponentReconciler struct { Recorder record.EventRecorder Release cluster.Release + name string m *odhManager.Manager instanceFactory func() (components.ComponentObject, error) } @@ -54,6 +54,7 @@ func NewComponentReconciler[T components.ComponentObject](ctx context.Context, m Log: ctrl.Log.WithName("controllers").WithName(name), Recorder: mgr.GetEventRecorderFor(name), Release: cluster.GetRelease(), + name: name, m: odhManager.New(mgr), instanceFactory: func() (components.ComponentObject, error) { t := reflect.TypeOf(*new(T)).Elem() @@ -180,24 +181,16 @@ func (r *ComponentReconciler) Reconcile(ctx context.Context, req ctrl.Request) ( } } - // - // update status with standard update mechanism as the SSA one seems causing - // a weird issue on some openshift releases: - // - // failed to create typed patch object (...): .status.url: field not declared in schema - // - err = r.Client.Status().Update( + err = r.Client.ApplyStatus( ctx, rr.Instance, + client.FieldOwner(r.name), + client.ForceOwnership, ) - switch { - case err == nil: - return ctrl.Result{}, nil - case k8serr.IsConflict(err): - l.Info("conflict detected while updating status, retrying") - return ctrl.Result{Requeue: true}, nil - default: + if err != nil { return ctrl.Result{}, err } + + return ctrl.Result{}, err } diff --git a/pkg/controller/reconciler/component_reconciler_support.go b/pkg/controller/reconciler/component_reconciler_support.go index 38715bfe92b..74bdc1533bd 100644 --- a/pkg/controller/reconciler/component_reconciler_support.go +++ b/pkg/controller/reconciler/component_reconciler_support.go @@ -137,7 +137,15 @@ func (b *ComponentReconcilerBuilder[T]) WithEventFilter(p predicate.Predicate) * func (b *ComponentReconcilerBuilder[T]) Build(ctx context.Context) (*ComponentReconciler, error) { name := b.componentName if name == "" { - name = b.input.object.GetObjectKind().GroupVersionKind().Kind + kinds, _, err := b.mgr.GetScheme().ObjectKinds(b.input.object) + if err != nil { + return nil, err + } + if len(kinds) != 1 { + return nil, fmt.Errorf("expected exactly one kind of object, got %d", len(kinds)) + } + + name = kinds[0].Kind name = strings.ToLower(name) } diff --git a/tests/e2e/controller_test.go b/tests/e2e/controller_test.go index 5d45cbb90db..e1aca7abfa4 100644 --- a/tests/e2e/controller_test.go +++ b/tests/e2e/controller_test.go @@ -13,10 +13,7 @@ import ( monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" autoscalingv1 "k8s.io/api/autoscaling/v1" apiextv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" utilruntime "k8s.io/apimachinery/pkg/util/runtime" k8sclient "k8s.io/client-go/kubernetes" clientgoscheme "k8s.io/client-go/kubernetes/scheme" @@ -30,7 +27,6 @@ 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/pkg/resources" ) var ( @@ -60,78 +56,6 @@ type testContext struct { ctx context.Context } -func (tc *testContext) List( - gvk schema.GroupVersionKind, - option ...client.ListOption, -) func() ([]unstructured.Unstructured, error) { - return func() ([]unstructured.Unstructured, error) { - items := unstructured.UnstructuredList{} - items.SetGroupVersionKind(gvk) - - err := tc.customClient.List(tc.ctx, &items, option...) - if err != nil { - return nil, err - } - - return items.Items, nil - } -} - -func (tc *testContext) Get( - gvk schema.GroupVersionKind, - ns string, - name string, - option ...client.GetOption, -) func() (*unstructured.Unstructured, error) { - return func() (*unstructured.Unstructured, error) { - u := unstructured.Unstructured{} - u.SetGroupVersionKind(gvk) - - err := tc.customClient.Get(tc.ctx, client.ObjectKey{Namespace: ns, Name: name}, &u, option...) - if err != nil { - return nil, err - } - - return &u, nil - } -} -func (tc *testContext) MergePatch( - obj client.Object, - patch []byte, -) func() (*unstructured.Unstructured, error) { - return func() (*unstructured.Unstructured, error) { - u, err := resources.ToUnstructured(obj) - if err != nil { - return nil, err - } - - err = tc.customClient.Patch(tc.ctx, u, client.RawPatch(types.MergePatchType, patch)) - if err != nil { - return nil, err - } - - return u, nil - } -} - -func (tc *testContext) updateComponent(fn func(dsc *dscv1.Components)) func() error { - return func() error { - err := tc.customClient.Get(tc.ctx, types.NamespacedName{Name: tc.testDsc.Name}, tc.testDsc) - if err != nil { - return err - } - - fn(&tc.testDsc.Spec.Components) - - err = tc.customClient.Update(tc.ctx, tc.testDsc) - if err != nil { - return err - } - - return nil - } -} - func NewTestContext() (*testContext, error) { // GetConfig(): If KUBECONFIG env variable is set, it is used to create // the client, else the inClusterConfig() is used. diff --git a/tests/e2e/modelregistry_test.go b/tests/e2e/modelregistry_test.go index 5d1d168142b..417c6cc7896 100644 --- a/tests/e2e/modelregistry_test.go +++ b/tests/e2e/modelregistry_test.go @@ -8,6 +8,9 @@ import ( "github.com/stretchr/testify/require" autoscalingv1 "k8s.io/api/autoscaling/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" componentsv1 "github.com/opendatahub-io/opendatahub-operator/v2/apis/components/v1" @@ -16,6 +19,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq" . "github.com/onsi/gomega" @@ -25,6 +29,88 @@ type ModelRegistryTestCtx struct { *testContext } +func (mr *ModelRegistryTestCtx) WithT(t *testing.T) *WithT { + t.Helper() + + g := NewWithT(t) + g.SetDefaultEventuallyTimeout(generalWaitTimeout) + g.SetDefaultEventuallyPollingInterval(1 * time.Second) + + return g +} + +func (mr *ModelRegistryTestCtx) List( + gvk schema.GroupVersionKind, + option ...client.ListOption, +) func() ([]unstructured.Unstructured, error) { + return func() ([]unstructured.Unstructured, error) { + items := unstructured.UnstructuredList{} + items.SetGroupVersionKind(gvk) + + err := mr.customClient.List(mr.ctx, &items, option...) + if err != nil { + return nil, err + } + + return items.Items, nil + } +} + +func (mr *ModelRegistryTestCtx) Get( + gvk schema.GroupVersionKind, + ns string, + name string, + option ...client.GetOption, +) func() (*unstructured.Unstructured, error) { + return func() (*unstructured.Unstructured, error) { + u := unstructured.Unstructured{} + u.SetGroupVersionKind(gvk) + + err := mr.customClient.Get(mr.ctx, client.ObjectKey{Namespace: ns, Name: name}, &u, option...) + if err != nil { + return nil, err + } + + return &u, nil + } +} +func (mr *ModelRegistryTestCtx) MergePatch( + obj client.Object, + patch []byte, +) func() (*unstructured.Unstructured, error) { + return func() (*unstructured.Unstructured, error) { + u, err := resources.ToUnstructured(obj) + if err != nil { + return nil, err + } + + err = mr.customClient.Patch(mr.ctx, u, client.RawPatch(types.MergePatchType, patch)) + if err != nil { + return nil, err + } + + return u, nil + } +} + +func (mr *ModelRegistryTestCtx) updateComponent(fn func(dsc *dscv1.Components)) func() error { + return func() error { + err := mr.customClient.Get(mr.ctx, types.NamespacedName{Name: mr.testDsc.Name}, mr.testDsc) + if err != nil { + return err + } + + fn(&mr.testDsc.Spec.Components) + + err = mr.customClient.Update(mr.ctx, mr.testDsc) + if err != nil { + return err + } + + return nil + } +} + func modelRegistryTestSuite(t *testing.T) { tc, err := NewTestContext() require.NoError(t, err) @@ -44,32 +130,28 @@ func modelRegistryTestSuite(t *testing.T) { }) } -func (tc *ModelRegistryTestCtx) validateModelRegistryInstance(t *testing.T) { - g := NewWithT(t) - g.SetDefaultEventuallyTimeout(generalWaitTimeout) - g.SetDefaultEventuallyPollingInterval(1 * time.Second) +func (mr *ModelRegistryTestCtx) validateModelRegistryInstance(t *testing.T) { + g := mr.WithT(t) g.Eventually( - tc.List(gvk.ModelRegistry), + mr.List(gvk.ModelRegistry), ).Should(And( HaveLen(1), HaveEach(And( jq.Match(`.metadata.ownerReferences[0].kind == "%s"`, gvk.DataScienceCluster.Kind), - jq.Match(`.spec.registriesNamespace == "%s"`, tc.testDsc.Spec.Components.ModelRegistry.RegistriesNamespace), + jq.Match(`.spec.registriesNamespace == "%s"`, mr.testDsc.Spec.Components.ModelRegistry.RegistriesNamespace), jq.Match(`.status.phase == "%s"`, readyStatus), )), )) } -func (tc *ModelRegistryTestCtx) validateOperandsOwnerReferences(t *testing.T) { - g := NewWithT(t) - g.SetDefaultEventuallyTimeout(generalWaitTimeout) - g.SetDefaultEventuallyPollingInterval(generalRetryInterval) +func (mr *ModelRegistryTestCtx) validateOperandsOwnerReferences(t *testing.T) { + g := mr.WithT(t) g.Eventually( - tc.List( + mr.List( gvk.Deployment, - client.InNamespace(tc.applicationsNamespace), + client.InNamespace(mr.applicationsNamespace), client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, ), ).Should(And( @@ -80,13 +162,11 @@ func (tc *ModelRegistryTestCtx) validateOperandsOwnerReferences(t *testing.T) { )) } -func (tc *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t *testing.T) { - g := NewWithT(t) - g.SetDefaultEventuallyTimeout(generalWaitTimeout) - g.SetDefaultEventuallyPollingInterval(generalRetryInterval) +func (mr *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t *testing.T) { + g := mr.WithT(t) - appDeployments, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).List( - tc.ctx, + appDeployments, err := mr.kubeClient.AppsV1().Deployments(mr.applicationsNamespace).List( + mr.ctx, metav1.ListOptions{ LabelSelector: labels.ComponentManagedBy + "=" + componentsv1.ModelRegistryInstanceName, }, @@ -109,8 +189,8 @@ func (tc *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t * Status: autoscalingv1.ScaleStatus{}, } - updatedDep, err := tc.kubeClient.AppsV1().Deployments(tc.applicationsNamespace).UpdateScale( - tc.ctx, + updatedDep, err := mr.kubeClient.AppsV1().Deployments(mr.applicationsNamespace).UpdateScale( + mr.ctx, testDeployment.Name, patchedReplica, metav1.UpdateOptions{}, @@ -120,9 +200,9 @@ func (tc *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t * g.Expect(updatedDep.Spec.Replicas).Should(Equal(patchedReplica.Spec.Replicas)) g.Eventually( - tc.List( + mr.List( gvk.Deployment, - client.InNamespace(tc.applicationsNamespace), + client.InNamespace(mr.applicationsNamespace), client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, ), ).Should(And( @@ -133,9 +213,9 @@ func (tc *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t * )) g.Consistently( - tc.List( + mr.List( gvk.Deployment, - client.InNamespace(tc.applicationsNamespace), + client.InNamespace(mr.applicationsNamespace), client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, ), ).WithTimeout(30 * time.Second).WithPolling(1 * time.Second).Should(And( @@ -146,18 +226,16 @@ func (tc *ModelRegistryTestCtx) validateUpdateModelRegistryOperandsResources(t * )) } -func (tc *ModelRegistryTestCtx) validateModelRegistryCert(t *testing.T) { - g := NewWithT(t) - g.SetDefaultEventuallyTimeout(generalWaitTimeout) - g.SetDefaultEventuallyPollingInterval(generalRetryInterval) +func (mr *ModelRegistryTestCtx) validateModelRegistryCert(t *testing.T) { + g := mr.WithT(t) - is, err := cluster.FindDefaultIngressSecret(tc.ctx, tc.customClient) + is, err := cluster.FindDefaultIngressSecret(mr.ctx, mr.customClient) g.Expect(err).ShouldNot(HaveOccurred()) g.Eventually( - tc.Get( + mr.Get( gvk.Secret, - tc.testDSCI.Spec.ServiceMesh.ControlPlane.Namespace, + mr.testDSCI.Spec.ServiceMesh.ControlPlane.Namespace, modelregistry.DefaultModelRegistryCert, ), ).Should(And( @@ -167,27 +245,23 @@ func (tc *ModelRegistryTestCtx) validateModelRegistryCert(t *testing.T) { )) } -func (tc *ModelRegistryTestCtx) validateModelRegistryServiceMeshMember(t *testing.T) { - g := NewWithT(t) - g.SetDefaultEventuallyTimeout(generalWaitTimeout) - g.SetDefaultEventuallyPollingInterval(generalRetryInterval) +func (mr *ModelRegistryTestCtx) validateModelRegistryServiceMeshMember(t *testing.T) { + g := mr.WithT(t) g.Eventually( - tc.Get(gvk.ServiceMeshMember, modelregistry.DefaultModelRegistriesNamespace, "default"), + mr.Get(gvk.ServiceMeshMember, modelregistry.DefaultModelRegistriesNamespace, "default"), ).Should( jq.Match(`.spec | has("controlPlaneRef")`), ) } -func (tc *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { - g := NewWithT(t) - g.SetDefaultEventuallyTimeout(generalWaitTimeout) - g.SetDefaultEventuallyPollingInterval(generalRetryInterval) +func (mr *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { + g := mr.WithT(t) g.Eventually( - tc.List( + mr.List( gvk.Deployment, - client.InNamespace(tc.applicationsNamespace), + client.InNamespace(mr.applicationsNamespace), client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, ), ).Should( @@ -195,7 +269,7 @@ func (tc *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { ) g.Eventually( - tc.updateComponent(func(c *dscv1.Components) { + mr.updateComponent(func(c *dscv1.Components) { c.ModelRegistry.ManagementState = operatorv1.Removed }), ).ShouldNot( @@ -203,9 +277,9 @@ func (tc *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { ) g.Eventually( - tc.List( + mr.List( gvk.Deployment, - client.InNamespace(tc.applicationsNamespace), + client.InNamespace(mr.applicationsNamespace), client.MatchingLabels{labels.ComponentManagedBy: componentsv1.ModelRegistryInstanceName}, ), ).Should( @@ -213,7 +287,7 @@ func (tc *ModelRegistryTestCtx) validateModelRegistryDisabled(t *testing.T) { ) g.Eventually( - tc.List(gvk.ModelRegistry), + mr.List(gvk.ModelRegistry), ).Should( BeEmpty(), ) From 6e74e0ad79c2bcd7321f7df7d64c8b2ea3894d59 Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Mon, 4 Nov 2024 16:35:00 +0100 Subject: [PATCH 3/5] Create ModelRegistry component API and reconciler (findings) --- ...atahub-operator.clusterserviceversion.yaml | 2 +- .../components/modelregistry/modelregistry.go | 4 ++-- .../modelregistry_controller_actions.go | 24 +++++++++---------- .../modelregistry/modelregistry_support.go | 4 ++-- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml index 69a254b1301..d55386df0e6 100644 --- a/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml +++ b/bundle/manifests/opendatahub-operator.clusterserviceversion.yaml @@ -1210,7 +1210,7 @@ spec: value: /opt/manifests - name: ODH_PLATFORM_TYPE value: OpenDataHub - image: ttl.sh/882e644b-532e-4feb-95d4-2bd23fb2e6df:2h + image: REPLACE_IMAGE:latest imagePullPolicy: Always livenessProbe: httpGet: diff --git a/controllers/components/modelregistry/modelregistry.go b/controllers/components/modelregistry/modelregistry.go index 3ff8d69bfd4..cc9786a896f 100644 --- a/controllers/components/modelregistry/modelregistry.go +++ b/controllers/components/modelregistry/modelregistry.go @@ -13,8 +13,8 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations" ) -func Init(platform cluster.Platform) error { - mi := baseManifestInfo(platform, BaseManifestsSourcePath) +func Init(_ cluster.Platform) error { + mi := baseManifestInfo(BaseManifestsSourcePath) params := make(map[string]string) for k, v := range imagesMap { diff --git a/controllers/components/modelregistry/modelregistry_controller_actions.go b/controllers/components/modelregistry/modelregistry_controller_actions.go index 9f0f848e307..59f63564ff7 100644 --- a/controllers/components/modelregistry/modelregistry_controller_actions.go +++ b/controllers/components/modelregistry/modelregistry_controller_actions.go @@ -29,7 +29,7 @@ const ( ) func gate(_ context.Context, rr *odhtypes.ReconciliationRequest) error { - obj, ok := rr.Instance.(odhtypes.ResourceObject) + mr, ok := rr.Instance.(*componentsv1.ModelRegistry) if !ok { return fmt.Errorf("resource instance %v is not a ResourceObject", rr.Instance) } @@ -38,7 +38,7 @@ func gate(_ context.Context, rr *odhtypes.ReconciliationRequest) error { return nil } - s := obj.GetStatus() + s := mr.GetStatus() s.Phase = "NotReady" meta.SetStatusCondition(&s.Conditions, metav1.Condition{ @@ -53,17 +53,17 @@ func gate(_ context.Context, rr *odhtypes.ReconciliationRequest) error { } func initialize(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { - c, ok := rr.Instance.(*componentsv1.ModelRegistry) + mr, ok := rr.Instance.(*componentsv1.ModelRegistry) if !ok { return fmt.Errorf("resource instance %v is not a componentsv1.ModelRegistry)", rr.Instance) } rr.Manifests = []odhtypes.ManifestInfo{ - baseManifestInfo(rr.Release.Name, BaseManifestsSourcePath), - extraManifestInfo(rr.Release.Name, BaseManifestsSourcePath), + baseManifestInfo(BaseManifestsSourcePath), + extraManifestInfo(BaseManifestsSourcePath), } - df := c.GetDevFlags() + df := mr.GetDevFlags() if df == nil { return nil @@ -81,8 +81,8 @@ func initialize(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { if df.Manifests[0].SourcePath != "" { rr.Manifests = []odhtypes.ManifestInfo{ - baseManifestInfo(rr.Release.Name, df.Manifests[0].SourcePath), - extraManifestInfo(rr.Release.Name, df.Manifests[0].SourcePath), + baseManifestInfo(df.Manifests[0].SourcePath), + extraManifestInfo(df.Manifests[0].SourcePath), } } @@ -90,7 +90,7 @@ func initialize(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { } func configureDependencies(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { - c, ok := rr.Instance.(*componentsv1.ModelRegistry) + mr, ok := rr.Instance.(*componentsv1.ModelRegistry) if !ok { return fmt.Errorf("resource instance %v is not a componentsv1.ModelRegistry)", rr.Instance) } @@ -100,11 +100,11 @@ func configureDependencies(ctx context.Context, rr *odhtypes.ReconciliationReque if err := rr.AddResource( &corev1.Namespace{ ObjectMeta: metav1.ObjectMeta{ - Name: c.Spec.RegistriesNamespace, + Name: mr.Spec.RegistriesNamespace, }, }, ); err != nil { - return fmt.Errorf("failed to add namespace %s to manifests", c.Spec.RegistriesNamespace) + return fmt.Errorf("failed to add namespace %s to manifests", mr.Spec.RegistriesNamespace) } // Secret @@ -130,7 +130,7 @@ func configureDependencies(ctx context.Context, rr *odhtypes.ReconciliationReque // Service Mesh - smm, err := createServiceMeshMember(rr.DSCI, c.Spec.RegistriesNamespace) + smm, err := createServiceMeshMember(rr.DSCI, mr.Spec.RegistriesNamespace) if err != nil { return fmt.Errorf("failed to create ServiceMesh Member: %w", err) } diff --git a/controllers/components/modelregistry/modelregistry_support.go b/controllers/components/modelregistry/modelregistry_support.go index 2ba98d3487b..b3d0e29d78e 100644 --- a/controllers/components/modelregistry/modelregistry_support.go +++ b/controllers/components/modelregistry/modelregistry_support.go @@ -45,7 +45,7 @@ var ( //go:embed resources/servicemesh-member.tmpl.yaml var smmTemplate string -func baseManifestInfo(_ cluster.Platform, sourcePath string) odhtypes.ManifestInfo { +func baseManifestInfo(sourcePath string) odhtypes.ManifestInfo { return odhtypes.ManifestInfo{ Path: deploy.DefaultManifestPath, ContextDir: ComponentName, @@ -53,7 +53,7 @@ func baseManifestInfo(_ cluster.Platform, sourcePath string) odhtypes.ManifestIn } } -func extraManifestInfo(_ cluster.Platform, sourcePath string) odhtypes.ManifestInfo { +func extraManifestInfo(sourcePath string) odhtypes.ManifestInfo { return odhtypes.ManifestInfo{ Path: deploy.DefaultManifestPath, ContextDir: ComponentName, From 5f888dadf103a981bb1ba6d2e186bfd1c317115d Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Mon, 4 Nov 2024 17:45:10 +0100 Subject: [PATCH 4/5] Update modelregistry_controller_actions.go Co-authored-by: Gerard Ryan --- .../modelregistry/modelregistry_controller_actions.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/components/modelregistry/modelregistry_controller_actions.go b/controllers/components/modelregistry/modelregistry_controller_actions.go index 59f63564ff7..d5a3459aa75 100644 --- a/controllers/components/modelregistry/modelregistry_controller_actions.go +++ b/controllers/components/modelregistry/modelregistry_controller_actions.go @@ -31,7 +31,7 @@ const ( func gate(_ context.Context, rr *odhtypes.ReconciliationRequest) error { mr, ok := rr.Instance.(*componentsv1.ModelRegistry) if !ok { - return fmt.Errorf("resource instance %v is not a ResourceObject", rr.Instance) + return fmt.Errorf("resource instance %v is not a componentsv1.ModelRegistry", rr.Instance) } if rr.DSCI.Spec.ServiceMesh != nil && rr.DSCI.Spec.ServiceMesh.ManagementState == operatorv1.Managed { From 40e60530bed91d8cc5c1fed73f460bcaea1d8c3f Mon Sep 17 00:00:00 2001 From: Luca Burgazzoli Date: Tue, 5 Nov 2024 09:45:12 +0100 Subject: [PATCH 5/5] Create ModelRegistry component API and reconciler (findings) --- .../modelregistry/modelregistry_controller.go | 2 +- .../modelregistry_controller_actions.go | 13 ++++--------- controllers/status/status.go | 5 +++++ pkg/controller/predicates/resources/resources.go | 15 --------------- tests/e2e/helper_test.go | 3 ++- 5 files changed, 12 insertions(+), 26 deletions(-) diff --git a/controllers/components/modelregistry/modelregistry_controller.go b/controllers/components/modelregistry/modelregistry_controller.go index f80456bd256..20e4bbff3e1 100644 --- a/controllers/components/modelregistry/modelregistry_controller.go +++ b/controllers/components/modelregistry/modelregistry_controller.go @@ -71,7 +71,7 @@ func NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error { handlers.ToNamed(componentsv1.ModelRegistryInstanceName), builder.WithPredicates(ingressSecret(ctx, mgr.GetClient()))). // actions - WithAction(gate). + WithAction(checkPreConditions). WithAction(initialize). WithAction(configureDependencies). WithAction(render.NewAction( diff --git a/controllers/components/modelregistry/modelregistry_controller_actions.go b/controllers/components/modelregistry/modelregistry_controller_actions.go index d5a3459aa75..b487e94adce 100644 --- a/controllers/components/modelregistry/modelregistry_controller_actions.go +++ b/controllers/components/modelregistry/modelregistry_controller_actions.go @@ -23,12 +23,7 @@ import ( _ "embed" ) -const ( - ServiceMeshNotConfiguredReason = "ServiceMeshNotConfigured" - ServiceMeshNotConfiguredMessage = "ServiceMesh needs to be set to 'Managed' in DSCI CR, it is required by Model Registry" -) - -func gate(_ context.Context, rr *odhtypes.ReconciliationRequest) error { +func checkPreConditions(_ context.Context, rr *odhtypes.ReconciliationRequest) error { mr, ok := rr.Instance.(*componentsv1.ModelRegistry) if !ok { return fmt.Errorf("resource instance %v is not a componentsv1.ModelRegistry", rr.Instance) @@ -44,12 +39,12 @@ func gate(_ context.Context, rr *odhtypes.ReconciliationRequest) error { meta.SetStatusCondition(&s.Conditions, metav1.Condition{ Type: status.ConditionTypeReady, Status: metav1.ConditionFalse, - Reason: ServiceMeshNotConfiguredReason, - Message: ServiceMeshNotConfiguredMessage, + Reason: status.ServiceMeshNotConfiguredReason, + Message: status.ServiceMeshNotConfiguredMessage, ObservedGeneration: s.ObservedGeneration, }) - return odherrors.NewStopError(ServiceMeshNotConfiguredMessage) + return odherrors.NewStopError(status.ServiceMeshNotConfiguredMessage) } func initialize(ctx context.Context, rr *odhtypes.ReconciliationRequest) error { diff --git a/controllers/status/status.go b/controllers/status/status.go index b47c23a037e..71558191eab 100644 --- a/controllers/status/status.go +++ b/controllers/status/status.go @@ -89,6 +89,11 @@ const ( ReadySuffix = "Ready" ) +const ( + ServiceMeshNotConfiguredReason = "ServiceMeshNotConfigured" + ServiceMeshNotConfiguredMessage = "ServiceMesh needs to be set to 'Managed' in DSCI CR" +) + // SetProgressingCondition sets the ProgressingCondition to True and other conditions to false or // Unknown. Used when we are just starting to reconcile, and there are no existing conditions. func SetProgressingCondition(conditions *[]conditionsv1.Condition, reason string, message string) { diff --git a/pkg/controller/predicates/resources/resources.go b/pkg/controller/predicates/resources/resources.go index 71435e399dd..8a072af1c89 100644 --- a/pkg/controller/predicates/resources/resources.go +++ b/pkg/controller/predicates/resources/resources.go @@ -2,7 +2,6 @@ package resources import ( appsv1 "k8s.io/api/apps/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/predicate" ) @@ -37,17 +36,3 @@ func (DeploymentPredicate) Update(e event.UpdateEvent) bool { func NewDeploymentPredicate() *DeploymentPredicate { return &DeploymentPredicate{} } - -func NamespacedNamed(nn types.NamespacedName) predicate.Funcs { - return predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { - return false - }, - DeleteFunc: func(e event.DeleteEvent) bool { - return e.Object.GetName() == nn.Name && e.Object.GetNamespace() == nn.Namespace - }, - UpdateFunc: func(e event.UpdateEvent) bool { - return e.ObjectNew.GetName() == nn.Name && e.ObjectNew.GetNamespace() == nn.Namespace - }, - } -} diff --git a/tests/e2e/helper_test.go b/tests/e2e/helper_test.go index b9dc2159a96..ef82d7ad6db 100644 --- a/tests/e2e/helper_test.go +++ b/tests/e2e/helper_test.go @@ -35,6 +35,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/components/modelregistry" ) const ( @@ -171,7 +172,7 @@ func setupDSCInstance(name string) *dscv1.DataScienceCluster { ManagementState: operatorv1.Managed, }, ModelRegistryCommonSpec: componentsv1.ModelRegistryCommonSpec{ - RegistriesNamespace: "odh-model-registries", + RegistriesNamespace: modelregistry.DefaultModelRegistriesNamespace, }, }, TrainingOperator: trainingoperator.TrainingOperator{