Skip to content

Commit

Permalink
feat(isvc): made name and url optional - added support to svc url ann…
Browse files Browse the repository at this point in the history 
…otation

Signed-off-by: Alessio Pragliola <[email protected]>
  • Loading branch information
@Al-Pragliola
Al-Pragliola committed Jan 3, 2025
1 parent 5113434 commit ce17ed9
Show file tree
Hide file tree
Showing 6 changed files with 283 additions and 46 deletions.
55 changes: 46 additions & 9 deletions pkg/inferenceservice-controller/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ type InferenceServiceController struct {
modelRegistryNameLabel string
modelRegistryURLAnnotation string
modelRegistryFinalizer string
serviceURLAnnotation string
defaultModelRegistryNamespace string
}

Expand All @@ -44,6 +45,7 @@ func NewInferenceServiceController(
mrNameLabel,
mrURLAnnotation,
mrFinalizer,
serviceURLAnnotation,
defaultMRNamespace string,
) *InferenceServiceController {
httpClient := http.DefaultClient
Expand All @@ -66,6 +68,7 @@ func NewInferenceServiceController(
modelRegistryNameLabel: mrNameLabel,
modelRegistryURLAnnotation: mrURLAnnotation,
modelRegistryFinalizer: mrFinalizer,
serviceURLAnnotation: serviceURLAnnotation,
defaultModelRegistryNamespace: defaultMRNamespace,
}
}
Expand Down Expand Up @@ -112,9 +115,8 @@ func (r *InferenceServiceController) Reconcile(ctx context.Context, req ctrl.Req
}

if !okMrName && !okMrUrl {
// Early check: it's required to have the model registry name or url set in the ISVC
log.Error(fmt.Errorf("missing model registry name or url, unable to link ISVC to Model Registry, skipping InferenceService"), "Stop ModelRegistry InferenceService reconciliation")
return ctrl.Result{}, nil
// Early check: it's optional to have the model registry name or url set in the ISVC, but if not set, it will fail if there's more than one model registry in the namespace
log.Info(fmt.Sprintf("missing %s or %s, will try to connect to the Model Registry service in the namespace %s", r.modelRegistryNameLabel, r.modelRegistryURLAnnotation, mrNamespace))
}

if mrNSFromISVC, ok := isvc.Labels[r.modelRegistryNamespaceLabel]; ok {
Expand All @@ -124,7 +126,7 @@ func (r *InferenceServiceController) Reconcile(ctx context.Context, req ctrl.Req
log.Info("Creating model registry service..")
mrApi, err := r.initModelRegistryService(ctx, log, mrName, mrNamespace, mrUrl)
if err != nil {
log.Error(err, "Unable to initialize Model Registry Service")
log.Error(err, "Unable to initialize Model Registry service")
return ctrl.Result{}, err
}

Expand Down Expand Up @@ -229,11 +231,11 @@ func (r *InferenceServiceController) initModelRegistryService(ctx context.Contex
log1 := log.WithValues("mr-namespace", namespace, "mr-name", name)

if url == "" {
log1.Info("Retrieving api http port from deployed model registry service")
log1.Info("Retrieving url from deployed model registry service")

url, err = r.getMRUrlFromService(ctx, name, namespace)
if err != nil {
log1.Error(err, "Unable to fetch the Model Registry Service")
log1.Error(err, "Unable to fetch the Model Registry service")
return nil, err
}
}
Expand All @@ -253,15 +255,50 @@ func (r *InferenceServiceController) initModelRegistryService(ctx context.Contex
}

func (r *InferenceServiceController) getMRUrlFromService(ctx context.Context, name, namespace string) (string, error) {
svc, err := r.getMRService(ctx, name, namespace)
if err != nil {
return "", fmt.Errorf("unable to find the Model Registry service: %w", err)
}

return r.buildURLFromService(svc)
}

func (r *InferenceServiceController) getMRService(ctx context.Context, name, namespace string) (*corev1.Service, error) {
if name == "" {
svcList := &corev1.ServiceList{}

if err := r.client.List(ctx, svcList, client.InNamespace(namespace), client.MatchingLabels{"component": "model-registry"}); err != nil {
return nil, fmt.Errorf("unable to list services in the namespace %s: %w", namespace, err)
}

if len(svcList.Items) == 0 {
return nil, fmt.Errorf("no model registry services found in the namespace %s", namespace)
}

if len(svcList.Items) > 1 {
return nil, fmt.Errorf("more than one model registry service found in the namespace %s, consider to specify the name in the label %s", namespace, r.modelRegistryNameLabel)
}

return &svcList.Items[0], nil
}

svc := &corev1.Service{}

err := r.client.Get(ctx, types.NamespacedName{Name: name, Namespace: namespace}, svc)
if err != nil {
return "", err
return nil, err
}

return svc, nil
}

func (r *InferenceServiceController) buildURLFromService(svc *corev1.Service) (string, error) {
var restApiPort *int32

if url, ok := svc.Annotations[r.modelRegistryURLAnnotation]; ok {
return fmt.Sprintf("https://%s", url), nil
}

for _, port := range svc.Spec.Ports {
if port.Name == "http-api" {
restApiPort = &port.Port
Expand All @@ -270,10 +307,10 @@ func (r *InferenceServiceController) getMRUrlFromService(ctx context.Context, na
}

if restApiPort == nil {
return "", fmt.Errorf("unable to find the http port in the Model Registry Service")
return "", fmt.Errorf("unable to find the http port in the Model Registry service")
}

return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", name, namespace, *restApiPort), nil
return fmt.Sprintf("http://%s.%s.svc.cluster.local:%d", svc.Name, svc.Namespace, *restApiPort), nil
}

func (r *InferenceServiceController) createMRInferenceService(
Expand Down
195 changes: 195 additions & 0 deletions pkg/inferenceservice-controller/controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/types"
)
Expand All @@ -16,10 +17,204 @@ var _ = Describe("InferenceService Controller", func() {
When("Creating a new InferenceService with Model Registry labels", func() {
It("If a label with inference service id is missing, it should add it after creating the required resources on model registry", func() {
const CorrectInferenceServicePath = "./testdata/inferenceservices/inference-service-correct.yaml"
const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml"
const namespace = "correct"

ns := &corev1.Namespace{}

ns.SetName(namespace)

if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

mrSvc := &corev1.Service{}
Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed())

mrSvc.SetNamespace(namespace)

if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

inferenceService := &kservev1beta1.InferenceService{}
Expect(ConvertFileToStructuredResource(CorrectInferenceServicePath, inferenceService)).To(Succeed())

inferenceService.SetNamespace(namespace)

inferenceService.Labels[namespaceLabel] = namespace

if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

Eventually(func() error {
isvc := &kservev1beta1.InferenceService{}
err := cli.Get(ctx, types.NamespacedName{
Name: inferenceService.Name,
Namespace: inferenceService.Namespace,
}, isvc)
if err != nil {
return err
}

if isvc.Labels[inferenceServiceIDLabel] != "1" {
return fmt.Errorf("Label for InferenceServiceID is not set, got %s", isvc.Labels[inferenceServiceIDLabel])
}

return nil
}, 10*time.Second, 1*time.Second).Should(Succeed())
})
})

When("Creating a new InferenceService without a Model Registry name", func() {
It("Should successfully create the InferenceService if there's just one model registry in the namespace", func() {
const InferenceServiceMissingNamePath = "./testdata/inferenceservices/inference-service-missing-name.yaml"
const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml"
const namespace = "correct-no-name"

ns := &corev1.Namespace{}

ns.SetName(namespace)

if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

mrSvc := &corev1.Service{}
Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed())

mrSvc.SetNamespace(namespace)

if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

inferenceService := &kservev1beta1.InferenceService{}
Expect(ConvertFileToStructuredResource(InferenceServiceMissingNamePath, inferenceService)).To(Succeed())

inferenceService.SetNamespace(namespace)

inferenceService.Labels[namespaceLabel] = namespace

if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

Eventually(func() error {
isvc := &kservev1beta1.InferenceService{}
err := cli.Get(ctx, types.NamespacedName{
Name: inferenceService.Name,
Namespace: inferenceService.Namespace,
}, isvc)
if err != nil {
return err
}

if isvc.Labels[inferenceServiceIDLabel] != "1" {
return fmt.Errorf("Label for InferenceServiceID is not set, got %s", isvc.Labels[inferenceServiceIDLabel])
}

return nil
}, 10*time.Second, 1*time.Second).Should(Succeed())
})

It("Should fail to create the InferenceService if there are multiple model registries in the namespace", func() {
const InferenceServiceMissingNamePath = "./testdata/inferenceservices/inference-service-missing-name.yaml"
const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml"
const namespace = "fail-no-name"

ns := &corev1.Namespace{}

ns.SetName(namespace)

if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

mrSvc := &corev1.Service{}
Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed())

mrSvc.SetNamespace(namespace)

if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

mrSvc2 := &corev1.Service{}
Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc2)).To(Succeed())

mrSvc2.SetNamespace(namespace)
mrSvc2.SetName("model-registry-2")

if err := cli.Create(ctx, mrSvc2); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

inferenceService := &kservev1beta1.InferenceService{}
Expect(ConvertFileToStructuredResource(InferenceServiceMissingNamePath, inferenceService)).To(Succeed())

inferenceService.SetNamespace(namespace)

inferenceService.Labels[namespaceLabel] = namespace

if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

Consistently(func() error {
isvc := &kservev1beta1.InferenceService{}
err := cli.Get(ctx, types.NamespacedName{
Name: inferenceService.Name,
Namespace: inferenceService.Namespace,
}, isvc)
if err != nil {
return err
}

if isvc.Labels[inferenceServiceIDLabel] != "1" {
return fmt.Errorf("Label for InferenceServiceID is not set, got %s", isvc.Labels[inferenceServiceIDLabel])
}

return nil
}, 5*time.Second, 1*time.Second).Should(Not(Succeed()))
})
})

When("Creating a new InferenceService with a Model Registry service specifies an annotation URL", func() {
It("Should successfully create the InferenceService with the correct URL", func() {
const CorrectInferenceServicePath = "./testdata/inferenceservices/inference-service-correct.yaml"
const ModelRegistrySVCPath = "./testdata/deploy/model-registry-svc.yaml"
const namespace = "correct-annotation-url"

ns := &corev1.Namespace{}

ns.SetName(namespace)

if err := cli.Create(ctx, ns); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

mrSvc := &corev1.Service{}
Expect(ConvertFileToStructuredResource(ModelRegistrySVCPath, mrSvc)).To(Succeed())

mrSvc.SetNamespace(namespace)

mrSvc.Annotations = map[string]string{
urlAnnotation: "model-registry.svc.cluster.local:8080",
}

if err := cli.Create(ctx, mrSvc); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}

inferenceService := &kservev1beta1.InferenceService{}
Expect(ConvertFileToStructuredResource(CorrectInferenceServicePath, inferenceService)).To(Succeed())

inferenceService.SetNamespace(namespace)

inferenceService.Labels[namespaceLabel] = namespace

if err := cli.Create(ctx, inferenceService); err != nil && !errors.IsAlreadyExists(err) {
Fail(err.Error())
}
Expand Down
Loading

0 comments on commit ce17ed9

Please sign in to comment.