feat: add force parameter to the upgrade command

[serbrech]

Is this a request for help?:
No

---

Is this an ISSUE or FEATURE REQUEST? (choose one):
Feature Proposal

---

description:
We want to be able to force the upgrade process on a cluster without changing the kubernetes version. This will ensure that all components of the cluster are healthy and using the latest validated version for a given kubernetes version.

proposed cli:
For an existing kubernetes 1.12.5 cluster, we want the following command to go through each nodes of the cluster and "refresh" them if necessary (update to latest valid version).

> aks-engine upgrade --force --location westus --resource-group rg --subsctiption $sub --upgrade-version 1.12.5

Open questions:

• What should the behavior of the --force flag be for a usual next k8s version upgrade?
  Same behavior

• What happens for same-version upgrade?
  A: Currently, when a node is already on the requested version, the upgrade either fail validation, or the node upgrade ends up being a no-op. --force runs the upgrade process on these nodes as well

• Should the --force flag allow other arbitrary upgrade (not valid per our get-versions matrix)?
  A : yes, using --force will allow any arbitrary version jump, or even downgrade. You're on your own.

Related
implements #522
Azure/acs-engine#3810

[codecov]

Codecov Report

Merging #525 into master will increase coverage by 0.58%.
The diff coverage is 68%.

@@            Coverage Diff             @@
##           master     #525      +/-   ##
==========================================
+ Coverage   56.69%   57.28%   +0.58%     
==========================================
  Files          91       91              
  Lines       13905    13938      +33     
==========================================
+ Hits         7884     7984     +100     
+ Misses       5355     5284      -71     
- Partials      666      670       +4

[jackfrancis]

This is going in the right direction. The business logic implementation should be focussed on doing actual operations, and should be decomposed in such a way so as to be general and composable. Validation should happen at a higher layer, and should inform particular implementation composition patterns. (As opposed to the validation being statically coupled to the business logic itself.)

[CecileRobertMichon]

Did an online review with Stephane. To summarize:

1. We need to make sure that the "retry failed upgrade" functionality still works without the current version in the list of available upgrades in cmd validation.

2. Separating the PR in smaller chunks to permit easier review and faster revert if necessary might be a good idea but will leave it up to Stephane to decide if the trade off is worth the additional work to separate.

3. Removing the upgradable validation in upgradeCluster reduces the strength of validation:

• the cmd validation only checks if the upgrade is valid based on the version string in the apimodel which does not necessarily reflect the actual state of things and can be easily modified by the user
• if an upgrade fails, we don't have in place validation to check that the user retries upgrade to the same version which means that the following could happen:
  User starts upgrade of 1.9.5 cluster to 1.10.7. Upgrade fails leaving some nodes in 1.10.7 and some in 1.9.5. User upgrades to 1.9.6: this will pass validation because the apimodel will still have version 1.9.5 (and 1.9.5 -> 1.9.6 is a valid upgrade) but will cause a downgrade of two of the nodes. Per-node validation would have previously caught this invalid downgrade operation but removing per-node validation makes us vulnerable in such scenarios.

[tariq1890]

lgtm

[CecileRobertMichon]

lgtm, one question about unit tests being maintainable with version deprecation in the future

[serbrech]

Good point, I considered it but didn't fix it. For --force, this has no incidence as it should just always pass. for not --force, the base data we rely on for the test will be changing. The only way to fix this is to fake the version/valid upgrade table. Can we add it as a //TODO in the backlog, I can look into it before the end of the week.

…

On Thu, Feb 28, 2019 at 11:10 AM Cecile Robert-Michon < ***@***.***> wrote: ***@***.**** approved this pull request. lgtm, one question about unit tests being maintainable with version deprecation in the future — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub <#525 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AATRze-zyLWC_D94eD6XBovoqGwNSDG-ks5vSCm5gaJpZM4a-ksm> .

[CecileRobertMichon]

why not use common.GetLatestPatchVersion to dynamically set the version?

[serbrech]

why not use common.GetLatestPatchVersion to dynamically set the version?

I think there is an easier way :
common.AllKubernetesSupportedVersions is public, so I can simply set it to a minimal map that makes sense for the test case. :)

[CecileRobertMichon]

/lgtm

[acs-bot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

[CecileRobertMichon]

merging manually because windows 1.13 test failure was a flake.