fix: add masterSubnet to azuredeploy.parameters.json while using custom VNET #431

zhiweiv · 2019-02-02T05:01:22Z

Reason for Change:

masterSubnet is used by Windows nodes, it is missing from azuredeploy.parameters.json while using custom VNET, we can add it to azuredeploy.parameters.json manually while building cluster as a workaround, however while scaling cluster, aks-engine updates the cluster directly, we don't get change to update the template, the new added Windows nodes do not join the cluster due to missing masterSubnet. This PR make it work at least while master and agents using same custom VNET.

Issue Fixed:

Fixes #210
Fixes #341

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
tested upgrade from previous version

Notes:

…n while using custom VNET

codecov · 2019-02-02T05:17:55Z

Codecov Report

Merging #431 into master will increase coverage by 13.85%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           master     #431       +/-   ##
===========================================
+ Coverage   53.38%   67.24%   +13.85%     
===========================================
  Files          95      121       +26     
  Lines       14374    20429     +6055     
===========================================
+ Hits         7674    13737     +6063     
+ Misses       6037     5809      -228     
- Partials      663      883      +220

pkg/engine/params.go

…n while using custom VNET

tariq1890 · 2019-02-05T06:15:56Z

pkg/engine/params.go

@@ -64,6 +64,7 @@ func getParameters(cs *api.ContainerService, generatorCode string, aksEngineVers
 			}
 			if properties.OrchestratorProfile.IsKubernetes() {
 				addValue(parametersMap, "vnetCidr", properties.MasterProfile.VnetCidr)
+				addValue(parametersMap, "masterSubnet", properties.MasterProfile.VnetCidr)


Why are we not adding this in params_k8s.go ?

Note to self: Check the pre-conditions for populating VnetCidr

if properties.MasterProfile.IsCustomVNET() { addValue(parametersMap, "masterVnetSubnetID", properties.MasterProfile.VnetSubnetID) if properties.MasterProfile.IsVirtualMachineScaleSets() { addValue(parametersMap, "agentVnetSubnetID", properties.MasterProfile.AgentVnetSubnetID) } if properties.OrchestratorProfile.IsKubernetes() { addValue(parametersMap, "vnetCidr", properties.MasterProfile.VnetCidr) addValue(parametersMap, "masterSubnet", properties.MasterProfile.VnetCidr) } } else { addValue(parametersMap, "masterSubnet", properties.MasterProfile.Subnet) addValue(parametersMap, "agentSubnet", properties.MasterProfile.AgentSubnet) }

This is the whole part of related code, masterSubnet is added in else condition, I followed this rule to add my code here. Do I need to move it to params_k8s.go?

I don't think setting redundant parameters is advisable. i.e having masterSubnet and VnetCidr point to the same value. If there are references to masterSubnet in windows scenarios. They should probably reference VnetCidr.

I'll let @PatrickLang chime in as well.

The vnetCidr is not used in any condition now.

Then I believe we should change that to use VnetCidr, not duplicate values across different parameters.

There are a few parameters related to custom VNET, they are vnetCidr/kubeClusterCidr/masterSubnet/agentSubnet. vnetCidr is not used, agentSubnet is a null value. kubeClusterCidr and masterSubnet are added to azure cni's exception list only.

I guess there were some thoughts for custom VNET, but not fully implemented yet on Windows. From my side this PR is more as a workaround with minimal change to make things work.

@PatrickLang
Any ideas?

PatrickLang · 2019-02-11T22:32:48Z

/LGTM

@jsturtevant - do you have anyone looking for this functionality and if so would you be able to help document this?

acs-bot · 2019-02-11T22:33:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PatrickLang, zhiweiv
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mboersma

If they are not already assigned, you can assign the PR to them by writing /assign @mboersma in a comment when ready.

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

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

tariq1890 · 2019-02-11T22:50:13Z

/hold

stale · 2019-03-14T08:02:20Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

zhiweiv · 2019-03-14T08:29:21Z

@tariq1890 @PatrickLang @CecileRobertMichon
Do we have change to merge this to trunk, currently I am using local build with changes from this PR. It is inconvenient because I have to rebuild often to catch up new version.

CecileRobertMichon · 2019-03-14T15:49:54Z

/azp run pr-e2e

azure-pipelines · 2019-03-14T15:50:23Z

Azure Pipelines successfully started running 1 pipeline(s).

CecileRobertMichon · 2019-03-25T19:08:04Z

@tariq1890 what is still holding this PR?

tariq1890 · 2019-03-25T20:12:21Z

I am enquiring if there is a way instead of going for a workaround here. Having values duplicated across multiple parameters seems like a code/design smell to me.

zhiweiv · 2019-04-03T05:24:15Z

The PR maybe not a good solution, I will keep using local build as workaround. AKS-Engine doesn't work for Windows nodes with custom VNET by default now, hope this can be fixed sometime.

rogersollberger · 2019-05-08T11:21:12Z

Is it true, this fix is still not in the current version (v0.35.1) ? So there is currently no way to use a hybrid setup with custom vnet than to manually add the mastersubnet after aks-engine generate to azuredeploy.parameters.json?

zhiweiv · 2019-05-08T11:25:43Z

@rogersollberger
This PR is closed without being merged.
You can fork and modify the code like this PR on your branch, then use your own build of AKS-Engine, that is the way I am using now.

add masterSubnet with value of VnetCidr to azuredeploy.parameters.jso…

30bebeb

…n while using custom VNET

acs-bot added the size/XS label Feb 2, 2019

CecileRobertMichon reviewed Feb 4, 2019

View reviewed changes

pkg/engine/params.go Outdated Show resolved Hide resolved

CecileRobertMichon requested a review from PatrickLang February 4, 2019 17:27

zhiweiv added 2 commits February 5, 2019 10:55

Merge remote-tracking branch 'upstream/master'

02591a1

add masterSubnet with value of VnetCidr to azuredeploy.parameters.jso…

d573cea

…n while using custom VNET

tariq1890 reviewed Feb 5, 2019

View reviewed changes

acs-bot assigned PatrickLang Feb 11, 2019

acs-bot added the lgtm label Feb 11, 2019

acs-bot added the do-not-merge/hold label Feb 11, 2019

stale bot added the stale label Mar 14, 2019

stale bot removed the stale label Mar 14, 2019

zhiweiv mentioned this pull request Apr 2, 2019

About known issue of custom vnet for Windows in custom-vnet.md #371

Closed

zhiweiv closed this Apr 3, 2019

zhiweiv mentioned this pull request May 11, 2019

Question: AKS Windows supports custom vnet, AKS-E doesn't #1289

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add masterSubnet to azuredeploy.parameters.json while using custom VNET #431

fix: add masterSubnet to azuredeploy.parameters.json while using custom VNET #431

zhiweiv commented Feb 2, 2019 •

edited

Loading

codecov bot commented Feb 2, 2019 •

edited

Loading

tariq1890 Feb 5, 2019

tariq1890 Feb 5, 2019

zhiweiv Feb 5, 2019

tariq1890 Feb 5, 2019

zhiweiv Feb 6, 2019 •

edited

Loading

tariq1890 Feb 11, 2019

zhiweiv Feb 12, 2019

PatrickLang commented Feb 11, 2019

acs-bot commented Feb 11, 2019

tariq1890 commented Feb 11, 2019

stale bot commented Mar 14, 2019

zhiweiv commented Mar 14, 2019

CecileRobertMichon commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

CecileRobertMichon commented Mar 25, 2019

tariq1890 commented Mar 25, 2019

zhiweiv commented Apr 3, 2019

rogersollberger commented May 8, 2019

zhiweiv commented May 8, 2019 •

edited

Loading

fix: add masterSubnet to azuredeploy.parameters.json while using custom VNET #431

fix: add masterSubnet to azuredeploy.parameters.json while using custom VNET #431

Conversation

zhiweiv commented Feb 2, 2019 • edited Loading

codecov bot commented Feb 2, 2019 • edited Loading

Codecov Report

tariq1890 Feb 5, 2019

Choose a reason for hiding this comment

tariq1890 Feb 5, 2019

Choose a reason for hiding this comment

zhiweiv Feb 5, 2019

Choose a reason for hiding this comment

tariq1890 Feb 5, 2019

Choose a reason for hiding this comment

zhiweiv Feb 6, 2019 • edited Loading

Choose a reason for hiding this comment

tariq1890 Feb 11, 2019

Choose a reason for hiding this comment

zhiweiv Feb 12, 2019

Choose a reason for hiding this comment

PatrickLang commented Feb 11, 2019

acs-bot commented Feb 11, 2019

tariq1890 commented Feb 11, 2019

stale bot commented Mar 14, 2019

zhiweiv commented Mar 14, 2019

CecileRobertMichon commented Mar 14, 2019

azure-pipelines bot commented Mar 14, 2019

CecileRobertMichon commented Mar 25, 2019

tariq1890 commented Mar 25, 2019

zhiweiv commented Apr 3, 2019

rogersollberger commented May 8, 2019

zhiweiv commented May 8, 2019 • edited Loading

zhiweiv commented Feb 2, 2019 •

edited

Loading

codecov bot commented Feb 2, 2019 •

edited

Loading

zhiweiv Feb 6, 2019 •

edited

Loading

zhiweiv commented May 8, 2019 •

edited

Loading