Convert HPA controller to support HPA v2 mechanics #41272

DirectXMan12 · 2017-02-10T23:25:26Z

This PR converts the HPA controller to support the mechanics from HPA v2.
The HPA controller continues to make use of the HPA v1 client, but utilizes
the conversion logic to work with autoscaling/v2alpha1 objects internally.

It is the follow-up PR to #36033 and part of kubernetes/enhancements#117.

Release note:

NONE

k8s-github-robot · 2017-02-10T23:25:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: DirectXMan12

Needs approval from an approver in each of these OWNERS Files:

pkg/controller/podautoscaler/OWNERS

We suggest the following people:
cc @derekwaynecarr
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-reviewable · 2017-02-10T23:26:31Z

This change is

DirectXMan12 · 2017-02-10T23:28:14Z

cc @jszczepkowski

DirectXMan12 · 2017-02-11T00:03:05Z

cc @caesarxuchao @deads2k @liggitt @lavalamp re the discussion the other day about different object versions in the controller on Slack, hopefully this solution doesn't make you recoil in horror :-P

jszczepkowski · 2017-02-15T14:05:40Z

pkg/controller/podautoscaler/metrics/utilization.go

@@ -23,9 +23,10 @@ import (
 // GetResourceUtilizationRatio takes in a set of metrics, a set of matching requests,
 // and a target utilization percentage, and calcuates the the ratio of
 // desired to actual utilization (returning that and the actual utilization)
-func GetResourceUtilizationRatio(metrics PodResourceInfo, requests map[string]int64, targetUtilization int32) (float64, int32, error) {
+func GetResourceUtilizationRatio(metrics PodMetricsInfo, requests map[string]int64, targetUtilization int32) (float64, int32, int64, error) {


Can you please name the returned values? Now it is unclear which one is which.

Also the comment should be updated.

jszczepkowski · 2017-02-15T14:07:52Z

pkg/controller/podautoscaler/metrics/utilization.go

 }

 // GetMetricUtilizationRatio takes in a set of metrics and a target utilization value,
 // and calcuates the ratio of desired to actual utilization
 // (returning that and the actual utilization)
-func GetMetricUtilizationRatio(metrics PodMetricsInfo, targetUtilization float64) (float64, float64) {
-	metricsTotal := float64(0)
+func GetMetricUtilizationRatio(metrics PodMetricsInfo, targetUtilization int64) (float64, int64) {


Can you name the returned values?

jszczepkowski · 2017-02-15T15:47:30Z

pkg/controller/podautoscaler/replica_calculator.go

+// GetMetricReplicas calculates the desired replica count based on a target metric utilization
+// (as a milli-value) for pods matching the given selector in the given namespace, and the
+// current replica count
+func (c *ReplicaCalculator) GetMetricReplicas(currentReplicas int32, targetUtilization int64, metricName string, namespace string, selector labels.Selector) (replicaCount int32, utilization int64, timestamp time.Time, err error) {


It seems that methods GetMetricReplicas and GetRawResourceReplicas are similar. Did you consider making their implementation to use a common method?

jszczepkowski · 2017-02-15T15:57:07Z

pkg/controller/podautoscaler/replica_calculator.go

+
+// GetObjectMetricReplicas calculates the desired replica count based on a target metric utilization (as a milli-value)
+// for the given object in the given namespace, and the current replica count.
+func (c *ReplicaCalculator) GetObjectMetricReplicas(currentReplicas int32, targetUtilization int64, metricName string, namespace string, objectRef *autoscaling.CrossVersionObjectReference) (replicaCount int32, utilization int64, timestamp time.Time, err error) {


Do you have a unittest for this method?

there isn't one because the existing replica calc test is structured around the actual metrics client, so it needs a working implementation of GetObjectMetrics, which we don't have (it just returns a "not-implemented" error). I can either a) restructure the entire test, or b) add in the test in part 3 of this PR, which adds support for custom metrics using the custom metrics API types.

jszczepkowski · 2017-02-16T12:09:50Z

pkg/controller/podautoscaler/horizontal_test.go

@@ -704,20 +683,6 @@ func TestScaleUpCMUnreadyNoScaleWouldScaleDown(t *testing.T) {
 	tc.runTest(t)
 }

-func TestDefaultScaleDown(t *testing.T) {


Why was this test removed?

We don't have implicit defaulting any more -- it's done a the API level (defaulting for v2alpha1, and conversion for v1 for reasons of method signatures and JSON unmarshalling).

jszczepkowski · 2017-02-16T12:10:45Z

pkg/controller/podautoscaler/horizontal_test.go

@@ -504,58 +526,6 @@ func (tc *testCase) runTest(t *testing.T) {
 	tc.verifyResults(t)
 }

-func TestDefaultScaleUpRC(t *testing.T) {


Why were these tests removed?

jszczepkowski · 2017-02-16T13:11:38Z

pkg/controller/podautoscaler/horizontal.go

@@ -139,141 +150,140 @@ func (a *HorizontalController) Run(stopCh <-chan struct{}) {
 	glog.Infof("Shutting down HPA Controller")
 }

-// getLastScaleTime returns the hpa's last scale time or the hpa's creation time if the last scale time is nil.
-func getLastScaleTime(hpa *autoscaling.HorizontalPodAutoscaler) time.Time {


Don't we need it anymore? Where is lastScaleTime set to CreationTimestamp?

don't think this was intentional -- probably got lost amongst the rebases

Ah, I know why this got removed -- it's largely irrelevent with the improved handling for missing pods -- under the new scheme, we'll only error out if no metrics at all are present, which will only occur a) right after the deployment is initially created, or b) if we have an actual error.

jszczepkowski · 2017-02-16T13:43:21Z

pkg/controller/podautoscaler/horizontal.go


-	for _, customMetricTarget := range targetList.Items {
+	for i, metricSpec := range metricSpecs {
 		if scale.Status.Selector == nil {


I don't understand this check: scale.Status.Selector is array, we should use len to get its length. I guess this check will be always false.

selector is a map, and technically maps can be nil. But I think this was from when we were using the internal version, and never got properly updated. I'll fix it.

This commit converts the HPA controller over to using the new version of the HorizontalPodAutoscaler object found in autoscaling/v2alpha1. Note that while the autoscaler will accept requests for object metrics, the scale client will return an error on attempts to get object metrics (since that requires the new custom metrics API, which is not yet implemented). This also enables the HPA object in v2alpha1 as a retrievable API version by default.

DirectXMan12 · 2017-02-16T20:03:53Z

@jszczepkowski I believe I've address all of your comments (either as code changes or as replies). PTAL.

jszczepkowski · 2017-02-17T09:51:40Z

/lgtm /approve

Please, fix e2e tests.

DirectXMan12 · 2017-02-17T16:01:27Z

@k8s-bot gce etcd3 e2e test this

Unsure if this was a flake, but looks like it might be. Let's try again...

DirectXMan12 · 2017-02-17T18:55:33Z

@k8s-bot cvm gke e2e test this

@k8s-bot kubemark e2e test this

looks like the first one was a flake, let's try the rest again

There was a bug in the HPA v1 conversion logic that would occur when a custom metric and a metric that was encoded in v1 as targetCPUUtilizationPercentage were used at the same time. In this case, the custom metric could overwrite the CPU metric, or vice versa. This fixes that bug, and ensures that the fuzzer tests round-tripping with multiple metrics.

k8s-github-robot · 2017-02-20T09:52:19Z

Automatic merge from submit-queue

deads2k · 2017-02-21T15:01:27Z

pkg/controller/podautoscaler/horizontal_test.go

-		return true, obj, nil
+
+		// and... convert to autoscaling v1 to return the right type
+		objv1, err := UnsafeConvertToVersionVia(obj, autoscalingv1.SchemeGroupVersion)


do a deep copy first.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 10, 2017

k8s-github-robot assigned fgrzadkowski Feb 10, 2017

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Feb 10, 2017

DirectXMan12 mentioned this pull request Feb 10, 2017

Arbitrary/Custom Metrics in the Horizontal Pod Autoscaler kubernetes/enhancements#117

Closed

7 tasks

DirectXMan12 force-pushed the feature/hpa-v2-controller branch from 82d086a to 2057c2b Compare February 10, 2017 23:58

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 10, 2017

fgrzadkowski assigned jszczepkowski and unassigned fgrzadkowski Feb 13, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

DirectXMan12 force-pushed the feature/hpa-v2-controller branch from 2057c2b to 728d7d0 Compare February 14, 2017 16:10

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

DirectXMan12 mentioned this pull request Feb 14, 2017

Dynamic Scale Client #41441

Closed

jszczepkowski reviewed Feb 15, 2017

View reviewed changes

jszczepkowski reviewed Feb 16, 2017

View reviewed changes

DirectXMan12 force-pushed the feature/hpa-v2-controller branch from 728d7d0 to 7846827 Compare February 16, 2017 20:03

jszczepkowski added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 20, 2017

k8s-github-robot merged commit 2f0e5ba into kubernetes:master Feb 20, 2017

DirectXMan12 deleted the feature/hpa-v2-controller branch February 20, 2017 16:54

ncdc mentioned this pull request Feb 21, 2017

flake: podautoscaler unit test failing #41768

Closed

deads2k reviewed Feb 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert HPA controller to support HPA v2 mechanics #41272

Convert HPA controller to support HPA v2 mechanics #41272

DirectXMan12 commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

k8s-reviewable commented Feb 10, 2017

DirectXMan12 commented Feb 10, 2017

DirectXMan12 commented Feb 11, 2017

jszczepkowski Feb 15, 2017

jszczepkowski Feb 15, 2017

jszczepkowski Feb 15, 2017

jszczepkowski Feb 15, 2017

jszczepkowski Feb 15, 2017

DirectXMan12 Feb 16, 2017

jszczepkowski Feb 16, 2017

DirectXMan12 Feb 16, 2017

jszczepkowski Feb 16, 2017

jszczepkowski Feb 16, 2017

DirectXMan12 Feb 16, 2017

DirectXMan12 Feb 16, 2017

jszczepkowski Feb 16, 2017

DirectXMan12 Feb 16, 2017

DirectXMan12 commented Feb 16, 2017

jszczepkowski commented Feb 17, 2017

DirectXMan12 commented Feb 17, 2017

DirectXMan12 commented Feb 17, 2017

k8s-github-robot commented Feb 20, 2017

deads2k Feb 21, 2017

Convert HPA controller to support HPA v2 mechanics #41272

Convert HPA controller to support HPA v2 mechanics #41272

Conversation

DirectXMan12 commented Feb 10, 2017

k8s-github-robot commented Feb 10, 2017

k8s-reviewable commented Feb 10, 2017

DirectXMan12 commented Feb 10, 2017

DirectXMan12 commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Feb 16, 2017

jszczepkowski commented Feb 17, 2017

DirectXMan12 commented Feb 17, 2017

DirectXMan12 commented Feb 17, 2017

k8s-github-robot commented Feb 20, 2017

Choose a reason for hiding this comment