do we need this if we have validation instead?

Right. Can be removed.

Done.

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

Ok.

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?

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" ?

Done.

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

Done.

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.

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?

ACK

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
}

Done.

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

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

Done.

Document default behavior.

Done.

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?

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

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

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.

Shorten to "Recommended" ? Maybe..

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

ACK

ACK

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

Done.

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.

I think we need patchStrategy and patchMergeKey on all lists.

@pwittrock

Done.