HPA metrics specificity improvements by damemi · Pull Request #64097 · kubernetes/kubernetes

damemi · 2018-05-21T14:21:32Z

What this PR does / why we need it:
Improves available specificity for HPA metrics by adding metric selector fields for metrics of Pods and Objects.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Implements this KEP: kubernetes/community#2055

Special notes for your reviewer:
Need to add/update tests?

Release note:

Introduces autoscaling/v2beta2 and custom_metrics/v1beta2, which implement metric selectors for Object and Pods metrics, as well as allowing AverageValue targets on Objects, similar to External metrics.

/assign @DirectXMan12

DirectXMan12 · 2018-05-29T17:37:13Z

/ok-to-test

DirectXMan12 · 2018-05-30T20:39:57Z

For the APIServices int test, you need to enable the API version in PKG/master/master.go

DirectXMan12 · 2018-05-30T20:50:31Z

Make sure we have validation on the metric target

DirectXMan12 · 2018-05-30T23:41:38Z

you missed some cases of switching to scheme.ParameterCodec in the custom metrics client.

DirectXMan12 · 2018-05-30T23:48:56Z

conversions aren't quite right -- you don't properly populate the type field in the metric target, it looks like

DirectXMan12 · 2018-05-30T23:52:46Z

pkg/apis/autoscaling/v2beta1/conversion.go

why are these nil? If they're not needed, don't implement them

That's what I thought, but without them defined as nil they get generated for some reason:

# k8s.io/kubernetes/pkg/apis/autoscaling/v2beta1 pkg/apis/autoscaling/v2beta1/zz_generated.conversion.go:507:12: undefined: Convert_v2beta1_CrossVersionObjectReference_To_autoscaling_MetricTarget pkg/apis/autoscaling/v2beta1/zz_generated.conversion.go:519:12: undefined: Convert_autoscaling_MetricTarget_To_v2beta1_CrossVersionObjectReference !!! [0530 21:24:29] Call tree: !!! [0530 21:24:29] 1: /home/mdame/go/src/k8s.io/kubernetes/hack/lib/golang.sh:595 kube::golang::build_binaries_for_platform(...) !!! [0530 21:24:29] 2: hack/make-rules/build.sh:27 kube::golang::build_binaries(...) !!! [0530 21:24:29] Call tree: !!! [0530 21:24:29] 1: hack/make-rules/build.sh:27 kube::golang::build_binaries(...) !!! [0530 21:24:29] Call tree: !!! [0530 21:24:29] 1: hack/make-rules/build.sh:27 kube::golang::build_binaries(...) make: *** [Makefile:92: all] Error 1

(Output from just running make with those functions removed and zz_generated.conversion deleted)

I think it has to do with the fact that Target in v2beta1 is a CrossVersionObjectReference, but in v2beta2 we renamed that to DescribedObject and created a new field called Target that is a MetricTarget. So it's seeing Target=Target and trying to auto-convert CrossVersionObjectReference->MetricTarget and telling me I need to manually convert it (which I can't)

If I'm wrong, or there's a way around that, I can make the change (or I can add a comment explaining these functions, which I probably should have done first)

k8s-github-robot · 2018-07-16T18:22:25Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@DirectXMan12 @damemi @lavalamp

Pull Request Labels

sig/autoscaling: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/feature: New functionality.

Help

damemi · 2018-08-09T16:53:50Z

@smarterclayton did you have any more feedback you'd like to add to this?

smarterclayton · 2018-08-12T18:13:37Z

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

These need to be sentences - newlines are not considered significant and will cause this to be run on output.

smarterclayton · 2018-08-12T18:14:37Z

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

You need to explain how this is different from CurrentAverageValue. Reading the godoc, I have no idea.

This was actually added in error -- averageValue is sufficient for pods metrics. Thanks for catching, removed

smarterclayton · 2018-08-13T18:52:03Z

pkg/apis/autoscaling/v2beta2/defaults.go

Is it ever valid to have no metrics? I think you need to add as a comment on the Metrics field in v2beta2 about the default. I.e "if not set, the default metric will be X% CPU utilization" (fill in the X)

This is just following the defaults set by previous versions of the autoscaling API. Added a comment to the API about the default percentage (currently 80)

smarterclayton · 2018-08-13T18:52:45Z

staging/src/k8s.io/metrics/pkg/apis/custom_metrics/v1beta1/types.go

Describe what happens if this is empty.

smarterclayton · 2018-08-13T18:53:29Z

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

Explain what selector means to the user in this context. Why would they set it, what happens if it is empty, etc.

Done (also in other areas this appears-- v2beta1 and v2beta2)

smarterclayton · 2018-08-13T18:53:34Z

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

Explain what selector means to the user in this context. Why would they set it, what happens if it is empty, etc.

smarterclayton · 2018-08-13T18:53:37Z

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

Explain what selector means to the user in this context. Why would they set it, what happens if it is empty, etc.

@smarterclayton updated with your feedback, is there anything else that stands out?

damemi · 2018-08-14T16:43:11Z

/retest

krzyzacy · 2018-08-14T18:16:16Z

/retest

…etrics/v1beta2

smarterclayton · 2018-08-27T17:55:01Z

/approve

DirectXMan12 · 2018-08-27T18:27:37Z

/lgtm

k8s-ci-robot · 2018-08-27T18:27:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, DirectXMan12, smarterclayton

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~api/OWNERS~~ [smarterclayton]
~~cmd/kube-apiserver/OWNERS~~ [smarterclayton]
~~docs/api-reference/OWNERS~~ [smarterclayton]
~~hack/OWNERS~~ [smarterclayton]
~~pkg/OWNERS~~ [smarterclayton]
~~staging/OWNERS~~ [smarterclayton]
~~test/e2e/instrumentation/OWNERS~~ [DirectXMan12,smarterclayton]
~~test/integration/dryrun/OWNERS~~ [smarterclayton]
~~test/integration/etcd/OWNERS~~ [smarterclayton]

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-08-27T20:48:11Z

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

k8s-ci-robot assigned DirectXMan12 May 21, 2018

k8s-ci-robot requested review from caesarxuchao and dchen1107 May 21, 2018 14:21

k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/new-api labels May 21, 2018

damemi force-pushed the hpa-metrics-specificity branch 4 times, most recently from fc43898 to 5054c35 Compare May 23, 2018 18:22

damemi changed the title ~~HPA metrics specificity improvements~~ [wip] HPA metrics specificity improvements May 24, 2018

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 24, 2018

damemi force-pushed the hpa-metrics-specificity branch from 4dd4d94 to 0b8d822 Compare May 25, 2018 14:51

DirectXMan12 mentioned this pull request May 25, 2018

HPA Specificity Improvements kubernetes/website#8757

Merged

damemi force-pushed the hpa-metrics-specificity branch 2 times, most recently from 85ab679 to dfc07b8 Compare May 29, 2018 00:51

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 29, 2018

damemi force-pushed the hpa-metrics-specificity branch 4 times, most recently from b55aa0b to 0cf93da Compare May 30, 2018 17:44

DirectXMan12 reviewed May 30, 2018

View reviewed changes

smarterclayton reviewed Aug 12, 2018

View reviewed changes

smarterclayton reviewed Aug 13, 2018

View reviewed changes

DirectXMan12 mentioned this pull request Aug 21, 2018

Add option for using prometheus labels instead running selector against API server kubernetes-sigs/prometheus-adapter#1

Closed

damemi added 8 commits August 27, 2018 11:07

Autoscaling v2beta2 and custom metrics v1beta2 APIs

5d1ee16

Update autoscaling conversion and validation for v2beta2 inclusion

a79916f

Implement autoscaling/v2beta2 features in HPA controller

c7102ee

Update metrics API to include autoscaling/v2beta2 changes

e31eff0

Update printers to include changes from autoscaling/v2beta2

17740a2

Add autoscaling/v2beta2 and custom_metrics/v1beta2 to necessary files

f407700

Generate files and modifications for autoscaling/v2beta2 and custom_m…

77d7f9c

…etrics/v1beta2

Add dry run test for hpa v2beta2

dd7e81a

MaciekPytel mentioned this pull request Aug 29, 2018

HPA no longer works with adapters exposing Custom Metrics v1beta1 API #68011

Closed

damemi mentioned this pull request Aug 29, 2018

Support backwards compatibility for v1beta1 custom metrics client #68015

Merged

damemi mentioned this pull request Sep 18, 2018

Update custom metrics conversion functions #68795

Merged

damemi mentioned this pull request May 30, 2019

REQUEST: New membership for damemi kubernetes/org#860

Closed

6 tasks

This was referenced Dec 6, 2021

Add hpa v2beta2 to deprecation guide for v1.23 kubernetes/website#30720

Closed

Add hpa v2beta1 and v2beta2 to deprecation guide kubernetes/website#30776

Merged

Conversation

damemi commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DirectXMan12 commented May 29, 2018

Uh oh!

DirectXMan12 commented May 30, 2018

Uh oh!

DirectXMan12 commented May 30, 2018

Uh oh!

DirectXMan12 commented May 30, 2018

Uh oh!

DirectXMan12 commented May 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damemi May 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-github-robot commented Jul 16, 2018

Uh oh!

damemi commented Aug 9, 2018

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!

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!

damemi commented Aug 14, 2018

Uh oh!

krzyzacy commented Aug 14, 2018

Uh oh!

smarterclayton commented Aug 27, 2018

Uh oh!

DirectXMan12 commented Aug 27, 2018

Uh oh!

k8s-ci-robot commented Aug 27, 2018

Uh oh!

k8s-github-robot commented Aug 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

damemi commented May 21, 2018 •

edited

Loading

damemi May 31, 2018 •

edited

Loading