Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HPA metrics specificity improvements #64097

Merged
merged 8 commits into from
Aug 27, 2018
Merged

HPA metrics specificity improvements #64097

merged 8 commits into from
Aug 27, 2018

Conversation

damemi
Copy link
Contributor

@damemi damemi commented May 21, 2018 •
edited
Loading

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

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 21, 2018
@k8s-github-robot 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 damemi force-pushed the hpa-metrics-specificity branch 4 times, most recently from fc43898 to 5054c35 Compare May 23, 2018 18:22
@damemi damemi changed the title HPA metrics specificity improvements [wip] HPA metrics specificity improvements May 24, 2018
@k8s-ci-robot 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 damemi force-pushed the hpa-metrics-specificity branch from 4dd4d94 to 0b8d822 Compare May 25, 2018 14:51
@DirectXMan12 DirectXMan12 mentioned this pull request May 25, 2018
Merged
@damemi damemi force-pushed the hpa-metrics-specificity branch 2 times, most recently from 85ab679 to dfc07b8 Compare May 29, 2018 00:51
@DirectXMan12
Copy link
Contributor

/ok-to-test

@k8s-ci-robot 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 damemi force-pushed the hpa-metrics-specificity branch 4 times, most recently from b55aa0b to 0cf93da Compare May 30, 2018 17:44
@DirectXMan12
Copy link
Contributor

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

@DirectXMan12
Copy link
Contributor

Make sure we have validation on the metric target

@DirectXMan12
Copy link
Contributor

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

@DirectXMan12
Copy link
Contributor

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

DirectXMan12
DirectXMan12 reviewed May 30, 2018
View reviewed changes
pkg/apis/autoscaling/v2beta1/conversion.go
return nil
}

func Convert_autoscaling_MetricTarget_To_v2beta1_CrossVersionObjectReference(in *autoscaling.MetricTarget, out *autoscalingv2beta1.CrossVersionObjectReference, s conversion.Scope) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@damemi damemi May 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 24, 2018
@damemi damemi force-pushed the hpa-metrics-specificity branch from e08ec81 to 9e7ab02 Compare August 24, 2018 19:55
@k8s-ci-robot k8s-ci-robot added sig/cli Categorizes an issue or PR as relevant to SIG CLI. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 24, 2018
@damemi damemi force-pushed the hpa-metrics-specificity branch from 9e7ab02 to 88eca22 Compare August 27, 2018 14:25
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2018
@damemi damemi force-pushed the hpa-metrics-specificity branch from 88eca22 to d9f0d32 Compare August 27, 2018 14:40
@damemi damemi force-pushed the hpa-metrics-specificity branch from d9f0d32 to 354e146 Compare August 27, 2018 15:09
@damemi damemi force-pushed the hpa-metrics-specificity branch from 354e146 to dd7e81a Compare August 27, 2018 15:37
@smarterclayton
Copy link
Contributor

/approve

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 27, 2018
@DirectXMan12
Copy link
Contributor

/lgtm

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

[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

Needs approval from an approver in each of these files:

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
Copy link

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-github-robot k8s-github-robot merged commit fdb5707 into kubernetes:master Aug 27, 2018
@MaciekPytel MaciekPytel mentioned this pull request Aug 29, 2018
Closed
@damemi damemi mentioned this pull request Aug 29, 2018
Merged
@damemi damemi mentioned this pull request Sep 18, 2018
Merged
@damemi damemi mentioned this pull request May 30, 2019
Closed
6 tasks
This was referenced Dec 6, 2021
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@dchen1107 dchen1107 Awaiting requested review from dchen1107

Assignees

@DirectXMan12 DirectXMan12

@lavalamp lavalamp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.
9 participants
@damemi @DirectXMan12 @k8s-ci-robot @smarterclayton @zparnold @krzyzacy @BenTheElder @k8s-github-robot @lavalamp