fix: persist VMSS name in API model #4051
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4051 +/- ##
==========================================
+ Coverage 73.22% 73.24% +0.02%
==========================================
Files 135 135
Lines 20640 20640
==========================================
+ Hits 15113 15118 +5
+ Misses 4550 4545 -5
Partials 977 977
Continue to review full report at Codecov.
|
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
jackfrancis
force-pushed
the
windows-addpool
branch
from
bfbcdde to
66182d7
Compare
| } | ||
|
|
||
| if vmss.Sku != nil { | ||
| sc.newDesiredAgentCount = int(*vmss.Sku.Capacity) | ||
| uc.agentPool.Count = sc.newDesiredAgentCount | ||
| break |
There was a problem hiding this comment.
Why is the additional break needed here and wasn't before? Looks like it will stop processing and kick you out of the for loop.
There was a problem hiding this comment.
Right, so basically the current implementation enumerates through every VMSS no matter what. Even after it finds the VMSS that matches the pool in the API model and updates the scale command instance to reflect the current VMSS capacity (because aks-engine update is a aks-engine scale under the hood that does not change the instance count) and updates the API model (a convenience to reflect in the apimodel.JSON what the new count is at the time we run this operation), we keep enumerating through the VMSS array if there are more.
There is always only going to be one matching VMSS, so once we find it, we should break out of the enumeration and not waste any more cycles.
So yeah, this is just us improving things since we're mucking around in this area anyways.
Hope that makes sense. (Also definitely check that my reasoning is sound!)
There was a problem hiding this comment.
sounds about right just wanted to make sure it was intentional :-)
Are there other checks in place that affirm always only going to be one matching VMSS? Is there any chance that something could go wrong and there are two which would lead to strange behavior?
There was a problem hiding this comment.
It does look like scale is equivalently inefficient:
https://github.com/Azure/aks-engine/blob/release-v0.58.0/cmd/scale.go#L410
There was a problem hiding this comment.
Azure VMSS API will guarantee uniquess of VMSS names in a resource group; here's how we enforce that in aks-engine:
https://github.com/Azure/aks-engine/blob/release-v0.58.0/pkg/api/vlabs/validate.go#L516
| @@ -441,6 +436,7 @@ func (sc *scaleCmd) run(cmd *cobra.Command, args []string) error { | |||
|
|
|||
| currentNodeCount = int(*vmss.Sku.Capacity) | |||
| highestUsedIndex = 0 | |||
| break | |||
There was a problem hiding this comment.
@jsturtevant added break here as well to be consistent
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jackfrancis, mboersma 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 |
Reason for Change:
This PR adds a new
VMSSNameproperty to theAgentPoolProfiletype struct, to persist the definitive VMSS name (when the node pool is VMSS) for future cluster operations.The VMSS name is a computed property that depends in part upon the particular index value of the pool in the API model
agentPoolProfilesarray at the time of cluster creation; but over time that order may change (for example a pool may be removed, introducing a "hole" in the sequence, and effectively decrementing each pool's index by one). This solution continues to rely upon this undesirable solution (let's not throw away the baby with the bath water), but it does so only one time during a cluster operation when the API model data structure is stable, and once the property is computed it is persisted for all time in the API model to match the actual VMSS "name" property, which will remain stable and unique so long as the VMSS exists in the resource group.This also fixes a particular edge case w/ respect to
aks-engine addpooland Windows node pools (theaddpooloperation always operates against a singleAgentPoolProfile(the new one), and thus the index is always zero: which means if the cluster was created with similarly named Windows pool at that index originally, theaddpoolcommand will produce an ARM template that actually modifies that existing pool, rather than adding an entirely new one.Issue Fixed:
Fixes #4055
Credit Where Due:
Does this change contain code from or inspired by another project?
If "Yes," did you notify that project's maintainers and provide attribution?
Requirements:
Notes: