chore: Add EnableAHUB in WindowsProfile

[AbelHu]

Reason for Change:

Allow users to use Azure hybrid benefit for Windows server.

Issue Fixed:

Requirements:

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

Notes:

[mboersma]

/azp run pr-e2e

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[codecov]

Codecov Report

Merging #3322 into master will increase coverage by 0.60%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3322      +/-   ##
==========================================
+ Coverage   72.57%   73.17%   +0.60%     
==========================================
  Files         147      147              
  Lines       25215    24923     -292     
==========================================
- Hits        18300    18238      -62     
+ Misses       5782     5559     -223     
+ Partials     1133     1126       -7     

Impacted Files	Coverage Δ
pkg/api/vlabs/types.go	73.30% <ø> (ø)
pkg/api/converterfromapi.go	94.01% <100.00%> (+0.36%)	⬆️
pkg/api/convertertoapi.go	93.50% <100.00%> (+0.33%)	⬆️
pkg/api/types.go	94.37% <100.00%> (+0.02%)	⬆️
pkg/engine/virtualmachines.go	80.98% <100.00%> (+0.13%)	⬆️
pkg/engine/virtualmachinescalesets.go	81.28% <100.00%> (+0.09%)	⬆️
pkg/engine/artifacts.go	99.12% <0.00%> (-0.88%)	⬇️
pkg/api/addons.go	97.86% <0.00%> (-0.05%)	⬇️
pkg/api/common/const.go	40.00% <0.00%> (ø)
pkg/api/k8s_versions.go	100.00% <0.00%> (ø)
... and 3 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fa283b...1bbf235. Read the comment docs.

[jackfrancis]

@AbelHu How can we add E2E test coverage for this? Can the current Windows VHD image be associated with a LicenseType?

[AbelHu]

@jackfrancis From my understanding and the test result, LicenseType is only a property of VMSS for Windows and there is no additional change for the Windows VHD.
I manually ran aks-engine deploy with and without LicenseType and both passed. How about to add "LicenseType": "Windows_Server" in windowsProfile of the file test\e2e\test_cluster_configs\windows\cse_benchmark_aks_vhds.json?

[AbelHu]

One question for the type WindowsLicenseType I added in common/types.go: Is this correct? Or should I add this separately in vlabs and api, and then convert them to *string in pkg/api/converterfromapi.go and pkg/api/convertertoapi.go?

Refine the PR to follow the style of KubeProxyMode

[jackfrancis]

@AbelHu I see the WindowsLicenseType type as being defined in both the api and vlabs packages, and then converted. Strictly speaking you could define this thinly wrapped string type in a common library that both api and vlabs imported (like common), to avoid the unnecessary conversion.

The only consideration would be whether or not the evolution of that type definition might want to evolve discretely for the api and vlabs data structures. In this case I don't think it will.

[AbelHu]

@jackfrancis Agree with your point. That is why I defined it in common in the original PR. But later I found that common/types.go is never introduced in AKS and it seems like to introduce many conflicts for namespace in AKS unit tests so I define them separately for vlabs and api.

[marosset]

@AbelHu Does AKS ever plan to use liscenseType of Windows_Client here? If not it might be good to add validation to fail if licenseType is set to Windows_Client.

[marosset]

Can you either add comments in docs/validation about this only working for scale sets or set this up for availability sets too?

In the portal I see on option to 'use Azure Hybrid Benefit' when creating non-scaleset VMs which get added to availability sets?

[AbelHu]

Can you either add comments in docs/validation about this only working for scale sets or set this up for availability sets too?

In the portal I see on option to 'use Azure Hybrid Benefit' when creating non-scaleset VMs which get added to availability sets?

I will add the comment. AKS only supports Windows with VMSS. If later we need to support this in VMAS, I can help to add the support later.

@marosset And if many aks-engine users are using VMAS with Windows and want to enable AHUB, I can help to do it in this PR if you can share how I can test VMAS with Windows in aks-engine.

[AbelHu]

@AbelHu Does AKS ever plan to use liscenseType of Windows_Client here? If not it might be good to add validation to fail if licenseType is set to Windows_Client.

AKS only uses Windows_Server and will validate this in AKS side. It seems like that SDK and API allows users to set it to Windows_Client so I think that it may be better to keep this option. But I will check this with our PM. If we can confirm that Windows_Server is the only option for AKS, we can use a bool EnableAHUB in WindowsProfile and set the value LicenseType to Windows_Server when generating ARM template.

[AbelHu]

@marosset @jackfrancis @mboersma I will update this PR with below changes soon:

1. Support VMAS by adding code in pkg\engine\virtualmachines.go
2. Change LicenseType (*string) to EnableAHUB (*bool)

[marosset]

Sounds good!

[AbelHu]

@keikhara helped to confirm that AKS only needs to set LicenseType to Windows_Server. Now I have updated this PR. I also test VMAS with examples/windows/kubernetes-wincni.json and it works.

Please help to review it. @marosset @jackfrancis @mboersma

[AbelHu]

@marosset Could you help to take a look at the updated PR? Thanks.

[marosset]

Can we add one e2e test config that exercises this behavior?

you can add a new json file at https://github.com/Azure/aks-engine/tree/master/test/e2e/test_cluster_configs/windows with in enabled and also set

"env": {
	"SKIP_TESTS": "true",
},
"options": {
	"allowedOrchestratorVersions": ["latestReleasedVersion"]
},

in the top (like in https://github.com/Azure/aks-engine/blob/master/test/e2e/test_cluster_configs/windows/vhd_url.json)

[marosset]

Looks good after we get one e2e test config with this set to true.

[AbelHu]

Can we add one e2e test config that exercises this behavior?

you can add a new json file at https://github.com/Azure/aks-engine/tree/master/test/e2e/test_cluster_configs/windows with in enabled and also set

"env": {
	"SKIP_TESTS": "true",
},
"options": {
	"allowedOrchestratorVersions": ["latestReleasedVersion"]
},

in the top (like in https://github.com/Azure/aks-engine/blob/master/test/e2e/test_cluster_configs/windows/vhd_url.json)

Added now. Why I did not add it is because it is difficult to verify it after enabling AHUB. currently it only can verify whether the cluster can be created successfully with enabling AHUB.

[marosset]

Can we add one e2e test config that exercises this behavior?
you can add a new json file at https://github.com/Azure/aks-engine/tree/master/test/e2e/test_cluster_configs/windows with in enabled and also set

"env": {
	"SKIP_TESTS": "true",
},
"options": {
	"allowedOrchestratorVersions": ["latestReleasedVersion"]
},

in the top (like in https://github.com/Azure/aks-engine/blob/master/test/e2e/test_cluster_configs/windows/vhd_url.json)

Added now. Why I did not add it is because it is difficult to verify it after enabling AHUB. currently it only can verify whether the cluster can be created successfully with enabling AHUB.

That is alright, the SKIP_TESTS equals true will cause the e2e test to only validate deployment and will skip running the actually e2e tests.
Thanks or adding this!

[marosset]

/lgtm

[acs-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AbelHu, marosset

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 [marosset]

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