Apply suggestions from code review

Co-authored-by: Edgar Hernández <[email protected]>
Signed-off-by: Andrea Lamparelli <[email protected]>

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"
 )

controllers/mr_inferenceservice_controller.go

@@ -7,27 +7,21 @@ 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"
 	"github.com/opendatahub-io/model-registry/pkg/openapi"
 	"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,23 +126,19 @@ 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
 		is, err = r.createMRInferenceService(log, mr, isvc, *servingEnvironment.Id, registeredModelId, modelVersionId)
 		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

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:

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:

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:

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))

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())
 }