Implement core scripts and logic for running upgrades #211

xmudrii · 2019-02-22T12:19:45Z

What this PR does / why we need it:

implement scripts for upgrading kubeadm and kubelet (cluster upgrade scripts #198),
implements scripts for invoking kubeadm upgrade on both leader and follower nodes (cluster upgrade scripts #198),
applies kubeone.io/upgrade-in-progress label before starting the upgrade process,
removes kubeone.io/upgrade-in-progress label after successfully finishing the upgrade process,
adds function for determining the hostname (ported from installer),
slightly refactors the code to changes made in use github.com/pkg/errors #210

Breaking changes: I propose to rename the kubeone.io/upgrading-in-process label to kubeone.io/upgrade-in-progress because that seems more grammatically correct.

This PR is supposed to be merged once we have scripts for upgrading packages in the place, until then
/hold

This PR is based on the Kubernetes documentation: Upgrading kubeadm HA clusters from v1.12 to v1.13.

Questions for reviewers (check review comments for other questions):

Do we need to run kubeadm plan? To my understanding, it basically does nothing beside showing what will be changed. This can be useful with verbose, but not beside that. I done some testing and it even doesn't error if you put non-existing version or something similar. (see Implement core scripts and logic for running upgrades #211 (comment))

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #198

Release note:

NONE

/assign @kron4eg

kron4eg · 2019-02-22T12:53:07Z

AFAIK, kubeadm plan is for the actual living being to interactively see what's going to happen. Thus we don't need it.

kron4eg · 2019-02-22T14:32:08Z

It looks like it makes sense to join this PR with #199

xmudrii · 2019-02-25T12:35:13Z

/hold cancel

kron4eg · 2019-02-25T13:01:32Z

/lgtm
/approve

kubermatic-bot · 2019-02-25T13:01:34Z

LGTM label has been added.

Git tree hash: 13d2cb7982c33cd2d140e109e3aec9bace1f6bc2

kubermatic-bot · 2019-02-25T13:01:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kron4eg

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:

~~OWNERS~~ [kron4eg]

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

xmudrii

As this PR is already reviewed, this can be a follow-up if anything needs to be changed.

xmudrii · 2019-02-25T12:45:13Z

pkg/upgrader/upgrade/kubeadm_package.go

+	return err
+}
+
+func upgradeKubeadmDebian(ctx *util.Context) error {


This function could actually be a single function like upgradeKubeadmExecutor(*util.Context, string). Actually, it could be reused for kubelet upgrade as well as they have the same variables and basically do the same thing.

We have similar situation in the installer package as well.

xmudrii · 2019-02-25T12:46:08Z

pkg/upgrader/upgrade/kubelet_package.go

+	return err
+}
+
+func upgradeKubeletDebian(ctx *util.Context) error {


As mentioned in a comment above, this could be a single function for both kubeadm and kubelet

xmudrii · 2019-02-25T12:46:31Z

pkg/upgrader/upgrade/upgrade.go

@@ -7,18 +7,30 @@ import (
 )

 const (
-	labelUpgradeLock      = "kubeone.io/upgrading-in-process"
+	labelUpgradeLock      = "kubeone.io/upgrade-in-progress"


If we decide to go with this change, the proposal should be updated as well (can be done in this PR)

xmudrii · 2019-02-25T12:53:49Z

pkg/upgrader/upgrade/upgrade.go

+		{fn: determineHostname, errMsg: "unable to determine hostname"},
+		{fn: determineOS, errMsg: "unable to determine operating system"},
+		{fn: runPreflightChecks, errMsg: "preflight checks failed"},
+		{fn: upgradeLeader, errMsg: "unable to upgrade leader control plane"},


I was thinking a lot how to handle this but still unsure is this the right way..

I have two big functions in upgrade_leader.go and upgrade_follower.go that contains all tasks for leader and followers. Reason for creating a big function instead of just adding task by task here is that I was not sure how to control the process correctly.

In case of the leader, it would be quite easy, as we have only leader and just listing tasks would do the job. This might not be a case for followers. I believe that if we started upgrading one follower, we should fully finish upgrade there before proceeding to the next follower. If I'd add task by task here instead of using one big function, it might not be easy to ensure that multiple tasks will finish on the same node before doing same for another.

Of course, it's possible to use different approach, but I wanted to be consistent and not expand on too much.

xmudrii · 2019-02-25T12:55:58Z

pkg/upgrader/upgrade/util.go

+}
+
+// mergeStringMap merges two string maps into destination string map
+func mergeStringMap(modified *bool, destination *map[string]string, required map[string]string) {


We already implement this function in the template package. There are two questions: should we reuse function from another, non-upgardes related, package? Or should we instead create an util (e.g. pkg/util) package and put such functions there? Although, I'm against pkg/util because it's an anti-pattern in Go, but having packages in pkg/util, like pkg/util/merge would work.

no more utils please. and those that we already have should be dismissed, and refactored into conscientiously named packages

xmudrii · 2019-02-25T12:59:20Z

pkg/upgrader/upgrade/util.go

+	corev1types "k8s.io/client-go/kubernetes/typed/core/v1"
+)
+
+func determineHostname(ctx *util.Context) error {


determineHostname and determineOS are implemented in the installer package in a similar form. We should consider moving those functions to an utility package instead of just reimplementing them everywhere.

xmudrii · 2019-02-25T13:00:24Z

pkg/upgrader/upgrade/upgrade_leader.go

+}
+
+func upgradeLeaderExecutor(ctx *util.Context, node *config.HostConfig, conn ssh.Connection) error {
+	logger := ctx.Logger.WithField("node", node.PublicAddress)


Not sure is this needed, but I thought it would be nice to have on what node the task is executed.

xmudrii · 2019-02-25T13:00:41Z

pkg/upgrader/upgrade/upgrade_leader.go

+func upgradeLeaderExecutor(ctx *util.Context, node *config.HostConfig, conn ssh.Connection) error {
+	logger := ctx.Logger.WithField("node", node.PublicAddress)
+
+	logger.Infoln("Labeling leader control plane…")


Do we need leader in logs or just go with control plane?

xmudrii · 2019-02-25T13:01:27Z

pkg/upgrader/upgrade/upgrade_follower.go

+}
+
+func upgradeFollowerExecutor(ctx *util.Context, node *config.HostConfig, conn ssh.Connection) error {
+	ctx.Logger.Infoln("Labeling follower control plane…")


RunTaskOnFollowers automatically modifies logger to include the IP address, so we don't need to create a new logger like for upgradeLeaderExecutor.

xmudrii · 2019-02-25T13:01:45Z

pkg/upgrader/upgrade/upgrade_follower.go

+		return errors.Wrap(err, "failed to label leader control plane node")
+	}
+
+	ctx.Logger.Infoln("Upgrading kubeadm on follower control plane…")


Similar as for leader, do we need follower in logs?

xmudrii requested a review from kron4eg February 22, 2019 12:19

kubermatic-bot added the release-note-none Denotes a PR that doesn't merit a release note. label Feb 22, 2019

kubermatic-bot assigned kron4eg Feb 22, 2019

kubermatic-bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 22, 2019

xmudrii force-pushed the upgrades/implementation branch from 054683e to f6134ec Compare February 25, 2019 10:25

Implement core scripts and logic for running upgrades

cfa2df5

xmudrii force-pushed the upgrades/implementation branch from f6134ec to cfa2df5 Compare February 25, 2019 10:29

kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 25, 2019

kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2019

kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2019

kubermatic-bot merged commit 500b87e into master Feb 25, 2019

kubermatic-bot deleted the upgrades/implementation branch February 25, 2019 13:02

xmudrii commented Feb 25, 2019

View reviewed changes

kdomanski mentioned this pull request Feb 26, 2019

Implement upgrade of the cluster #1

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement core scripts and logic for running upgrades #211

Implement core scripts and logic for running upgrades #211

xmudrii commented Feb 22, 2019 •

edited

Loading

kron4eg commented Feb 22, 2019

kron4eg commented Feb 22, 2019

xmudrii commented Feb 25, 2019

kron4eg commented Feb 25, 2019

kubermatic-bot commented Feb 25, 2019

kubermatic-bot commented Feb 25, 2019

xmudrii left a comment

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

kron4eg Feb 25, 2019

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

xmudrii Feb 25, 2019

Implement core scripts and logic for running upgrades #211

Implement core scripts and logic for running upgrades #211

Conversation

xmudrii commented Feb 22, 2019 • edited Loading

kron4eg commented Feb 22, 2019

kron4eg commented Feb 22, 2019

xmudrii commented Feb 25, 2019

kron4eg commented Feb 25, 2019

kubermatic-bot commented Feb 25, 2019

kubermatic-bot commented Feb 25, 2019

xmudrii 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

xmudrii commented Feb 22, 2019 •

edited

Loading