VPA: Remove v1beta2

[adrianmoisey]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Remove v1beta2 API

Which issue(s) this PR fixes:

Fixes #5922

Special notes for your reviewer:

I'm unsure if there is anything else that needs to happen to remove this API

Does this PR introduce a user-facing change?

Remove deprecated v1beta2 APIs

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

[voelzmo]

Hey @adrianmoisey thanks for doing this PR! I love changes which consist basically only of removing lines!

I have two asks before this is ready to be merged from my point of view:

• I think we should add a comment to the README similar to what we did for the migration from v1beta1 to v1beta2. Contrary to the previous api migration, there aren't any changes necessary for users (v1 didn't make any incompatible changes), except for changing v1beta2 to v1 in their VPA objects.
  • People can find out if they need to to anything by looking at the recommender metrics, e.g. sum(vpa_recommender_vpa_objects_count) by (api)
• There seems to be an unused redis.yaml and redis_deprecated.yaml in the examples folder, and they still use v1beta2. I think we should also remove those two. Or are those used in a weird test setup somewhere?

[adrianmoisey]

• I think we should add a comment to the README similar to what we did for the migration from v1beta1 to v1beta2. Contrary to the previous api migration, there aren't any changes necessary for users (v1 didn't make any incompatible changes), except for changing v1beta2 to v1 in their VPA objects.

  • People can find out if they need to to anything by looking at the recommender metrics, e.g. sum(vpa_recommender_vpa_objects_count) by (api)

Yup, good call out here. Done in a193b1a

• There seems to be an unused redis.yaml and redis_deprecated.yaml in the examples folder, and they still use v1beta2. I think we should also remove those two. Or are those used in a weird test setup somewhere?

This is a little bit of a a strange one. These aren't for v1beta2, they're for v1beta1.
From what I can tell, they are referenced in the MIGRATE.md file.

I was considering deleting the YAML files and the README, but I imagine that someone on an old (ancient?) version may need those instructions to migrate. Additionally, the MIGRATE.md file is quite out of date (ie: it doesn't mention v1)

I'm unsure if we should leave them as is, and remove them all in the future. Or just delete them all now, as they are in git history.

Thoughts?

[voelzmo]

This is a little bit of a a strange one. These aren't for v1beta2, they're for v1beta1.
From what I can tell, they are referenced in the MIGRATE.md file.

I didn't find references to the redis.yaml file there, but only references to the hamster_deprecated.yaml – I think we can safely remove it. I'm also fine to leave them in there, don't want to block this PR on that decision.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianmoisey, voelzmo

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:

• vertical-pod-autoscaler/OWNERS [voelzmo]

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

[liggitt]

what ensures all existing persisted data in v1beta2 format is migrated to the new storage format? is there a reason to remove this rather than switching it to served:false by default?

[adrianmoisey]

I'm not too sure really. I had a look at what was done with v1beta1, and since it wasn't listed here, I assumed it just needed to be deleted.
Is there a best practice that we can follow to remove APIs?

[voelzmo]

Hey @liggitt, thanks for jumping in here!

what ensures all existing persisted data in v1beta2 format is migrated to the new storage format

Just to elaborate on our thought process:
v1beta2 has storage: false since vpa release 0.10.0, which happened 3 years ago: 99a4a7a

Technically, I guess you're right that it could happen that people still have VPAs from 3 years ago in their clusters. In the previous occasion which @adrianmoisey mentioned, the onus was put on the user to do this: https://github.com/kubernetes/autoscaler/blob/master/vertical-pod-autoscaler/MIGRATE.md#notice-on-switching-to-v1beta2-version-03x-to-040

We can also just set served: false and continue with this path, as you suggested.

EDIT: fixed the dates, sorry.

[liggitt]

v1beta2 has storage: false since vpa release 0.10.0, which happened 3 years ago: 99a4a7a

unless something always automatically writes to all existing VPA objects, just the passage of time doesn't guarantee existing objects have been touched / updated

I would start by just setting served: false, since that ensures you won't break existing persisted data.

I also don't really know how user-facing this API is... consumers of this API who have users still writing manifests against v1beta2 would consider dropping serving of it a breaking change, and might not be able to consume updates any more... starting with served: false gives them a trivial way to switch back to served: true and start more active outreach / migration of their users