Support parallel scaling on StatefulSets #44899

smarterclayton · 2017-04-25T04:41:56Z

StatefulSets now include an alpha scaling feature accessible by setting the `spec.podManagementPolicy` field to `Parallel`.  The controller will not wait for pods to be ready before adding the other pods, and will replace deleted pods as needed.  Since parallel scaling creates pods out of order, you cannot depend on predictable membership changes within your set.

k8s-reviewable · 2017-04-25T04:42:04Z

This change is

kow3ns · 2017-04-25T15:07:06Z

pkg/apis/apps/v1beta1/types.go

+	// StatefulsetBurstAnnotation instructs the stateful set controller to not wait when replacing pods from the set,
+	// and that it may create pods out of order. If enabling this annotation, ensure that your set is capable of
+	// forming a safe quorum even if pod creation is out of order. Accepts "true".
+	StatefulSetBurstAnnotation = "statefulset.alpha.kubernetes.io/burst"


Consider just making it a field in the Spec? We know we need this functionality, and we already have use cases for it. Starting with an annotation might be more trouble than its worth if we intend to upgrade to a field before GA.

Yeah I need to circle back on what exactly what we want to do for this. Will hunt that down.

Initial proposal in the issue was for an annotation, @bgrant0607 seems to favor just adding the field if we want to evolve it from an annotation to a field. I prefer the field primarily for validation integration and API clarity, but, as the annotation is functionally equivalent, if you want to go in that direction I'm on board.

kow3ns · 2017-04-25T15:16:13Z

pkg/controller/statefulset/stateful_set_control.go

 	}
 	return nil
 }

 var _ StatefulSetControlInterface = &defaultStatefulSetControl{}
+
+// setAllowsBurst is true if the alpha burst annotation is set.
+func setAllowsBurst(set *apps.StatefulSet) bool {


Move the helper to utils. If you keep the annotation instead of going with a field you're going to have to validate it here. You should report an error to runtime.ReportError if annotations value is not boolean.

If we report error here it will rate limit the controller. Argument for a field for sure. I'd say either way we can validate the annotation.

kow3ns · 2017-04-25T15:19:41Z

pkg/controller/statefulset/stateful_set_control.go

+// is created while any pod is unhealthy, and pods are terminated in descending order. The burst
+// strategy allows these constraints to be relaxed - pods will be created eagerly and terminated
+// in bulk. Clients using the burst strategy should be careful to ensure they understand the consistency
+// implications of having unpredictable numbers of pods available.
 func (ssc *defaultStatefulSetControl) UpdateStatefulSet(set *apps.StatefulSet, pods []*v1.Pod) error {


Update method internal comments to be consistent with the behavior of out of order creation and termination.

kow3ns · 2017-04-25T15:27:31Z

pkg/apis/apps/v1beta1/types.go

@@ -25,6 +25,10 @@ import (
 const (
 	// StatefulSetInitAnnotation if present, and set to false, indicates that a Pod's readiness should be ignored.
 	StatefulSetInitAnnotation = "pod.alpha.kubernetes.io/initialized"
+	// StatefulsetBurstAnnotation instructs the stateful set controller to not wait when replacing pods from the set,


Doesn't respect ordered termination either. Basically it preserves uniqueness but not ordering. This is captured in the control loop method comments and should be captured here as well.

kow3ns

You should consider using a field in the Spec instead of an annotation. Comments need to be updated. If you decide to keep the annotation we should do some validation as validation.go won't check for an actual boolean value. Other than that looks good.

0xmichalis · 2017-04-26T11:32:00Z

@kubernetes/sig-apps-pr-reviews

0xmichalis · 2017-04-26T14:49:37Z

Do we need an issue in the features repo?

smarterclayton · 2017-04-26T19:52:20Z

One already exists

smarterclayton · 2017-04-27T01:34:45Z

Sounds like we have consensus on a field. Although I'm not sure whether we want a struct for "change policy" or whether this is eventually part of the strategy for rollout. I can imagine that the policy during an update is different than normal behavior or initial rollout.

0xmichalis · 2017-04-27T09:08:24Z

pkg/controller/statefulset/stateful_set_control.go

+			// if the set does not allow bursting, return immediately
+			if monotonic {
+				return nil
+			}


You want to continue here?

yes, if you are in burst mode we scale all them up immediately.

I mean

if !isCreated(replicas[i]) { if err := ssc.podControl.CreateStatefulPod(set, replicas[i]); err != nil { return err } if monotonic { return nil } continue }

since the replica you just created is not going to fall under any of the following cases.

Ah, yes, thanks.

0xmichalis · 2017-04-27T09:22:29Z

Although I'm not sure whether we want a struct for "change policy" or whether this is eventually part of the strategy for rollout. I can imagine that the policy during an update is different than normal behavior or initial rollout.

The original issue is about forming initial quorum. We can constrain burst during a rollout with rollout-specific parameters (maxSurge-maxUnavailable?).

smarterclayton · 2017-04-28T15:30:36Z

It seems appropriate that deployment specific policy overrides / replaces steady state requirements.

smarterclayton · 2017-05-01T03:35:08Z

const OrderedReplacement ReplacementPolicyType = `Ordered`
const ImmediateReplacement ReplacementPolicyType = `Immediate`

type StatefulSetSpec struct {
  ...

  ReplacementPolicy ReplacementPolicyType
}

{"spec":{"replacementPolicy": "Ordered"}}

? Better description than "replacementPolicy"?

smarterclayton · 2017-05-20T15:04:04Z

Barring broken tests, can I get a final review pass on the rest of the PR (got one for the annotation version)?

0xmichalis · 2017-05-20T15:24:07Z

pkg/controller/statefulset/stateful_set_control_test.go

+	sort.Sort(ascendingOrdinal(pods))
+	for ord := 0; ord < len(pods); ord++ {
+		if !storageMatches(set, pods[ord]) {
+			return fmt.Errorf("Pods %s does not match the storage specification of StatefulSet %s ", pods[ord].Name, set.Name)


s/Pods/pod/

0xmichalis · 2017-05-20T15:24:52Z

pkg/controller/statefulset/stateful_set_control_test.go

 			}
 		}
+
 		if !identityMatches(set, pods[ord]) {
 			return fmt.Errorf("Pods %s does not match the identity specification of StatefulSet %s ",


s/Pods/pod/

0xmichalis · 2017-05-20T15:28:01Z

pkg/controller/statefulset/stateful_set_control_test.go

+			fnName = fnName[i+1:]
+		}
+		t.Run(
+			fmt.Sprintf("%s/Monotonic", fnName),


No, this is the name of the test case

ah, you get is an an arg, ack

0xmichalis · 2017-05-20T15:33:35Z

got one for the annotation version

Not sure what do you mean with this one, code lgtm

smarterclayton · 2017-05-20T17:16:14Z

I.e. previously this had LGTM before we talked about changing to field. Just wanted to get a LGTM on this after the field change.

…

On Sat, May 20, 2017 at 11:33 AM, Michail Kargakis ***@***.*** > wrote: got one for the annotation version Not sure what do you mean with this one, code lgtm — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44899 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p-5jd2G82UlzX73I0tUtqIzmxfbrks5r7wfXgaJpZM4NG_mo> .

0xmichalis · 2017-05-20T17:52:49Z

fields lgtm too

The alpha field podManagementPolicy defines how pods are created, deleted, and replaced. The new `Parallel` policy will replace pods as fast as possible, not waiting for the pod to be `Ready` or providing an order. This allows for advanced clustered software to take advantage of rapid changes in scale.

Use subtests to avoid duplicating entire suite of control logic.

k8s-ci-robot · 2017-05-21T20:28:00Z

@smarterclayton: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins Bazel Build	ef6bed8c193a61fcd04af3eb2007ab4ad150c09f	link	`@k8s-bot bazel test this`
Jenkins unit/integration	ce560929e99fe4fd95cbdaa80cc6a20068a656fa	link	`@k8s-bot unit test this`
Jenkins verification	ce560929e99fe4fd95cbdaa80cc6a20068a656fa	link	`@k8s-bot verify test this`
pull-kubernetes-federation-e2e-gce	`e40648d`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

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.

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/test-infra repository. I understand the commands that are listed here.

smarterclayton · 2017-05-21T20:57:45Z

Any other comments? Ready for final review.

kow3ns · 2017-05-22T18:00:42Z

/lgtm

k8s-github-robot · 2017-05-22T18:00:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kow3ns, smarterclayton

Needs approval from an approver in each of these OWNERS Files:

~~api/OWNERS~~ [smarterclayton]
~~docs/OWNERS~~ [smarterclayton]
~~pkg/api/OWNERS~~ [smarterclayton]
~~pkg/controller/statefulset/OWNERS~~ [kow3ns,smarterclayton]
~~pkg/registry/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/client-go/OWNERS~~ [smarterclayton]
~~test/OWNERS~~ [kow3ns,smarterclayton]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-05-23T02:07:08Z

Automatic merge from submit-queue (batch tested with PRs 38990, 45781, 46225, 44899, 43663)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 25, 2017

k8s-github-robot assigned foxish and bprashanth Apr 25, 2017

smarterclayton requested a review from kow3ns April 25, 2017 04:44

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Apr 25, 2017

smarterclayton force-pushed the burst branch from 436bec4 to c365167 Compare April 25, 2017 04:54

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 25, 2017

smarterclayton force-pushed the burst branch 2 times, most recently from 3a141ba to 03b3e2f Compare April 25, 2017 14:57

kow3ns reviewed Apr 25, 2017

View reviewed changes

smarterclayton force-pushed the burst branch from 03b3e2f to fc50b75 Compare April 25, 2017 16:41

kow3ns reviewed Apr 25, 2017

View reviewed changes

smarterclayton force-pushed the burst branch from fc50b75 to c179138 Compare April 25, 2017 18:11

smarterclayton force-pushed the burst branch from c179138 to abe8015 Compare April 26, 2017 18:32

0xmichalis reviewed Apr 27, 2017

View reviewed changes

smarterclayton force-pushed the burst branch from abe8015 to cf5204e Compare May 1, 2017 03:32

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 19, 2017

0xmichalis reviewed May 20, 2017

View reviewed changes

smarterclayton changed the title ~~Support burst scaling on StatefulSets~~ Support parallel scaling on StatefulSets May 20, 2017

smarterclayton force-pushed the burst branch from ef6bed8 to ce56092 Compare May 20, 2017 17:26

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2017

smarterclayton force-pushed the burst branch 4 times, most recently from a6b5f3d to 0b9532b Compare May 21, 2017 02:49

smarterclayton added 3 commits May 21, 2017 01:14

Combine statefulset burst and monotonic scaling tests

20d45af

Use subtests to avoid duplicating entire suite of control logic.

E2E test for statefulset burst

e40648d

smarterclayton force-pushed the burst branch from 0b9532b to e40648d Compare May 21, 2017 20:01

k8s-ci-robot assigned kow3ns May 22, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 22, 2017

k8s-github-robot merged commit c2c5051 into kubernetes:master May 23, 2017

bartlett-ops mentioned this pull request Nov 30, 2018

rabbitmq-ha restart after total node failure helm/charts#9645

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support parallel scaling on StatefulSets #44899

Support parallel scaling on StatefulSets #44899

smarterclayton commented Apr 25, 2017 •

edited

Loading

k8s-reviewable commented Apr 25, 2017

kow3ns Apr 25, 2017

smarterclayton Apr 25, 2017

kow3ns Apr 26, 2017

kow3ns Apr 25, 2017

smarterclayton Apr 25, 2017

kow3ns Apr 25, 2017

kow3ns Apr 25, 2017 •

edited

Loading

kow3ns left a comment

0xmichalis commented Apr 26, 2017

0xmichalis commented Apr 26, 2017

smarterclayton commented Apr 26, 2017

smarterclayton commented Apr 27, 2017

0xmichalis Apr 27, 2017

smarterclayton Apr 28, 2017

0xmichalis Apr 28, 2017

smarterclayton Apr 29, 2017

0xmichalis commented Apr 27, 2017

smarterclayton commented Apr 28, 2017

smarterclayton commented May 1, 2017

smarterclayton commented May 20, 2017

0xmichalis May 20, 2017

0xmichalis May 20, 2017

0xmichalis May 20, 2017

smarterclayton May 20, 2017

0xmichalis May 20, 2017

0xmichalis commented May 20, 2017

smarterclayton commented May 20, 2017 via email

0xmichalis commented May 20, 2017

k8s-ci-robot commented May 21, 2017 •

edited

Loading

smarterclayton commented May 21, 2017

kow3ns commented May 22, 2017

k8s-github-robot commented May 22, 2017

k8s-github-robot commented May 23, 2017

Support parallel scaling on StatefulSets #44899

Support parallel scaling on StatefulSets #44899

Conversation

smarterclayton commented Apr 25, 2017 • edited Loading

k8s-reviewable commented Apr 25, 2017

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

kow3ns Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

kow3ns left a comment

Choose a reason for hiding this comment

0xmichalis commented Apr 26, 2017

0xmichalis commented Apr 26, 2017

smarterclayton commented Apr 26, 2017

smarterclayton commented Apr 27, 2017

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

0xmichalis commented Apr 27, 2017

smarterclayton commented Apr 28, 2017

smarterclayton commented May 1, 2017

smarterclayton commented May 20, 2017

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

0xmichalis commented May 20, 2017

smarterclayton commented May 20, 2017 via email

0xmichalis commented May 20, 2017

k8s-ci-robot commented May 21, 2017 • edited Loading

smarterclayton commented May 21, 2017

kow3ns commented May 22, 2017

k8s-github-robot commented May 22, 2017

k8s-github-robot commented May 23, 2017

smarterclayton commented Apr 25, 2017 •

edited

Loading

kow3ns Apr 25, 2017 •

edited

Loading

k8s-ci-robot commented May 21, 2017 •

edited

Loading