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

Add external metric type to HPA API #60096

Merged
merged 3 commits into from Feb 23, 2018

Conversation

@MaciekPytel
Contributor

MaciekPytel commented Feb 20, 2018

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

This comment has been minimized.

Contributor

MaciekPytel commented Feb 20, 2018

/sig-autoscaling

@DirectXMan12 Can you take a look?

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

This comment has been minimized.

@DirectXMan12

DirectXMan12 Feb 21, 2018

Contributor

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

This comment has been minimized.

@MaciekPytel

MaciekPytel Feb 22, 2018

Contributor

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.

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

This comment has been minimized.

@DirectXMan12

DirectXMan12 Feb 21, 2018

Contributor

lowercase in public API docs

This comment has been minimized.

@MaciekPytel

MaciekPytel Feb 22, 2018

Contributor

fixed everywhere

@DirectXMan12

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

// 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).

This comment has been minimized.

@thockin

thockin Feb 21, 2018

Member

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?

This comment has been minimized.

@mwielgus

mwielgus Feb 21, 2018

Contributor

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.

This comment has been minimized.

@thockin

thockin Feb 22, 2018

Member

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

This comment has been minimized.

@MaciekPytel

MaciekPytel Feb 22, 2018

Contributor

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?

This comment has been minimized.

@thockin
@thockin

This comment has been minimized.

Member

thockin commented Feb 22, 2018

/approve

@thockin

This comment has been minimized.

Member

thockin commented Feb 22, 2018

/lgtm

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 22, 2018

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

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

@MaciekPytel

This comment has been minimized.

Contributor

MaciekPytel commented Feb 22, 2018

/hold

@MaciekPytel

This comment has been minimized.

Contributor

MaciekPytel commented Feb 22, 2018

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

@mwielgus

This comment has been minimized.

Contributor

mwielgus commented Feb 22, 2018

Corresponding proposal merged. Removing hold.

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 23, 2018

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.

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

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation MaciekPytel authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
allErrs := field.ErrorList{}
if len(src.MetricName) == 0 {
allErrs = append(allErrs, field.Required(fldPath.Child("metricName"), "must specify a metric name"))

This comment has been minimized.

@liggitt

liggitt Feb 26, 2018

Member

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

k8s-merge-robot added a commit that referenced this pull request Feb 27, 2018

Merge pull request #60424 from bskiba/em-kubectl
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
```

k8s-merge-robot added a commit that referenced this pull request Feb 27, 2018

Merge pull request #60243 from MaciekPytel/hpa_api_ext_imp
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment