fix: HPA equality check should include annotations #3650

terrytangyuan · 2024-04-29T14:29:21Z

This PR fixes an issue where:

Correct behavior: If it's a new ISVC with the annotation serving.kserve.io/autoscalerClass: "external", no HPA will be created.

Incorrect behavior: However, if it's an existing ISVC where HPA is already created with annotation serving.kserve.io/autoscalerClass: "hpa", our users want to change the annotation to "external" and expect that the existing HPA to be deleted.

spolti

/lgtm

pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go

yuzisun · 2024-05-04T14:39:16Z

pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go

+	return equality.Semantic.DeepEqual(desired.Spec, existing.Spec) && !autoscalerClassChanged
+}
+
+func shouldDeleteHPA(desired *autoscalingv2.HorizontalPodAutoscaler) bool {


I think we should only delete if it is changed from enabled to external otherwise delete throws error

Actually nvm, shouldDeleteHPA is called only when HPA exists. But when autoscaler is set to external it creates NoOpAutoscaler and does not reconcile

have you tested this? I think it will not work

Will fix and test it. Marking as draft for now

@yuzisun Please take another look. I tested a couple of scenarios and they worked as expected.

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

spolti

/lgtm

yuzisun · 2024-05-11T13:56:08Z

pkg/controller/v1beta1/inferenceservice/reconcilers/autoscaler/autoscaler_reconciler.go

 		return hpa.NewHPAReconciler(client, scheme, componentMeta, componentExt), nil
-	case constants.AutoscalerClassExternal:
-		return &NoOpAutoscaler{}, nil


So should we delete the NoOpAutoscaler class now if it is no longer used?

yuzisun · 2024-05-11T13:59:18Z

/approve

oss-prow-bot · 2024-05-11T13:59:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: spolti, terrytangyuan, yuzisun

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yuzisun]

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

* fix: HPA equality check should include annotations Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Only watch related autoscalerclass annotation Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * simplify Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Add missing delete action Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * fix logic Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> --------- Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

[RHOAIENG-6878][Cherry-pick][RHOAI-2.10] fix: HPA equality check should include annotations (kserve#3650)

[RHOAIENG-6577][Cherry-pick][RHOAI-2.8] fix: HPA equality check should include annotations (kserve#3650)

* fix: HPA equality check should include annotations Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Only watch related autoscalerclass annotation Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * simplify Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Add missing delete action Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * fix logic Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> --------- Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> Signed-off-by: asd981256 <asd981256@gmail.com>

* upgrade vllm/transformers version (#3671) upgrade vllm version Signed-off-by: Johnu George <johnugeorge109@gmail.com> * Add openai models endpoint (#3666) Signed-off-by: Curtis Maddalozzo <cmaddalozzo@bloomberg.net> * feat: Support customizable deployment strategy for RawDeployment mode. Fixes #3452 (#3603) * feat: Support customizable deployment strategy for RawDeployment mode Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * regen Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * lint Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Correctly apply rollingupdate Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * address comments Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Add validation Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> --------- Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Enable dtype support for huggingface server (#3613) * Enable dtype for huggingface server Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Set float16 as default. Fixup linter Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Add small comment to make the changes understandable Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Fixup linter Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Adapt to new huggingfacemodel Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Fixup merge :) Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Explicitly mention the behaviour of dtype flag on auto. Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Default to FP32 for encoder models Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Selectively add --dtype to parser. Use FP16 for GPU and FP32 for CPU Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Fixup linter Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Update poetry Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Use torch.float32 forr tests explicitly Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> --------- Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> * Add method for checking model health/readiness (#3673) Signed-off-by: Curtis Maddalozzo <cmaddalozzo@bloomberg.net> * fix for extract zip from gcs (#3510) * fix for extract zip from gcs Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * initial commit for gcs model download unittests Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * unittests for model download from gcs Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * black format fix Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * code verification Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> --------- Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> * Update Dockerfile and Readme (#3676) Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> * Update huggingface readme (#3678) * update wording for huggingface README small update to make readme easier to understand Signed-off-by: Alexa Griffith <agriffith50@bloomberg.net> * Update README.md Signed-off-by: Alexa Griffith agriffith50@bloomberg.net * Update python/huggingfaceserver/README.md Co-authored-by: Filippe Spolti <filippespolti@gmail.com> Signed-off-by: Alexa Griffith <agriffith50@bloomberg.net> * update vllm Signed-off-by: alexagriffith <agriffith50@bloomberg.net> * Update README.md --------- Signed-off-by: Alexa Griffith <agriffith50@bloomberg.net> Signed-off-by: Alexa Griffith agriffith50@bloomberg.net Signed-off-by: alexagriffith <agriffith50@bloomberg.net> Signed-off-by: Dan Sun <dsun20@bloomberg.net> Co-authored-by: Filippe Spolti <filippespolti@gmail.com> Co-authored-by: Dan Sun <dsun20@bloomberg.net> * fix: HPA equality check should include annotations (#3650) * fix: HPA equality check should include annotations Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Only watch related autoscalerclass annotation Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * simplify Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Add missing delete action Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * fix logic Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> --------- Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> * Fix: huggingface runtime in helm chart (#3679) fix huggingface runtime in chart Signed-off-by: Dan Sun <dsun20@bloomberg.net> * Fix: model id and model dir check order (#3680) * fix huggingface runtime in chart Signed-off-by: Dan Sun <dsun20@bloomberg.net> * Allow model_dir to be specified on template Signed-off-by: Dan Sun <dsun20@bloomberg.net> * Default model_dir to /mnt/models for HF Signed-off-by: Dan Sun <dsun20@bloomberg.net> * Lint format Signed-off-by: Dan Sun <dsun20@bloomberg.net> --------- Signed-off-by: Dan Sun <dsun20@bloomberg.net> * Fix:vLLM Model Supported check throwing circular dependency (#3688) * Fix:vLLM Model Supported check throwing circular dependency Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> * remove unwanted comments Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> * remove unwanted comments Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> * fix return case Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> * fix to check all arch in model config forr vllm support Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> * fixlint Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> --------- Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> * Fix: Allow null in Finish reason streaming response in vLLM (#3684) Fix: allow null in Finish reason Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> --------- Signed-off-by: Johnu George <johnugeorge109@gmail.com> Signed-off-by: Curtis Maddalozzo <cmaddalozzo@bloomberg.net> Signed-off-by: Yuan Tang <terrytangyuan@gmail.com> Signed-off-by: Dattu Sharma <venkatadattasainimmaturi@gmail.com> Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Signed-off-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> Signed-off-by: Alexa Griffith <agriffith50@bloomberg.net> Signed-off-by: Alexa Griffith agriffith50@bloomberg.net Signed-off-by: alexagriffith <agriffith50@bloomberg.net> Signed-off-by: Dan Sun <dsun20@bloomberg.net> Co-authored-by: Curtis Maddalozzo <cmaddalozzo@users.noreply.github.com> Co-authored-by: Yuan Tang <terrytangyuan@gmail.com> Co-authored-by: Datta Nimmaturi <39181234+Datta0@users.noreply.github.com> Co-authored-by: Andrews Arokiam <87992092+andyi2it@users.noreply.github.com> Co-authored-by: Gavrish Prabhu <gavrish.prabhu@nutanix.com> Co-authored-by: Alexa Griffith <agriffith50@bloomberg.net> Co-authored-by: Filippe Spolti <filippespolti@gmail.com> Co-authored-by: Dan Sun <dsun20@bloomberg.net>

oss-prow-bot bot requested review from alexagriffith and cmaddalozzo April 29, 2024 14:29

spolti approved these changes Apr 29, 2024

View reviewed changes

oss-prow-bot bot assigned spolti Apr 29, 2024

oss-prow-bot bot added the lgtm label Apr 29, 2024

yuzisun reviewed Apr 30, 2024

View reviewed changes

pkg/controller/v1beta1/inferenceservice/reconcilers/hpa/hpa_reconciler.go Outdated Show resolved Hide resolved

oss-prow-bot bot removed the lgtm label May 3, 2024

yuzisun reviewed May 4, 2024

View reviewed changes

terrytangyuan added 5 commits May 6, 2024 12:37

fix: HPA equality check should include annotations

225c22b

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

Only watch related autoscalerclass annotation

ede87b9

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

simplify

b70dcde

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

Add missing delete action

59df55d

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

fix

2991f05

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

terrytangyuan marked this pull request as draft May 6, 2024 21:11

oss-prow-bot bot added the do-not-merge/work-in-progress label May 6, 2024

fix logic

69685de

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

terrytangyuan force-pushed the fix-hpa-check branch from 4ca76ac to 69685de Compare May 7, 2024 00:25

terrytangyuan marked this pull request as ready for review May 7, 2024 00:26

oss-prow-bot bot removed the do-not-merge/work-in-progress label May 7, 2024

oss-prow-bot bot requested a review from yuzisun May 7, 2024 00:26

Empty-Commit

d1d73d0

Signed-off-by: Yuan Tang <terrytangyuan@gmail.com>

spolti approved these changes May 9, 2024

View reviewed changes

oss-prow-bot bot added the lgtm label May 9, 2024

yuzisun reviewed May 11, 2024

View reviewed changes

oss-prow-bot bot added the approved label May 11, 2024

yuzisun merged commit 56a2940 into kserve:master May 11, 2024
57 of 58 checks passed

terrytangyuan deleted the fix-hpa-check branch May 11, 2024 14:12

terrytangyuan mentioned this pull request May 13, 2024

[RHOAIENG-6878][Cherry-pick][RHOAI-2.10] fix: HPA equality check should include annotations (#3650) opendatahub-io/kserve#355

Merged

openshift-merge-bot bot added a commit to opendatahub-io/kserve that referenced this pull request May 13, 2024

Merge pull request #355 from terrytangyuan/odh-cp-hpa-delete

7536608

[RHOAIENG-6878][Cherry-pick][RHOAI-2.10] fix: HPA equality check should include annotations (kserve#3650)

openshift-merge-bot bot added a commit to red-hat-data-services/kserve that referenced this pull request May 13, 2024

Merge pull request #268 from terrytangyuan/rhds-cp-hpa-delete

e68e65e

[RHOAIENG-6577][Cherry-pick][RHOAI-2.8] fix: HPA equality check should include annotations (kserve#3650)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: HPA equality check should include annotations #3650

fix: HPA equality check should include annotations #3650

terrytangyuan commented Apr 29, 2024 •

edited

spolti left a comment

yuzisun May 4, 2024

yuzisun May 4, 2024 •

edited

yuzisun May 4, 2024

terrytangyuan May 6, 2024

terrytangyuan May 7, 2024

spolti left a comment

yuzisun May 11, 2024

yuzisun commented May 11, 2024

oss-prow-bot bot commented May 11, 2024

fix: HPA equality check should include annotations #3650

fix: HPA equality check should include annotations #3650

Conversation

terrytangyuan commented Apr 29, 2024 • edited

spolti left a comment

Choose a reason for hiding this comment

yuzisun May 4, 2024

Choose a reason for hiding this comment

yuzisun May 4, 2024 • edited

Choose a reason for hiding this comment

yuzisun May 4, 2024

Choose a reason for hiding this comment

terrytangyuan May 6, 2024

Choose a reason for hiding this comment

terrytangyuan May 7, 2024

Choose a reason for hiding this comment

spolti left a comment

Choose a reason for hiding this comment

yuzisun May 11, 2024

Choose a reason for hiding this comment

yuzisun commented May 11, 2024

oss-prow-bot bot commented May 11, 2024

terrytangyuan commented Apr 29, 2024 •

edited

yuzisun May 4, 2024 •

edited