Auto-update annotation for isvc. #4342

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #3299

Type of changes
Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

Test A No Annotation

Created a serving runtime
Created an isvc that uses that serving runtime and no annotation
Verified that the deployment has been created with the image specified in the serving runtime
Updated the serving runtime spec.containers[0].image with a new image
Verified that the deployment now had the updated image

Test B Annotation set to auto-update=true

Created a serving runtime
Created an isvc that uses that serving runtime with the annotation set to true
Verified the deployment was created correctly
Updated the serving runtime spec.containers[0].image with a new image
Verified that the deployment now had the updated image

Test C Annotation set to auto-update=false

Created a serving runtime
Created an isvc that uses that serving runtime with the annotation set to true
Verified the deployment was created correctly
Updated the serving runtime spec.containers[0].image with a new image
Verified that the deployment DID NOT have the updated image

Logs

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Checklist:

Have you added unit/e2e tests that prove your fix is effective or that this feature works?
Has code been commented, particularly in hard-to-understand areas?
Have you made corresponding changes to the documentation?

Release note:

Re-running failed tests

/rerun-all - rerun all failed workflows.
/rerun-workflow <workflow name> - rerun a specific failed workflow. Only one workflow name can be specified. Multiple /rerun-workflow commands are allowed per comment.

andresllh · 2025-03-27T20:29:47Z

/cc @spolti @israel-hdez

spolti

/lgtm

terrytangyuan

Thanks!

/lgtm
/approve

andresllh · 2025-04-07T14:29:11Z

/rerun-all

andresllh · 2025-04-07T15:18:34Z

/rerun-all

terrytangyuan

/lgtm
/approve

andresllh · 2025-04-07T18:46:11Z

/rerun-all

yuzisun · 2025-04-08T00:41:40Z

@andresllh from what I understand even when the annotation auto-update is set to false, the kserve controller can still “auto” update the deployment with period resync which happens every 10 hours. It is a bit counterintuitive for users with this behavior.

andresllh · 2025-04-08T14:03:17Z

@andresllh from what I understand even when the annotation auto-update is set to false, the kserve controller can still “auto” update the deployment with period resync which happens every 10 hours. It is a bit counterintuitive for users with this behavior.

@yuzisun based my implementation on this open issue. Specifically @Jooho comment. #3299 (comment)

andresllh · 2025-04-08T14:04:28Z

/rerun-all

andresllh · 2025-04-08T18:58:43Z

/rerun-all

terrytangyuan · 2025-04-08T19:15:06Z

/lgtm
/approve

terrytangyuan · 2025-04-08T19:17:02Z

Let’s make sure the comment from @yuzisun is addressed. @yuzisun Can you take another look?

sivanantha321 · 2025-04-09T10:31:54Z

@andresllh I think this needs a rebase from master

spolti · 2025-04-14T14:35:27Z

@andresllh can you please sign your commits?

andresllh · 2025-04-24T13:44:10Z

/rerun-all

andresllh · 2025-04-24T14:44:22Z

@yuzisun do you still have any comments or concerns regarding this?

israel-hdez · 2025-05-04T17:45:52Z

@andresllh from what I understand even when the annotation auto-update is set to false, the kserve controller can still “auto” update the deployment with period resync which happens every 10 hours. It is a bit counterintuitive for users with this behavior.

I agree with this. If we want a consistent experience, we need a way to freeze the deployment.

yuzisun · 2025-05-05T01:50:34Z

pkg/controller/v1beta1/inferenceservice/controller.go

+		if isvc.Spec.Predictor.Model.ProtocolVersion == nil {
+			continue
+		}
+		if slices.Contains(supportedModelFormatNames, isvc.Spec.Predictor.Model.ModelFormat.Name) && slices.Contains(protocolVersionsAsStrings, string(*isvc.Spec.Predictor.Model.ProtocolVersion)) {


runtime matching also includes priority, if the lower priority runtime is updated the inference service probably should not be reconciled. Also there is case that user explicitly the serving runtime name.

cc @sivanantha321 to take a look at this, feel it is duplicating with the logic we have in other places.

Sounds good, let me know how you would like me to proceed with this. I'll wait for a response from @sivanantha321 as well before I make any changes.

@andresllh How about adding a field for runtime in the ISVC status ? This will be also useful for the users to track what runtime is used by the current revision of ISVC. In this way we can avoid the complexities here. If the runtime name matches with the status field, we trigger the reconcil 9E88 e request for that ISVC.

@sivanantha321 that is a great idea, any objections @israel-hdez @yuzisun?
If not, I'll start on making this update.

One question, though, shouldn't it be updated in case the SR is updated, regardless of whether the ISVC has the annotation?

Correct, that is the current behavior. If the SR is updated and ISVC has the auto-update annotation set to true, or if it's missing, then it will update the ISVC.

Only if the auto-update annotation is set to false will it skip over.

Also, the idea proposed by @sivanantha321 would make things easier as well.
+1 to the suggested approach.

pkg/constants/constants.go

…new servingRuntimeName to isvc status Signed-off-by: Andres Llausas <allausas@redhat.com>

Signed-off-by: Andres Llausas <allausas@redhat.com>

…on initial creation. Signed-off-by: Andres Llausas <allausas@redhat.com>

Signed-off-by: Andres Llausas <allausas@redhat.com>

andresllh · 2025-05-21T21:15:14Z

@spolti @yuzisun @sivanantha321 I've made the changes. Can you please review when you get a chance?

andresllh · 2025-05-21T21:16:01Z

@andresllh from what I understand even when the annotation auto-update is set to false, the kserve controller can still “auto” update the deployment with period resync which happens every 10 hours. It is a bit counterintuitive for users with this behavior.

I agree with this. If we want a consistent experience, we need a way to freeze the deployment.

@israel-hdez my new implementation can "freeze" the deployment. Please take a look.

Signed-off-by: Andres Llausas <allausas@redhat.com>

spolti

Thanks @andresllh , imho it looks good.
/lgtm

spolti · 2025-06-03T01:03:47Z

@andresllh looks like there are some conflicts, can you please help with fixing them?

…tests-kserve-upstream

Signed-off-by: Andres Llausas <allausas@redhat.com>

yuzisun · 2025-06-11T07:43:57Z

@andresllh the test is failing

Summarizing 1 Failure:
  [FAIL] v1beta1 inference service controller When Updating a Serving Runtime [It] InferenceService should not reconcile the deployment if auto-update is disabled
  /home/runner/work/kserve/kserve/pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:2330

andresllh · 2025-06-11T15:06:10Z

@andresllh the test is failing

Summarizing 1 Failure:
  [FAIL] v1beta1 inference service controller When Updating a Serving Runtime [It] InferenceService should not reconcile the deployment if auto-update is disabled
  /home/runner/work/kserve/kserve/pkg/controller/v1beta1/inferenceservice/rawkube_controller_test.go:2330

@yuzisun Yeah, I'm looking into this. It passes when I run it locally, but for some reason it consistently fails in the CI.

andresllh force-pushed the fix-e2e-graph-tests-kserve-upstream branch from a4d4c44 to 886db72 Compare March 27, 2025 20:28

andresllh changed the title ~~[RHOAIENG-7127] Auto-update annotation for isvc.~~ Auto-update annotation for isvc. Mar 28, 2025

spolti approved these changes Mar 28, 2025

View reviewed changes

terrytangyuan previously approved these changes Apr 4, 2025

View reviewed changes

andresllh dismissed terrytangyuan’s stale review via b8b563f April 7, 2025 15:41

andresllh requested a review from terrytangyuan April 7, 2025 16:03

terrytangyuan previously approved these changes Apr 7, 2025

View reviewed changes

andresllh force-pushed the fix-e2e-graph-tests-kserve-upstream branch from b8b563f to c0c750c Compare April 7, 2025 18:47

andresllh dismissed terrytangyuan’s stale review via 4e9f15e April 8, 2025 19:09

andresllh force-pushed the fix-e2e-graph-tests-kserve-upstream branch from 4e9f15e to 7239beb Compare April 8, 2025 19:10

github-actions bot previously approved these changes Apr 8, 2025

View reviewed changes

github-actions bot added the lgtm label Apr 8, 2025

terrytangyuan removed the lgtm label Apr 8, 2025

terrytangyuan requested a review from yuzisun April 8, 2025 19:16

andresllh dismissed github-actions[bot]’s stale review via 60c0708 April 9, 2025 14:14

andresllh force-pushed the fix-e2e-graph-tests-kserve-upstream branch from a7db076 to 708ef8b Compare April 15, 2025 13:58

yuzisun reviewed May 5, 2025

View reviewed changes

spolti self-requested a review May 16, 2025 13:59

spolti reviewed May 21, 2025

View reviewed changes

pkg/constants/constants.go Outdated Show resolved Hide resolved

andresllh added 4 commits May 21, 2025 16:21

Addressed PR comments. Changed name of auto-update annotation. Added …

54cf2e9

…new servingRuntimeName to isvc status Signed-off-by: Andres Llausas <allausas@redhat.com>

Updated tests

cc1feb3

Signed-off-by: Andres Llausas <allausas@redhat.com>

Fixed isvc not becoming ready if disable-auto-update was set to true …

37aac73

…on initial creation. Signed-off-by: Andres Llausas <allausas@redhat.com>

Removed unused function from previous implementation.

fe38df0

Signed-off-by: Andres Llausas <allausas@redhat.com>

andresllh requested review from spolti, yuzisun and sivanantha321 May 21, 2025 21:14

andresllh added 5 commits May 22, 2025 10:06

Fixed merge conflicts

c504d5b

Signed-off-by: Andres Llausas <allausas@redhat.com>

Ran make precommit

adfe07e

Signed-off-by: Andres Llausas <allausas@redhat.com>

Fixed tests

e0a5e25

Signed-off-by: Andres Llausas <allausas@redhat.com>

Ran make precommit. Update openapi generated code

be262c5

Signed-off-by: Andres Llausas <allausas@redhat.com>

Retrigger CI

b4f7414

Signed-off-by: Andres Llausas <allausas@redhat.com>

spolti approved these changes May 27, 2025

View reviewed changes

Merge branch 'master' into fix-e2e-graph-tests-kserve-upstream

cb30658

andresllh and others added 6 commits June 6, 2025 09:37

Merge branch 'master' of github.com:kserve/kserve into fix-e2e-graph-…

5866562

…tests-kserve-upstream

Fixed failing tests

4e82417

Signed-off-by: Andres Llausas <allausas@redhat.com>

Fixed more tests

0ef92d6

Signed-off-by: Andres Llausas <allausas@redhat.com>

Retrigger CI

9f84bf3

Signed-off-by: Andres Llausas <allausas@redhat.com>

Merge branch 'master' into fix-e2e-graph-tests-kserve-upstream

0a1cfce

Merge branch 'master' into fix-e2e-graph-tests-kserve-upstream

620741a

Merge branch 'master' into fix-e2e-graph-tests-kserve-upstream

0bea5e7

Auto-update annotation for isvc. #4342

Are you sure you want to change the base?

Auto-update annotation for isvc. #4342

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!