Rotator tool to accelerate k8s cluster upgrades and node rotations. #1

stylianosrigas · 2021-02-16T18:33:05Z

Rotator is a tool meant to smooth and accelerate k8s cluster upgrades and node rotations. It offers automation on autoscaling group recognition and flexility on options such as, how fast to rotate nodes, drain retries, waiting time between rotations and drains as well as mater/worker node separation.

This tool can work as both a cli/api tool and an imported go package for use from other applications.

stylianosrigas · 2021-02-25T13:10:11Z

@stafot @angeloskyratzakos @Szymongib @gabrieljackson Sorry in advance for the huge PR, it is just all new and there was no easy way to split it.

gabrieljackson

Great work on this! A few thoughts and questions below.

api/api.go

gabrieljackson · 2021-02-25T23:56:03Z

aws/helpers.go

+		return "", nil
+	}
+
+	return *resp.Reservations[0].Instances[0].InstanceId, nil


Do we need to check for len() on either Reservations or Instances before referencing the first value? I honestly don't know so just checking.

Yes make sense an additional check imho.

Theoretically line 46 covers this. Because if AWS cannot find the instance it will return a nil for reservations. I think we don't need an extra check for that but 0/5 on that.

rotator/rotator.go

model/rotate_request.go

rotator/drain.go

gabrieljackson · 2021-02-26T00:21:28Z

rotator/drain.go

+
+// getPodsForDeletion receives resource info for a node, and returns all the pods from the given node that we
+// are planning on deleting. If there are any pods preventing us from deleting, we return that list in an error.
+func getPodsForDeletion(client kubernetes.Interface, node *corev1.Node, options *DrainOptions) (pods []corev1.Pod, err error) {


Same thing with named returns here. This file appears to originate from somewhere else so I will leave changing it up to you.

@gabrieljackson drain was the most complex part of the job because its not supported natively by k8s. This code here is a combination of open shift and k8s code adapted to our needs 😅 . Missed the returns somehow, should be good now.

Makefile

stafot · 2021-02-26T08:54:53Z

rotator/rotator.go

+			return err
+		}
+
+		newNodes := newNodes(nodeHostnames, autoscalingGroup.Nodes)


Could you explain a bit the logic here? Why autoscaling.Nodes are the old nodes?

@stafot sure, autoscaling.Nodes is the only source of truth that we need. It includes a list of all the nodes that the ASG included when we decided to initiate the rotation. This is the list against which we compare and check the progress of rotation.

stafot · 2021-02-26T11:45:01Z

Makefile

-# Build variables
-COMMIT_HASH ?= $(shell git rev-parse HEAD)
-BUILD_DATE  ?= $(shell date +%FT%T%z)
-


Ideally I would like to use these on tagging, but 0/5

stafot

Really great work, which could possible useful not only for Mattermost but for whole Open Source community.
Very nice tool!!

gabrieljackson

This looks good to me! Excited to try it out.

Makefile

Szymongib

Awesome tool! 🚀
I have listed some of my more or less minor concerns.

rotator/rotator.go

Szymongib · 2021-02-27T08:03:43Z

rotator/helpers.go

+func newNodes(allNodes, oldNodes []string) []string {
+	var newNodes []string
+	for _, node := range allNodes {
+		for _, oldNode := range oldNodes {
+			if node != oldNode {
+				newNodes = append(newNodes, node)
+			}
+		}
+	}
+	return newNodes
+}


Not sure if the logic is correct here.
Lets examine the following case:

allNodes = [n1, n2, n3, n4] oldNodes = [n2,n3]

The the function will return newNodes = [n1, n1, n2, n3, n4, n4]
If I understand correctly, you aimed to achieve [n1, n4]?

Amazing catch thanks a lot. This check is not that vital for the functionality because it is used to check that ASG nodes are up but it was doing duplicate work for no reason and I missed it. I changed to to a new function :)

cmd/rotator/server.go

Szymongib · 2021-03-01T08:20:10Z

rotator/helpers.go

+		return err
+	}
+	logger.Infof("Cluster with cluster ID %s is consisted of %d Autoscaling Groups", cluster.ClusterID, len(asgs))
+	var autoscalingGroups []AutoscalingGroup


Is this var needed? I see you collect all autoscaling groups in it but I do not see it being used after that.

Szymongib · 2021-03-01T08:29:28Z

k8s/helpers.go

+		if err != nil && k8sErrors.IsNotFound(err) {
+			logger.Infof("Node %s not found, waiting...", nodeName)
+		}


I wonder if it wouldn't be useful to also log the error if it is different than NotFound as in this case we won't know about it.

Split it to log the error if its not NotFound. It won't return the error as we want the timeout to handle any break.

Szymongib · 2021-03-01T08:42:20Z

rotator/rotator.go

+		}
+
+		if len(autoscalingGroup.Nodes) > 0 {


Shouldn't we pop nodes from autoscalingGroup here? Or are they removed somewhere else?

For workers we are popping the nodes in the Drain function. The reason for that is that in a group of worker nodes that are being rotated we want to drain/terminate/pop each one individually so it is done there.

Szymongib · 2021-03-01T08:56:18Z

rotator/helpers.go

+			err = Drain(clientset, []*corev1.Node{node}, drainOptions)
+			if err != nil {
+				if attempts--; attempts > 0 {
+					logger.Infof("Node %s drain failed, retrying...", nodeToDrain)
+					autoscalingGroup.DrainNodes([]string{nodeToDrain}, attempts-1, gracePeriod, wait, clientset, logger, nodeType)
+				} else {
+					return errors.Wrapf(err, "Failed to drain node %s", nodeToDrain)
+				}
+			}


I think it would be better to use loop here instead of recursion. Also I think that we reduce attempts here twice with attempts-- in line 90 and then passing attempts-1 to the DrainNodes function. In case of attempts = 3 there would actually be 2 attempts.

It could be rafactored to something like that (disclaimer: I did not test this code 😄 ):

err = Drain(clientset, []*corev1.Node{node}, drainOptions) for i := 1; i<attempts && err != nil; i++ { logger.Warnf("Failed to drain node %q on attempt %d, retrying up to %d times", nodesToDrain, i, attempts) err = Drain(clientset, []*corev1.Node{node}, drainOptions) } if err != nil { return errors.Wrapf(err, "Failed to drain node %s", nodeToDrain) }

Nice and clean, updated. Thanks a lot @Szymongib .

Makefile

Szymongib · 2021-03-01T09:22:07Z

Makefile

+	docker build \
+	--build-arg DOCKER_BUILD_IMAGE=$(DOCKER_BUILD_IMAGE) \
+	--build-arg DOCKER_BASE_IMAGE=$(DOCKER_BASE_IMAGE) \
+	. -f build/Dockerfile \


There actually is no Dockerfile, did you intend to add it in later PR?

Yeap that was my plan, but I already wrote about this in the readme so I added it now ;)

Makefile

Szymongib

Great work!

stylianosrigas added 5 commits February 16, 2021 18:30

First node rotator commit

65698ac

Code cleanup

001b621

Transform node rotator into package

ee8a00e

Code improvements, abbility to call as individual package

7734a75

Renaming node-rotator to rotator and adding readme

5e1a4be

stylianosrigas changed the title ~~WIP: Node rotator~~ Rotator tool to accelerate k8s cluster upgrades and node rotations. Feb 25, 2021

stylianosrigas requested review from Szymongib, stafot, angeloskyratzakos and gabrieljackson February 25, 2021 13:09

stylianosrigas added the 2: Dev Review Requires review by a core committer label Feb 25, 2021

stylianosrigas mentioned this pull request Feb 25, 2021

[MM-30932] Adding support for Rotator tool in cluster upgrades. mattermost/mattermost-cloud#423

Merged

gabrieljackson reviewed Feb 26, 2021

View reviewed changes

stafot reviewed Feb 26, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

stafot reviewed Feb 26, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

stafot reviewed Feb 26, 2021

View reviewed changes

Code improvements, cleanup and nits

bd0bd7f

stylianosrigas requested review from gabrieljackson and stafot February 26, 2021 11:32

stafot reviewed Feb 26, 2021

View reviewed changes

stafot approved these changes Feb 26, 2021

View reviewed changes

gabrieljackson approved these changes Mar 1, 2021

View reviewed changes

Makefile Outdated Show resolved Hide resolved

Szymongib reviewed Mar 1, 2021

View reviewed changes

Code improvements and cleanup

e08ffc4

stylianosrigas requested a review from Szymongib March 1, 2021 11:36

Szymongib approved these changes Mar 1, 2021

View reviewed changes

stylianosrigas merged commit eccb2da into main Mar 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rotator tool to accelerate k8s cluster upgrades and node rotations. #1

Rotator tool to accelerate k8s cluster upgrades and node rotations. #1

stylianosrigas commented Feb 16, 2021 •

edited

Loading

stylianosrigas commented Feb 25, 2021

gabrieljackson left a comment

gabrieljackson Feb 25, 2021

stafot Feb 26, 2021

stylianosrigas Feb 26, 2021

gabrieljackson Feb 26, 2021

stylianosrigas Feb 26, 2021

stafot Feb 26, 2021

stylianosrigas Feb 26, 2021

stafot Feb 26, 2021

stafot left a comment

gabrieljackson left a comment

Szymongib left a comment

Szymongib Feb 27, 2021

stylianosrigas Mar 1, 2021

Szymongib Mar 1, 2021

Szymongib Mar 1, 2021

stylianosrigas Mar 1, 2021

Szymongib Mar 1, 2021

stylianosrigas Mar 1, 2021

Szymongib Mar 1, 2021

stylianosrigas Mar 1, 2021

Szymongib Mar 1, 2021

stylianosrigas Mar 1, 2021

Szymongib left a comment

Rotator tool to accelerate k8s cluster upgrades and node rotations. #1

Rotator tool to accelerate k8s cluster upgrades and node rotations. #1

Conversation

stylianosrigas commented Feb 16, 2021 • edited Loading

stylianosrigas commented Feb 25, 2021

gabrieljackson 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

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

stafot left a comment

Choose a reason for hiding this comment

gabrieljackson left a comment

Choose a reason for hiding this comment

Szymongib 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

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

Choose a reason for hiding this comment

Szymongib left a comment

Choose a reason for hiding this comment

stylianosrigas commented Feb 16, 2021 •

edited

Loading