feat: Make cordon drain timeout configurable with --upgrade #1276
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: aramase If they are not already assigned, you can assign the PR to them by writing 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:
Approvers can indicate their approval by writing |
aramase
changed the title
Codecov Report
@@ Coverage Diff @@
## master #1276 +/- ##
==========================================
- Coverage 74.76% 74.73% -0.03%
==========================================
Files 128 128
Lines 18333 18348 +15
==========================================
+ Hits 13706 13713 +7
- Misses 3836 3843 +7
- Partials 791 792 +1 |
aramase
changed the title
|
@CecileRobertMichon Had to fix unit tests. Let me know what you think. |
cmd/upgrade.go
Outdated
| @@ -70,6 +72,7 @@ func newUpgradeCmd() *cobra.Command { | |||
| f.StringVar(&uc.deploymentDirectory, "deployment-dir", "", "the location of the output from `generate`") | |||
| f.StringVarP(&uc.upgradeVersion, "upgrade-version", "k", "", "desired kubernetes version (required)") | |||
| f.IntVar(&uc.timeoutInMinutes, "vm-timeout", -1, "how long to wait for each vm to be upgraded in minutes") | |||
| f.IntVar(&uc.cordonDrainTimeoutInMinutes, "cordon-drain-timeout", 20, "how long to wait for each vm to be cordoned in minutes") | |||
There was a problem hiding this comment.
let's move the default logic to the upgrader code (similar to how timeoutInMinutes is doing it) because AKS also calls upgrader so we want to make sure that the timeout isn't nil for them.
In other words, let's set the arg default to -1 and only set uc.cordonDrainTimeout if the timeout != -1. Then in the upgrader, do something like https://github.com/Azure/aks-engine/blob/master/pkg/operations/kubernetesupgrade/upgrader.go#L261.
We could arguably leave the 20 default here as well instead of setting it to -1 but having a default timeout const in one place will make it easier to change the default in the future.
There was a problem hiding this comment.
Yeah, I was wondering why the default was being set there instead of here for vm upgrade timeout. Now I know 😊 I’ll make the change.
There was a problem hiding this comment.
I've made the changes to set the default value in upgrader.
aramase
force-pushed
the
cordondraintimeout
branch
from
f3020be to
0f285b9
Compare
| @@ -497,6 +505,11 @@ func (ku *Upgrader) upgradeAgentScaleSets(ctx context.Context) error { | |||
|
|
|||
| ku.logger.Infof("Successfully set capacity for VMSS %s", vmssToUpgrade.Name) | |||
|
|
|||
| cordonDrainTimeout := defaultCordonDrainTimeout | |||
There was a problem hiding this comment.
Let's settle on a common "use default value if not provided" pattern. Lines 269-273 are doing the same thing, it seems, but in a sort of inverse way.
|
lgtm pending upgrade validation thanks @aramase! |
Reason for Change:
Make the cordon drain timeout value configurable in
--upgradecommand.Issue Fixed:
Fixes #950
Requirements:
Notes: