Deprecate VPA #1835

rexagod · 2022-09-15T06:28:51Z

Deprecate VPA metrics.
Inform users about the deprecation (print a warning to stdout).
Document an alternative approach using the CRD capabilities we have now.

Signed-off-by: Pranshu Srivastava rexagod@gmail.com

How does this change affect the cardinality of KSM: No change (VPA metrics will be removed in v2.9.0).

Which issue(s) this PR fixes: Partially targets #1718.

fpetkovski

Thanks.

/lgtm
/hold

internal/store/builder.go

dgrisonnet · 2022-09-23T17:05:51Z

docs/verticalpodautoscaler-metrics.md

+kind: CustomResourceStateMetrics
+spec:
+  resources:
+    - groupVersionKind:
+        group: autoscaling.k8s.io
+        kind: "VerticalPodAutoscaler"
+        version: "v1"
+      metrics:
+        - name: "generation"
+          help: "VPA Generation"
+          each:
+            type: Gauge
+            gauge:
+              path: [metadata, generation]


Could we provide them with a manifest that generates exactly the same metrics as we had before? I know that we won't be able to get the exact same names so it would get great to also point the name changes.

dgrisonnet · 2022-09-23T17:06:54Z

docs/verticalpodautoscaler-metrics.md

-| kube_verticalpodautoscaler_spec_updatepolicy_updatemode                                     | Gauge       | `namespace`=&lt;namespace&gt; <br> `target_api_version`=&lt;api version&gt; <br> `target_kind`=&lt;target kind&gt; <br> `target_name`=&lt;target name&gt; <br> `update_mode`=&lt;foo&gt; <br> `verticalpodautoscaler`=&lt;vertical pod autoscaler name&gt; | EXPERIMENTAL                                                                                                                                                |
+## DEPRECATION NOTICE
+
+From v2.8.0 onwards, `vericalpodautoscalers` will be removed from the list of default resources. This means that specifying that in the `--resource` flag will **not** generate metrics for the same. In order to generate `verticalpodautoscalers` metrics, you will have to explicitly specify it in `--custom-resource-state-config*` (either the inline yaml, or the configuration file), like so:


Since v2.6.0 has already been released, we should only start removing VPA in v2.9.0 since the deprecation period should be of at least 2 releases.

IMHO, we should bump version to v3 if we remove VPA metrics.

Breaking changes used to be reserved for major releases when following semantic versioning, but what we've seen in recent years is that projects with only minor releases are healthy and can still handle breaking changes properly as long as they have clear deprecation policies, which is the case for Kubernetes for example. IMHO major releases should only be reserved for major rewrites, which don't include VPA metrics removal. If we are not confident that the deprecation period is enough, I am fine with increasing it, but I don't think waiting for a major release is the right call here.

What do you think @fpetkovski?

I can understand that major releases can become quite painful with go versioning. Personally, I prefer going with the least surprise for users. Users won't be happy when their VPA metrics are gone and they did not see it coming.

The problem is that there is no reason to cut a new major today besides removing the VPA metrics, which is not a big enough reason to justify a new major version in my opinion. Leaving a long enough deprecation period should already be good enough to not surprise the users. At the very least if we are not confident enough that our users are going through our changelogs, we can add a log line when the vpa store is used saying that it is deprecated and will be removed in version x.y.z.

Experimental features should not have stability guarantees.

Apologies here, I wasn't aware that VPA never made it stable. I'm fine with deprecating and removing without bumping major here. Thanks for explaining!

dgrisonnet · 2022-10-14T10:37:45Z

The unit tests are failing, you need to the update the test metadata to include the deprecation notice: https://github.com/kubernetes/kube-state-metrics/blob/master/internal/store/verticalpodautoscaler_test.go#L33-L40

rexagod · 2022-11-06T22:54:43Z

Moved to v3, ready for review.

rexagod · 2022-11-08T23:10:05Z

cc @dgrisonnet @mrueg @logicalhan

docs/verticalpodautoscaler-metrics.md

dgrisonnet · 2022-11-09T06:42:28Z

I am still very much against cutting a new major version of ksm just for deprecating VPA metrics. This is not an overhaul of the codebase, we are just deprecating some metrics and replacing them with new ones. See it as a renaming of metrics rather than a complete removal.

Also, it is worth noting that all the VPA metrics are experimental so although they have been in the codebase for a while, users shouldn't expect them to never change.

Holding until we've reached a consensus.
/hold

logicalhan · 2022-11-09T17:13:35Z

I am still very much against cutting a new major version of ksm just for deprecating VPA metrics. This is not an overhaul of the codebase, we are just deprecating some metrics and replacing them with new ones. See it as a renaming of metrics rather than a complete removal.

Also, it is worth noting that all the VPA metrics are experimental so although they have been in the codebase for a while, users shouldn't expect them to never change.

Holding until we've reached a consensus. /hold

Yeah, cutting a release for deleting experimental metrics seems a bit much to me too.

rexagod · 2022-11-09T20:28:42Z

Reverted to v2.9.0 (#1835 (comment)).

docs/verticalpodautoscaler-metrics.md

Deprecate VPA metrics in v2.9.0. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

mrueg · 2022-11-10T16:14:15Z

/approve

rexagod · 2022-11-11T08:39:09Z

@mrueg Can you please /lgtm (if all looks good here)?

mrueg · 2022-11-14T18:43:56Z

/lgtm
/hold cancel
Proceeding here, thanks for your contribution @rexagod!

k8s-ci-robot · 2022-11-14T18:44:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fpetkovski, mrueg, rexagod

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:

~~OWNERS~~ [fpetkovski,mrueg]

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

hajdukda · 2023-08-09T20:10:53Z

So what are the alternatives now to get those metrics ? Sry starting with vpa just to have free recommendations instead of paying 15k to some random saas.

dgrisonnet · 2023-08-10T08:20:39Z

There is a doc explaining how to get them back in https://github.com/kubernetes/kube-state-metrics/blob/main/docs/customresourcestate-metrics.md#verticalpodautoscaler

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 15, 2022

k8s-ci-robot requested review from dgrisonnet and fpetkovski September 15, 2022 06:29

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 15, 2022

rexagod marked this pull request as draft September 15, 2022 06:37

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 15, 2022

rexagod marked this pull request as ready for review September 18, 2022 18:08

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 18, 2022

rexagod force-pushed the 1790 branch from 28094f7 to 7a84a5e Compare September 18, 2022 18:22

fpetkovski approved these changes Sep 23, 2022

View reviewed changes

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

k8s-ci-robot assigned fpetkovski Sep 23, 2022

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 23, 2022

dgrisonnet reviewed Sep 23, 2022

View reviewed changes

rexagod force-pushed the 1790 branch from 7a84a5e to f8ac205 Compare September 26, 2022 10:39

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 26, 2022

rexagod force-pushed the 1790 branch from f8ac205 to 50120a8 Compare September 26, 2022 10:43

rexagod force-pushed the 1790 branch 3 times, most recently from 3583628 to 40ca779 Compare November 6, 2022 20:56

logicalhan reviewed Nov 8, 2022

View reviewed changes

docs/verticalpodautoscaler-metrics.md Outdated Show resolved Hide resolved

rexagod force-pushed the 1790 branch from 40ca779 to 3d9432c Compare November 9, 2022 00:11

rexagod force-pushed the 1790 branch from 3d9432c to a6bcaf4 Compare November 9, 2022 20:27

rexagod force-pushed the 1790 branch from a6bcaf4 to e672324 Compare November 9, 2022 20:29

mrueg reviewed Nov 9, 2022

View reviewed changes

docs/verticalpodautoscaler-metrics.md Outdated Show resolved Hide resolved

Deprecate VPA

2279fb2

Deprecate VPA metrics in v2.9.0. Signed-off-by: Pranshu Srivastava <rexagod@gmail.com>

rexagod force-pushed the 1790 branch from e672324 to 2279fb2 Compare November 9, 2022 20:44

rexagod requested a review from mrueg November 10, 2022 16:12

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

k8s-ci-robot assigned mrueg Nov 14, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 14, 2022

k8s-ci-robot merged commit 9edd02b into kubernetes:master Nov 14, 2022

mrueg mentioned this pull request Nov 27, 2022

Support verticalpodautoscaler v1 #1718

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate VPA #1835

Deprecate VPA #1835

rexagod commented Sep 15, 2022 •

edited

Loading

fpetkovski left a comment

dgrisonnet Sep 23, 2022

dgrisonnet Sep 23, 2022

mrueg Oct 5, 2022 •

edited

Loading

dgrisonnet Oct 14, 2022

mrueg Oct 14, 2022

dgrisonnet Nov 9, 2022 •

edited

Loading

logicalhan Nov 9, 2022

mrueg Nov 9, 2022

dgrisonnet commented Oct 14, 2022

rexagod commented Nov 6, 2022

rexagod commented Nov 8, 2022

dgrisonnet commented Nov 9, 2022

logicalhan commented Nov 9, 2022

rexagod commented Nov 9, 2022 •

edited

Loading

mrueg commented Nov 10, 2022

rexagod commented Nov 11, 2022

mrueg commented Nov 14, 2022

k8s-ci-robot commented Nov 14, 2022

hajdukda commented Aug 9, 2023 •

edited

Loading

dgrisonnet commented Aug 10, 2023

Deprecate VPA #1835

Deprecate VPA #1835

Conversation

rexagod commented Sep 15, 2022 • edited Loading

fpetkovski left a comment

Choose a reason for hiding this comment

dgrisonnet Sep 23, 2022

Choose a reason for hiding this comment

dgrisonnet Sep 23, 2022

Choose a reason for hiding this comment

mrueg Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

dgrisonnet Oct 14, 2022

Choose a reason for hiding this comment

mrueg Oct 14, 2022

Choose a reason for hiding this comment

dgrisonnet Nov 9, 2022 • edited Loading

Choose a reason for hiding this comment

logicalhan Nov 9, 2022

Choose a reason for hiding this comment

mrueg Nov 9, 2022

Choose a reason for hiding this comment

dgrisonnet commented Oct 14, 2022

rexagod commented Nov 6, 2022

rexagod commented Nov 8, 2022

dgrisonnet commented Nov 9, 2022

logicalhan commented Nov 9, 2022

rexagod commented Nov 9, 2022 • edited Loading

mrueg commented Nov 10, 2022

rexagod commented Nov 11, 2022

mrueg commented Nov 14, 2022

k8s-ci-robot commented Nov 14, 2022

hajdukda commented Aug 9, 2023 • edited Loading

dgrisonnet commented Aug 10, 2023

rexagod commented Sep 15, 2022 •

edited

Loading

mrueg Oct 5, 2022 •

edited

Loading

dgrisonnet Nov 9, 2022 •

edited

Loading

rexagod commented Nov 9, 2022 •

edited

Loading

hajdukda commented Aug 9, 2023 •

edited

Loading