Added functionality for specifying target average value for object me… #72872

arjunrn · 2019-01-13T15:53:43Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds the feature of using average values with object metrics in horizontal pod autoscalers.

Which issue(s) this PR fixes:
Fixes #72824

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-01-13T15:53:45Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2019-01-13T15:53:50Z

Hi @arjunrn. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mwielgus

How was this change tested?

arjunrn · 2019-01-14T17:24:23Z

@mwielgus I have not tested it with anything more than the unit tests. The logic is the same as External type metric sources so I mostly reused that both in the functionality and the unit tests. If there are e2e tests I could add some tests there as well. Or do you have some other suggestions?

MaciekPytel · 2019-01-14T19:11:13Z

I'm not saying you need to add e2e test, but if you wanted to there are e2e tests for custom metrics in https://github.com/kubernetes/kubernetes/blob/master/test/e2e/autoscaling/custom_metrics_stackdriver_autoscaling.go. Problem is those are all using stackdriver, so you may not have an option to run it locally.
It should be pretty easy to add a test by combining existing object metrics test and external metrics with average value test.

I think it would be ok to merge a reasonably looking test and just letting CI run it to make sure it works.

arjunrn · 2019-01-15T19:19:21Z

@MaciekPytel I have already added unit tests both for the replica calculator and the controller which are based on existing tests. Is there anything which you think is missing?

MaciekPytel · 2019-01-15T19:27:59Z

@arjunrn I'm good. I don't think e2e test is necessary for this change, but since you mentioned it I commented on how to add it.

mwielgus · 2019-01-16T05:51:29Z

I want to confirm that you build K8S release with this PR compiled in, ran it, tested it manually and it worked fine.

arjunrn · 2019-01-18T10:42:25Z

@mwielgus I applied this PR to the 1.12.4 release and tested it with a deployment for ingress metrics. It works as expected. The only problem is that when the HPA is described it configured to use only targetValue metrics and doesn't display the current metrics. Looks something like this:

Name:                                                      coffee-2
Namespace:                                                 default
Labels:                                                    <none>
Annotations:                                               kubectl.kubernetes.io/last-applied-configuration:
                                                             {"apiVersion":"autoscaling/v2beta2","kind":"HorizontalPodAutoscaler","metadata":{"annotations":{},"name":"coffee-2","namespace":"default"}...
CreationTimestamp:                                         Fri, 18 Jan 2019 10:05:11 +0100
Reference:                                                 Deployment/coffee-2
Metrics:                                                   ( current / target )
  "requests-per-second,coffee-2" on Ingress/cafe-ingress:  0 / 0
Min replicas:                                              1
Max replicas:                                              10
Deployment pods:                                           10 current / 10 desired
Conditions:
  Type            Status  Reason               Message
  ----            ------  ------               -------
  AbleToScale     True    ScaleDownStabilized  recent recommendations were higher than current one, applying the highest recent recommendation
  ScalingActive   True    ValidMetricFound     the HPA was able to successfully calculate a replica count from external metric requests-per-second,coffee-2(nil)
  ScalingLimited  False   DesiredWithinRange   the desired count is within the acceptable range
Events:
  Type    Reason             Age                From                       Message
  ----    ------             ----               ----                       -------
  Normal  SuccessfulRescale  62m                horizontal-pod-autoscaler  New size: 2; reason: All metrics below target
  Normal  SuccessfulRescale  61m (x2 over 77m)  horizontal-pod-autoscaler  New size: 1; reason: All metrics below target
  Normal  SuccessfulRescale  40m (x2 over 70m)  horizontal-pod-autoscaler  New size: 3; reason: external metric requests-per-second,coffee-2(nil) above target
  Normal  SuccessfulRescale  37m                horizontal-pod-autoscaler  New size: 5; reason: external metric requests-per-second,coffee-2(nil) above target
  Normal  SuccessfulRescale  28m                horizontal-pod-autoscaler  New size: 10; reason: external metric requests-per-second,coffee-2(nil) above target

I would like to fix this as well before the PR is merged.

arjunrn · 2019-01-18T16:41:51Z

@mwielgus Can you review this PR now? I've added the changes for describing the HPA.

MaciekPytel · 2019-01-23T17:50:36Z

/ok-to-test

Signed-off-by: Arjun Naik <arjun.rn@gmail.com>

mwielgus · 2019-01-28T14:17:57Z

/lgtm

mwielgus · 2019-01-28T14:18:08Z

/approve

arjunrn · 2019-01-28T14:21:50Z

/assign @smarterclayton

soltysh

/approve
kubectl changes

k8s-ci-robot · 2019-02-01T13:18:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arjunrn, mwielgus, soltysh

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:

~~pkg/controller/podautoscaler/OWNERS~~ [mwielgus]
~~pkg/kubectl/OWNERS~~ [soltysh]

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

tedyu · 2019-02-01T16:25:50Z

pkg/controller/podautoscaler/horizontal.go

-				Value: resource.NewMilliQuantity(utilizationProposal, resource.DecimalSI),
+		}
+		return replicaCountProposal, timestampProposal, fmt.Sprintf("%s metric %s", metricSpec.Object.DescribedObject.Kind, metricSpec.Object.Metric.Name), nil
+	} else if metricSpec.Object.Target.Type == autoscalingv2.AverageValueMetricType {


nit: 'else' can be omitted

mrgleeco · 2019-03-06T05:52:12Z

Any ideas when this will land in a release?

arunpmohan · 2019-05-10T03:41:18Z

Is there a workaround for this in older releases ? When prometheus adapter is used for JVM pods then Object type is the only way for custom metrics and targetAverageValue not working kills us . Which release will this be coming ?

mikkeloscar · 2019-05-10T05:34:59Z

@arunpmohan which version are you on? If you are on v1.12 or later then https://github.com/zalando-incubator/kube-metrics-adapter could maybe be an option for you, it will support averageValue for prometheus once we merge: zalando-incubator/kube-metrics-adapter#53

arunpmohan · 2019-05-10T14:47:20Z

Thanks for your reply.I am on 1.13.4 and i was k8s-prometheus-adapter for JVM metrics https://github.com/DirectXMan12/k8s-prometheus-adapter. So with the link you said will it replace the prometheus adapter or the metric server ? I am guessing metric server and i will still need prometheus adapter. correct ? Also is there a helm chart available to deploy that. I couldn't find any.

mikkeloscar · 2019-05-10T15:45:45Z

It's an alternative to the k8s-prometheus-adapter. You can read the difference here: https://github.com/zalando-incubator/kube-metrics-adapter#prometheus-collector

We don't have a helm chart yet.

This adds support for `averageValue` for the `request-per-second` metric based on Ingress Objects. This is only supported from Kubernetes `>=v1.14` (kubernetes/kubernetes#72872). When defining the HPA with `autoscaling/v2beta1` you still need to define `targetValue` even though it won't be used when `averageValue` is set. Once we default to `autoscaling/v2beta2` this akward API will be gone. Signed-off-by: Mikkel Oscar Lyderik Larsen <mikkel.larsen@zalando.de>

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 13, 2019

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jan 13, 2019

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 13, 2019

k8s-ci-robot requested review from jszczepkowski and MaciekPytel January 13, 2019 15:54

mwielgus self-assigned this Jan 14, 2019

mwielgus self-requested a review January 14, 2019 16:13

mwielgus reviewed Jan 14, 2019

View reviewed changes

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Jan 18, 2019

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jan 23, 2019

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 Jan 23, 2019

Added functionality to use target average value for object metrics

c99d505

Signed-off-by: Arjun Naik <arjun.rn@gmail.com>

arjunrn force-pushed the object-average-value branch from c7e9454 to c99d505 Compare January 23, 2019 20:00

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 28, 2019

k8s-ci-robot assigned smarterclayton Jan 28, 2019

soltysh approved these changes Feb 1, 2019

View reviewed changes

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

k8s-ci-robot merged commit a3f74bd into kubernetes:master Feb 1, 2019

tedyu reviewed Feb 1, 2019

View reviewed changes

arjunrn deleted the object-average-value branch February 23, 2019 15:36

arunpmohan mentioned this pull request May 10, 2019

Empty Metrics fetch from API kubernetes-sigs/prometheus-adapter#139

Closed

mikkeloscar mentioned this pull request Jul 27, 2019

Add support for averageValue for request-per-second Skipper metric zalando-incubator/kube-metrics-adapter#68

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added functionality for specifying target average value for object me… #72872

Added functionality for specifying target average value for object me… #72872

arjunrn commented Jan 13, 2019

k8s-ci-robot commented Jan 13, 2019

k8s-ci-robot commented Jan 13, 2019

mwielgus left a comment

arjunrn commented Jan 14, 2019 •

edited

MaciekPytel commented Jan 14, 2019

arjunrn commented Jan 15, 2019

MaciekPytel commented Jan 15, 2019

mwielgus commented Jan 16, 2019 •

edited

arjunrn commented Jan 18, 2019

arjunrn commented Jan 18, 2019

MaciekPytel commented Jan 23, 2019

mwielgus commented Jan 28, 2019

mwielgus commented Jan 28, 2019

arjunrn commented Jan 28, 2019

soltysh left a comment

k8s-ci-robot commented Feb 1, 2019

tedyu Feb 1, 2019

mrgleeco commented Mar 6, 2019

arunpmohan commented May 10, 2019

mikkeloscar commented May 10, 2019

arunpmohan commented May 10, 2019 •

edited

mikkeloscar commented May 10, 2019

Added functionality for specifying target average value for object me… #72872

Added functionality for specifying target average value for object me… #72872

Conversation

arjunrn commented Jan 13, 2019

k8s-ci-robot commented Jan 13, 2019

k8s-ci-robot commented Jan 13, 2019

mwielgus left a comment

Choose a reason for hiding this comment

arjunrn commented Jan 14, 2019 • edited

MaciekPytel commented Jan 14, 2019

arjunrn commented Jan 15, 2019

MaciekPytel commented Jan 15, 2019

mwielgus commented Jan 16, 2019 • edited

arjunrn commented Jan 18, 2019

arjunrn commented Jan 18, 2019

MaciekPytel commented Jan 23, 2019

mwielgus commented Jan 28, 2019

mwielgus commented Jan 28, 2019

arjunrn commented Jan 28, 2019

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 1, 2019

tedyu Feb 1, 2019

Choose a reason for hiding this comment

mrgleeco commented Mar 6, 2019

arunpmohan commented May 10, 2019

mikkeloscar commented May 10, 2019

arunpmohan commented May 10, 2019 • edited

mikkeloscar commented May 10, 2019

arjunrn commented Jan 14, 2019 •

edited

mwielgus commented Jan 16, 2019 •

edited

arunpmohan commented May 10, 2019 •

edited