Add retention policy metrics for KEP-1847 #1876

mattcary · 2022-11-02T22:23:45Z

What this PR does / why we need it:

Add metrics for new StatefulSet fields from KEP-1847.

How does this change affect the cardinality of KSM: (increases, decreases or does not change cardinality)
Increases it by adding a new metric that has a cardinality of 5 x # stateful sets.

mattcary · 2022-11-07T17:58:07Z

/retest

mattcary · 2022-11-07T18:03:51Z

/retest

mattcary · 2022-11-07T18:12:48Z

/retest

mattcary · 2022-11-07T18:41:02Z

Hi, this is failing the e2e tests because the field in question is behind a feature gate.

I have a conversation going on kubernetes/kubernetes#111299 which led to this PR, but haven't gotten any response.

Does anyone here have any hints on how to get around the feature gate problem? Thanks!

mrueg · 2022-11-08T13:33:42Z

internal/store/statefulset_test.go

@@ -63,6 +63,7 @@ func TestStatefulSetStore(t *testing.T) {
 				# HELP kube_statefulset_created [STABLE] Unix creation timestamp
 				# HELP kube_statefulset_labels [STABLE] Kubernetes labels converted to Prometheus labels.
 				# HELP kube_statefulset_metadata_generation [STABLE] Sequence number representing a specific generation of the desired state for the StatefulSet.
+                # HELP kube_statefulset_persistent_volume_claim_retention_policy Count of retention policy for StatefulSet template PVCs


indentation here seems to be off

thx, tabs vs spaces is the gift that keeps on giving...

internal/store/statefulset_test.go

mrueg · 2022-11-08T13:35:23Z

internal/store/statefulset_test.go

 		if err := c.run(); err != nil {
-			t.Errorf("unexpected collecting result in %vth run:\n%s", i, err)
+			t.Errorf("unexpected collecting result in %v%s run:\n%s", i+1, ord, err)


Suggested change

t.Errorf("unexpected collecting result in %v%s run:\n%s", i+1, ord, err)

t.Errorf("unexpected collecting result in run %v:\n%s" i, err)

Let's reduce complexity here and rephrase the error message instead

Done, note that we need to use i+1 to have the index match the names used in the test cases (which is the practical way to figure out which test failed).

mattcary · 2022-11-08T17:47:48Z

@mrueg thanks for the comments, do you have any advice on the e2e test failure? I'm stuck with what to do.

logicalhan · 2022-11-08T17:50:06Z

@mrueg thanks for the comments, do you have any advice on the e2e test failure? I'm stuck with what to do.

This seems to the be error:

Error from server (BadRequest): error when creating "tests/manifests/statefulset.yaml": StatefulSet in version "v1" cannot be handled as a StatefulSet: strict decoding error: unknown field "spec.template.spec.persistentVolumeClaimRetentionPolicy"

Is this a new field?

mattcary · 2022-11-08T17:55:56Z

@mrueg thanks for the comments, do you have any advice on the e2e test failure? I'm stuck with what to do.

This seems to the be error:
Error from server (BadRequest): error when creating "tests/manifests/statefulset.yaml": StatefulSet in version "v1" cannot be handled as a StatefulSet: strict decoding error: unknown field "spec.template.spec.persistentVolumeClaimRetentionPolicy"
Is this a new field?

Yes, exactly.

I guess I can just remove the new field from the testing manifest, and skip e2e testing on this feature?

mrueg · 2022-11-08T18:01:50Z

examples/standard/deployment.yaml

@@ -21,7 +21,7 @@ spec:
    spec:
      automountServiceAccountToken: true
      containers:
-      - image: registry.k8s.io/kube-state-metrics/kube-state-metrics:v2.6.0
+      - image: registry.k8s.io/kube-state-metrics/kube-state-metrics-amd64:v2.6.0


This got probably added by accident

yeah everytime I run the e2e tests by hand this gets change :-/

internal/store/statefulset_test.go

logicalhan

/lgtm

dgrisonnet · 2022-11-08T18:23:41Z

internal/store/statefulset.go

@@ -193,6 +193,30 @@ func statefulSetMetricFamilies(allowAnnotationsList, allowLabelsList []string) [
 				}
 			}),
 		),
+		*generator.NewFamilyGeneratorWithStability(
+			"kube_statefulset_persistent_volume_claim_retention_policy",


Suggested change

"kube_statefulset_persistent_volume_claim_retention_policy",

"kube_statefulset_persistentvolumeclaim_retention_policy",

Since we are talking about the PersistentVolumeClaim kubernetes resource, it should be put in one word.

dgrisonnet · 2022-11-08T18:26:54Z

internal/store/statefulset_test.go

@@ -95,6 +97,7 @@ func TestStatefulSetStore(t *testing.T) {
 				kube_statefulset_status_observed_generation{namespace="ns1",statefulset="statefulset1"} 1
 				kube_statefulset_replicas{namespace="ns1",statefulset="statefulset1"} 3
 				kube_statefulset_metadata_generation{namespace="ns1",statefulset="statefulset1"} 3
+ 				kube_statefulset_persistent_volume_claim_retention_policy{namespace="ns1",statefulset="statefulset1",when_deleted="nil",when_scaled="nil"} 1


If the retention policy is nil it would be better to not expose the metric since it doesn't bring much value and the less timeseries we have to store, the better.

mrueg · 2022-11-08T19:17:56Z

docs/statefulset-metrics.md

@@ -11,6 +11,7 @@
 | kube_statefulset_status_observed_generation | Gauge | `statefulset`=&lt;statefulset-name&gt; <br> `namespace`=&lt;statefulset-namespace&gt;  | STABLE |
 | kube_statefulset_replicas | Gauge | `statefulset`=&lt;statefulset-name&gt; <br> `namespace`=&lt;statefulset-namespace&gt;  | STABLE |
 | kube_statefulset_metadata_generation | Gauge | `statefulset`=&lt;statefulset-name&gt; <br> `namespace`=&lt;statefulset-namespace&gt;  | STABLE |
+| kube_statefulset_persistent_volume_claim_retention_policy | Gauge | `statefulset`=&lt;statefulset-name&gt; <br> `namespace`=&lt;statefulset-namespace&gt; <br> `when_deleted`=&lt;statefulset-when-deleted-pvc-policy&gt; <br> `when_scaled`=&lt;statefulset-when-scaled-pvc-policy&gt; | ALPHA |


Suggested change

| kube_statefulset_persistent_volume_claim_retention_policy | Gauge | `statefulset`=<statefulset-name> `namespace`=<statefulset-namespace> `when_deleted`=<statefulset-when-deleted-pvc-policy> `when_scaled`=<statefulset-when-scaled-pvc-policy> | ALPHA |

| kube_statefulset_persistentvolumeclaim_retention_policy | Gauge | `statefulset`=<statefulset-name> `namespace`=<statefulset-namespace> `when_deleted`=<statefulset-when-deleted-pvc-policy> `when_scaled`=<statefulset-when-scaled-pvc-policy> | ALPHA |

Yup, just saw the presubmit fail on that :-) thx

logicalhan

/lgtm
/approve

mattcary · 2022-11-08T20:13:26Z

@mrueg Any other comments? Sorry to be a pest but this is blocking a k8s change that wants to make the code freeze. Thank you for your quick review!

mrueg · 2022-11-08T20:56:31Z

Thanks for the contribution!

/lgtm

k8s-ci-robot · 2022-11-08T20:56:38Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: logicalhan, mattcary, mrueg

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~~ [mrueg]

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

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 2, 2022

k8s-ci-robot requested review from dgrisonnet and fpetkovski November 2, 2022 22:23

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 2, 2022

mattcary changed the title ~~Add retention policy metrics for KEP-1847~~ [WIP] Add retention policy metrics for KEP-1847 Nov 2, 2022

mattcary mentioned this pull request Nov 2, 2022

Add StatefulSet controller metrics kubernetes/kubernetes#111299

Closed

mattcary force-pushed the kep1847 branch from 65867c5 to 6f124d5 Compare November 7, 2022 17:57

mattcary force-pushed the kep1847 branch from 6f124d5 to cb88723 Compare November 7, 2022 18:03

mattcary force-pushed the kep1847 branch 2 times, most recently from c71fd2a to 64a468c Compare November 7, 2022 18:11

mattcary marked this pull request as ready for review November 7, 2022 18:39

mattcary changed the title ~~[WIP] Add retention policy metrics for KEP-1847~~ Add retention policy metrics for KEP-1847 Nov 7, 2022

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

k8s-ci-robot requested a review from logicalhan November 7, 2022 18:39

janetkuo mentioned this pull request Nov 7, 2022

Move StatefulSetAutoDeletePVC feature gate to beta kubernetes/kubernetes#111300

Closed

mrueg reviewed Nov 8, 2022

View reviewed changes

internal/store/statefulset_test.go Outdated Show resolved Hide resolved

mrueg reviewed Nov 8, 2022

View reviewed changes

internal/store/statefulset_test.go Outdated Show resolved Hide resolved

mrueg reviewed Nov 8, 2022

View reviewed changes

mattcary force-pushed the kep1847 branch from 64a468c to 040191c Compare November 8, 2022 17:53

mattcary force-pushed the kep1847 branch from 040191c to fd8350b Compare November 8, 2022 17:57

mrueg reviewed Nov 8, 2022

View reviewed changes

mattcary force-pushed the kep1847 branch 3 times, most recently from 5f5af02 to 0a8b56d Compare November 8, 2022 18:08

logicalhan reviewed Nov 8, 2022

View reviewed changes

k8s-ci-robot assigned logicalhan Nov 8, 2022

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

dgrisonnet reviewed Nov 8, 2022

View reviewed changes

mattcary force-pushed the kep1847 branch from 0a8b56d to 8a97a07 Compare November 8, 2022 19:02

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

mrueg reviewed Nov 8, 2022

View reviewed changes

Add retention policy metrics for KEP-1847

b6c5667

mattcary force-pushed the kep1847 branch from 8a97a07 to b6c5667 Compare November 8, 2022 19:42

logicalhan approved these changes Nov 8, 2022

View reviewed changes

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

k8s-ci-robot assigned mrueg Nov 8, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 8, 2022

k8s-ci-robot merged commit dc1f1f3 into kubernetes:master Nov 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add retention policy metrics for KEP-1847 #1876

Add retention policy metrics for KEP-1847 #1876

mattcary commented Nov 2, 2022

mattcary commented Nov 7, 2022

mattcary commented Nov 7, 2022

mattcary commented Nov 7, 2022

mattcary commented Nov 7, 2022

mrueg Nov 8, 2022

mattcary Nov 8, 2022

mrueg Nov 8, 2022

mattcary Nov 8, 2022

mattcary commented Nov 8, 2022

logicalhan commented Nov 8, 2022

mattcary commented Nov 8, 2022

mrueg Nov 8, 2022

mattcary Nov 8, 2022

logicalhan left a comment

dgrisonnet Nov 8, 2022

mattcary Nov 8, 2022

dgrisonnet Nov 8, 2022

mattcary Nov 8, 2022

mrueg Nov 8, 2022

mattcary Nov 8, 2022

logicalhan left a comment

mattcary commented Nov 8, 2022

mrueg commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022

	t.Errorf("unexpected collecting result in %v%s run:\n%s", i+1, ord, err)
	t.Errorf("unexpected collecting result in run %v:\n%s" i, err)

	"kube_statefulset_persistent_volume_claim_retention_policy",
	"kube_statefulset_persistentvolumeclaim_retention_policy",

	\| kube_statefulset_persistent_volume_claim_retention_policy \| Gauge \| `statefulset`=<statefulset-name> <br> `namespace`=<statefulset-namespace> <br> `when_deleted`=<statefulset-when-deleted-pvc-policy> <br> `when_scaled`=<statefulset-when-scaled-pvc-policy> \| ALPHA \|
	\| kube_statefulset_persistentvolumeclaim_retention_policy \| Gauge \| `statefulset`=<statefulset-name> <br> `namespace`=<statefulset-namespace> <br> `when_deleted`=<statefulset-when-deleted-pvc-policy> <br> `when_scaled`=<statefulset-when-scaled-pvc-policy> \| ALPHA \|

Add retention policy metrics for KEP-1847 #1876

Add retention policy metrics for KEP-1847 #1876

Conversation

mattcary commented Nov 2, 2022

mattcary commented Nov 7, 2022

mattcary commented Nov 7, 2022

mattcary commented Nov 7, 2022

mattcary commented Nov 7, 2022

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

mattcary commented Nov 8, 2022

logicalhan commented Nov 8, 2022

mattcary commented Nov 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logicalhan 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

logicalhan left a comment

Choose a reason for hiding this comment

mattcary commented Nov 8, 2022

mrueg commented Nov 8, 2022

k8s-ci-robot commented Nov 8, 2022