fix: Windows + VMSS external svc routing broken #1028
Conversation
CecileRobertMichon
changed the title
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
@adelina-t the unit tests failed on the lint step. |
|
Will check. Thank you! |
|
These test failures are real, looking into them |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
The VMSSs for Linux and Windows agentpools follow a different naming patterns. However, when populating azure.json, aks-engine does not take that into account and we end up with the wrong name for the primareyScaleSetName variable. Thus, the LB created for service will have a empty backend pool, as there isn't a VMSS with the desired name. Properly populating the ARM variable for the primaryScaleSet will fix this issue. Fixes: Azure#809
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -898,12 +898,22 @@ func (p *Properties) GetNSGName() string { | |||
|
|
|||
| // GetPrimaryAvailabilitySetName returns the name of the primary availability set of the cluster | |||
| func (p *Properties) GetPrimaryAvailabilitySetName() string { | |||
There was a problem hiding this comment.
@xizhamsft this method is called to populate AKS metadata for the cluster record. It doesn't make sense that we'd have a static value for PrimaryAvailabilitySetName when there is no AvailabilitySet in the cluster. Does changing this method to return an empty string have any breaking effects on the AKS VMSS scenario?
| masterVars["vmType"] = "standard" | ||
| } | ||
| masterVars["primaryScaleSetName"] = cs.Properties.GetPrimaryScaleSetName() |
There was a problem hiding this comment.
We can just set this variable using the getter now that the getter returns an empty string if the primary agent pool is not vmss.
| @@ -124,7 +124,7 @@ func TestK8sVars(t *testing.T) { | |||
| "nsgName": "[concat(variables('masterVMNamePrefix'), 'nsg')]", | |||
| "orchestratorNameVersionTag": "Kubernetes:" + testK8sVersion, | |||
| "primaryAvailabilitySetName": "", | |||
| "primaryScaleSetName": "[concat(parameters('orchestratorName'), '-agentpool1-',parameters('nameSuffix'), '-vmss')]", | |||
| "primaryScaleSetName": cs.Properties.GetPrimaryScaleSetName(), | |||
There was a problem hiding this comment.
In other words, we're validating that the value of the ARM variable primaryScaleSetName derives from the canonical primaryScaleSetName() method, not from an ARM expression string.
| @@ -968,12 +968,6 @@ func (t *TemplateGenerator) getTemplateFuncMap(cs *api.ContainerService) templat | |||
| "GetMasterEtcdClientPort": func() int { | |||
| return DefaultMasterEtcdClientPort | |||
| }, | |||
| "GetPrimaryAvailabilitySetName": func() string { | |||
There was a problem hiding this comment.
These template layer functions are never called.
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run pr-e2e |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Codecov Report
@@ Coverage Diff @@
## master #1028 +/- ##
==========================================
+ Coverage 74.51% 74.57% +0.06%
==========================================
Files 131 131
Lines 17963 17969 +6
==========================================
+ Hits 13385 13401 +16
+ Misses 3829 3820 -9
+ Partials 749 748 -1 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adelina-t, 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 |
* fix: "Windows + VMSS external svc routing broken" The VMSSs for Linux and Windows agentpools follow a different naming patterns. However, when populating azure.json, aks-engine does not take that into account and we end up with the wrong name for the primareyScaleSetName variable. Thus, the LB created for service will have a empty backend pool, as there isn't a VMSS with the desired name. Properly populating the ARM variable for the primaryScaleSet will fix this issue. Fixes: Azure#809 * chore: fix unit tests, protect against nil dereference * chore: more tests, get rid of dead code, rationalize GetPrimaryAvailabilitySetName * chore: lint * test: fix unit tests
The VMSSs for Linux and Windows agentpools follow a different
naming patterns. However, when populating azure.json, aks-engine
does not take that into account and we end up with the wrong name
for the primareyScaleSetName variable. Thus, the LB created for
service will have a empty backend pool, as there isn't a
VMSS with the desired name.
Properly populating the ARM variable for the primaryScaleSet will
fix this issue.
Fixes: #809
Reason for Change:
Issue Fixed:
Requirements:
Notes: