HPA v2beta2 #12278

enoodle · 2021-11-15T17:11:16Z

Update the HPA autoscaling version to v2beta2 to allow setting window and in the future also scale rate

Proposed Changes

Using the v2beta2 hpa autoscaler to allow setting stabilization window and in the future will allow defining scalUp/scaleDown policies using the max-scale-up-rate/max-scale-down-rate.
If found, Uses the /window annotation for the HPA autoscaler.

Release Note

Generated HPAs are now v2beta2.
Window annotation will be set to HPA stabilization window

knative-prow-robot · 2021-11-15T17:11:29Z

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.

markusthoemmes

Let's do a liiiiittle more splitting. My goal would be to have no behavioral change in this first PR and focus just on the API changes.

markusthoemmes · 2021-11-15T17:12:45Z

pkg/apis/autoscaling/annotation_validation.go

-		if annotations[ClassAnnotationKey] == HPA && (annotations[MetricAnnotationKey] == CPU || annotations[MetricAnnotationKey] == Memory) {
-			return apis.ErrInvalidKeyName(WindowAnnotationKey, apis.CurrentField, fmt.Sprintf("%s for %s %s", HPA,
-				MetricAnnotationKey, annotations[MetricAnnotationKey]))
-		}


Let's drop this for a subsequent PR.

markusthoemmes · 2021-11-15T17:13:08Z

pkg/apis/autoscaling/annotation_validation_test.go

-	}, {
-		name:        "value too short and invalid class for /window annotation",
-		annotations: map[string]string{WindowAnnotationKey: "1s", ClassAnnotationKey: HPA, MetricAnnotationKey: CPU},
-		expectErr:   fmt.Sprintf("invalid key name %q: \n%s for %s %s", WindowAnnotationKey, HPA, MetricAnnotationKey, CPU),


Split into the other PR please.

markusthoemmes · 2021-11-15T17:13:30Z

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

+	if window, hasWindow := pa.Window(); hasWindow {
+		windowSeconds := int32(window.Seconds())
+		hpa.Spec.Behavior = &autoscalingv2beta2.HorizontalPodAutoscalerBehavior{
+			ScaleDown: &autoscalingv2beta2.HPAScalingRules{
+				StabilizationWindowSeconds: &windowSeconds,
+			},
+			ScaleUp: &autoscalingv2beta2.HPAScalingRules{
+				StabilizationWindowSeconds: &windowSeconds,
+			},
+		}
+	}


The other PR please :)

markusthoemmes · 2021-11-15T17:14:18Z

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

+				Resource: &autoscalingv2beta2.ResourceMetricSource{
+					Name: corev1.ResourceMemory,
+					Target: autoscalingv2beta2.MetricTarget{
+						Type:         "AverageValue",


Is there a constant for AverageValue and Utilization maybe?

markusthoemmes · 2021-11-15T17:16:27Z

/ok-to-test

codecov · 2021-11-15T17:37:49Z

Codecov Report

Merging #12278 (ba6b557) into main (eb83774) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #12278      +/-   ##
==========================================
+ Coverage   87.25%   87.31%   +0.05%     
==========================================
  Files         195      195              
  Lines        9605     9610       +5     
==========================================
+ Hits         8381     8391      +10     
+ Misses        941      939       -2     
+ Partials      283      280       -3

Impacted Files	Coverage Δ
pkg/reconciler/autoscaling/hpa/controller.go	`100.00% <ø> (ø)`
pkg/reconciler/autoscaling/hpa/hpa.go	`91.17% <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	`86.15% <0.00%> (+1.53%)`	⬆️
pkg/autoscaler/statforwarder/processor.go	`94.44% <0.00%> (+5.55%)`	⬆️
pkg/autoscaler/statforwarder/forwarder.go	`96.29% <0.00%> (+5.55%)`	⬆️

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 eb83774...ba6b557. Read the comment docs.

psschwei

Does the e2e test also need to be updated to v2beta2?

nader-ziada

The only thing I see missing is the e2e test Paul mentioned, otherwise the PR changes the version without any behavioural changes and tests are passing, so looking good

markusthoemmes · 2021-11-15T18:37:28Z

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

+				Resource: &autoscalingv2beta2.ResourceMetricSource{
+					Name: corev1.ResourceCPU,
+					Target: autoscalingv2beta2.MetricTarget{
+						Type:               "Utilization",


Should use the same constant as above (throughout this file)

nader-ziada · 2021-11-15T21:59:26Z

lgtm

markusthoemmes

I checked the availability of the API and v2beta1 seems to have been deprecated in 1.19, so this is timely!

Thanks!

/lgtm
/approve

knative-prow-robot · 2021-11-16T10:08:20Z

[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:

~~OWNERS~~ [markusthoemmes]

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

enoodle added 2 commits November 15, 2021 18:49

use autoscaling/v2beta2 hpa and set its window

1d1b430

update tests

c234499

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

enoodle mentioned this pull request Nov 15, 2021

Custom Metrics HPA Autoscaler #12277

Merged

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 added 2 commits November 15, 2021 19:39

remove stabilization window for hpa

d3ad75a

use autoscaling constatnts

0ff0097

enoodle force-pushed the hpa-v2beta2 branch from 10abc37 to 0ff0097 Compare November 15, 2021 17:39

markusthoemmes requested review from psschwei and nader-ziada November 15, 2021 18:05

psschwei reviewed Nov 15, 2021

View reviewed changes

nader-ziada reviewed Nov 15, 2021

View reviewed changes

markusthoemmes reviewed Nov 15, 2021

View reviewed changes

enoodle added 2 commits November 15, 2021 21:44

update the e2e test

62f2053

use autoscalingv2beta2 constants in test

ba6b557

knative-prow-robot added the area/test-and-release It flags unit/e2e/conformance/perf test issues for product features label Nov 15, 2021

markusthoemmes approved these changes Nov 16, 2021

View reviewed changes

knative-prow-robot assigned markusthoemmes Nov 16, 2021

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

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

knative-prow-robot merged commit 532231b into knative:main Nov 16, 2021

HPA v2beta2 #12278

HPA v2beta2 #12278

Uh oh!

Conversation

enoodle commented Nov 15, 2021

Proposed Changes

Uh oh!

knative-prow-robot commented Nov 15, 2021

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!

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!

markusthoemmes 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!

psschwei left a comment

Choose a reason for hiding this comment

Uh oh!

nader-ziada 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!

nader-ziada commented Nov 15, 2021

Uh oh!

markusthoemmes left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Nov 16, 2021

Uh oh!

Uh oh!

codecov bot commented Nov 15, 2021 •

edited

Loading