Add container based scaling to HPA #90691

arjunrn · 2020-05-02T19:22:15Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR introduces a new metric source called ContainerResourceMetricSource and a corresponding ContainerResourceMetricStatus types which are used to specify metric targets for individual containers in the target pods and the status of those metrics.

Which issue(s) this PR fixes:

Fixes #86349

Special notes for your reviewer:

/label api-review

Does this PR introduce a user-facing change?:

Introduces a metric source for HPAs which allows scaling based on container resource usage.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-autoscaling/0001-container-resource-autoscaling.md:
- [Usage]: https://github.com/kubernetes/website/pull/23523

k8s-ci-robot · 2020-05-02T19:22:23Z

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.

arjunrn · 2020-05-02T19:26:49Z

/label api-review

arjunrn · 2020-05-20T12:41:59Z

/assign @josephburnett

liggitt · 2020-05-21T18:02:17Z

Thanks for the PR. It looks like the linked proposal is not yet marked as implementable and is missing test plan info. Unflagging for API review.

fejta-bot · 2020-05-21T18:37:24Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

arjunrn · 2020-06-03T09:34:15Z

/label api-review

pkg/features/kube_features.go

pkg/registry/autoscaling/horizontalpodautoscaler/strategy.go

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

pkg/apis/autoscaling/validation/validation.go

pwittrock · 2020-09-15T20:16:14Z

@arjunrn please split out the cli changes into a separate PR, or at least commit.

soltysh

sig-cli related changes lgtm

liggitt

one comment on API docs, one question about the containerResource name validation, rebase, then lgtm

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

liggitt · 2020-10-21T03:57:05Z

pkg/apis/autoscaling/validation/validation.go

+	if len(src.Name) == 0 {
+		allErrs = append(allErrs, field.Required(fldPath.Child("name"), "must specify a resource name"))
+	} else {
+		if !helper.IsStandardContainerResourceName(string(src.Name)) {


is this intending to allow all resources a container could request/limit? if so, this doesn't allow as much as validateContainerResourceName. is that ok?

if this intending to allow only cpu/memory? if so, this also allows ephemeral-storage and hugepage resources. is that ok?

Right now the resource field for the regular resource type does not have any validation in it. So users can specify even ephemeral-storage and hugepage- resources for scaling. However metrics-server which is used by the HPA controller does not support those resources at the moment. But if they were, the HPA controller would support them out of the box because the resource name is not validated. So should I now restrict which resources can be specified(limit to cpu and memory) or should I be a bit more permissive and allow all container resources.

cc @josephburnett

I think replica_calculator.go is pretty generic all the way through. So we should allow any valid resource.

@josephburnett @liggitt In that case the current validation is correct. So I will leave this as is.

In that case the current validation is correct

are you sure? this validation is narrower than validateContainerResourceName, so it would disallow resources a container could request, right?

I misunderstood your first comment. You're right this is more restrictive. So now I have 3 options:

Copy the validateContainerResourceName function.

Turn the validateContainerResourceName into a public function and add tests.

Remove the validation to keep it the same as validation for the resource name in the ResourceMetricSource.

I prefer option 2. WDYT?

2 sounds good to me

@liggitt I've made changes. Can you take another look.

liggitt · 2020-10-21T12:32:11Z

API doc updates lgtm. I'll defer to @josephburnett on the desired allowed values for the container resource. Once that is determined, add validation unit tests demonstrating the allowed/disallowed values.

…sources Signed-off-by: Arjun Naik <anaik@redhat.com>

liggitt · 2020-10-22T14:43:25Z

/lgtm
/approve

k8s-ci-robot · 2020-10-22T14:43:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: arjunrn, josephburnett, liggitt, 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:

~~api/OWNERS~~ [liggitt]
~~pkg/apis/OWNERS~~ [liggitt]
~~pkg/controller/podautoscaler/OWNERS~~ [josephburnett,liggitt,soltysh]
~~pkg/features/OWNERS~~ [liggitt,soltysh]
~~pkg/printers/OWNERS~~ [liggitt,soltysh]
~~pkg/registry/OWNERS~~ [liggitt]
~~staging/src/k8s.io/api/OWNERS~~ [liggitt]
~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]
~~staging/src/k8s.io/kubectl/OWNERS~~ [liggitt,soltysh]

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

liggitt · 2020-10-22T14:43:57Z

thanks, changes look good

fejta-bot · 2020-10-23T07:05:28Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

arjunrn · 2020-10-23T12:06:24Z

/retest

k8s-ci-robot requested review from brendandburns and caesarxuchao May 2, 2020 19:24

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label May 2, 2020

arjunrn force-pushed the container-resource-hpa branch from bd6fa22 to e6a2ed7 Compare May 6, 2020 10:43

k8s-ci-robot assigned josephburnett May 20, 2020

liggitt removed the api-review Categorizes an issue or PR as actively needing an API review. label May 21, 2020

arjunrn mentioned this pull request May 21, 2020

Mark KEP implementable and add test plan kubernetes/enhancements#1805

Merged

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jun 3, 2020

liggitt reviewed Sep 3, 2020

View reviewed changes

pkg/features/kube_features.go Outdated Show resolved Hide resolved

liggitt reviewed Sep 4, 2020

View reviewed changes

arjunrn force-pushed the container-resource-hpa branch 2 times, most recently from edc135a to 1051fbe Compare September 13, 2020 14:33

arjunrn force-pushed the container-resource-hpa branch from 1051fbe to 36c04e9 Compare September 16, 2020 08:05

soltysh approved these changes Sep 16, 2020

View reviewed changes

arjunrn force-pushed the container-resource-hpa branch from 36c04e9 to a14e75a Compare October 12, 2020 14:29

arjunrn mentioned this pull request Oct 12, 2020

Container Resource based Pod Autoscaling kubernetes/enhancements#1610

Closed

liggitt reviewed Oct 21, 2020

View reviewed changes

arjunrn force-pushed the container-resource-hpa branch 2 times, most recently from e1e437c to 1b730d9 Compare October 21, 2020 09:29

arjunrn force-pushed the container-resource-hpa branch from 1b730d9 to 1b3d3a3 Compare October 21, 2020 17:50

Added functionality and API for pod autoscaling based on container re…

0fec7b0

…sources Signed-off-by: Arjun Naik <anaik@redhat.com>

arjunrn force-pushed the container-resource-hpa branch from 1b3d3a3 to 0fec7b0 Compare October 21, 2020 19:17

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 22, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 22, 2020

k8s-ci-robot merged commit ec453ff into kubernetes:master Oct 23, 2020

k8s-ci-robot added this to the v1.20 milestone Oct 23, 2020

arjunrn deleted the container-resource-hpa branch October 23, 2020 12:57

github-actions bot mentioned this pull request Nov 3, 2020

Week Ending November 1, 2020 dev-obs/actus#260

Open

howardjohn mentioned this pull request Nov 24, 2020

Support container based HPA scaling istio/istio#29165

Closed

silenceper mentioned this pull request Nov 25, 2020

Allow cpu/memory scaler to target a specific container in the pod kedacore/keda#1378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add container based scaling to HPA #90691

Add container based scaling to HPA #90691

arjunrn commented May 2, 2020 •

edited

Loading

k8s-ci-robot commented May 2, 2020

arjunrn commented May 2, 2020

arjunrn commented May 20, 2020

liggitt commented May 21, 2020 •

edited

Loading

fejta-bot commented May 21, 2020

arjunrn commented Jun 3, 2020

pwittrock commented Sep 15, 2020

soltysh left a comment

liggitt left a comment

liggitt Oct 21, 2020

arjunrn Oct 21, 2020

arjunrn Oct 21, 2020

josephburnett Oct 21, 2020 •

edited

Loading

arjunrn Oct 21, 2020

liggitt Oct 21, 2020

arjunrn Oct 21, 2020

liggitt Oct 21, 2020

arjunrn Oct 22, 2020

liggitt commented Oct 21, 2020

liggitt commented Oct 22, 2020

k8s-ci-robot commented Oct 22, 2020

liggitt commented Oct 22, 2020

fejta-bot commented Oct 23, 2020

arjunrn commented Oct 23, 2020

Add container based scaling to HPA #90691

Add container based scaling to HPA #90691

Conversation

arjunrn commented May 2, 2020 • edited Loading

k8s-ci-robot commented May 2, 2020

arjunrn commented May 2, 2020

arjunrn commented May 20, 2020

liggitt commented May 21, 2020 • edited Loading

fejta-bot commented May 21, 2020

arjunrn commented Jun 3, 2020

pwittrock commented Sep 15, 2020

soltysh left a comment

Choose a reason for hiding this comment

liggitt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephburnett Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt commented Oct 21, 2020

liggitt commented Oct 22, 2020

k8s-ci-robot commented Oct 22, 2020

liggitt commented Oct 22, 2020

fejta-bot commented Oct 23, 2020

arjunrn commented Oct 23, 2020

arjunrn commented May 2, 2020 •

edited

Loading

liggitt commented May 21, 2020 •

edited

Loading

josephburnett Oct 21, 2020 •

edited

Loading