Add min ready seconds impl #101316

ravisantoshgudimetla · 2021-04-21T12:31:37Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

#100842 introduced featuregate. This PR implements the logic behind it.

Which issue(s) this PR fixes:

Partially Fixes# #65098.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Implement minReadySeconds for the StatefulSets.

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

[KEP]: https://github.com/kubernetes/enhancements/pull/2607
[Other doc]: https://github.com/kubernetes/website/pull/27683

ravisantoshgudimetla · 2021-04-22T00:30:45Z

/sig apps

fedebongio · 2021-04-22T20:05:44Z

/remove-sig api-machinery

ravisantoshgudimetla · 2021-04-28T22:47:43Z

/retest

fedebongio · 2021-04-29T20:06:11Z

/remove-sig api-machinery

soltysh

I'm missing unit test ensuring the new functionality is working as expected. I'd at least require following tests:

requeue if available replicas is less than requested
ensure available is updated when pods switches to available

pkg/controller/statefulset/stateful_set.go

pkg/controller/statefulset/stateful_set_control.go

soltysh · 2021-06-01T16:04:59Z

pkg/controller/statefulset/stateful_set_control.go

@@ -458,7 +481,7 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
 		}
 	}

-	// At this point, all of the current Replicas are Running and Ready, we can consider termination.
+	// At this point, all of the current Replicas are Running and Ready or Available, we can consider termination.


Ready and Available, you can't be one or the other, IsPodAvailable checks IsPodReady in the first place.

pkg/controller/statefulset/stateful_set_control.go

soltysh · 2021-06-01T16:06:28Z

pkg/controller/statefulset/stateful_set_control.go

@@ -486,6 +509,15 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
 				firstUnhealthyPod.Name)
 			return &status, nil
 		}
+		// if we are in monotic mode and the condemned target is not the first unhealthy Pod, block.
+		if utilfeature.DefaultFeatureGate.Enabled(features.StatefulSetMinReadySeconds) && isRunningAndAvailable(condemned[target], set.Spec.MinReadySeconds) && monotonic && condemned[target] != firstUnhealthyPod {


pkg/controller/statefulset/stateful_set_utils.go

ravisantoshgudimetla

Thank you for the reviews @atiratree and @soltysh for the reviews. I updated the PR based on your feedback. PTAL

pkg/controller/statefulset/stateful_set_control.go

pkg/controller/statefulset/stateful_set_utils.go

atiratree · 2021-06-02T16:36:21Z

pkg/controller/statefulset/stateful_set_control.go

+		// If we have a Pod that has been created but is not available we can not make progress.
+		// We must ensure that all for each Pod, when we create it, all of its predecessors, with respect to its
+		// ordinal, are Available.
+		// Since available is superset of Ready, once we have this featuregate enabled by default, we can remove the


personally I would prefer TODO or some similar label here :-) but don't know what are the conventions for this..

ravisantoshgudimetla · 2021-06-15T11:42:34Z

/test pull-kubernetes-integration

soltysh

A few minor nits, and this can land.
/triage accepted
/priority backlog
/approve

soltysh · 2021-06-15T15:20:28Z

pkg/controller/statefulset/stateful_set_control.go

 	}
-
+	klog.V(1).Infof("%v", currentStatus)


This looks like a debug leftover.

soltysh · 2021-06-15T15:22:41Z

pkg/controller/statefulset/stateful_set_control.go

+					status.AvailableReplicas++
+				}
+			} else {
+				klog.V(4).Infof("minReadySeconds featuregate is disabled")


This will be very chatty and unnecessary in most cases, I'd probably drop it.

soltysh · 2021-06-15T15:23:55Z

pkg/controller/statefulset/stateful_set_control.go

@@ -486,6 +509,17 @@ func (ssc *defaultStatefulSetControl) updateStatefulSet(
 				firstUnhealthyPod.Name)
 			return &status, nil
 		}
+		// if we are in monotic mode and the condemned target is not the first unhealthy Pod, block.


s/monotoic/monotonic

soltysh · 2021-06-15T15:27:13Z

pkg/controller/statefulset/stateful_set_control_test.go

+		inputSTS               *apps.StatefulSet
+		expectedActiveReplicas int32
+		readyDuration          time.Duration
+		enabled                bool


nit: featureGateEnabled to make it explicitly clear what it does

soltysh · 2021-06-15T15:30:40Z

Removing api-change label, which was here b/c the PR was originally based on API changes which landed in separate PR.

kubernetes#100842 introduced featuregate. This PR implements the logic behind it.

soltysh

/lgtm
/kind feature

k8s-ci-robot · 2021-06-15T19:12:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ravisantoshgudimetla, soltysh

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:

~~pkg/controller/statefulset/OWNERS~~ [soltysh]
~~test/OWNERS~~ [soltysh]

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

lubronzhan · 2021-11-13T06:19:42Z

pkg/controller/statefulset/stateful_set_control_test.go

@@ -1993,6 +2100,7 @@ func scaleUpStatefulSetControl(set *apps.StatefulSet,
 		if err := invariants(set, spc); err != nil {
 			return err
 		}
+		//fmt.Printf("Ravig pod conditions %v %v", set.Status.ReadyReplicas, *set.Spec.Replicas)


This should be removed?

Yup, feel free to open a PR and ping me with it :)

ravisantoshgudimetla changed the title ~~Add min ready seconds impl~~ [wip] Add min ready seconds impl Apr 21, 2021

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 Apr 21, 2021

k8s-ci-robot requested review from davidopp and deads2k April 21, 2021 12:32

ravisantoshgudimetla force-pushed the add-minReadySeconds-impl branch from fa2f0e2 to d9bc222 Compare April 21, 2021 22:00

k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Apr 21, 2021

ravisantoshgudimetla force-pushed the add-minReadySeconds-impl branch 2 times, most recently from f5744f3 to 1c6c06a Compare April 21, 2021 22:09

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 22, 2021

ravisantoshgudimetla force-pushed the add-minReadySeconds-impl branch from 1c6c06a to 123d9de Compare April 28, 2021 15:12

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 28, 2021

k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Apr 29, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 12, 2021

soltysh requested changes Jun 1, 2021

View reviewed changes

ravisantoshgudimetla force-pushed the add-minReadySeconds-impl branch from caa5347 to 3b55b4b Compare June 2, 2021 13:05

ravisantoshgudimetla commented Jun 2, 2021

View reviewed changes

pkg/controller/statefulset/stateful_set_control.go Outdated Show resolved Hide resolved

pkg/controller/statefulset/stateful_set_utils.go Show resolved Hide resolved

atiratree reviewed Jun 2, 2021

View reviewed changes

ravisantoshgudimetla force-pushed the add-minReadySeconds-impl branch from 3b55b4b to 82b7478 Compare June 7, 2021 15:22

soltysh approved these changes Jun 15, 2021

View reviewed changes

soltysh removed the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Jun 15, 2021

k8s-ci-robot added the do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. label Jun 15, 2021

statefulsets: MinReadySeconds implementation

ceb1dbd

kubernetes#100842 introduced featuregate. This PR implements the logic behind it.

ravisantoshgudimetla force-pushed the add-minReadySeconds-impl branch from 82b7478 to ceb1dbd Compare June 15, 2021 17:01

soltysh approved these changes Jun 15, 2021

View reviewed changes

k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Jun 15, 2021

k8s-ci-robot merged commit a5be86f into kubernetes:master Jun 15, 2021

k8s-ci-robot added this to the v1.22 milestone Jun 15, 2021

This was referenced Jun 22, 2021

Add STS minReadySeconds to alpha kubernetes/test-infra#22651

Merged

StatefulSet minReadySeconds e2e #103073

Merged

JamesLaverack mentioned this pull request Jun 23, 2021

Add minReadySeconds to Statefulsets kubernetes/enhancements#2599

Closed

12 tasks

philipgough mentioned this pull request Sep 3, 2021

Add .spec.minReadySeconds as configurable option prometheus-operator/prometheus-operator#3711

Closed

atiratree mentioned this pull request Oct 4, 2021

REQUEST: New membership for atiratree kubernetes/org#3026

Closed

7 tasks

lubronzhan reviewed Nov 13, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add min ready seconds impl #101316

Add min ready seconds impl #101316

ravisantoshgudimetla commented Apr 21, 2021 •

edited

Loading

ravisantoshgudimetla commented Apr 22, 2021

fedebongio commented Apr 22, 2021

ravisantoshgudimetla commented Apr 28, 2021

fedebongio commented Apr 29, 2021

soltysh left a comment •

edited

Loading

soltysh Jun 1, 2021

soltysh Jun 1, 2021

ravisantoshgudimetla left a comment

atiratree Jun 2, 2021

ravisantoshgudimetla Jun 7, 2021

atiratree Jun 7, 2021

ravisantoshgudimetla commented Jun 15, 2021

soltysh left a comment

soltysh Jun 15, 2021

soltysh Jun 15, 2021

soltysh Jun 15, 2021

soltysh Jun 15, 2021

soltysh commented Jun 15, 2021

soltysh left a comment

k8s-ci-robot commented Jun 15, 2021

lubronzhan Nov 13, 2021

soltysh Nov 16, 2021

Add min ready seconds impl #101316

Add min ready seconds impl #101316

Conversation

ravisantoshgudimetla commented Apr 21, 2021 • edited Loading

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.:

ravisantoshgudimetla commented Apr 22, 2021

fedebongio commented Apr 22, 2021

ravisantoshgudimetla commented Apr 28, 2021

fedebongio commented Apr 29, 2021

soltysh left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla 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

ravisantoshgudimetla commented Jun 15, 2021

soltysh 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

soltysh commented Jun 15, 2021

soltysh left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla commented Apr 21, 2021 •

edited

Loading

soltysh left a comment •

edited

Loading