Add Vertical Pod Autoscaler to autoscaling/v2beta1 #63797

kgrygiel · 2018-05-14T13:27:19Z

What this PR does / why we need it:
Adds Vertical Pod Autoscaler (https://github.com/kubernetes/community/blob/master/contributors/design-proposals/autoscaling/vertical-pod-autoscaler.md) to the autoscaling API (which currently has the Horizontal Pod Autoscaler).
This is needed for the Vertical Pod Autoscaler beta.

Special notes for your reviewer:

/cc @thockin @mwielgus @DirectXMan12

FYI. changes that add pkg/registry/autoscaling/verticalpodautoscaler/... will follow.

Release note:

NONE

thockin · 2018-05-16T00:13:18Z

Is this ready for review?

mwielgus · 2018-05-16T18:12:13Z

@thockin Yes, the api is ready for review.

mwielgus · 2018-05-24T20:39:51Z

/lgtm

from the sig-autoscaling point of view.

@DirectXMan12 - last chance to veto :).

DirectXMan12 · 2018-05-25T14:06:23Z

hold on, I had some comments that never got sent
/hold

DirectXMan12 · 2018-05-22T18:53:45Z

pkg/apis/autoscaling/types.go

+
+var (
+	// Configured indicates whether the VPA recommender was able to load a valid VPA spec.
+	Configured VerticalPodAutoscalerConditionType = "Configured"


do we need this if we have validation instead?

Right. Can be removed.

DirectXMan12 · 2018-05-22T18:55:37Z

pkg/apis/autoscaling/types.go

+	// UpdateModeAuto means that autoscaler assigns resources on pod creation
+	// and additionally can update them during the lifetime of the pod,
+	// including evicting / rescheduling the pod.
+	UpdateModeAuto UpdateMode = "Auto"


This should be "AutoRecreate", so that we can add "AutoInPlace" when we have in-place updates, and not be confusing (recreate is still valuable for things that need to do some resource-based config on creation, like environments that pre-set a heap size limit).

Good point. I suggest we actually may want both. I think many users would want to configure the best (least intrusive) actuation method available. Once we have in-place updates, "auto" will automatically get promoted to in-place without rewriting configs. WDYT?

DirectXMan12 · 2018-05-22T18:57:59Z

pkg/apis/autoscaling/types.go

+	Name string
+	// Whether autoscaler is enabled for the container. Defaults to "On".
+	// +optional
+	Mode ContainerScalingMode


do we want to just make this enabled and be a boolean?

There might be some other options added in the future. I would stay with enum.

Can we name the options in a less "this should be a bool" way ?

I can imagine potentially more interesting policies in future, if users need it, e.g. "OnlyScaleUp". However I can't think of better names for the current options..

"Auto" and "Off" ?

DirectXMan12 · 2018-05-22T18:59:51Z

pkg/apis/autoscaling/types.go

+}
+
+// PodResourcePolicy controls how autoscaler computes the recommended resources
+// for containers belonging to the pod.


// PodResourcePolicy controls how autoscaler computes the recommended resources // or containers belonging to the pod. There must either be an entry for every named pod, // or a wildcard entry.

DirectXMan12 · 2018-05-22T19:00:23Z

pkg/apis/autoscaling/types.go

+)
+
+// RecommendedPodResources is the recommendation of resources computed by
+// autoscaler.


... It contains a recommendation for each container in the pod

DirectXMan12 · 2018-05-25T14:08:44Z

(just some minor nits)

thockin

I only reviewed the API parts. A few relatively minor comments overall.

thockin · 2018-05-29T16:19:09Z

pkg/apis/autoscaling/types.go

+type VerticalPodAutoscalerStatus struct {
+	// The time when the status was last refreshed.
+	// +optional
+	LastUpdateTime metav1.Time


Can you explain the need for this? It's not present in any other APIs

agree... heartbeat-style fields tend to cause scale issues (write traffic persists even in steady state)

The purpose was to let the user know about the freshness of the recommendation. But I understand your concern and I'm ok with deleting it.

thockin · 2018-05-29T16:30:18Z

pkg/apis/autoscaling/types.go

+
+// VerticalPodAutoscalerCondition describes the state of
+// a VerticalPodAutoscaler at a certain point.
+type VerticalPodAutoscalerCondition struct {


I think we could consider some common definition of Condition. Several APIs define their own FooCondition struct but they are all almost identical. The strong typing of the string doesn't protect against illegal values (Go does not have enums) anyway.

@kubernetes/api-reviewers
@bgrant0607 @smarterclayton

ACK. However in this step I would rather keep vpa consistent with hpa and perhaps consider having both (and others) changed together in a larger rewrite?

thockin · 2018-05-29T16:33:23Z

pkg/apis/autoscaling/types.go

+)
+
+// PodUpdatePolicy describes the rules on how changes are applied to the pods.
+type PodUpdatePolicy struct {


This is a little invasive, but Can we organize these files in Go-introduction order? I find it makes reviews much easier.

E.g.

type VPA { Spec VPASpec Status VPAStatus } type VPASpec { Foo FooType Bar BarType } type FooType { Whatever EnumString } Type EnumString Type BarType type VPAStatus { Qux QuxType }

thockin · 2018-05-29T17:25:55Z

pkg/apis/autoscaling/types.go

+	Name string
+	// Whether autoscaler is enabled for the container. Defaults to "On".
+	// +optional
+	Mode ContainerScalingMode


Can we name the options in a less "this should be a bool" way ?

thockin · 2018-06-04T20:35:11Z

pkg/apis/autoscaling/types.go

+
+	// Describes the rules on how changes are applied to the pods.
+	// +optional
+	UpdatePolicy PodUpdatePolicy


If this is optional, what is the default? Document.

thockin · 2018-06-04T20:35:22Z

pkg/apis/autoscaling/types.go

+
+	// Controls how the autoscaler computes recommended resources.
+	// +optional
+	ResourcePolicy PodResourcePolicy


Document default behavior.

thockin · 2018-06-04T20:37:04Z

pkg/apis/autoscaling/types.go

+type PodUpdatePolicy struct {
+	// Controls when autoscaler applies changes to the pod resoures.
+	// +optional
+	UpdateMode UpdateMode


Why is this optional, when the struct itself is optional above? Put another way, is there a plausible future where the user specifies VP.spec.updatePolicy and provides a struct that does not include mode?

Also, all truly optional fields should be nil-able, (pointers or slices or maps)

PodUpdatePolicy may in future consist of multiple optional settings. It might be possible to specify PodUpdatePolicy with some other settings but leaving the mode unset (i.e. use the default).

Reg. nil-able fields: I've changed all optional fields to pointers, although I excluded "enum" string fields, because they support a natural empty value (an empty string) and I think this approach is widely used across other APIs elsewhere. WDYT?

thockin · 2018-06-04T20:40:14Z

pkg/apis/autoscaling/types.go

+
+	// Specification of the behavior of the autoscaler.
+	// More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#spec-and-status.
+	// +optional


is spec really optional? What is the behavior if it is not provided? Document.

Removed optional.
Not sure why other APIs make it optional.

thockin · 2018-06-04T20:41:37Z

pkg/apis/autoscaling/types.go

+// named container and optionally a single wildcard entry with Name = "*", which
+// handles all containers that don't have individual policies.
+type PodResourcePolicy struct {
+	// Per-container resource policies.


default behavior if omitted?

Added a comment to ResourcePolicy PodResourcePolicy. In general this policy allows to provide constraints on how recommendations are computed for each container. If some container is not mentioned, there are no constraints on it.

thockin · 2018-06-04T20:46:13Z

pkg/apis/autoscaling/types.go

+
+var (
+	// RecommendationProvided indicates whether the VPA recommender was able to calculate a recommendation.
+	RecommendationProvided VerticalPodAutoscalerConditionType = "RecommendationProvided"


Shorten to "Recommended" ? Maybe..

Not sure about this one.. I would prefer it to be as self-descriptive as possible.

thockin · 2018-06-04T20:46:23Z

pkg/apis/autoscaling/types.go

+
+// VerticalPodAutoscalerCondition describes the state of
+// a VerticalPodAutoscaler at a certain point.
+type VerticalPodAutoscalerCondition struct {


thockin · 2018-06-04T20:49:31Z

pkg/apis/autoscaling/validation/vpa.go

+func validateVerticalPodAutoscalerSpec(spec *autoscaling.VerticalPodAutoscalerSpec, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if spec != nil {
+		if spec.Selector != nil {


usually I see this flipped. Required test then valid test. Minor.

thockin · 2018-06-04T20:51:37Z

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

+type VerticalPodAutoscalerSpec struct {
+	// A label query that determines the set of pods controlled by the Autoscaler.
+	// More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors
+	Selector *metav1.LabelSelector `json:"selector,omitempty" protobuf:"bytes,1,opt,name=selector"`


Is this optional or not? Doesn't say +optional, but is a pointer and proto says 'opt' ?

Fixed the annotation. Technically a nil selector is legal (matches nothing), however we disallowed it during validation. As far as I could see this approach was used elsewhere.

thockin · 2018-06-04T20:53:06Z

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

+type PodResourcePolicy struct {
+	// Per-container resource policies.
+	// +optional
+	ContainerPolicies []ContainerResourcePolicy `json:"containerPolicies,omitempty" protobuf:"bytes,1,rep,name=containerPolicies"`


I think we need patchStrategy and patchMergeKey on all lists.

@pwittrock

mwielgus · 2018-06-06T19:03:33Z

Adding approved-for-milestone as a sig-lead.

k8s-github-robot · 2018-06-06T19:06:38Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@kgrygiel @mwielgus @thockin

Pull Request Labels

sig/autoscaling: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/feature: New functionality.

Help

mwielgus · 2018-06-06T20:33:43Z

/retest

jberkus · 2018-06-06T21:55:16Z

/hold

Taking discussion to Slack, because I'm concerned about the state of this PR.

jberkus · 2018-06-06T21:59:57Z

If @DirectXMan12 's objections were resolved on slack or elsewhere, please let me know in a comment and remove the hold. Thanks!

Also, the release team needs to know: (a) what is the risk associated with this patch? (b) are there docs for this?

mwielgus · 2018-06-07T00:48:33Z

@jberkus @DirectXMan12 is on vacation, however his objections seem to be resolved. @DirectXMan12 was one of the reviewer of the original proposal for VPA kubernetes/community#338.

The risk associated with this PR is minimal. It is the api that is handled by an opt-in, independent component. Documentation for the feature is provided currently here:

https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler

We will expand the main K8S doc soon.

jberkus · 2018-06-07T04:12:33Z

/remove-hold

Removing hold per discussion with SIG.

jberkus · 2018-06-07T04:13:40Z

sigh.

/hold cancel

k8s-github-robot · 2018-06-07T04:35:33Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-07T05:54:08Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@jberkus

Automatic merge from submit-queue (batch tested with PRs 64836, 64890). 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>. Revert: Add Vertical Pod Autoscaler to autoscaling/v2beta1 Reverts #63797 per [discussion](https://kubernetes.slack.com/archives/C09R1LV8S/p1528400528000615) with @jberkus and @mwielgus The scope of the follow-ups required in #64286 was not well understood when the API PR was merged, and we are now past code freeze for 1.11 This can be reopened against 1.12 ```release-note NONE ```

@jberkus

Automatic merge from submit-queue (batch tested with PRs 64836, 64890). 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>. Revert: Add Vertical Pod Autoscaler to autoscaling/v2beta1 Reverts kubernetes/kubernetes#63797 per [discussion](https://kubernetes.slack.com/archives/C09R1LV8S/p1528400528000615) with @jberkus and @mwielgus The scope of the follow-ups required in kubernetes/kubernetes#64286 was not well understood when the API PR was merged, and we are now past code freeze for 1.11 This can be reopened against 1.12 ```release-note NONE ``` Kubernetes-commit: 169df7434155cbbc22f1532cba8e0a9588e29ad8

@jberkus

Automatic merge from submit-queue (batch tested with PRs 64836, 64890). 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>. Revert: Add Vertical Pod Autoscaler to autoscaling/v2beta1 Reverts kubernetes/kubernetes#63797 per [discussion](https://kubernetes.slack.com/archives/C09R1LV8S/p1528400528000615) with @jberkus and @mwielgus The scope of the follow-ups required in kubernetes/kubernetes#64286 was not well understood when the API PR was merged, and we are now past code freeze for 1.11 This can be reopened against 1.12 ```release-note NONE ``` Kubernetes-commit: 169df7434155cbbc22f1532cba8e0a9588e29ad8

See the API review: kubernetes/kubernetes#63797

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 14, 2018

k8s-ci-robot requested review from DirectXMan12, mwielgus and thockin May 14, 2018 13:27

k8s-ci-robot added 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. labels May 14, 2018

k8s-github-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label May 14, 2018

kgrygiel force-pushed the master branch 3 times, most recently from adf2996 to b396dd5 Compare May 15, 2018 22:12

kgrygiel force-pushed the master branch 2 times, most recently from 314992a to fc4a756 Compare May 16, 2018 14:59

mwielgus added the sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. label May 16, 2018

kgrygiel mentioned this pull request May 24, 2018

API Server & kubectl logic for Vertical Pod Autoscaler #64286

Closed

k8s-ci-robot assigned mwielgus May 24, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2018

DirectXMan12 suggested changes May 25, 2018

View reviewed changes

kgrygiel force-pushed the master branch from fc4a756 to f9617c5 Compare May 28, 2018 12:17

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2018

kgrygiel force-pushed the master branch from f9617c5 to 5e70c83 Compare May 28, 2018 15:03

thockin reviewed May 29, 2018

View reviewed changes

kgrygiel force-pushed the master branch 2 times, most recently from e94d35d to 39c25a1 Compare May 30, 2018 14:54

thockin reviewed Jun 4, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 6, 2018

mwielgus added status/approved-for-milestone priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. kind/feature Categorizes issue or PR as related to a new feature. labels Jun 6, 2018

k8s-github-robot removed the milestone/incomplete-labels label Jun 6, 2018

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 6, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 7, 2018

k8s-github-robot merged commit 63c90bb into kubernetes:master Jun 7, 2018

liggitt mentioned this pull request Jun 7, 2018

Revert: Add Vertical Pod Autoscaler to autoscaling/v2beta1 #64890

Merged

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 7, 2018

kgrygiel pushed a commit to kgrygiel/autoscaler that referenced this pull request Jun 14, 2018

Update the VPA API based on the API review (pull/63797).

2c44d6e

See the API review: kubernetes/kubernetes#63797

kgrygiel pushed a commit to kgrygiel/autoscaler that referenced this pull request Jun 14, 2018

Update the VPA API based on the API review (pull/63797).

d8e7823

See the API review: kubernetes/kubernetes#63797

This was referenced Jun 14, 2018

Changes to the VPA API based on the v2beta1 API review kubernetes/autoscaler#968

Merged

Add Vertical Pod Autoscaler to autoscaling/v2beta1 #65279

Closed

This was referenced Apr 22, 2020

vpa and vpacheckpoint CRDs have KubernetesAPIApprovalPolicyConformant condition from MissingAnnotation kubernetes/autoscaler#3069

Closed

Add api-approved label to the Vertical Pod Autoscaler API kubernetes/autoscaler#3080

Merged

Add Vertical Pod Autoscaler to autoscaling/v2beta1 #63797

Add Vertical Pod Autoscaler to autoscaling/v2beta1 #63797

Conversation

kgrygiel commented May 14, 2018 • edited by liggitt

thockin commented May 16, 2018

mwielgus commented May 16, 2018

mwielgus commented May 24, 2018

DirectXMan12 commented May 25, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented May 25, 2018

thockin 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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwielgus commented Jun 6, 2018

k8s-github-robot commented Jun 6, 2018

mwielgus commented Jun 6, 2018

jberkus commented Jun 6, 2018

jberkus commented Jun 6, 2018 • edited

mwielgus commented Jun 7, 2018

jberkus commented Jun 7, 2018

jberkus commented Jun 7, 2018

k8s-github-robot commented Jun 7, 2018

k8s-github-robot commented Jun 7, 2018

kgrygiel commented May 14, 2018 •

edited by liggitt

jberkus commented Jun 6, 2018 •

edited