Add support for parallel node updates within a statefulset #283

nbroyles · 2021-03-11T01:06:41Z

This commits allows multiple nodes to be updated and deployed
at the same time within a statefulset. Opting into this behavior
requires the user to update the ClusterSpec to set OnDeleteUpdateStrategy
to true. When this is set, the operator will watch for the
operator.m3db.io/parallel-update annotation on statefulsets. When found,
the operator will attempt to update each statefulset, rolling N many
pods at a time where N is the value of the parallel-update annotation.

Thanks for contributing to the M3DB Operator! We'd love to accept your contribution, but we require that most issues
outside of trivial changes such as typo corrections have an issue associated with the pull request. So please open an
issue first!

This commits allows multiple nodes to be updated and deployed at the same time within a statefulset. Opting into this behavior requires the user to update the ClusterSpec to set OnDeleteUpdateStrategy to true. When this is set, the operator will watch for the operator.m3db.io/parallel-update annotation on statefulsets. When found, the operator will attempt to update each statefulset, rolling N many pods at a time where N is the value of the parallel-update annotation.

nbroyles · 2021-03-11T01:08:21Z

pkg/controller/controller_test.go

 		mu.Unlock()

 		if seen != len(opts.expectedStatefulSets) {
 			time.Sleep(100 * time.Millisecond)
-			continue


This continue and break logic actually caused the test method to exit before statefulset updates were finished so just remove and rely on the iterations limit.

ryanhall07

generally LG

pkg/controller/controller.go

ryanhall07 · 2021-03-11T03:39:49Z

pkg/controller/controller.go

+			zap.Int64("generation", expected.Generation),
+			zap.Int64("observed", expected.Status.ObservedGeneration),
+		)
+	} else if _, ok := actual.Annotations[annotations.ParallelUpdateInProgress]; !ok {


it seems like this method is doing a lot with several different flows. is there anyway we can break this up?

it seems updating the sts is the same between the 2 different rollout strategies, so that's probably something we could break out.

Yeah, simplified this to make it a little more clear.

jeromefroe

LGTM! Just had one edge question on how the StatefulSet controller handles changes to a set's update strategy.

jeromefroe · 2021-03-11T14:57:09Z

pkg/controller/controller.go

+	expected *appsv1.StatefulSet,
+	update bool,
+) (bool, error) {
+	if !update {


nit: would it make sense to move the logic for checking the update bool to where we call updateWithRollingUpdateStrategy or updateWithOnDeleteStrategy. Seems if update is false both functions become noop's so just wondering if it would be simpler to only call them when update is true.

We actually still want to call updateWithOnDeleteStrategy even if we're not updating the statefulset as we may still need to roll nodes in the set. Simplified the logic to make this a little more clear

jeromefroe · 2021-03-11T15:17:35Z

pkg/controller/controller.go

+		// The expectation is that these update methods will return false when they have updated
+		// actual pods in the statefulset. This will stop any further processing of pods until
+		// the updated ones are back healthy
+		if cluster.Spec.OnDeleteUpdateStrategy {


Do we know if we were to update a StatefulSet's template and its updateStrategy if the controller will use the new updateStrategy to update the set or the old one? Just wondering about a potential edge case where we update both, and the StatefulSet controller used the old strategy for the update but because we're checking cluster.Spec.OnDeleteUpdateStrategy here we expect the new one will be used. I would think the controller would use the new strategy that's in the update but Kube has surprised me before 😄

Yeah, so I just tested this out and it does respect the new updateStrategy. So when you change the template and set OnDelete, k8s (correctly, imo) waits for you to roll the statefulset pods to pick up the update.

codecov · 2021-03-11T17:02:59Z

Codecov Report

Merging #283 (73ea7b9) into master (b722629) will increase coverage by 0.10%.
The diff coverage is 61.90%.

@@            Coverage Diff             @@
##           master     #283      +/-   ##
==========================================
+ Coverage   75.99%   76.09%   +0.10%     
==========================================
  Files          32       32              
  Lines        2416     2548     +132     
==========================================
+ Hits         1836     1939     +103     
+ Misses        432      430       -2     
- Partials      148      179      +31

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b722629...73ea7b9. Read the comment docs.

schallert · 2021-03-11T17:29:08Z

pkg/controller/controller.go

+				c.logger.Error(err.Error())
+				return err
+			}
+			processNext = false


Rather than have processNext = false here and continue this loop, can we just return nil so that we exit + re-trigger the entire reconciliation loop? That way we'll run all of our safety checks again.

AFAICT this would also match the old post-update behavior.

lol, yes, that's much simpler. Will update.

Actually, wait. Only if the updateStrategy is rolling update can we do that. Still probably nice to bail early if using rolling update, so will modify for that.

OK, pushed a change to clarify this logic even more. It's worth noting that none of the semantics around this loop have changed here. In both the RollingUpdate and OnDelete cases, if changes are made to a statefulset, the operator exits this loop and relies on event updates (i.e. pod ready) to continue updating.

schallert · 2021-03-11T17:33:22Z

pkg/controller/controller.go

+			}
+			names = append(names, pod.Name)
+		}
+		logger.Info("restarting pods", zap.Any("pods", names))


Nit: you can use zap.Strings here since names is a []string.

schallert · 2021-03-11T17:35:08Z

pkg/controller/controller.go

+		// NB(nate): K8s handles this for you when using the RollingUpdate update strategy.
+		// However, since OnDelete removes k8s from the pod update process, it's our
+		// responsibility to set this once done rolling out.
+		set.Status.CurrentReplicas = set.Status.UpdatedReplicas
+		set.Status.CurrentRevision = set.Status.UpdateRevision


This is surprising / also concerning to have to reconcile k8s' status for it. Are there docs somewhere on this?

If we have to do this, I would also prefer we use Update here rather than a patch. That way if k8s has modified the statefulset in between when we decided to change this field + when we made the patch, we'll get a conflict and have to re-reconcile.

Yeah, it's very unfortunate. I pieced it together from a bug report (kubernetes/kubernetes#73492) and looking at the update code (https://github.com/kubernetes/kubernetes/blob/053e7f4b0430e1f3814bb16bdb506cd59bb26593/pkg/controller/statefulset/stateful_set_utils.go#L377). But definitely let me know if I'm off base here.

Also, was looking closer at the StatefulSetInterface and saw this method:

UpdateStatus(*v1.StatefulSet) (*v1.StatefulSet, error)

What are your thoughts on this change then?

sts.Status.CurrentReplicas = sts.Status.UpdatedReplicas sts.Status.CurrentRevision = sts.Status.UpdateRevision if sts, err = c.kubeClient.AppsV1().StatefulSets(cluster.Namespace).UpdateStatus(sts); err != nil { return false, err } if err = c.patchStatefulSet(sts, func(set *appsv1.StatefulSet) { delete(set.Annotations, annotations.ParallelUpdateInProgress) }); err != nil { return false, err }

Would that also generate a conflict since it's no longer patching for the revision update? Also, seems safe to patch to remove the annotation, but correct me if I'm wrong.

This SGTM 👍 .

schallert · 2021-03-11T17:35:41Z

pkg/controller/controller.go

+	return false, nil
+}
+
+func (c *M3DBController) getMaxPodsToUpdate(actual *appsv1.StatefulSet) (int, error) {


Can this be a free function rather than a method on *M3DBController?

schallert

LGTM. Very nice! The changes make this easy to follow.

schallert · 2021-03-11T23:14:43Z

pkg/controller/controller.go

+		// NB(nate): K8s handles this for you when using the RollingUpdate update strategy.
+		// However, since OnDelete removes k8s from the pod update process, it's our
+		// responsibility to set this once done rolling out.
+		set.Status.CurrentReplicas = set.Status.UpdatedReplicas
+		set.Status.CurrentRevision = set.Status.UpdateRevision


This SGTM 👍 .

ryanhall07

thanks for iterating. LG!

ryanhall07 · 2021-03-12T15:07:46Z

pkg/controller/controller.go

+}
+
+// updateStatefulSetNodes returns true if it updates any pods
+func (c *M3DBController) updateStatefulSetNodes(


nit: it would be nice to be consistent with nodes vs pods in the terminology.

i lean towards pods since this is k8s code

* master: Backwards compatibility when using the original update annoation with an OnDelete update strategy (#284) Add support for parallel node updates within a statefulset (#283) Support namespace ExtendedOptions in cluster spec (#282) [controller] Support multi instance placement add (#275) [gomod] Update M3DB dependency (#277) [cmd] Fix instrument package name (#280) # Conflicts: # pkg/k8sops/annotations/annotations.go

nbroyles commented Mar 11, 2021

View reviewed changes

nbroyles requested review from schallert, jeromefroe and ryanhall07 March 11, 2021 01:08

ryanhall07 reviewed Mar 11, 2021

View reviewed changes

Run openapi gen

424b30a

jeromefroe approved these changes Mar 11, 2021

View reviewed changes

nbroyles added 2 commits March 11, 2021 11:29

PR feedback: simplify update logic

10c92ca

Appease our linter overlords

cefc1e3

schallert reviewed Mar 11, 2021

View reviewed changes

PR feedback

e398fd2

schallert approved these changes Mar 11, 2021

View reviewed changes

Use UpdateStatuses method

ef4ab64

ryanhall07 approved these changes Mar 12, 2021

View reviewed changes

nbroyles added 2 commits March 15, 2021 11:21

Improve selection criteria for pods to update

910ebd8

Lint

a7b54bf

nbroyles force-pushed the nb/parallel branch from ce25688 to a1beaf1 Compare March 16, 2021 15:59

Minor refactors and cleanup

73ea7b9

nbroyles force-pushed the nb/parallel branch from a1beaf1 to 73ea7b9 Compare March 16, 2021 16:15

nbroyles merged commit 8972589 into master Mar 16, 2021

nbroyles deleted the nb/parallel branch March 16, 2021 16:22

Add support for parallel node updates within a statefulset #283

Add support for parallel node updates within a statefulset #283

Uh oh!

Conversation

nbroyles commented Mar 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanhall07 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeromefroe 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!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

nbroyles Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

schallert left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanhall07 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!

Uh oh!

codecov bot commented Mar 11, 2021 •

edited

Loading

nbroyles Mar 11, 2021 •

edited

Loading