Add external metric type to HPA API #60096

MaciekPytel · 2018-02-20T21:22:33Z

What this PR does / why we need it:
Add external metric type to HPA API proposed in xref kubernetes/community#1801

Release note:

Allows HorizontalPodAutoscaler to use global metrics not associated with any Kubernetes object (for example metrics from a hoster service running outside of Kubernetes cluster).

MaciekPytel · 2018-02-20T21:22:57Z

/sig-autoscaling

@DirectXMan12 Can you take a look?

DirectXMan12 · 2018-02-21T22:07:39Z

pkg/apis/autoscaling/validation/validation.go

+	}
+
+	if src.TargetAverageValue != nil && src.TargetAverageValue.Sign() != 1 {
+		allErrs = append(allErrs, field.Invalid(fldPath.Child("targetAverageValue"), src.TargetAverageValue, "must be positive"))


we'll want to re-evaluate this later, because it's not strictly true for custom metrics

Sure. I'm not sure what the specific use-case is and how do we want to handle metric value changing signs, but in general I see no reason not to allow it later on.

DirectXMan12 · 2018-02-21T22:09:50Z

staging/src/k8s.io/api/autoscaling/v2beta1/types.go

+type ExternalMetricSource struct {
+	// metricName is the name of the metric in question.
+	MetricName string `json:"metricName" protobuf:"bytes,1,name=metricName"`
+	// MetricSelector is used to identify a specific time series


lowercase in public API docs

fixed everywhere

DirectXMan12

minor nits inline. This also needs HPA changes themselves, no?

thockin · 2018-02-21T23:24:35Z

pkg/apis/autoscaling/types.go

+	// MetricSelector is used to identify a specific time series
+	// within a given metric.
+	MetricSelector metav1.LabelSelector
+	// TargetValue is the target value of the metric (as a quantity).


Since these next fields represent a one-of, they should probably be in a struct. This should document clearly what happens if no value is specified.

e.g

external: metricName: foobar target: value: 100

What do you think?

There is no big difference between grouping 2 fields in a struct and having them "inline". However in other places in this version of the api we have the target inline. So structs will be kindof inconsistent. We can do it in v2beta2, changing target to structs both in ObjectMetrics and ExernalMetrics.

No value behaviour should obviously be documented.

I can accept the consistency argument. We have very inconsistent use of one-of blocks throughout the system. Really this should be explicitly discriminated:

external: metricName: foobar target: type: Value value: 100

Documented that exactly one of the two must be specified. As @mwielgus wrote - we already have TargetAverageUtilization and TargetAverageValue in resource metrics. I agree with your argument, but I think consistency within a single API object should take precedence in this case. If we used different approach with external and left resource as is we would end up with HPA objects looking like this:

- resource: targetAverageValue: 100 - external: target: type: Average value: 100

I believe I'm in agreement with @DirectXMan12 that we want a v2beta2 to (at minimum) add targetAverageValue to object metrics. This would already be backward incompatible, so maybe we can consider changing all targets to structs then?

thockin · 2018-02-22T17:00:04Z

/approve

thockin · 2018-02-22T17:00:34Z

/lgtm

k8s-ci-robot · 2018-02-22T17:00:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MaciekPytel, thockin

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:

~~api/OWNERS~~ [thockin]
~~docs/api-reference/OWNERS~~ [thockin]
~~pkg/apis/OWNERS~~ [thockin]
~~staging/src/k8s.io/api/OWNERS~~ [thockin]

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

MaciekPytel · 2018-02-22T17:03:21Z

/hold

MaciekPytel · 2018-02-22T19:31:53Z

@DirectXMan12 I've created a separate PR with implementation (to keep things more readable): #60243.

mwielgus · 2018-02-22T19:37:50Z

Corresponding proposal merged. Removing hold.

k8s-github-robot · 2018-02-23T07:17:40Z

Automatic merge from submit-queue (batch tested with PRs 60208, 60084, 60183, 59713, 60096). If you want to cherry-pick this change to another branch, please follow the instructions here.

liggitt · 2018-02-26T15:15:44Z

pkg/apis/autoscaling/validation/validation.go

+	allErrs := field.ErrorList{}
+
+	if len(src.MetricName) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath.Child("metricName"), "must specify a metric name"))


since this will be used as a single path segment in URL-building, this should be validated with path.IsValidPathSegmentName

@MaciekPytel

Automatic merge from submit-queue (batch tested with PRs 59310, 60424, 60308, 60436, 60020). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Add support for external metrics in kubectl **What this PR does / why we need it**: Add support for metrics coming from external sources when describing or printing horizontal pod autoscaler objects. API change: #60096 @MaciekPytel @DirectXMan12 **Release note**: ``` NONE ```

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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. Implement external metric in HPA This implement the changes to HPA introduced in #60096

snowplayfire · 2020-06-10T07:32:40Z

I want to add a new field for hpa like this PR, and I write 'Convert_xxx' and 'autoConvert_xxx' for the new field. But conversion-gengenerate and override my manual 'Convert_xxx' and 'autoConvert_xxx' when compile.

Can anyone tell me how to avoid the override? Or Is there any docs about conversion-gen?

Thanks very much!

thockin · 2020-06-15T23:09:39Z

Most of the time, conversion code can be auto-generated. Occasionally you need to to help it out. The conversion code generator will create autoConvert* functions. If it does not find a manually defined Convert* function, it will generate that as a wrapper.

To add manual code, you create the Convert function, which should first call autoConvert* and then you make whatever manual changes you need to make.

MaciekPytel requested review from DirectXMan12 and mwielgus February 20, 2018 21:22

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 20, 2018

k8s-ci-robot requested review from caesarxuchao, dims, nikhiljindal and zmerlynn February 20, 2018 21:22

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

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

MaciekPytel removed request for dims and nikhiljindal February 20, 2018 21:27

MaciekPytel force-pushed the hpa_api_ext branch from 01c8838 to 737d743 Compare February 21, 2018 09:12

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 21, 2018

DirectXMan12 reviewed Feb 21, 2018

View reviewed changes

DirectXMan12 suggested changes Feb 21, 2018

View reviewed changes

thockin reviewed Feb 21, 2018

View reviewed changes

MaciekPytel added 2 commits February 22, 2018 14:44

Add external metric type to HPA API

8a002d8

Autogenerated code for HPA external metrics

079f3f1

MaciekPytel force-pushed the hpa_api_ext branch from c3d9886 to a6bca10 Compare February 22, 2018 14:02

Validation for HPA external metrics

602aaaf

MaciekPytel force-pushed the hpa_api_ext branch from a6bca10 to 602aaaf Compare February 22, 2018 14:53

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

k8s-ci-robot assigned thockin Feb 22, 2018

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

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

MaciekPytel mentioned this pull request Feb 22, 2018

Implement external metric in HPA #60243

Merged

mwielgus removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 22, 2018

k8s-github-robot merged commit b22b785 into kubernetes:master Feb 23, 2018

bskiba mentioned this pull request Feb 26, 2018

Add support for external metrics in kubectl #60424

Merged

liggitt reviewed Feb 26, 2018

View reviewed changes

MaciekPytel mentioned this pull request Apr 19, 2018

KEP: Enhance HPA Metrics Specificity kubernetes/community#2055

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add external metric type to HPA API #60096

Add external metric type to HPA API #60096

MaciekPytel commented Feb 20, 2018 •

edited by thockin

MaciekPytel commented Feb 20, 2018

DirectXMan12 Feb 21, 2018

MaciekPytel Feb 22, 2018

DirectXMan12 Feb 21, 2018

MaciekPytel Feb 22, 2018

DirectXMan12 left a comment

thockin Feb 21, 2018

mwielgus Feb 21, 2018 •

edited

thockin Feb 22, 2018

MaciekPytel Feb 22, 2018

thockin Feb 22, 2018

thockin commented Feb 22, 2018

thockin commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

MaciekPytel commented Feb 22, 2018

MaciekPytel commented Feb 22, 2018

mwielgus commented Feb 22, 2018

k8s-github-robot commented Feb 23, 2018

liggitt Feb 26, 2018

snowplayfire commented Jun 10, 2020

thockin commented Jun 15, 2020

Add external metric type to HPA API #60096

Add external metric type to HPA API #60096

Conversation

MaciekPytel commented Feb 20, 2018 • edited by thockin

MaciekPytel commented Feb 20, 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

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwielgus Feb 21, 2018 • edited

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

thockin commented Feb 22, 2018

thockin commented Feb 22, 2018

k8s-ci-robot commented Feb 22, 2018

MaciekPytel commented Feb 22, 2018

MaciekPytel commented Feb 22, 2018

mwielgus commented Feb 22, 2018

k8s-github-robot commented Feb 23, 2018

Choose a reason for hiding this comment

snowplayfire commented Jun 10, 2020

thockin commented Jun 15, 2020

MaciekPytel commented Feb 20, 2018 •

edited by thockin

mwielgus Feb 21, 2018 •

edited