feat: support cloud-node-manager on Windows clusters #3044
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3044 +/- ##
==========================================
+ Coverage 70.55% 70.58% +0.03%
==========================================
Files 145 145
Lines 25169 25219 +50
==========================================
+ Hits 17759 17802 +43
- Misses 6307 6312 +5
- Partials 1103 1105 +2
Continue to review full report at Codecov.
|
chewong
changed the title
chewong
requested review from
jackfrancis and
mboersma
and removed request for
jackfrancis
|
Working on another branch on top of this PR which enables CSI proxy and CSI drivers by default when cloud-controller-manager is enabled on Windows cluster. |
| coreComponents = append(coreComponents, "cloud-controller-manager") | ||
| if common.IsKubernetesVersionGe(eng.ExpandedDefinition.Properties.OrchestratorProfile.OrchestratorVersion, "1.13.0") { | ||
| coreComponents = append(coreComponents, common.CloudControllerManagerComponentName) | ||
| if eng.ExpandedDefinition.Properties.ShouldEnableAzureCloudAddon(common.AzureDiskCSIDriverAddonName) && |
There was a problem hiding this comment.
Should we re-use the ShouldEnableAzureCloudAddon method in E2E? Is there a reason why we can't simply refer to the state of the api model to see whether or not the addon is enabled or not?
There was a problem hiding this comment.
I don't think we will need to reuse it since those addons are turned on/off based on ShouldEnableAzureCloudAddon anyways. I will change it to eng.HasAddon().
| cpu: 50m | ||
| memory: 50Mi | ||
| limits: | ||
| cpu: 2000m |
There was a problem hiding this comment.
These limits seem overgenerous, but I guess that's what we have been using for Linux already.
| @@ -1718,26 +1721,6 @@ func (a *Properties) validateCustomKubeComponent() error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (a *Properties) validatePrivateAzureRegistryServer() error { | |||
There was a problem hiding this comment.
Is this removal related to the cloud-node-manager for Windows change?
There was a problem hiding this comment.
It's kinda related since I was using a cloud-node-manager image in my own ACR for testing. I don't think we should only be allowed to use privateAzureRegistryServer when custom{Component}Image is defined. That's why I removed this validation.
| Expect(labels).To(HaveKeyWithValue("failure-domain.beta.kubernetes.io/region", region)) | ||
| Expect(labels).To(HaveKeyWithValue("topology.kubernetes.io/region", region)) | ||
| var instanceType string | ||
| switch role { |
There was a problem hiding this comment.
Where is the role variable defined?
There was a problem hiding this comment.
aks-engine/test/e2e/kubernetes/kubernetes_test.go
Lines 2097 to 2104 in 98220f9
There was a problem hiding this comment.
lolz thanks for clicking the accordion icon for me :)
|
E2E test results on Jenkins: |
| @@ -1,21 +1,20 @@ | |||
| { | |||
| "env": { | |||
| "GINKGO_SKIP": "should be able to attach azure file" | |||
There was a problem hiding this comment.
Skipping this test case because azurefile-csi-driver isn't supported in Windows clusters yet. https://github.com/chewong/aks-engine/tree/windows-csi will address this issue and I will open a second PR after this PR is merged.
Reason for Change:
Supports out-of-tree cloud-controller-manager and cloud-node-manager on Windows clusters with K8s >= 1.18
Issue Fixed:
Requirements:
Notes: