Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(isvc): made name and url optional - added support to svc url annotation #683

Merged
merged 1 commit into from
Feb 6, 2025
Merged

feat(isvc): made name and url optional - added support to svc url annotation #683

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 46 additions & 9 deletions pkg/inferenceservice-controller/controller.go
View file
Open in desktop
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
Open in desktop
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
Loading