-
Notifications
You must be signed in to change notification settings - Fork 527
chore: use Azure CCM for k8s 1.16 and later #2161
chore: use Azure CCM for k8s 1.16 and later #2161
Conversation
Let's revisit this after #2191 merges because I anticipate some (small) refactorings needed. |
af7ae25
to
f9d851e
Compare
f1de322
to
c8233a5
Compare
Codecov Report
@@ Coverage Diff @@
## master #2161 +/- ##
=========================================
+ Coverage 71.6% 71.6% +<.01%
=========================================
Files 142 142
Lines 24817 24863 +46
=========================================
+ Hits 17770 17804 +34
- Misses 5911 5923 +12
Partials 1136 1136 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Thanks a lot.
c8233a5
to
d1de290
Compare
d1de290
to
05e2a3c
Compare
@mboersma any updates on this? |
05e2a3c
to
5a12f90
Compare
@feiskyer I think it's ready to merge, just trying to get CI to agree and looking for reviews from the AKS Engine team. |
CI reports that we may be exceeding custom data max length for 1.17 clusters on this PR. :-( |
a2d5c49
to
0d0cd72
Compare
0d0cd72
to
1b54c10
Compare
4b659a5
to
7ab1bc3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want:
addons_test.go
UT to validate the expected cloud-node-manager addon default behaviorsvalidate.go
foo to throw an error if cloud-node-manager addon is explicitly enabled, but isn't supported by an appropriate cluster config (CCM disabled, running the wrong version of k8s, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm pending the requested UT
code looks good, bon courage!
@@ -3056,11 +3100,7 @@ func TestSetAddonsConfig(t *testing.T) { | |||
Enabled: to.BoolPtr(false), | |||
}, | |||
{ | |||
Name: AzureFileCSIDriverAddonName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a duplicated pair of entries introduced in an earlier PR.
@@ -3323,11 +3363,7 @@ func TestSetAddonsConfig(t *testing.T) { | |||
Enabled: to.BoolPtr(false), | |||
}, | |||
{ | |||
Name: AzureFileCSIDriverAddonName, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: duplicate entries removed.
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, 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:
Switches to the out-of-tree cloud provider components if
useCloudControllerManager
is enabled and the Kubernetes version is 1.16.x or later. These components are considered alpha status currently but intended to become the default by 1.17 final release (early December!).Issue Fixed:
Fixes #1915
Requirements:
Notes:
The cloud-controller-manager is deployed as a static pod manifest as it already was, and the cloud-node-manager is deployed as an addon. This is odd but I would argue the best we can do, but I'm open to feedback.