Add PRR information for maxUnavailable #2688

krmayankk · 2021-05-07T03:46:46Z

wojtek-t · 2021-05-07T12:38:33Z

keps/sig-apps/961-maxunavailable-for-statefulset/kep.yaml

  - "@janetkuo"
  - "@kow3ns"
+prr-approvers:
+  - "@johnbelamaric"


You can remove of it (we replaced that with prod-readiness/ files)

[alternatively, please change to me]

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

wojtek-t · 2021-05-07T12:58:26Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+
+- [x] Feature gate (also fill in values in `kep.yaml`)
+  - Feature gate name: MaxUnavailableStatefulSet
+  - Components depending on the feature gate: StatefulSet


kube-apiserver and kube-controller-manager

[in kube-apiserver you need a handling for the new field - the field is accepted only if feature gate is enabled]

wojtek-t · 2021-05-07T13:00:15Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+###### What happens if we reenable the feature if it was previously rolled back?
+
+If the feature is enabled and then later disabled, we will revert the value of 
+maxUnavailable to the default value so that customers are not surprised.


That's not what we want (unless I misunderstood you) - see the comment above also.

If the field is set in the API, it shouldn't silently disappeared - it's just the controller who should ignore its existence.

agree, that is what we are doing https://github.com/kubernetes/kubernetes/pull/82162/files#diff-a1e25eb50e4b2aed8947c2caa5d6d1ddfef1a5f2f95fdf71f2388c657823ee13R529

The link above is about controller code - which is useful to know. But i would also like to explicitly write that the field in the object itself is not modified.

we start here and get the value of maxUnavailable from this new field in StatefulSet.

We again check if the feature flag is disabled we reset maxUnaavailable local variable to 1 and then the rest of the code uses this local variable.

The text is misleading, because we won't be reverting the value itself.
Also - it's about disabling - not reenabling.

Can you pleas change to something along the lines with: we will restore the desired behavior for StatefulSets for which the maxunavailable field wasn't deleted after the feature gate was disabled

can you actually delete a field after a feature gate was disabled ?

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

wojtek-t · 2021-05-07T13:02:26Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+
+###### What specific metrics should inform a rollback?
+The customer should look at the behavior of the maxUnavailable and if the rollout
+is not respecting the maxUnavailable, the feature should be rolledback.


How can they look at it? metrics?

added more specific data, they are not really metrics

wojtek-t · 2021-05-07T13:02:51Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+is not respecting the maxUnavailable, the feature should be rolledback.
+
+###### Were upgrade and rollback tested? Was the upgrade->downgrade->upgrade path tested?
+No


Please add that it will be tested when graduating to Beta (and add this to graduation criteria).

wojtek-t · 2021-05-07T13:04:57Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+```
+
+###### How can someone using this feature know that it is working for their instance?
+Ensure the number of replicas in your StatefulSet is more than 2. 


This isn't really what we want. We don't ask how to test the behavior.

We're rather asking how to check if the feature is generally working.

[The question is not strictly required for Alpha - but basically I would expect some metric to be exposed from statefule-set controller that will be reporting e.g. if during any reconciliation more pods than expected were not ready, or sth like that]

added some more data

Sorry - let me be more explicit. This isn't actionable for cluster operators at all.

An operator cannot track upgrades of individual statefulsets and check what is happening during those.
The operator usually also cannot trigger actions on existing StatefulSets or created their own.

Metrics are the mechanism for this.

changed to TODO for when we target beta. @kow3ns if you have advice on this or adding something to this effect in alpha

wojtek-t · 2021-05-07T13:06:20Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+### Dependencies
+
+###### Does this feature depend on any specific services running in the cluster?
+controller-manager 


Please remove - this is somewhat obvious.

There aren't any additional dependencies (we more want to ensure that you don't depend on things like dns, additonal agents, cloud-provider, etc.)

wojtek-t · 2021-05-07T13:07:41Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+A new field gets added to every StatefulSet object which occupies 32bits.
+
+###### Will enabling / using this feature result in increasing time taken by any operations covered by existing SLIs/SLOs?
+The increasing time per StatefulSet for rolling updates should be negligible.


We don't have any SLOs that cover statefulsets.

So the answer is basically "no"

alculquicondor · 2021-05-07T13:59:03Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

 #### Implementation

-TBD: Will be updated after we have agreed on the semantics being discussed above.
+The alpha release we are going with Choice 4 with support for both PMP=Parallel and PMP=OrderedReady.


Parallel is clear to use behavior 2. What is the behavior for OrderedReady? 3 would make the most sense to me, but was there consensus?

Better remove the paragraph Recommended Choice and just go with the details in Implementation.

alculquicondor · 2021-05-07T14:02:14Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

 ## Graduation Criteria

 - Alpha: Initial support for maxUnavailable in StatefulSets added. Disabled by default. 
 - Beta:  Enabled by default with default value of 1. 


This seems to imply that default value is not 1 in alpha when the feature is enabled. Please clarify.

alculquicondor · 2021-05-07T14:23:05Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+### Scalability
+
+###### Will enabling / using this feature result in any new API calls?
+This feature when set, makes an extra loop over the total pods that belong


first sentence seems unnecessary.

alculquicondor · 2021-05-07T14:23:21Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+to the StatefulSet.  It doesnt make any extra API calls.
+
+###### Will enabling / using this feature result in introducing new API types?
+This feature adds a new field in the StatefulSet RollingUpdate, called maxUnavailable.


answer here should be no

alculquicondor · 2021-05-07T14:24:00Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+
+
+###### Will enabling / using this feature result in increasing size or count of the existing API objects?
+A new field gets added to every StatefulSet object which occupies 32bits.


is it? doesn't percentage require more bits to represent?

wojtek-t · 2021-05-11T05:59:00Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+###### What happens if we reenable the feature if it was previously rolled back?
+
+If the feature is enabled and then later disabled, we will revert the value of 
+maxUnavailable to the default value so that customers are not surprised.


The link above is about controller code - which is useful to know. But i would also like to explicitly write that the field in the object itself is not modified.

wojtek-t · 2021-05-11T06:02:58Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+
+###### How can a rollout or rollback fail? Can it impact already running workloads?
+
+No, a rollout or rollback cannot fail.


Rollout can always fail. One can imagine a bug in strategy that is causing kube-apiserver to crash.

when you say bug in strategy what do you mean ?

updated the text

wojtek-t · 2021-05-11T06:04:46Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+to a very high number and due to some bug, the StatefulSet pods are not coming up.
+
+###### What specific metrics should inform a rollback?
+The customer should look at StatefulSet.Status.Replicas to ensure if that number is always 


This isn't a metric. An operator cannot track that for all statefulsets potentially across many different clusters.
We really need a metric for this.

[This question is not strictly required for Beta - so you can change to TODO or sth and figure it out for beta.

changed to TODO

wojtek-t · 2021-05-11T06:08:15Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+```
+
+###### How can someone using this feature know that it is working for their instance?
+Ensure the number of replicas in your StatefulSet is more than 2. 


Sorry - let me be more explicit. This isn't actionable for cluster operators at all.

An operator cannot track upgrades of individual statefulsets and check what is happening during those.
The operator usually also cannot trigger actions on existing StatefulSets or created their own.

Metrics are the mechanism for this.

wojtek-t · 2021-05-13T05:55:12Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md


 - Upgrades
- When upgrading from a release without this feature, to a release with maxUnavailable, we will set maxUnavailable to 1. This would give users the same default
+ When upgrading from a release without this feature, to a release with maxUnavailable, we will default maxUnavailable to 1. This would give users the same default


It's not about upgrades per-se. Let's state that "we will default to 1 for backward compatibility".

wojtek-t · 2021-05-13T05:56:39Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

 - Downgrades
 When downgrading from a release with this feature, to a release without maxUnavailable, there are two cases
- -- if maxUnavailable is greater than 1 -- in this case user  can see unexpected behavior(Find out what is the recommendation here(Warning, disable upgrade, drop field, etc? )
+ -- if maxUnavailable is greater than 1, there are two more cases:-


nit: can you make it look like list in the "view" format?

wojtek-t · 2021-05-13T06:02:45Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+###### What happens if we reenable the feature if it was previously rolled back?
+
+If the feature is enabled and then later disabled, we will revert the value of 
+maxUnavailable to the default value so that customers are not surprised.


The text is misleading, because we won't be reverting the value itself.
Also - it's about disabling - not reenabling.

Can you pleas change to something along the lines with: we will restore the desired behavior for StatefulSets for which the maxunavailable field wasn't deleted after the feature gate was disabled

wojtek-t · 2021-05-13T06:04:16Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+maxUnavailable to a number greater than 1,  but the invariants and the logic will ensure that there are never more than
+maxUnavailable pods with the same identity and never more than maxUnavailable being deleted.
+
+It can impact already running workloads if service owner accidentally sets maxUnavailable


Let's remove this one - if someone "accidentally sets a feature" - many features cna be harmful :)

wojtek-t · 2021-05-13T06:53:01Z

/assign @soltysh @janetkuo

This will need sig-apps approval also...

wojtek-t · 2021-05-13T08:19:21Z

I'm fine with this for Alpha

/approve PRR

soltysh

/lgtm
/approve
this is good starting point for alpha, it will have to go through more careful scrutiny for beta

soltysh · 2021-05-13T09:00:09Z

keps/sig-apps/961-maxunavailable-for-statefulset/README.md

+## Test Plan
+For `Alpha`, unit tests and e2e tests will be added to test functionality at both
+with feature flag enabled and disabled. Defaults will be verified so that users
+who donot set this flag are not surprised at all.


k8s-ci-robot · 2021-05-13T09:03:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krmayankk, soltysh, wojtek-t

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:

~~keps/prod-readiness/OWNERS~~ [wojtek-t]
~~keps/sig-apps/OWNERS~~ [soltysh]

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

carlisia · 2021-05-19T01:40:00Z

Hello @krmayankk 👋, 1.22 Docs Shadow here.
This enhancement is marked as ‘Needs Docs’ for the 1.22 release.

Please follow the steps detailed in the documentation to open a PR against dev-1.22 branch in the k/website repo. This PR can be just a placeholder at this time and must be created before Fri July 9, 11:59 PM PDT.

Also, take a look at Documenting for a release to familiarize yourself with the docs requirement for the release.

Thank you! 🙏

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 7, 2021

k8s-ci-robot requested review from kow3ns and soltysh May 7, 2021 03:46

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 7, 2021

krmayankk mentioned this pull request May 7, 2021

maxUnavailable for StatefulSets #961

Open

8 tasks

wojtek-t self-assigned this May 7, 2021

wojtek-t reviewed May 7, 2021

View reviewed changes

alculquicondor reviewed May 7, 2021

View reviewed changes

wojtek-t reviewed May 11, 2021

View reviewed changes

wojtek-t reviewed May 13, 2021

View reviewed changes

k8s-ci-robot assigned janetkuo and soltysh May 13, 2021

Add PRR information for maxUnavailable

11b2f1f

soltysh approved these changes May 13, 2021

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 13, 2021

k8s-ci-robot merged commit 922efbc into kubernetes:master May 13, 2021

k8s-ci-robot added this to the v1.22 milestone May 13, 2021



		###### Will enabling / using this feature result in increasing size or count of the existing API objects?
		A new field gets added to every StatefulSet object which occupies 32bits.


		###### How can a rollout or rollback fail? Can it impact already running workloads?

		No, a rollout or rollback cannot fail.

Add PRR information for maxUnavailable #2688

Add PRR information for maxUnavailable #2688

Uh oh!

Conversation

krmayankk commented May 7, 2021 • edited by soltysh Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

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!

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!

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

krmayankk commented May 7, 2021 •

edited by soltysh

Loading