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.

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

KEP: Enhance HPA Metrics Specificity #2055

Merged

Conversation

DirectXMan12
Copy link
Contributor

This KEP introduces additional tools into the autoscaling/v2 API for
working with metrics, including support for a metrics label selector,
and expanded support for the targetAverageValue field.

This PR also introduces a KEP folder for SIG Autoscaling.

@k8s-ci-robot k8s-ci-robot added 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 Apr 19, 2018
@k8s-ci-robot k8s-ci-robot added the sig/architecture Categorizes an issue or PR as relevant to SIG Architecture. label Apr 19, 2018
@DirectXMan12
Copy link
Contributor Author

cc @kubernetes/sig-autoscaling-feature-requests @kubernetes/sig-instrumentation-feature-requests

@k8s-ci-robot k8s-ci-robot added sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. kind/feature Categorizes issue or PR as related to a new feature. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Apr 19, 2018
@MaciekPytel
Copy link
Contributor

MaciekPytel commented Apr 19, 2018

If we're already changing the layout of the API maybe it would be worth it to move target to a separate unified struct (similar to what you do with moving fields describing metric to MetricSpec)? With that change your example would look like that:

- type: Object
  object:
    target:
      kind: StatefulSet 
      apiVersion: apps/v1
      name: foo
    targetValue: # not very happy with the name, I'd use target except the name would conflict
      type: Value
      value: 2
    metric:
      name: queue_length
      selector: "queue=some-jobs"

@thockin suggested that approach during his review of external metric API: kubernetes/kubernetes#60096 (review) (you need to unfold the comment). WDYT?

metrics adapters support the new changes -- this is up to those adapters
maintainers.

## Proposal
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this misses a single part - what do we do if multiple time series are matched by selector? In external metrics API we sum all the values (we assume the user may not want to drill down on some dimension captured by metric, for example use all HTTP requests, regardless of response code). My assumption is we want to do the same? We should say it explicitly though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If multiple series are matched, the adapter must deal with it, similarly to the current CM API (see my other comment).

API:

```go
type MetricValue struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

In external metrics API we return a list of values, one for each time series. I think we need to do that in CM API as well or we need to say that it's adapters job to aggregate all time series into a single value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the intention was that it's the adapter's job to aggregate, if necessary. That's the approach we took before the specificity, and I missed that the external metrics API doesn't do that (I'd prefer if it did). I'll note that explicitly

@DirectXMan12 DirectXMan12 force-pushed the proposal/hpa-object-metric-updates branch from fe703c2 to 6889e19 Compare May 21, 2018 19:08
@DirectXMan12
Copy link
Contributor Author

Factored in targetXXX unification, added note about what to do in case of multiple series.

@kubernetes/api-reviewers I'd also like to make the utilization serialize more sanely, but I want to make sure it's acceptible. Utilization would still be an int (e.g. 40) internally, but it would serialize as a string with a percent sign (e.g. 40%) to make it more obvious what's going on. Utilization continues to be one of the more confusing subjects for users, and I figure this will at least help a bit.

@jdumars jdumars added this to Backlog in KEP Tracking Jun 5, 2018
@jdumars jdumars moved this from Backlog to In SIG Review in KEP Tracking Jun 25, 2018
k8s-github-robot pushed a commit to kubernetes/kubernetes that referenced this pull request Aug 27, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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-publishing-bot added a commit to kubernetes/api that referenced this pull request Aug 28, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
k8s-publishing-bot added a commit to kubernetes/client-go that referenced this pull request Aug 28, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Aug 28, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
k8s-publishing-bot added a commit to kubernetes/kube-aggregator that referenced this pull request Aug 28, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
k8s-publishing-bot added a commit to kubernetes/sample-apiserver that referenced this pull request Aug 28, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 28, 2018
sttts pushed a commit to sttts/apiserver that referenced this pull request Sep 5, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
k8s-publishing-bot added a commit to kubernetes/apiserver that referenced this pull request Sep 6, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
k8s-publishing-bot added a commit to kubernetes/cli-runtime that referenced this pull request Sep 8, 2018
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 <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

HPA metrics specificity improvements

**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**:

```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

Kubernetes-commit: fdb5707194d56cbbd0da11c5be3a2a5653e714c9
@fejta-bot
Copy link

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 27, 2018
@justaugustus
Copy link
Member

/kind kep
/remove-lifecycle rotten

@k8s-ci-robot k8s-ci-robot added kind/kep and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Oct 13, 2018
@justaugustus
Copy link
Member

REMINDER: KEPs are moving to k/enhancements on November 30. Please attempt to merge this KEP before then to signal consensus.
For more details on this change, review this thread.

Any questions regarding this move should be directed to that thread and not asked on GitHub.

This introduces a KEP folder for SIG autoscaling, similar to the ones
for other SIGs.
@DirectXMan12
Copy link
Contributor Author

@jdumars @justaugustus this is ready to be merged (the implementation was done a bit ago), but it needs higher-level LGTM to be merged because it introduces the sig-autoscaling directory for the first time.

@justaugustus
Copy link
Member

/test pull-community-verify

This KEP introduces additional tools into the autoscaling/v2 API for
working with metrics, including support for a metrics label selector,
and expanded support for the `targetAverageValue` field.
@DirectXMan12 DirectXMan12 force-pushed the proposal/hpa-object-metric-updates branch from 008cfd4 to 9248f9e Compare November 27, 2018 23:19
@justaugustus
Copy link
Member

KEP structure:
/lgtm

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

jdumars commented Nov 28, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jdumars

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 28, 2018
@k8s-ci-robot k8s-ci-robot merged commit 8b556bf into kubernetes:master Nov 28, 2018
@DirectXMan12 DirectXMan12 deleted the proposal/hpa-object-metric-updates branch November 28, 2018 18:11
justaugustus pushed a commit to justaugustus/community that referenced this pull request Dec 1, 2018
…ect-metric-updates

KEP: Enhance HPA Metrics Specificity
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. 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/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
No open projects
KEP Tracking
  
In SIG Review
Development

Successfully merging this pull request may close these issues.

None yet

6 participants