From e368740243fb37a0375da68b67e488e7127a6a3c Mon Sep 17 00:00:00 2001 From: Andrea Lamparelli Date: Thu, 8 Feb 2024 10:05:33 +0100 Subject: [PATCH] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Edgar Hernández Signed-off-by: Andrea Lamparelli --- controllers/constants/constants.go | 8 +-- controllers/mr_inferenceservice_controller.go | 60 +++---------------- .../deploy/inference-service-to-delete.yaml | 2 +- .../inference-service-with-model-version.yaml | 4 +- ...ference-service-without-model-version.yaml | 2 +- test/e2e/e2e_suite_test.go | 11 +++- test/e2e/modelregistry_controller_e2e_test.go | 28 ++++----- 7 files changed, 40 insertions(+), 75 deletions(-) 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..745760a5 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 { @@ -181,8 +171,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 +184,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) } 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()) }