diff --git a/controllers/constants/constants.go b/controllers/constants/constants.go index e2961ca7..b1837a58 100644 --- a/controllers/constants/constants.go +++ b/controllers/constants/constants.go @@ -25,8 +25,8 @@ const ( // model registry const ( MLMDAddressEnv = "MLMD_ADDRESS" - ModelRegistryNamespaceLabel = "mr-namespace" - ModelRegistryInferenceServiceIdLabel = "mr-inference-service-id" - ModelRegistryModelVersionIdLabel = "mr-model-version-id" - ModelRegistryRegisteredModelIdLabel = "mr-registered-model-id" + ModelRegistryNamespaceLabel = "modelregistry.opendatahub.io/namespace" + ModelRegistryInferenceServiceIdLabel = "modelregistry.opendatahub.io/inference-service-id" + ModelRegistryModelVersionIdLabel = "modelregistry.opendatahub.io/model-version-id" + ModelRegistryRegisteredModelIdLabel = "modelregistry.opendatahub.io/registered-model-id" ) diff --git a/controllers/mr_inferenceservice_controller.go b/controllers/mr_inferenceservice_controller.go index 60542446..3d92dc70 100644 --- a/controllers/mr_inferenceservice_controller.go +++ b/controllers/mr_inferenceservice_controller.go @@ -7,7 +7,6 @@ import ( "time" "github.com/go-logr/logr" - kservev1alpha1 "github.com/kserve/kserve/pkg/apis/serving/v1alpha1" kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1" "github.com/opendatahub-io/model-registry/pkg/api" "github.com/opendatahub-io/model-registry/pkg/core" @@ -15,19 +14,14 @@ import ( "github.com/opendatahub-io/odh-model-controller/controllers/comparators" "github.com/opendatahub-io/odh-model-controller/controllers/constants" "github.com/opendatahub-io/odh-model-controller/controllers/processors" - monitoringv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1" "google.golang.org/grpc" "google.golang.org/grpc/credentials/insecure" corev1 "k8s.io/api/core/v1" apierrs "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/types" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/reconcile" - "sigs.k8s.io/controller-runtime/pkg/source" ) const modelRegistryFinalizer = "modelregistry.opendatahub.io/finalizer" @@ -59,18 +53,18 @@ func (r *ModelRegistryInferenceServiceReconciler) Reconcile(ctx context.Context, isvc := &kservev1beta1.InferenceService{} err := r.client.Get(ctx, req.NamespacedName, isvc) if err != nil && apierrs.IsNotFound(err) { - log.Error(err, "Stop ModelRegistry InferenceService reconciliation") + log.V(1).Info("Stop ModelRegistry InferenceService reconciliation, ISVC not found.") return ctrl.Result{}, nil } else if err != nil { log.Error(err, "Unable to fetch the InferenceService") return ctrl.Result{}, err } - isId, okIsId := isvc.Labels[constants.ModelRegistryInferenceServiceIdLabel] + mrIsvcId, okMrIsvcId := isvc.Labels[constants.ModelRegistryInferenceServiceIdLabel] registeredModelId, okRegisteredModelId := isvc.Labels[constants.ModelRegistryRegisteredModelIdLabel] modelVersionId, okModelVersionId := isvc.Labels[constants.ModelRegistryModelVersionIdLabel] - if !okIsId && !(okRegisteredModelId || okModelVersionId) { + if !okMrIsvcId && !(okRegisteredModelId || okModelVersionId) { // Early check: no model registry specific labels set in the ISVC, ignore the CR log.Error(fmt.Errorf("missing model registry specific label, unable to link ISVC to Model Registry, skipping InferenceService"), "Stop ModelRegistry InferenceService reconciliation") return ctrl.Result{}, nil @@ -132,12 +126,12 @@ func (r *ModelRegistryInferenceServiceReconciler) Reconcile(ctx context.Context, var is *openapi.InferenceService - if okIsId { + if okMrIsvcId { // Retrieve the IS from model registry using the id - log.Info("Retrieving model registry InferenceService by id", "isId", isId) - is, err = mr.GetInferenceServiceById(isId) + log.Info("Retrieving model registry InferenceService by id", "mrIsvcId", mrIsvcId) + is, err = mr.GetInferenceServiceById(mrIsvcId) if err != nil { - return ctrl.Result{}, fmt.Errorf("unable to find InferenceService with id %s in model registry: %w", isId, err) + return ctrl.Result{}, fmt.Errorf("unable to find InferenceService with id %s in model registry: %w", mrIsvcId, err) } } else if okRegisteredModelId || okModelVersionId { // No corresponding InferenceService in model registry, create new one @@ -145,10 +139,6 @@ func (r *ModelRegistryInferenceServiceReconciler) Reconcile(ctx context.Context, if err != nil { return ctrl.Result{}, err } - } else { - // No expected labels set in the ISVC - log.Error(fmt.Errorf("missing label, unable to link ISVC to Model Registry, skipping InferenceService"), "Stop ModelRegistry InferenceService reconciliation") - return ctrl.Result{}, nil } if is == nil { @@ -163,8 +153,6 @@ func (r *ModelRegistryInferenceServiceReconciler) Reconcile(ctx context.Context, } if controllerutil.ContainsFinalizer(isvc, modelRegistryFinalizer) { - log.Info("InferenceService going to be deleted from cluster, setting desired state to UNDEPLOYED in model registry") - log.Info("Removing Finalizer for modelRegistry after successfully perform the operations") if ok := controllerutil.RemoveFinalizer(isvc, modelRegistryFinalizer); !ok { log.Error(err, "Failed to remove modelRegistry finalizer for InferenceService") @@ -181,8 +169,6 @@ func (r *ModelRegistryInferenceServiceReconciler) Reconcile(ctx context.Context, // Update the ISVC label, set the newly created IS id if not present yet desired := isvc.DeepCopy() desired.Labels[constants.ModelRegistryInferenceServiceIdLabel] = *is.Id - delete(desired.Labels, constants.ModelRegistryRegisteredModelIdLabel) - delete(desired.Labels, constants.ModelRegistryModelVersionIdLabel) err = r.processDelta(ctx, log, desired, isvc) if err != nil { @@ -196,39 +182,7 @@ func (r *ModelRegistryInferenceServiceReconciler) Reconcile(ctx context.Context, // SetupWithManager sets up the controller with the Manager. func (r *ModelRegistryInferenceServiceReconciler) SetupWithManager(mgr ctrl.Manager) error { builder := ctrl.NewControllerManagedBy(mgr). - For(&kservev1beta1.InferenceService{}). - Owns(&kservev1alpha1.ServingRuntime{}). - Owns(&corev1.Namespace{}). - Owns(&monitoringv1.PodMonitor{}). - Watches(&source.Kind{Type: &kservev1alpha1.ServingRuntime{}}, - handler.EnqueueRequestsFromMapFunc(func(o client.Object) []reconcile.Request { - r.log.Info("Reconcile event triggered by serving runtime: " + o.GetName()) - inferenceServicesList := &kservev1beta1.InferenceServiceList{} - opts := []client.ListOption{client.InNamespace(o.GetNamespace())} - - // Todo: Get only Inference Services that are deploying on the specific serving runtime - err := r.client.List(context.TODO(), inferenceServicesList, opts...) - if err != nil { - r.log.Info("Error getting list of inference services for namespace") - return []reconcile.Request{} - } - - if len(inferenceServicesList.Items) == 0 { - r.log.Info("No InferenceServices found for Serving Runtime: " + o.GetName()) - return []reconcile.Request{} - } - - reconcileRequests := make([]reconcile.Request, 0, len(inferenceServicesList.Items)) - for _, inferenceService := range inferenceServicesList.Items { - reconcileRequests = append(reconcileRequests, reconcile.Request{ - NamespacedName: types.NamespacedName{ - Name: inferenceService.Name, - Namespace: inferenceService.Namespace, - }, - }) - } - return reconcileRequests - })) + For(&kservev1beta1.InferenceService{}) return builder.Complete(r) } @@ -333,11 +287,14 @@ func (r *ModelRegistryInferenceServiceReconciler) createMRInferenceService( } // onDeletion mark model registry inference service to UNDEPLOYED desired state -func (r *ModelRegistryInferenceServiceReconciler) onDeletion(mr api.ModelRegistryApi, log logr.Logger, isvc *kservev1beta1.InferenceService, is *openapi.InferenceService) error { +func (r *ModelRegistryInferenceServiceReconciler) onDeletion(mr api.ModelRegistryApi, log logr.Logger, isvc *kservev1beta1.InferenceService, is *openapi.InferenceService) (err error) { log.Info("Running onDeletion logic") - is.DesiredState = openapi.INFERENCESERVICESTATE_UNDEPLOYED.Ptr() + if is.DesiredState != nil && *is.DesiredState != openapi.INFERENCESERVICESTATE_UNDEPLOYED { + log.Info("InferenceService going to be deleted from cluster, setting desired state to UNDEPLOYED in model registry") + is.DesiredState = openapi.INFERENCESERVICESTATE_UNDEPLOYED.Ptr() - _, err := mr.UpsertInferenceService(is) + _, err = mr.UpsertInferenceService(is) + } return err } @@ -407,7 +364,7 @@ func IgnoreDeletingErrors(err error) error { if err == nil { return nil } - if apierrs.IsNotFound(err) || apierrs.IsConflict(err) { + if apierrs.IsNotFound(err) { return nil } return err diff --git a/test/data/deploy/inference-service-to-delete.yaml b/test/data/deploy/inference-service-to-delete.yaml index f8928d20..ea48e16c 100644 --- a/test/data/deploy/inference-service-to-delete.yaml +++ b/test/data/deploy/inference-service-to-delete.yaml @@ -4,7 +4,7 @@ metadata: name: dummy-inference-service namespace: default labels: - "mr-inference-service-id": "4" + "modelregistry.opendatahub.io/inference-service-id": "4" finalizers: - modelregistry.opendatahub.io/finalizer spec: diff --git a/test/data/deploy/inference-service-with-model-version.yaml b/test/data/deploy/inference-service-with-model-version.yaml index ec7098f8..82a9f465 100644 --- a/test/data/deploy/inference-service-with-model-version.yaml +++ b/test/data/deploy/inference-service-with-model-version.yaml @@ -4,8 +4,8 @@ metadata: name: dummy-inference-service namespace: default labels: - "mr-registered-model-id": "1" - "mr-model-version-id": "2" + "modelregistry.opendatahub.io/registered-model-id": "1" + "modelregistry.opendatahub.io/model-version-id": "2" spec: predictor: model: diff --git a/test/data/deploy/inference-service-without-model-version.yaml b/test/data/deploy/inference-service-without-model-version.yaml index 7b29f940..0f7ada21 100644 --- a/test/data/deploy/inference-service-without-model-version.yaml +++ b/test/data/deploy/inference-service-without-model-version.yaml @@ -4,7 +4,7 @@ metadata: name: dummy-inference-service namespace: default labels: - "mr-registered-model-id": "1" + "modelregistry.opendatahub.io/registered-model-id": "1" spec: predictor: model: diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 70044e89..f07c0608 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -3,6 +3,7 @@ package e2e import ( "context" "fmt" + "os" "testing" "time" @@ -26,6 +27,7 @@ import ( ) const ( + KubectlCmdEnv = "KUBECTL" WorkingNamespace = "default" ModelRegistryDeploymentPath = "./test/data/model-registry/modelregistry_deployment.yaml" ModelRegistryDatabaseDeploymentPath = "./test/data/model-registry/database_deployment.yaml" @@ -38,7 +40,8 @@ const ( ) var ( - scheme = runtime.NewScheme() + scheme = runtime.NewScheme() + kubectl = "kubectl" ctx context.Context cancel context.CancelFunc @@ -75,6 +78,12 @@ var _ = BeforeSuite(func() { Expect(err).NotTo(HaveOccurred()) Expect(cli).NotTo(BeNil()) + // Override kubectl cmd + cmd, ok := os.LookupEnv(KubectlCmdEnv) + if ok && cmd != "" { + kubectl = cmd + } + // Register API objects utilruntime.Must(clientgoscheme.AddToScheme(scheme)) utilruntime.Must(kservev1alpha1.AddToScheme(scheme)) diff --git a/test/e2e/modelregistry_controller_e2e_test.go b/test/e2e/modelregistry_controller_e2e_test.go index ae17c3ad..2af456d2 100644 --- a/test/e2e/modelregistry_controller_e2e_test.go +++ b/test/e2e/modelregistry_controller_e2e_test.go @@ -69,18 +69,18 @@ var _ = Describe("ModelRegistry controller e2e", func() { Expect(modelVersion.GetId()).To(Equal("2")) Expect(modelArtifact.GetId()).To(Equal("1")) - _, err := utils.Run(exec.Command("kubectl", "apply", "-f", ServingRuntimePath1)) + _, err := utils.Run(exec.Command(kubectl, "apply", "-f", ServingRuntimePath1)) Expect(err).ToNot(HaveOccurred()) }) AfterEach(func() { By("removing finalizers from inference service") - _, err := utils.Run(exec.Command("kubectl", "patch", "inferenceservice", "dummy-inference-service", "--type", "json", "--patch", "[ { \"op\": \"remove\", \"path\": \"/metadata/finalizers\" } ]")) + _, err := utils.Run(exec.Command(kubectl, "patch", "inferenceservice", "dummy-inference-service", "--type", "json", "--patch", "[ { \"op\": \"remove\", \"path\": \"/metadata/finalizers\" } ]")) Expect(err).ToNot(HaveOccurred()) }) It("the controller should create InferenceService with specific model version in model registry", func() { - _, err := utils.Run(exec.Command("kubectl", "apply", "-f", InferenceServiceWithModelVersionPath)) + _, err := utils.Run(exec.Command(kubectl, "apply", "-f", InferenceServiceWithModelVersionPath)) Expect(err).ToNot(HaveOccurred()) var is *openapi.InferenceService @@ -109,14 +109,14 @@ var _ = Describe("ModelRegistry controller e2e", func() { return ok }, timeout, interval).Should(BeTrue()) - Expect(actualISVC.Labels[constants.ModelRegistryRegisteredModelIdLabel]).To(Equal("")) - Expect(actualISVC.Labels[constants.ModelRegistryModelVersionIdLabel]).To(Equal("")) + Expect(actualISVC.Labels[constants.ModelRegistryRegisteredModelIdLabel]).To(Equal("1")) + Expect(actualISVC.Labels[constants.ModelRegistryModelVersionIdLabel]).To(Equal("2")) Expect(actualISVC.Finalizers[0]).To(Equal("modelregistry.opendatahub.io/finalizer")) }) It("the controller should create InferenceService without specific model version in model registry", func() { - _, err := utils.Run(exec.Command("kubectl", "apply", "-f", InferenceServiceWithoutModelVersionPath)) + _, err := utils.Run(exec.Command(kubectl, "apply", "-f", InferenceServiceWithoutModelVersionPath)) Expect(err).ToNot(HaveOccurred()) var is *openapi.InferenceService @@ -144,7 +144,7 @@ var _ = Describe("ModelRegistry controller e2e", func() { return ok }, timeout, interval).Should(BeTrue()) - Expect(actualISVC.Labels[constants.ModelRegistryRegisteredModelIdLabel]).To(Equal("")) + Expect(actualISVC.Labels[constants.ModelRegistryRegisteredModelIdLabel]).To(Equal("1")) Expect(actualISVC.Labels[constants.ModelRegistryModelVersionIdLabel]).To(Equal("")) Expect(actualISVC.Finalizers[0]).To(Equal("modelregistry.opendatahub.io/finalizer")) }) @@ -176,15 +176,15 @@ var _ = Describe("ModelRegistry controller e2e", func() { Expect(err).ToNot(HaveOccurred()) Expect(inferenceService.GetId()).To(Equal("4")) - _, err = utils.Run(exec.Command("kubectl", "apply", "-f", ServingRuntimePath1)) + _, err = utils.Run(exec.Command(kubectl, "apply", "-f", ServingRuntimePath1)) Expect(err).ToNot(HaveOccurred()) - _, err = utils.Run(exec.Command("kubectl", "apply", "-f", InferenceServiceWithInfServiceIdPath)) + _, err = utils.Run(exec.Command(kubectl, "apply", "-f", InferenceServiceWithInfServiceIdPath)) Expect(err).ToNot(HaveOccurred()) }) It("the controller should set the InferenceService desired state to UNDEPLOYED", func() { - _, err := utils.Run(exec.Command("kubectl", "delete", "-f", InferenceServiceWithInfServiceIdPath)) + _, err := utils.Run(exec.Command(kubectl, "delete", "-f", InferenceServiceWithInfServiceIdPath)) Expect(err).ToNot(HaveOccurred()) var is *openapi.InferenceService @@ -212,11 +212,11 @@ var _ = Describe("ModelRegistry controller e2e", func() { // deployAndCheckModelRegistry setup model registry deployments and creates model registry client connection func deployAndCheckModelRegistry() api.ModelRegistryApi { - cmd := exec.Command("kubectl", "apply", "-f", ModelRegistryDatabaseDeploymentPath) + cmd := exec.Command(kubectl, "apply", "-f", ModelRegistryDatabaseDeploymentPath) _, err := utils.Run(cmd) Expect(err).ToNot(HaveOccurred()) - cmd = exec.Command("kubectl", "apply", "-f", ModelRegistryDeploymentPath) + cmd = exec.Command(kubectl, "apply", "-f", ModelRegistryDeploymentPath) _, err = utils.Run(cmd) Expect(err).ToNot(HaveOccurred()) @@ -257,11 +257,11 @@ func deployAndCheckModelRegistry() api.ModelRegistryApi { // undeployModelRegistry cleanup model registry deployments func undeployModelRegistry() { - cmd := exec.Command("kubectl", "delete", "-f", ModelRegistryDeploymentPath) + cmd := exec.Command(kubectl, "delete", "-f", ModelRegistryDeploymentPath) _, err = utils.Run(cmd) Expect(err).ToNot(HaveOccurred()) - cmd = exec.Command("kubectl", "delete", "-f", ModelRegistryDatabaseDeploymentPath) + cmd = exec.Command(kubectl, "delete", "-f", ModelRegistryDatabaseDeploymentPath) _, err = utils.Run(cmd) Expect(err).ToNot(HaveOccurred()) }