Implement external metric in HPA #60243

MaciekPytel · 2018-02-22T19:27:00Z

This implement the changes to HPA introduced in #60096

k8s-ci-robot · 2018-02-22T19:27:02Z

@MaciekPytel: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

MaciekPytel · 2018-02-22T19:30:14Z

/hold
This is based on #60096, #60079 and #60220. It must not be merged before all those.

Note: this PR is just implementation for implementation in HPA (last 2 commits). Everything else is just the result of including commits from PRs listed above.

/release-note-none
/sig-autoscaling

DirectXMan12

comments inline

DirectXMan12 · 2018-02-22T19:33:15Z

pkg/controller/podautoscaler/horizontal.go

+					setCondition(hpa, autoscalingv2.ScalingActive, v1.ConditionFalse, "FailedGetExternalMetric", "the HPA was unable to compute the replica count: %v", err)
+					return 0, "", nil, time.Time{}, fmt.Errorf("failed to get external metric %s: %v", metricSpec.External.MetricName, err)
+				}
+				metricNameProposal = fmt.Sprintf("external metric %s", metricSpec.External.MetricName)


should also list label selector here.

DirectXMan12 · 2018-02-22T19:35:14Z

pkg/controller/podautoscaler/horizontal_test.go

@@ -466,8 +467,8 @@ func (tc *testCase) prepareTestClient(t *testing.T) (*fake.Clientset, *metricsfa
 			metrics := &cmapi.MetricValueList{}

 			// multiple objects
-			assert.Equal(t, "pods", getForAction.GetResource().Resource, "the type of object that we requested multiple metrics for should have been pods")
-			assert.Equal(t, "qps", getForAction.GetMetricName(), "the metric name requested should have been qps, as specified in the metric spec")
+			assert.Equal(t, "pods", getForAction.GetResource().Resource, fmt.Sprintf("got: %v, want: pods", getForAction.GetResource().Resource))


no need for the extra fmt.Sprintf, IIRC -- the terminating arguments here are themselves a format string.

also, what's with this change -- we already have a "want" and "got" as part of the assert. The message is just providing more context, and the way you've changed it provides less.

DirectXMan12 · 2018-02-22T19:36:54Z

pkg/controller/podautoscaler/horizontal_test.go

+
+		metrics := &emapi.ExternalMetricValueList{}
+
+		assert.Equal(t, "qps", getForAction.GetMetricName(), fmt.Sprintf("got: %v, want: qps", getForAction.GetMetricName()))


the message here should be more expressive (see above)

DirectXMan12 · 2018-02-22T19:38:38Z

pkg/controller/podautoscaler/replica_calculator.go

+		// update number of replicas if the change is large enough
+		replicaCount = int32(math.Ceil(float64(utilization) / float64(targetUtilizationPerPod)))
+	}
+	utilization = int64(math.Ceil(float64(utilization) / float64(currentReplicas)))


This needs to follow the same logic as pods, so it should factor in unready pods.

The logic for handling unready pods in pods is about how to account for such pods when calculating total value of metric. In this case we get a single metric value and just calculate the desired number of pods. I don't see how logic in pods could apply here?

DirectXMan12 · 2018-02-22T19:39:00Z

pkg/controller/podautoscaler/replica_calculator_test.go

@@ -292,7 +326,7 @@ func (tc *replicaCalcTestCase) runTest(t *testing.T) {
 		require.NoError(t, err, "there should not have been an error calculating the replica count")
 		assert.Equal(t, tc.expectedReplicas, outReplicas, "replicas should be as expected")
 		assert.Equal(t, tc.metric.expectedUtilization, outUtilization, "utilization should be as expected")
-		assert.True(t, tc.timestamp.Equal(outTimestamp), "timestamp should be as expected")
+		assert.True(t, tc.timestamp.Equal(outTimestamp), fmt.Sprintf("got: %v, want: %v", outTimestamp, tc.timestamp))


again, please don't make the message redundant and less expressive

DirectXMan12

minor testing nit inline. otherwise looks good

DirectXMan12 · 2018-02-26T16:19:54Z

pkg/controller/podautoscaler/replica_calculator_test.go

+			levels:                  []int64{8600},
+			perPodTargetUtilization: 2150,
+			expectedUtilization:     2867,
+			selector:                &metav1.LabelSelector{},


can we get a test that uses LabelSelector and verifies that it's passed through correctly?

Added LabelSelector to most unittests, created a new test case without it.

DirectXMan12 · 2018-02-26T16:21:47Z

pkg/controller/podautoscaler/replica_calculator.go

+		// update number of replicas if the change is large enough
+		replicaCount = int32(math.Ceil(float64(utilization) / float64(targetUtilizationPerPod)))
+	}
+	utilization = int64(math.Ceil(float64(utilization) / float64(currentReplicas)))


DirectXMan12

you should be good once tests pass

DirectXMan12 · 2018-02-26T19:17:39Z

/lgtm
/approve

MaciekPytel · 2018-02-26T22:15:41Z

/retest

MaciekPytel · 2018-02-27T09:29:25Z

/hold cancel

mwielgus · 2018-02-27T13:14:05Z

pkg/controller/podautoscaler/horizontal.go

@@ -299,6 +299,45 @@ func (a *HorizontalController) computeReplicasForMetrics(hpa *autoscalingv2.Hori
 					},
 				}
 			}
+		case autoscalingv2.ExternalMetricSourceType:
+			if metricSpec.External.TargetAverageValue != nil {


Has it been verified manually E2e?

mwielgus · 2018-02-27T13:14:24Z

pkg/controller/podautoscaler/horizontal.go

+						CurrentAverageValue: resource.NewMilliQuantity(utilizationProposal, resource.DecimalSI),
+					},
+				}
+			} else if metricSpec.External.TargetValue != nil {


Has it been validated manually e2e?

wojtek-t · 2018-02-27T13:14:59Z

/approve no-issue

mwielgus · 2018-02-27T13:15:23Z

pkg/controller/podautoscaler/horizontal.go

+						CurrentValue:   *resource.NewMilliQuantity(utilizationProposal, resource.DecimalSI),
+					},
+				}
+			} else {


Do we have a check for this in validate logic?

Yes, https://github.com/kubernetes/kubernetes/blob/master/pkg/apis/autoscaling/validation/validation.go#L200

mwielgus · 2018-02-27T13:17:24Z

pkg/controller/podautoscaler/metrics/interfaces.go

+
+	// GetExternalMetric gets all the values of a given external metric
+	// that match the specified selector.
+	GetExternalMetric(metricName string, namespace string, selector labels.Selector) ([]int64, time.Time, error)


Lets include Millis in the name of the metrics. It is not obvious from the method name that it returns the value in millis. Here and elsewhere. Can be next PR.

Ok, I'll do it in a PR later on (it's just a naming change, no functional impact).

mwielgus · 2018-02-27T13:18:42Z

/lgtm

mwielgus · 2018-02-27T13:18:48Z

/approve

k8s-ci-robot · 2018-02-27T13:18:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, MaciekPytel, mwielgus, wojtek-t

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:

~~cmd/kube-controller-manager/OWNERS~~ [wojtek-t]
~~pkg/controller/podautoscaler/OWNERS~~ [DirectXMan12,MaciekPytel,mwielgus,wojtek-t]

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

k8s-github-robot · 2018-02-27T16:25:46Z

Automatic merge from submit-queue (batch tested with PRs 60433, 59982, 59128, 60243, 60440). If you want to cherry-pick this change to another branch, please follow the instructions here.

MaciekPytel requested review from DirectXMan12 and mwielgus February 22, 2018 19:27

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 22, 2018

k8s-ci-robot requested review from caesarxuchao, david-mcmahon, eparis, erictune, ixdy, lavalamp, liggitt and luxas February 22, 2018 19:29

ixdy removed their request for review February 22, 2018 19:30

MaciekPytel mentioned this pull request Feb 22, 2018

Add external metric type to HPA API #60096

Merged

MaciekPytel removed request for caesarxuchao, david-mcmahon, eparis, erictune, lavalamp, liggitt and luxas February 22, 2018 19:33

mwielgus added the sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. label Feb 22, 2018

DirectXMan12 suggested changes Feb 22, 2018

View reviewed changes

MaciekPytel force-pushed the hpa_api_ext_imp branch from 0c0c767 to d8657e0 Compare February 26, 2018 12:05

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 26, 2018

DirectXMan12 suggested changes Feb 26, 2018

View reviewed changes

MaciekPytel force-pushed the hpa_api_ext_imp branch from d8657e0 to 37204e8 Compare February 26, 2018 17:20

DirectXMan12 approved these changes Feb 26, 2018

View reviewed changes

k8s-ci-robot assigned DirectXMan12 Feb 26, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2018

MaciekPytel force-pushed the hpa_api_ext_imp branch from 37204e8 to 9928ae0 Compare February 27, 2018 09:28

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 27, 2018

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

MaciekPytel and others added 2 commits February 27, 2018 14:10

Add external metrics client to HPA rest client

66f4f90

Implement external metrics in HPA

e58411c

MaciekPytel force-pushed the hpa_api_ext_imp branch from 9928ae0 to e58411c Compare February 27, 2018 13:12

mwielgus reviewed Feb 27, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 27, 2018

mwielgus reviewed Feb 27, 2018

View reviewed changes

k8s-ci-robot assigned mwielgus Feb 27, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 27, 2018

k8s-github-robot merged commit a2ddca7 into kubernetes:master Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement external metric in HPA #60243

Implement external metric in HPA #60243

MaciekPytel commented Feb 22, 2018 •

edited

k8s-ci-robot commented Feb 22, 2018

MaciekPytel commented Feb 22, 2018

DirectXMan12 left a comment

DirectXMan12 Feb 22, 2018

DirectXMan12 Feb 22, 2018

DirectXMan12 Feb 22, 2018

DirectXMan12 Feb 22, 2018

DirectXMan12 Feb 22, 2018

MaciekPytel Feb 23, 2018

DirectXMan12 Feb 26, 2018

DirectXMan12 Feb 22, 2018

DirectXMan12 left a comment

DirectXMan12 Feb 26, 2018

MaciekPytel Feb 26, 2018

DirectXMan12 Feb 26, 2018

DirectXMan12 left a comment

DirectXMan12 commented Feb 26, 2018

MaciekPytel commented Feb 26, 2018

MaciekPytel commented Feb 27, 2018

mwielgus Feb 27, 2018

MaciekPytel Feb 27, 2018

mwielgus Feb 27, 2018

MaciekPytel Feb 27, 2018

wojtek-t commented Feb 27, 2018

mwielgus Feb 27, 2018

MaciekPytel Feb 27, 2018

mwielgus Feb 27, 2018

MaciekPytel Feb 27, 2018

mwielgus commented Feb 27, 2018

mwielgus commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018


		metrics := &emapi.ExternalMetricValueList{}

		assert.Equal(t, "qps", getForAction.GetMetricName(), fmt.Sprintf("got: %v, want: qps", getForAction.GetMetricName()))

Implement external metric in HPA #60243

Implement external metric in HPA #60243

Conversation

MaciekPytel commented Feb 22, 2018 • edited

k8s-ci-robot commented Feb 22, 2018

MaciekPytel commented Feb 22, 2018

DirectXMan12 left a 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 left a 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 left a comment

Choose a reason for hiding this comment

DirectXMan12 commented Feb 26, 2018

MaciekPytel commented Feb 26, 2018

MaciekPytel commented Feb 27, 2018

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

wojtek-t commented Feb 27, 2018

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

mwielgus commented Feb 27, 2018

mwielgus commented Feb 27, 2018

k8s-ci-robot commented Feb 27, 2018

k8s-github-robot commented Feb 27, 2018

MaciekPytel commented Feb 22, 2018 •

edited