Add OVS driver installation in initContainer for Antrea Windows agent #6312
Conversation
|
/test-windows-all |
|
/test-windows-e2e |
|
/test-windows-containerd-e2e |
| @@ -34,6 +34,23 @@ data: | |||
| cp $mountPath/etc/antrea/antrea-cni.conflist c:/etc/cni/net.d/10-antrea.conflist | |||
| mkdir -force c:/k/antrea/bin | |||
| cp $mountPath/k/antrea/bin/antctl.exe c:/k/antrea/bin/antctl.exe | |||
There was a problem hiding this comment.
You should add this script to path "build/yamls/windows/containerd-with-ovs" and update containerd-with-ovs.yml to enforce generating the final manifest with make actions, rather than directly modify the final yml.
There was a problem hiding this comment.
Thanks, will update it.
| # Install OVS Driver | ||
| netcfg -l $OVSDriverDir/ovsext.inf -c s -i OVSExt | ||
| } | ||
| # Update Ovs Driver |
There was a problem hiding this comment.
Why shall we copy the driver to Windows host? I remember you mentioned that Windows host can load the installed driver even if the file is not persistent on disk.
BTW, even if we have to copy the driver files, it should be taken in Install-OVSDriver-Containerd.ps1 rather than in Run-AntreaOVS-Containerd.ps1 right?
There was a problem hiding this comment.
The file path in the initcontainer cannot be shared with the OVS container. Therefore, after installing the driver in the initcontainer, we copy the driver file to the host to ensure it persists. Once the OVS container starts, it copies the driver from the host to its own file path. This ensures that the driver used is the same one installed in the initcontainer.
|
/test-windows-containerd-e2e |
|
/test-windows-containerd-e2e |
|
/test-windows-containerd-e2e |
| @@ -1,5 +1,24 @@ | |||
| apiVersion: v1 | |||
| data: | |||
| Install-OVSDriver-Containerd.ps1: | | |||
There was a problem hiding this comment.
I feel not necessary to highlight "Containerd" in the name by now.
There was a problem hiding this comment.
Sure, I will update all related script name.
| @@ -0,0 +1,18 @@ | |||
| $ErrorActionPreference = "Stop" | |||
| mkdir -force c:/openvswitch/driver | |||
There was a problem hiding this comment.
No need to create this directory?
There was a problem hiding this comment.
It seems we don't have this folder in the container..
There was a problem hiding this comment.
This command is trying to create directory on the host as your are using path "C:/openvswitch", and I didn't find any consumption of the path in your change.
There was a problem hiding this comment.
Got you points, will remove it, thanks.
|
|
||
| # Check if OVSExt driver is already installed | ||
| $driverStatus = netcfg -q ovsext | ||
| if ($driverStatus -like '*not installed*') { |
There was a problem hiding this comment.
Do we have a plan to install certificates including the self-signed certs in the init-container when installing OVS driver?
There was a problem hiding this comment.
I recall our discussion on related topic: for the officially signed OVS, Antrea requires no additional actions. However, for other OVS releases, it is the user's responsibility to add the certificate before running Antrea. We have already highlighted this in the current documentation(https://github.com/gran-vmv/antrea/blob/ipam-e2e-k8s/docs/windows.md#1-optional-install-ovs-provided-by-antrea-or-your-own)
There was a problem hiding this comment.
Got it, then we may need to cover the possible failure if user tries to install unsigned OVS on Windows host.
| if ($driverStatus -like '*not installed*') { | ||
| # Install OVS Driver | ||
| $result = netcfg -l $OVSDriverDir/ovsext.inf -c s -i OVSExt | ||
| if ($result -like '*failed*') { |
There was a problem hiding this comment.
($result -like 'failed') is checking whether the variable $result contains the word "failed", it will enter here only when error message contains failed, we might want to add another else to capture any unknown errors
else {
# Handle unexpected output
Write-Host "Unexpected output from netcfg: $result"
exit 1
}
and can prefer to move Write-Host "OVSExt driver has been installed" at the end
There was a problem hiding this comment.
We can only determine whether it's a success or a failure from the result of netcfg (It doesn't return typical error message which can be handled by "try-catch"). In testing, I found that netcfg always returns the 'failed' when it fails, which means it can serve as a basis for determining if the command has failed. For the else branch, I agree we can move it at the end for easier maintenance.
By integrating the OVS driver installation into an initContainer, we ensure that the necessary driver is installed before the main containers start, and the driver's presence is checked only once during the pod's lifecycle. Signed-off-by: Shuyang Xin <gavinx@vmware.com>
| $driverStatus = netcfg -q ovsext | ||
| if ($driverStatus -like '*not installed*') { | ||
| # Install OVS Driver | ||
| $result = netcfg -l $OVSDriverDir/ovsext.inf -c s -i OVSExt |
There was a problem hiding this comment.
Shall we have a try on a fresh Windows host which didn't install OVS driver before? I asked this because I remember that if we install the unsigned OVS driver on a Windows host for the first time, a pop-up window is given. But if we operate with powershell silently, we may not catch the signal. Then what would be the result? Is it containing "failed" in the output?
There was a problem hiding this comment.
After discussion with @wenyingd , we decided to create another PR to enhance the features related to certificates and the OVS driver installation. I created an issue for further discussion in #6358. This PR will not affect the existing driver installation method, and we hope to prioritize its merging because PR #6311 also depends on the modifications in this PR.
luolanzone
added
the
area/OS/windows
|
/test-windows-all |
By integrating the OVS driver installation into an initContainer, we ensure that the necessary driver is installed before the main containers start, and the driver's presence is checked only once during the pod's lifecycle.