Adding metrics for Maxunavailable feature in StatefulSet by Edwinhr716 · Pull Request #130951 · kubernetes/kubernetes

Edwinhr716 · 2025-03-20T20:25:56Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

Adds a metric to track how many times there has been a maxunavailable violation, requirement for kubernetes/enhancements#961 beta graduation.

Which issue(s) this PR fixes:

Part of kubernetes/enhancements#961

Special notes for your reviewer:

This is a follow up to the discussion on the KEP update PR kubernetes/enhancements#4474 (comment).

General consensus seems to be that this metric should be in tree instead of in kube-state-metrics.

Open question:

Should the metric be generic like the one exposed by deployment?

cc @atiratree @dgrisonnet @wojtek-t who were part of the original discussion.

Does this PR introduce a user-facing change?

Adds metric for Maxunavailable feature

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

Edwinhr716 · 2025-03-20T20:27:21Z

/assign @janetkuo @soltysh

janetkuo · 2025-03-20T22:45:40Z

/triage accepted

Edwinhr716 · 2025-03-21T00:51:40Z

/retest

k8s-triage-robot · 2025-03-21T01:15:08Z

This PR may require stable metrics review.

Stable metrics are guaranteed to not change. Please review the documentation for the requirements and lifecycle of stable metrics and ensure that your metrics meet these guidelines.

dims · 2025-03-24T12:57:01Z

cc @xiaohongchen1991

janetkuo · 2025-03-26T23:09:49Z

LGTM in general after the presubmit check failure is fixed. @soltysh would you like to take a look as well?

soltysh · 2025-09-12T12:31:25Z

+			return err
+		}
+	}
+	metrics.MaxUnavailable.WithLabelValues(set.Namespace, set.Name, podManagementPolicy).Set(float64(maxUnavailable))


Given this functionality is tied to MaxUnavailableStatefulSet feature gate, I believe we should wrap this entire block in if utilfeature.DefaultFeatureGate.Enabled(features.MaxUnavailableStatefulSet) { block.

soltysh · 2025-09-12T12:36:01Z

@@ -1013,7 +1015,7 @@ func TestStatefulSetControlRollingUpdateWithMaxUnavailable(t *testing.T) {
 		// if pod 4 ready, start to update pod 3, even though 5 is not ready
 		spc.setPodRunning(set, 4)
 		spc.setPodRunning(set, 5)


Since you're fixing it, drop this line. You're setting pod 5 running further below, so this line is confusing.

soltysh · 2025-09-12T12:37:23Z

-		}
-		if len(pods) != totalPods {
-			t.Fatalf("Expected create pods 2/3, got pods %v", pods)
+		// In OrderedReady mode, only 5 pods exist at this point (pod 5 not created yet)


Suggested change

// In OrderedReady mode, only 5 pods exist at this point (pod 5 not created yet)

// In OrderedReady mode, only 4 pods exist at this point (pod 5 not created yet)

soltysh · 2025-09-12T12:40:04Z

 		spc.setPodRunning(set, 5)
-		spc.setPodReady(set, 5)
-		originalPods, _ = spc.setPodReady(set, 3)
+		originalPods, _ = spc.setPodReadyCondition(set, 3, true)


Something is off here, the comment claims pods 3,4,5 are ready, but we're only setting 3 & 5 running, and only 3 ready, why?

the comment is correct. The test sets the pod 4 in running state in line 1058.

I see what you're saying, we're setting the pod running in both simpleOrderedVerificationFn and simpleParallelVerificationFn, which on one hand is somewhat misguiding. It seems like this particular test case deserves a re-write, but that's pre-existing, so definitely not part of this PR.

added metric to list of stable metrics changed when metric gets incremented addressed comments fixed stable metrics list

Co-authored-by: Filip Křepinský <[email protected]>

Signed-off-by: Heba Elayoty <[email protected]>

k8s-ci-robot · 2025-09-17T00:12:08Z

@Edwinhr716: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-linter-hints	`b9aa7c2`	link	false	`/test pull-kubernetes-linter-hints`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

soltysh

/lgtm
/approve

soltysh · 2025-09-17T11:32:34Z

 		spc.setPodRunning(set, 5)
-		spc.setPodReady(set, 5)
-		originalPods, _ = spc.setPodReady(set, 3)
+		originalPods, _ = spc.setPodReadyCondition(set, 3, true)


I see what you're saying, we're setting the pod running in both simpleOrderedVerificationFn and simpleParallelVerificationFn, which on one hand is somewhat misguiding. It seems like this particular test case deserves a re-write, but that's pre-existing, so definitely not part of this PR.

soltysh · 2025-09-17T11:35:17Z

+		expectedUnavailableReplicasValue int
+	}
+
+	testFn := func(test *testcase, t *testing.T) {


This pattern, where we define check function beforehand, even though it could easily be part of the invocation down below is confusing. But that's not blocking this particular PR.

I don't prefer it either, but wanted to follow the same pattern in other tests in this file.

k8s-ci-robot · 2025-09-17T11:42:13Z

LGTM label has been added.

Details

Git tree hash: 9df13ab1ec357cf3f7b86497d127faaf6f4787bd

soltysh · 2025-09-17T11:42:16Z

/label tide/merge-method-squash

k8s-ci-robot · 2025-09-17T11:42:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Edwinhr716, hashim21223445, janetkuo, soltysh

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:

~~pkg/controller/statefulset/OWNERS~~ [janetkuo,soltysh]

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 requested review from krmayankk and mimowo March 20, 2025 20:26

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 20, 2025

github-project-automation Bot added this to SIG Apps Mar 20, 2025

github-project-automation Bot moved this to Needs Triage in SIG Apps Mar 20, 2025

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 20, 2025

k8s-ci-robot assigned janetkuo and soltysh Mar 20, 2025

k8s-ci-robot added area/stable-metrics Issues or PRs involving stable metrics sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. labels Mar 20, 2025

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Mar 20, 2025

janetkuo reviewed Mar 20, 2025

View reviewed changes

Comment thread pkg/controller/statefulset/stateful_set_control.go Outdated

janetkuo reviewed Mar 24, 2025

View reviewed changes

Comment thread pkg/controller/statefulset/stateful_set_control.go Outdated

Comment thread pkg/controller/statefulset/metrics/metrics.go Outdated

janetkuo reviewed Mar 25, 2025

View reviewed changes

Comment thread test/instrumentation/testdata/stable-metrics-list.yaml Outdated

soltysh reviewed Sep 12, 2025

View reviewed changes

helayoty force-pushed the maxunavailable_metrics branch from c2f7485 to 7630021 Compare September 16, 2025 23:12

Edwinhr716 and others added 14 commits September 16, 2025 16:50

adding maxunavailable_violation metric

fe93a21

added metric to list of stable metrics changed when metric gets incremented addressed comments fixed stable metrics list

Update pkg/controller/statefulset/metrics/metrics.go

8c92c6c

Co-authored-by: Filip Křepinský <[email protected]>

Update the metric and log verbosity level

62b8da5

Address false positives metric count

99a1669

Signed-off-by: Heba Elayoty <[email protected]>

Implement maxUnavailable and UnavailableReplicas metrics

13086af

Signed-off-by: Heba Elayoty <[email protected]>

fix lint fmt

aa9dfe9

Signed-off-by: Heba Elayoty <[email protected]>

update tests

2060aae

Signed-off-by: Heba Elayoty <[email protected]>

se metrics to 1 as a default

a30850f

log for true validation only and update func sig.

2241bd1

Move maxUnavailable metric to the updateStatefulSetStatus

7d04009

Signed-off-by: Heba Elayoty <[email protected]>

change metrics stability level to Alpha

50d11bf

Signed-off-by: Heba Elayoty <[email protected]>

fix unit test

cc47ff0

Signed-off-by: Heba Elayoty <[email protected]>

fix linting issue

51751d1

Signed-off-by: Heba Elayoty <[email protected]>

Address code review feedback

b9aa7c2

Signed-off-by: Heba Elayoty <[email protected]>

helayoty force-pushed the maxunavailable_metrics branch from 7630021 to b9aa7c2 Compare September 16, 2025 23:50

soltysh approved these changes Sep 17, 2025

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2025

k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 17, 2025

k8s-ci-robot merged commit fa90713 into kubernetes:master Sep 17, 2025
12 of 13 checks passed

k8s-ci-robot added this to the v1.35 milestone Sep 17, 2025

github-project-automation Bot moved this from In Progress to Done in SIG Apps Sep 17, 2025

github-project-automation Bot moved this from In Progress to Done in SIG Instrumentation Sep 17, 2025

upodroid mentioned this pull request Oct 2, 2025

cos patch borg-land/kubernetes#1

Merged

nmn3m mentioned this pull request Mar 11, 2026

sig-instrumentation: request reviewer role for nmn3m kubernetes/org#6206

Closed

	// In OrderedReady mode, only 5 pods exist at this point (pod 5 not created yet)
	// In OrderedReady mode, only 4 pods exist at this point (pod 5 not created yet)

Conversation

Edwinhr716 commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

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

Uh oh!

Edwinhr716 commented Mar 20, 2025

Uh oh!

janetkuo commented Mar 20, 2025

Uh oh!

Uh oh!

Edwinhr716 commented Mar 21, 2025

Uh oh!

k8s-triage-robot commented Mar 21, 2025

Uh oh!

dims commented Mar 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

janetkuo commented Mar 26, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 17, 2025

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Sep 17, 2025

Uh oh!

soltysh commented Sep 17, 2025

Uh oh!

k8s-ci-robot commented Sep 17, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

Edwinhr716 commented Mar 20, 2025 •

edited

Loading