refactor: upgrade code refactor part 1 #287

CecileRobertMichon · 2019-01-11T00:41:04Z

Reason for Change:

This is an initial refactor of the upgrade code. More to follow.

Issue Fixed:

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:
TODO:

Add unit tests
Run upgrade back compat tests

cmd/upgrade.go

pkg/operations/kubernetesupgrade/upgradecluster.go

jackfrancis · 2019-01-11T00:56:05Z

pkg/operations/kubernetesupgrade/upgradecluster.go

-// getClusterNodeVersion returns a node's "orchestrator:version" via Kubernetes API or VM tag.
-func (uc *UpgradeCluster) getClusterNodeVersion(client armhelpers.KubernetesClient, name string, tags map[string]*string) string {
+// getNodeVersion returns a node's current Kubernetes version via Kubernetes API or VM tag.
+func (uc *UpgradeCluster) getNodeVersion(client armhelpers.KubernetesClient, name string, tags map[string]*string) string {


Can we reverse the precedence order here? Shouldn't we prefer to check the k8s API first, and then fall back to the tag?

This is the same behavior that was implemented in @mboersma's PR to stop requiring tags Azure/acs-engine#4100 so I'll defer to him for choosing this approach over the other.

I think the idea was that we maintain existing behavior to limit risk and only use the alternative method if we can't find the tags.

If we do decide we want to reverse the order, I would argue that's a behavior change and it doesn't belong in this refactor PR (which is already dangerous enough as is).

I agree with Cécile's comment here. I think it's safer to leave it alone.

This way the existing behavior is still the default (use tags), and the k8s API is explicitly a fallback strategy. At the time, that was deemed to be less disruptive, but it has proved to be useful in failed upgrade cases: setting the tags is a safe way to "trick" the upgrade behavior to run again.

jackfrancis · 2019-01-11T01:01:35Z

Is this an appropriate PR to include consideration of the following changes:

implement "upgrade single node" as a composable unit
expose above as an option for a user

Use cases:

"I want to upgrade a single node and schedule some stuff onto it, do a/b testing with non-upgraded nodes and compare"
"My upgrade failed halfway through and I want to cleanup my cluster by manually upgrading the remaining nodes that are running an old version"

CecileRobertMichon · 2019-01-11T01:04:51Z

@jackfrancis short answer is no.

This is a refactor PR and the first of many. I tried to break down the changes so that they are easier to review/test. Adding new features is out of the scope of this PR.

We can definitely keep that in the backlog as a future addition though.

CecileRobertMichon · 2019-01-11T01:12:53Z

To clarify for reviewers: there are many more things I want to change/improve in the upgrade code area, this is only the beginning. I am breaking down the changes in smaller units of code changes to make it more manageable to review and limit risk of breakage since upgrade is a complicated/sensitive part of the code. Trying to change that, one step at a time :)

jackfrancis · 2019-01-12T00:21:37Z

cmd/upgrade.go

-	uc.agentPoolsToUpgrade = []string{}
-	log.Infoln(fmt.Sprintf("Gathering agent pool names..."))
+	uc.agentPoolsToUpgrade = make(map[string]bool)
+	uc.agentPoolsToUpgrade[kubernetesupgrade.MasterPoolName] = true


Is this additive? I don't see us adding the master in the previous agentPoolsToUpgrade string slice pattern

maybe I should rename this to poolsToUpgrade - the agentPoolsToUpgrade name doesn't really make sense

cmd/upgrade.go

jackfrancis · 2019-01-12T00:29:39Z

pkg/operations/kubernetesupgrade/upgradecluster.go

-	for _, poolName := range agentPoolsToUpgrade {
-		uc.AgentPoolsToUpgrade[poolName] = true
-	}
-	uc.AgentPoolsToUpgrade[MasterPoolName] = true


I think I see now what you did above.

yeah I just copied existing behavior but I don't really understand what AgentPoolsToUpgrade is used for yet

CecileRobertMichon · 2019-01-14T21:32:38Z

Scenarios tested so far:

✅ Windows 1.8 cluster with VMAS
✅ Linux 1.10 cluster with VMAS
🔴 Linux 1.12 cluster with VMSS agents (VMSS master upgrade is non-functional in current master)
✅ Linux 1.7 cluster with VMAS

CecileRobertMichon · 2019-01-14T21:39:37Z

pkg/operations/kubernetesupgrade/upgradecluster.go

 	if client != nil {
 		node, err := client.GetNode(name)
 		if err == nil {
-			return api.Kubernetes + ":" + strings.TrimPrefix(node.Status.NodeInfo.KubeletVersion, "v")


here we return just the k8s version instead of kubernetes:v1.x.x since this is in the kubernetesupgrade package, we only care about the version - not the orchestrator.

CecileRobertMichon · 2019-01-14T21:40:34Z

pkg/operations/kubernetesupgrade/upgradecluster.go

 		return uc.Translator.Errorf("Error while querying ARM for resources: %+v", err)
 	}

 	var upgrader UpgradeWorkFlow
 	upgradeVersion := uc.DataModel.Properties.OrchestratorProfile.OrchestratorVersion
 	uc.Logger.Infof("Upgrading to Kubernetes version %s\n", upgradeVersion)
-	switch {
-	case strings.HasPrefix(upgradeVersion, "1.6."):


if anyone knows a good reason why these separate upgraders are still needed, please speak up

👍 as far as I can tell, these structs provide no utility over the Upgrader base.

Maybe someone was anticipating we would need different behavior for each version upgrade? (Don't do that in advance because YAGNI.)

CecileRobertMichon · 2019-01-15T01:10:57Z

VMSS upgrade is failing with Error upgrading cluster: Drain did not complete within 1m0s

#298 might help

codecov · 2019-01-18T20:10:08Z

Codecov Report

Merging #287 into master will increase coverage by 0.02%.
The diff coverage is 43.75%.

@@            Coverage Diff            @@
##           master    #287      +/-   ##
=========================================
+ Coverage   53.17%   53.2%   +0.02%     
=========================================
  Files          95      95              
  Lines       14247   14216      -31     
=========================================
- Hits         7576    7563      -13     
+ Misses       6006    5988      -18     
  Partials      665     665

jackfrancis · 2019-01-19T00:50:20Z

pkg/operations/kubernetesupgrade/upgradecluster.go

@@ -204,41 +164,37 @@ func (uc *UpgradeCluster) getClusterNodeStatus(subscriptionID uuid.UUID, az armh
 		}

 		for _, vm := range vmListPage.Values() {
-			vmOrchestratorTypeAndVersion := uc.getClusterNodeVersion(kubeClient, *vm.Name, vm.Tags)
-			if vmOrchestratorTypeAndVersion == "" {
+			// Windows VMs contain a substring of the name suffix


What is the Windows-specific implications of the line(s) below?

strings.Contains(*(vm.Name), uc.NameSuffix[:4]+"k8s" is checking for VM naming specific to windows instead of looking for the whole name suffix

jackfrancis · 2019-01-19T00:54:40Z

pkg/operations/kubernetesupgrade/upgradecluster.go

@@ -394,28 +345,3 @@ func (uc *UpgradeCluster) addVMToAgentPool(vm compute.VirtualMachine, isUpgradab

 	return nil
 }
-


I actually kinda think we should keep this in, uncomment it, and standardize to persisting the template that we deploy to upgrade always.

But perhaps for a later exercise.

We already have logic for saving the modified template in upgrade.go https://github.com/CecileRobertMichon/aks-engine/blob/0b7a9702300ed5d033874a6ced2d2a140867fe18/cmd/upgrade.go#L281 so this commented out code is redundant.

I think we need to improve the logic for persisting templates and instead of passing them to a file, saving them in the RG or something like that but, like you said, for a later exercise.

jackfrancis · 2019-01-22T22:20:57Z

/lgtm

acs-bot · 2019-01-22T22:21:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon, jackfrancis

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~~ [CecileRobertMichon,jackfrancis]

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

refactor: some more upgradeCluster() refactoring refactor: getClusterNodeStatus fix: s/nodes/VMs fix: s/subset/substring fix: ClusterTopology is in kubernetesupgrade package refactor: simplify upgradable() func refactor: move upgradecluster to instance literal Revert "refactor: move upgradecluster to instance literal" This reverts commit ac6aacd. refactor: fix upgradeCluster parameters in unit tests fix import remove import cycle fix unit test error string

acs-bot · 2019-01-22T23:11:38Z

New changes are detected. LGTM label has been removed.

acs-bot added size/L approved labels Jan 11, 2019

jackfrancis reviewed Jan 11, 2019

View reviewed changes

cmd/upgrade.go Outdated Show resolved Hide resolved

CecileRobertMichon commented Jan 11, 2019

View reviewed changes

pkg/operations/kubernetesupgrade/upgradecluster.go Show resolved Hide resolved

CecileRobertMichon changed the title ~~refactor: upgrade code refactor part 1~~ [WIP] refactor: upgrade code refactor part 1 Jan 11, 2019

acs-bot added the do-not-merge/work-in-progress label Jan 11, 2019

CecileRobertMichon force-pushed the upgrade-refactor branch from 7ae3dba to 2849aea Compare January 11, 2019 00:54

jackfrancis reviewed Jan 11, 2019

View reviewed changes

jackfrancis reviewed Jan 12, 2019

View reviewed changes

cmd/upgrade.go Show resolved Hide resolved

jackfrancis reviewed Jan 12, 2019

View reviewed changes

CecileRobertMichon changed the title ~~[WIP] refactor: upgrade code refactor part 1~~ refactor: upgrade code refactor part 1 Jan 14, 2019

acs-bot removed the do-not-merge/work-in-progress label Jan 14, 2019

CecileRobertMichon commented Jan 14, 2019

View reviewed changes

CecileRobertMichon added the awaiting review label Jan 18, 2019

jackfrancis reviewed Jan 19, 2019

View reviewed changes

acs-bot assigned jackfrancis Jan 22, 2019

acs-bot added the lgtm label Jan 22, 2019

CecileRobertMichon force-pushed the upgrade-refactor branch from 14519b2 to 5795eea Compare January 22, 2019 23:11

acs-bot removed the lgtm label Jan 22, 2019

CecileRobertMichon merged commit 065ccd3 into Azure:master Jan 23, 2019

juhacket pushed a commit to juhacket/aks-engine that referenced this pull request Mar 14, 2019

refactor: upgrade code refactor (Azure#287)

d9a7b7f

CecileRobertMichon deleted the upgrade-refactor branch April 18, 2019 22:42

mboersma removed the awaiting review label Apr 29, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: upgrade code refactor part 1 #287

refactor: upgrade code refactor part 1 #287

CecileRobertMichon commented Jan 11, 2019 •

edited

Loading

jackfrancis Jan 11, 2019

CecileRobertMichon Jan 11, 2019

mboersma Jan 14, 2019

jackfrancis commented Jan 11, 2019

CecileRobertMichon commented Jan 11, 2019

CecileRobertMichon commented Jan 11, 2019

jackfrancis Jan 12, 2019

CecileRobertMichon Jan 12, 2019

jackfrancis Jan 12, 2019

CecileRobertMichon Jan 14, 2019

CecileRobertMichon commented Jan 14, 2019 •

edited

Loading

CecileRobertMichon Jan 14, 2019

CecileRobertMichon Jan 14, 2019

mboersma Jan 14, 2019

CecileRobertMichon commented Jan 15, 2019

codecov bot commented Jan 18, 2019 •

edited

Loading

jackfrancis Jan 19, 2019

CecileRobertMichon Jan 22, 2019

jackfrancis Jan 19, 2019

CecileRobertMichon Jan 22, 2019

jackfrancis commented Jan 22, 2019

acs-bot commented Jan 22, 2019

acs-bot commented Jan 22, 2019

		@@ -394,28 +345,3 @@ func (uc *UpgradeCluster) addVMToAgentPool(vm compute.VirtualMachine, isUpgradab

		return nil
		}

refactor: upgrade code refactor part 1 #287

refactor: upgrade code refactor part 1 #287

Conversation

CecileRobertMichon commented Jan 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackfrancis commented Jan 11, 2019

CecileRobertMichon commented Jan 11, 2019

CecileRobertMichon commented Jan 11, 2019

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

CecileRobertMichon commented Jan 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CecileRobertMichon commented Jan 15, 2019

codecov bot commented Jan 18, 2019 • edited Loading

Codecov Report

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

jackfrancis commented Jan 22, 2019

acs-bot commented Jan 22, 2019

acs-bot commented Jan 22, 2019

CecileRobertMichon commented Jan 11, 2019 •

edited

Loading

CecileRobertMichon commented Jan 14, 2019 •

edited

Loading

codecov bot commented Jan 18, 2019 •

edited

Loading