feat(controller): added reconciliation of inferenceservice url by Al-Pragliola · Pull Request #960 · kubeflow/hub

Al-Pragliola · 2025-04-10T13:25:49Z

needs #917

Description

In this PR, I have added a new field to the InferenceService customProperties, which is the 'url' field.
This field is used to reconcile the url from the status of the cluster InferenceService, and update the url in the customProperties.
Also, I have also added new unit tests to test the new functionality.

Why a customProperty and not a first class field?

Because the url is part of the InferenceService status, and not the spec so it is ephemeral and should be treated as such in my opinion.

How Has This Been Tested?

make test

And also, I have created a mock controller that periodically updates the URL in the InferenceService status, and the URL in the customProperties is updated accordingly.

package controllers

import (
	"context"
	"fmt"
	"time"

	kservev1beta1 "github.com/kserve/kserve/pkg/apis/serving/v1beta1"
	"k8s.io/apimachinery/pkg/api/errors"
	"k8s.io/apimachinery/pkg/runtime"
	"knative.dev/pkg/apis"
	ctrl "sigs.k8s.io/controller-runtime"
	"sigs.k8s.io/controller-runtime/pkg/client"
	"sigs.k8s.io/controller-runtime/pkg/log"
)

type InferenceServiceMockReconciler struct {
	client.Client
	Scheme *runtime.Scheme
}

// +kubebuilder:rbac:groups=serving.kserve.io,resources=inferenceservices,verbs=get;list;watch;update;patch
// +kubebuilder:rbac:groups=serving.kserve.io,resources=inferenceservices/status,verbs=get;list;watch;update;patch
// +kubebuilder:rbac:groups=serving.kserve.io,resources=inferenceservices/finalizers,verbs=get;list;watch;update;create;patch;delete
// +kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch

func (r *InferenceServiceMockReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
	_ = log.FromContext(ctx)

	isvc := &kservev1beta1.InferenceService{}
	err := r.Get(ctx, req.NamespacedName, isvc)
	if err != nil {
		if errors.IsNotFound(err) {
			// Resource not found, nothing to do
			return ctrl.Result{}, nil
		}
		// Error fetching resource
		return ctrl.Result{}, err
	}

	// Update the status.url field
	newUrl, err := apis.ParseURL(fmt.Sprintf("https://example.com/%d", time.Now().Unix()))
	if err != nil {
		return ctrl.Result{}, err
	}

	isvc.Status.URL = newUrl
	err = r.Status().Update(ctx, isvc)
	if err != nil {
		return ctrl.Result{}, err
	}

	// Requeue the reconcile loop after 30 seconds
	return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
}

func (r *InferenceServiceMockReconciler) SetupWithManager(mgr ctrl.Manager) error {
	return ctrl.NewControllerManagedBy(mgr).
		For(&kservev1beta1.InferenceService{}).
		Named("mockinferenceservice").
		Complete(r)
}

Merge criteria:

All the commits have been signed-off (To pass the DCO check)

The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work.
Code changes follow the kubeflow contribution guidelines.
For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Al-Pragliola · 2025-04-18T15:44:01Z

/cc @pboyd

pboyd · 2025-04-24T17:56:39Z

+		}
+
+		if isvc.Status.URL != nil {
+			(*isCreate.CustomProperties)["url"].MetadataStringValue.StringValue = isvc.Status.URL.String()


You need to initialize CustomProperties before this, right? (I think you have the same situation as this, which panics).

Need test cases for all combinations of url and customProperties set/unset, etc. in controller_test.go to verify.

@pboyd Good catch, it didn't happen in my tests because the .Status.URL always gets populated after the basic creation, so CustomProperties was empty but not nil

@dhirajsb thanks, I'll add more tests

dhirajsb · 2025-04-25T04:33:58Z

+		}
+
+		if isvc.Status.URL != nil {
+			(*isCreate.CustomProperties)["url"].MetadataStringValue.StringValue = isvc.Status.URL.String()


Need test cases for all combinations of url and customProperties set/unset, etc. in controller_test.go to verify.

dhirajsb · 2025-04-25T04:57:23Z

 	handler := http.NewServeMux()

+	servingEnvironments := new(sync.Map)
+	inferenceServices := new(sync.Map)


Maybe I missed it, but it doesn't look like these sync maps are needed, does it?

I was trying them while debugging a weird bug in tests and I forgot to switch back to regular maps, good catch reverted in 5847b3c

If I'm not mistaken, there is only one hardcoded id 1 being used. If so, does it even need a map?

Not for the time being, I'm going to keep it in case of more complex scenarios

google-oss-prow · 2025-04-25T04:58:08Z

@dhirajsb: changing LGTM is restricted to collaborators

Details

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Co-authored-by: Paul Boyd <paul@camelot.email> Signed-off-by: Alessio Pragliola <83355398+Al-Pragliola@users.noreply.github.com>

Co-authored-by: Paul Boyd <paul@camelot.email> Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

pboyd

/lgtm

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Al-Pragliola · 2025-05-02T14:16:25Z

added more tests @dhirajsb

dhirajsb

/lgtm

google-oss-prow · 2025-05-05T14:47:17Z

@dhirajsb: changing LGTM is restricted to collaborators

Details

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

tarilabs

thanks a lot @Al-Pragliola and all

/approve

google-oss-prow · 2025-05-05T15:06:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: tarilabs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [tarilabs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pboyd · 2025-05-05T15:09:04Z

/lgtm

google-oss-prow Bot added the do-not-merge/work-in-progress label Apr 10, 2025

google-oss-prow Bot requested review from andreyvelich and tarilabs April 10, 2025 13:25

google-oss-prow Bot added the size/XXL label Apr 10, 2025

github-actions Bot added Area/GitHub Area/Go REST server Area/Manifests labels Apr 10, 2025

Al-Pragliola force-pushed the al-pragliola-inference-service-url branch from 6c38ab1 to 99e8b4b Compare April 14, 2025 15:20

google-oss-prow Bot added size/L and removed size/XXL labels Apr 14, 2025

Al-Pragliola added 2 commits April 18, 2025 17:13

feat(controller): added reconciliation to inferenceservice url

7b07840

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

fix(controller): remove FIt from tests

636d409

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Al-Pragliola force-pushed the al-pragliola-inference-service-url branch from 99e8b4b to 636d409 Compare April 18, 2025 15:13

fix(controller): linting issues

e4cad3d

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

Al-Pragliola marked this pull request as ready for review April 18, 2025 15:43

google-oss-prow Bot removed the do-not-merge/work-in-progress label Apr 18, 2025

google-oss-prow Bot requested review from ckadner and zijianjoy April 18, 2025 15:43

google-oss-prow Bot requested a review from pboyd April 18, 2025 15:44

pboyd requested changes Apr 24, 2025

View reviewed changes

google-oss-prow Bot assigned pboyd Apr 24, 2025

dhirajsb suggested changes Apr 25, 2025

View reviewed changes

Al-Pragliola and others added 3 commits April 28, 2025 14:59

chore(controller): lower complexity of customproperties check

eefa0f5

Co-authored-by: Paul Boyd <paul@camelot.email> Signed-off-by: Alessio Pragliola <83355398+Al-Pragliola@users.noreply.github.com>

fix(controller): potential nil pointer exception

1930545

Co-authored-by: Paul Boyd <paul@camelot.email> Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

chore(controller): remove sync.Maps as are not needed

5847b3c

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

pboyd approved these changes Apr 28, 2025

View reviewed changes

google-oss-prow Bot added the lgtm label Apr 28, 2025

feat(controller): improve test coverage

ea588be

Signed-off-by: Alessio Pragliola <seth.pro@gmail.com>

google-oss-prow Bot added size/XL and removed lgtm size/L labels May 2, 2025

dhirajsb approved these changes May 5, 2025

View reviewed changes

Al-Pragliola requested a review from pboyd May 5, 2025 15:04

tarilabs reviewed May 5, 2025

View reviewed changes

google-oss-prow Bot added the approved label May 5, 2025

google-oss-prow Bot added the lgtm label May 5, 2025

google-oss-prow Bot merged commit 8258979 into kubeflow:main May 5, 2025
15 checks passed

Al-Pragliola deleted the al-pragliola-inference-service-url branch May 5, 2025 15:10

This was referenced May 12, 2025

[pull] main from kubeflow:main opendatahub-io/model-registry#198

Merged

periodic sync upstream KF to midstream ODH opendatahub-io/model-registry#215

Merged

Conversation

Al-Pragliola commented Apr 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How Has This Been Tested?

Merge criteria:

Uh oh!

Al-Pragliola commented Apr 18, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-prow Bot commented Apr 25, 2025

Uh oh!

pboyd left a comment

Choose a reason for hiding this comment

Uh oh!

Al-Pragliola commented May 2, 2025

Uh oh!

dhirajsb left a comment

Choose a reason for hiding this comment

Uh oh!

google-oss-prow Bot commented May 5, 2025

Uh oh!

tarilabs left a comment

Choose a reason for hiding this comment

Uh oh!

google-oss-prow Bot commented May 5, 2025

Uh oh!

pboyd commented May 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Al-Pragliola commented Apr 10, 2025 •

edited

Loading