Fix the issue that the VMSS upgrading doesn't work if the topology do… #3665
Conversation
…esn't have master nodes
ghost
assigned 
| @@ -383,6 +385,43 @@ func (ku *Upgrader) upgradeAgentPools(ctx context.Context) error { | |||
| } | |||
|
|
|||
| func (ku *Upgrader) upgradeAgentScaleSets(ctx context.Context) error { | |||
| if len(ku.ClusterTopology.AgentPoolScaleSetsToUpgrade) != 0 { | |||
There was a problem hiding this comment.
This should probably be > 0, right?
There was a problem hiding this comment.
thanks for the comments, I will change it to > 0 in next iteration.
There was a problem hiding this comment.
changed '!= 0' to '> 0' according to the comment
Codecov Report
@@ Coverage Diff @@
## master #3665 +/- ##
==========================================
- Coverage 55.72% 55.57% -0.15%
==========================================
Files 108 108
Lines 16026 16049 +23
==========================================
- Hits 8930 8919 -11
- Misses 6329 6363 +34
Partials 767 767 |
|
|
||
| random := rand.New(rand.NewSource(time.Now().UnixNano())) | ||
| deploymentSuffix := random.Int31() | ||
| deploymentName := fmt.Sprintf("agentscaleset-%s-%d", time.Now().Format("06-01-02T15.04.05"), deploymentSuffix) |
There was a problem hiding this comment.
time.Now().Format("06-01-02T15.04.05") why is the timestamp hardcoded here?
There was a problem hiding this comment.
This is not hardcoded timestamp, it defines a format to change the time.Now() to string, e.g. 18-08-12T22.33.17.
This new line just follows the existing pattern used on Line 59 in pkg\operations\kubernetesupgrade\upgradeagentnode.go
deploymentName := fmt.Sprintf("master-%s-%d", time.Now().Format("06-01-02T15.04.05"), deploymentSuffix)
| @@ -383,6 +385,43 @@ func (ku *Upgrader) upgradeAgentPools(ctx context.Context) error { | |||
| } | |||
|
|
|||
| func (ku *Upgrader) upgradeAgentScaleSets(ctx context.Context) error { | |||
| if len(ku.ClusterTopology.AgentPoolScaleSetsToUpgrade) > 0 { | |||
| // need to apply the ARM template with target kubernets version to the VMSS first in order that the new VMSS instances | |||
There was a problem hiding this comment.
kubernets --> Kubernetes
There was a problem hiding this comment.
Nice catch, thanks. Fix it in commit 3.
| if len(ku.ClusterTopology.AgentPoolScaleSetsToUpgrade) > 0 { | ||
| // need to apply the ARM template with target Kubernetes version to the VMSS first in order that the new VMSS instances | ||
| // created can get the expected Kubernetes version. Otherwise the new instances created still have old Kubernetes version | ||
| // if the topology dosn't have master nodes(so there is no ARM deployments in previous upgradeMasterNodes step) |
There was a problem hiding this comment.
the last line should be if the topology doesn't have master nodes (so there are no ARM deployments in previous upgradeMasterNodes step)
There was a problem hiding this comment.
good catch, thank you. Changed in commit 4.
|
/lgtm pending vmss upgrade tests (out of band) |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chengliangli0918, 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:
Approvers can indicate their approval by writing |
…esn't have master nodes
What this PR does / why we need it:
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close that issue when PR gets merged): fixes #Special notes for your reviewer:
If applicable:
Release note: