Custom Metrics HPA Autoscaler #12277

enoodle · 2021-11-15T15:11:04Z

This is based on #4112 , but tries to have a simpler solution with no added annotations, by assuming that any metric name other than cpu or memory is a custom metric name.

Proposed Changes

For HPA autoscaler if the metric name is not cpu or memory then it will be used as a custom metric on the pods with the target value used as averageUtilization value.

Release Note

Metrics with names other than "cpu" or "memory" are allowed as pod custom metrics.

knative-prow-robot · 2021-11-15T15:11:19Z

Hi @enoodle. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

codecov · 2021-11-15T15:17:11Z

Codecov Report

Merging #12277 (27b469d) into main (5e96dc0) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #12277      +/-   ##
==========================================
- Coverage   87.55%   87.53%   -0.02%     
==========================================
  Files         195      195              
  Lines        9600     9617      +17     
==========================================
+ Hits         8405     8418      +13     
- Misses        915      918       +3     
- Partials      280      281       +1

Impacted Files	Coverage Δ
pkg/apis/autoscaling/annotation_validation.go	`100.00% <100.00%> (ø)`
pkg/reconciler/autoscaling/hpa/resources/hpa.go	`100.00% <100.00%> (ø)`
pkg/autoscaler/statserver/server.go	`77.66% <0.00%> (-1.95%)`	⬇️
pkg/reconciler/configuration/configuration.go	`84.61% <0.00%> (-1.54%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e96dc0...27b469d. Read the comment docs.

markusthoemmes

I like the idea in general, but haven't fully reviewed. I think we should break the API change out first.

markusthoemmes · 2021-11-15T15:33:42Z

pkg/reconciler/autoscaling/hpa/controller.go

@@ -22,7 +22,7 @@ import (
 	networkingclient "knative.dev/networking/pkg/client/injection/client"
 	sksinformer "knative.dev/networking/pkg/client/injection/informers/networking/v1alpha1/serverlessservice"
 	kubeclient "knative.dev/pkg/client/injection/kube/client"
-	hpainformer "knative.dev/pkg/client/injection/kube/informers/autoscaling/v2beta1/horizontalpodautoscaler"
+	hpainformer "knative.dev/pkg/client/injection/kube/informers/autoscaling/v2beta2/horizontalpodautoscaler"


WDYT about making the API change to v2beta2 separate PR wise? It seems like that'd be a good change in isolation to the rest in this PR even. Would make the actual changes a lot smaller too.

OK, I will do that.

@markusthoemmes #12278

markusthoemmes · 2021-11-15T15:47:48Z

/ok-to-test

markusthoemmes · 2021-11-15T20:50:01Z

/hold

Until #12278 merges, which this'll rebase on I guess

nader-ziada · 2021-11-15T22:40:57Z

pkg/reconciler/autoscaling/hpa/resources/hpa.go

+				Type: autoscalingv2beta1.PodsMetricSourceType,
+				Pods: &autoscalingv2beta1.PodsMetricSource{
+					MetricName:         pa.Metric(),
+					TargetAverageValue: *targetQuantity,


is the assumption that we only want this to be TargetAverageValue? would we ever want TargetValue or TargetAverageUtilization ?

Sort of, yes. This seems like the most simple option out of the three. I wanted a simple change without adding new annotations. If we add an annotation we can allow the user to choose between them.

I'm not sure how it would clear for users that its only using the TargetAverage, can we at least document it somehow

Yes, that is right. I will add it here : https://github.com/knative/docs/blob/main/docs/serving/autoscaling/autoscaling-metrics.md

@nader-ziada knative/docs#4496

psschwei · 2021-11-16T14:31:02Z

pkg/apis/autoscaling/annotation_validation.go

@@ -225,8 +225,7 @@ func validateMetric(annotations map[string]string) *apis.FieldError {
 				return nil
 			}
 		case HPA:
-			switch metric {
-			case CPU, Memory:
+			if metric != "" {


I don't think this is very important (and pretty subjective, so please feel free to disregard this comment), but stylistically, using a switch here looks a little nicer to me (as it mirrors the KPA case above), i.e.

case HPA: switch metric { case "": break default: return nil }

I understand, I will make that change.

nader-ziada · 2021-11-17T14:39:51Z

I believe the hold can be removed now that #12278 is merged

enoodle · 2021-11-17T14:58:39Z

Yes, I have rebased this PR since #12278 was merged.

markusthoemmes · 2021-11-17T15:31:06Z

/hold cancel

nader-ziada · 2021-11-17T21:53:50Z

/lgtm

enoodle · 2021-11-22T11:51:48Z

rebased on changes to hpa test.

enoodle · 2021-12-01T13:09:08Z

/retest

nader-ziada · 2021-12-01T14:33:45Z

@markusthoemmes I think this is ready to merge, would you mind taking a look

markusthoemmes

/lgtm
/approve

knative-prow-robot · 2021-12-01T15:42:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enoodle, markusthoemmes

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:

~~pkg/apis/autoscaling/OWNERS~~ [markusthoemmes]
~~pkg/reconciler/autoscaling/OWNERS~~ [markusthoemmes]

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

google-cla bot added the cla: yes Indicates the PR's author has signed the CLA. label Nov 15, 2021

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 15, 2021

knative-prow-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. area/API API objects and controllers area/autoscale labels Nov 15, 2021

markusthoemmes reviewed Nov 15, 2021

View reviewed changes

knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 15, 2021

enoodle force-pushed the custom-metrics-hpa branch from b9a1dff to 40bcd5e Compare November 15, 2021 17:19

knative-prow-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 15, 2021

enoodle changed the title ~~Custom Metrics and Stabilization Window for HPA Autoscaler~~ Custom Metrics HPA Autoscaler Nov 15, 2021

markusthoemmes requested review from psschwei and nader-ziada November 15, 2021 20:49

knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2021

nader-ziada reviewed Nov 15, 2021

View reviewed changes

enoodle force-pushed the custom-metrics-hpa branch from 40bcd5e to 025b9b1 Compare November 16, 2021 11:30

psschwei reviewed Nov 16, 2021

View reviewed changes

enoodle mentioned this pull request Nov 17, 2021

adding documentation for hpa custom metrics knative/docs#4496

Merged

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 17, 2021

knative-prow-robot assigned nader-ziada Nov 17, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 17, 2021

knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2021

enoodle added 4 commits November 22, 2021 13:48

support custom metrics hpa

addb7d0

update tests

8e28ade

update to hpa autoscaling/v2beta2

c40d4ec

use switch case instead of if

27b469d

enoodle force-pushed the custom-metrics-hpa branch from 168852e to 27b469d Compare November 22, 2021 11:51

knative-prow-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Nov 22, 2021

markusthoemmes approved these changes Dec 1, 2021

View reviewed changes

knative-prow-robot assigned markusthoemmes Dec 1, 2021

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 1, 2021

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2021

knative-prow-robot merged commit b7aa4b7 into knative:main Dec 1, 2021

enoodle deleted the custom-metrics-hpa branch December 1, 2021 16:52

Custom Metrics HPA Autoscaler #12277

Custom Metrics HPA Autoscaler #12277

Uh oh!

Conversation

enoodle commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow-robot commented Nov 15, 2021

Uh oh!

codecov bot commented Nov 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

markusthoemmes left a comment

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!

markusthoemmes commented Nov 15, 2021

Uh oh!

markusthoemmes commented Nov 15, 2021

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!

nader-ziada commented Nov 17, 2021

Uh oh!

enoodle commented Nov 17, 2021

Uh oh!

markusthoemmes commented Nov 17, 2021

Uh oh!

nader-ziada commented Nov 17, 2021

Uh oh!

enoodle commented Nov 22, 2021

Uh oh!

enoodle commented Dec 1, 2021

Uh oh!

nader-ziada commented Dec 1, 2021

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Dec 1, 2021

Uh oh!

Uh oh!

enoodle commented Nov 15, 2021 •

edited

Loading

codecov bot commented Nov 15, 2021 •

edited

Loading