VPA: Add UpdateModeInPlaceOrRecreate to types

[maxcao13]

This just addes the UpdateModeInPlaceOrRecreate mode to the types so we can use it.

What type of PR is this?

/kind api-change

What this PR does / why we need it:

We need the new API to allow users to specify the new InPlaceOrRecreate mode as part of AEP-4016.

Which issue(s) this PR fixes:

This PR is part of the larger feature PR in #7673

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

[AEP-4016](https://github.com/kubernetes/autoscaler/tree/master/vertical-pod-autoscaler/enhancements/4016-in-place-updates-support)

[adrianmoisey]

/lgtm

[omerap12]

If we modify the CRD, shouldn’t we also update its version?

[maxcao13]

If we modify the CRD, shouldn’t we also update its version?

I'm not sure but it's probably a good idea since this is a big enhancement? I'd defer to an approver for this one.

[adrianmoisey]

If we modify the CRD, shouldn’t we also update its version?

I'm not sure but it's probably a good idea since this is a big enhancement? I'd defer to an approver for this one.

I'm not 100% sure, we need to get confirmation here, but since this is a backwards compatible change to the API, we don't need to bump it.

[omerap12]

If we modify the CRD, shouldn’t we also update its version?

I'm not sure but it's probably a good idea since this is a big enhancement? I'd defer to an approver for this one.

I'm not 100% sure, we need to get confirmation here, but since this is a backwards compatible change to the API, we don't need to bump it.

cc @voelzmo @raywainman

[adrianmoisey]

There seems to be some guidelines here:
https://github.com/kubernetes/community/blob/19094aa6e60eb4a481650c4cbdb94badd9919b5b/contributors/devel/sig-architecture/api_changes.md#on-compatibility

[raywainman]

Nice find on the guidelines @adrianmoisey. My understanding is that this change would fall under these new enum value guidelines.

From my interpretation, we are following the required steps here by guarding usage via a feature flag.

My understanding here is that we do not need a new API version to introduce this change.

Would love at least one more person to confirm though :)

cc @kwiesmueller

[maxcao13]

I'll link that part you mentioned for convenience: https://github.com/kubernetes/community/blob/19094aa6e60eb4a481650c4cbdb94badd9919b5b/contributors/devel/sig-architecture/api_changes.md#new-enum-value-in-existing-field

Going through the steps to ensure compatibility:

1. Add a feature gate to the API server to control enablement of the new enum value (and associated function):

Yes, we've done that in the other PR.

2. Update the documentation on the API type:

I can update the description a bit better to match the example:

type Frobber struct {
  // restartPolicy may be set to "Always" or "Never" (or "OnTuesday" if the alpha "FrobberRestartPolicyOnTuesday" feature is enabled).
  // Additional policies may be defined in the future.
  // Unrecognized policies should be treated as "Never".
  RestartPolicy string `json:"policy"
}

Never mind, it seems already seems to fine to me I think, unless people has other thoughts.

3. When validating the object, determine whether the new enum value should be allowed. This prevents new usage of the new value when the feature is disabled, while ensuring existing data is preserved. Ensuring existing data is preserved is needed so that when the feature is enabled by default in a future version n and data is unconditionally allowed to be persisted in the field, an n-1 API server (with the feature still disabled by default) will not choke on validation. The recommended place to do this is in the REST storage strategy's Validate/ValidateUpdate methods:

We don't use the REST storage strategy in VPA (to my knowledge), but we validate VPA during creation using the VPA webhook. I've already taken care of this in the big PR #7673, so that will come in later. (Prevents creation of the new field, but doesn't error if already persisted or updated: https://github.com/kubernetes/autoscaler/pull/7673/files#diff-ad66c76a76541b7991631925641b989fc402901440d8e0dcbc3591009eef52b9R126. I just need to add an extra isCreate to that if statement, and hopefully I don't forget :p)

Question is if and how we can test different VPA versions, though. Maybe a followup PR is needed?

4. In validation, validate the enum value based on the passed-in options:

Same as above, done in the big PR.

5. After at least one release, the feature can be promoted to beta or GA and enabled by default.

We'll take care of that later, and it's already documented in the AEP thanks to Adrian.

So I think we are good here not to update the CRD (coming from the author, so another confirmation would be awesome as Ray said), but please let me know if I missed anything.

[kwiesmueller]

Sounds good to me.
I wonder if we should make this clear on the field somehow (not sure if kubebuilder supports annotating a single enum option.
But at least in the doc we should say that it might not be supported yet or is feature guarded.

The guidelines are for kube-api, but we can translate them.
We should probably test that an older version of VPA doesn't panic/fail if it sees the new enum value. If it did we should fix that before we check-in this change (and even then it's a bit tricky).

Otherwise in the actual implementation as you discussed above we should comply with the validation guide. So a resource that has the field set, despite the feature being disabled, should be allowed to keep it iirc. while only new uses should be blocked if the feature is off.

[raywainman]

We should probably test that an older version of VPA doesn't panic/fail if it sees the new enum value. If it did we should fix that before we check-in this change (and even then it's a bit tricky).

That was my one worry as well, I quickly scanned the code and I believe we would simply treat this the same as Auto but it is worth validating at some point (I think we can totally do this during the "alpha" phase of this).

[raywainman]

/lgtm

/approval

[raywainman]

/hold

In case anyone else wants to take a look

[raywainman]

@maxcao13 maybe we let this one soak for another day or so and then remove the hold?

[maxcao13]

Sure, I'm good with that. Thanks!

[raywainman]

/unhold - this has soaked for long enough

[raywainman]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maxcao13, raywainman

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• vertical-pod-autoscaler/OWNERS [raywainman]

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

[raywainman]

/unhold