feat: install Windows csi proxy during cluster creation#2854
feat: install Windows csi proxy during cluster creation#2854acs-bot merged 17 commits intoAzure:masterfrom
Conversation
|
/cc @kkmsft @andyzhangx |
|
To test this add the following to WindowsProfile: (I'm updating clusterdefintion.md to document these fields right now) |
Codecov Report
@@ Coverage Diff @@
## master #2854 +/- ##
==========================================
+ Coverage 72.47% 72.49% +0.02%
==========================================
Files 140 141 +1
Lines 25612 25912 +300
==========================================
+ Hits 18562 18785 +223
- Misses 5981 6034 +53
- Partials 1069 1093 +24 |
| if e := validate.Var(w.AdminPassword, "required"); e != nil { | ||
| return errors.New("WindowsProfile.AdminPassword is required, when agent pool specifies Windows") | ||
| } | ||
| if !validatePasswordComplexity(w.AdminUsername, w.AdminPassword) { |
There was a problem hiding this comment.
Not related - I did some cleanup for how validation is performed for WindowsProfile here.
andyzhangx
left a comment
andyzhangx
left a comment
There was a problem hiding this comment.
do have a default csiProxyUrl now?
docs/topics/clusterdefinitions.md
Outdated
| | ----------------------------- | -------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | ||
| | adminUsername | yes | Username for the Windows adminstrator account created on each Windows node | | ||
| | adminPassword | yes | Password for the Windows adminstrator account created on each Windows node | | ||
| | csiProxyUrl | no | Path to a package containing csi proxy binaries for Windows. | |
There was a problem hiding this comment.
this is optional finally, we would have a default value for csiProxyUrl, right?
There was a problem hiding this comment.
Once we have stable builds we'll default this URL.
We'll also want to add the csi proxy and related container images to the Windows VHD.
There was a problem hiding this comment.
I added the URl of a package we can use for testing to the csi-proxy doc/markdown until we have official releases for the package.
|
@marosset any progress on this? does this PR work now? |
|
@andyzhangx - I'm going to pick this up again and try and get this merged today |
|
With the latest commit please use this package for testing https://kubernetesartifacts.azureedge.net/csi-proxy/master/binaries/csi-proxy.tar.gz |
marosset
changed the title
pkg/api/vlabs/types.go
Outdated
| } | ||
|
|
||
| // IsCsiProxyEnabled returns true if csi proxy service should be enable for Windows nodes | ||
| func (w *WindowsProfile) IsCsiProxyEnabled() bool { |
There was a problem hiding this comment.
Updating everywhere (except in the powershell code :))
|
I'll update casing everywhere... |
pkg/api/vlabs/types.go
Outdated
| if w.EnableCSIProxy != nil { | ||
| return *w.EnableCSIProxy | ||
| } | ||
| return false // DO NOT HARDCODE |
pkg/api/const.go
Outdated
| // DefaultEnableAutomaticUpdates determines the aks-engine provided default for enabling automatic updates | ||
| DefaultEnableAutomaticUpdates = true | ||
| // DefaultEnableCSIProxyWindows determines if CSI proxy should be enabled by default for Windows nodes | ||
| DefaultEnableCSIProxyWindows = false |
There was a problem hiding this comment.
you can move this to api/common/const.go if it'll be shared by api/vlabs
There was a problem hiding this comment.
oh that works - let me do that instead of what I just did
There was a problem hiding this comment.
I moved it to api/common/const.go
| if w.EnableCSIProxy != nil { | ||
| return *w.EnableCSIProxy | ||
| } | ||
| return DefaultEnableCSIProxyWindows |
There was a problem hiding this comment.
this needs to be common.DefaultEnableCSIProxyWindows (assuming common is a reference to the api/common package)
jackfrancis
left a comment
jackfrancis
left a comment
There was a problem hiding this comment.
added a final comment, lgtm
|
|
||
| if ($global:EnableCsiProxy) { | ||
| New-CsiProxyService -CsiProxyPackageUrl $global:CsiProxyUrl -KubeDir $global:KubeDir | ||
| # TODO: make kubelet depeond on csi-server-proxy service if enabled |
| -UseContainerD $useContainerD | ||
|
|
||
| if ($global:EnableCsiProxy) { | ||
| New-CsiProxyService -CsiProxyPackageUrl $global:CsiProxyUrl -KubeDir $global:KubeDir |
There was a problem hiding this comment.
Could we please add a note in the README on where the csi proxy logs can be found in default setup.
| tar -xzf $binaryPackage -C $tempdir | ||
| cp "$tempdir\build\server.exe" "$KubeDir\csi-proxy-server.exe" | ||
|
|
||
| del $tempdir -Recurse |
There was a problem hiding this comment.
Would this happen whenever a new node is installed ?
| # | ||
|
|
||
| if ($global:CsiProxyEnabled) { | ||
| Write-Log "Starting csi-proxy-server service" |
There was a problem hiding this comment.
I was thinking we would also be introducing service dependancy ? Would the start-service do sync start or would it do a start asynchronously potentially causing race scenarios with start of kubelet ?
There was a problem hiding this comment.
Oh yea - forgot we had discussed this.
I will add one.
|
|
||
| ## Requirements | ||
|
|
||
| - CSI Proxy for Windows requires Kubernetes version 1.18.0 or greater. |
There was a problem hiding this comment.
Can we specify beta here ? If this is a regular practice we can leave it as is...
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kkmsft, marosset The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@marosset our friendly acs-bot does not seem to have its |
|
whoops - i create a new PR this morning. |
6 participants
Reason for Change:
Issue Fixed:
Requirements:
Notes: