[Windows] Optimize the containerized OVS installation #6383

wenyingd · 2024-05-30T10:44:21Z

Add logic to check the installed OVSext drivers, if only the desired version
of driver is already installed, skip the installation; otherwise, remove the
existing the drivers and re-install.
Add logic to check the installed VC redistributable files, if the existing
installd vc_redist version is equal to or higher than the lowest requirement,
skip the installation; otherwise re-install the files provided in the image.
Optimize the logic of maintaining the system paths by removing the duplicated
path.
Optimize Uninstall-OVS script by removing OVS bin paths from the system path
after it is deleted.

wenyingd · 2024-05-30T10:58:13Z

@XinShuYang @antoninbas I have introduced a ps module "OVSHelper.psm1" in this change, which is used to provide general functions independent from the exact versions of OVS. So I place it in the container image directly, rather than put the functions in Install-OVSDriver.ps1 packaged in the configmap. I didn't add the functions to the existing Helper.psm1, because I want to only provide the necessary function in the PS module file in container images.

I am thinking how to co-work with the existing script Install-OVS.ps1, which is trying to install OVS driver and/or userspaces on the Windows Node and register as native service. In theory, I would prefer to use the new implementations in Install-OVS.ps1, and I also hope to keep that script is independent as it used to work. I don't have a good idea to directly copy the functions in the script or import the new module. Copy functions may introduce troubles in the future code maintenance; while import-module may introduce file dependencies for the script.

XinShuYang · 2024-05-30T11:28:12Z

build/yamls/antrea-windows-with-ovs.yml

      }
+    } else {
+      Write-Host "OVS driver is not installed via the host-process containers, please install it manually."


When Test-Path $OVSHelperScript fails, we can't determine if the driver has been installed on the host. I think please install it manually might be misleading.

Is this what you want?

if(-not (Test-Path $OVSHelperScript)) { $driverStatus = netcftg -q ovsext if ($driverStatus -like "*not installed*") { Write-Host "OVS driver is not installed on the Windows Node, please install it manually. } }

If there is no $OVSHelperScript provided in the image, we would ask the user to manually install the driver or update it but not operate from container.

hack/windows/OVSHelper.psm1

antoninbas · 2024-05-31T20:47:02Z

Let's merge #6360 first, given that the mentorship program is coming to an end and I don't want to have @shikharish to resolve conflicts.

docs/windows.md

hack/windows/Install-OVS.ps1

luolanzone · 2024-06-03T02:54:07Z

Please add proper description on PR summary and commit.

build/yamls/antrea-windows-with-ovs.yml

hack/windows/Install-OVS.ps1

docs/windows.md

rajnkamr · 2024-06-07T07:19:18Z

build/yamls/antrea-windows-with-ovs.yml

-      }
+    $OVSInstallScript = "$mountPath\k\antrea\Install-OVS.ps1"
+    if (Test-Path $OVSInstallScript) {
+      & $OVSInstallScript -InstallUserspace $false -LocalFile $mountPath\openvswitch


In case user want ot bring their own signed driver, User must be able to know if driver is already installed or not or failed to install , it might not be preferable to check log for failure later.
Instead Start-Job can also be used to schedule the script in background asynchronously, while capturing the status of the background script Install-OVS.ps1 installation using wait-job as and when it returns success or error ! and then just validate driver installation success or failure here as it may allow user to know if driver is installed ? and then they might want to uninstall it and use their own signed driver.

Got it , I can try with command "Start-Job" to run the script.

hack/windows/Install-OVS.ps1

antoninbas · 2024-06-07T18:27:36Z

hack/windows/Install-OVS.ps1

+            if ($ovsFile.BaseName -eq "openvswitch") {
+                return $ovsFile.FullName
+            }
+            Log "Unsupported local directory: $LocalFile. Only directories with a base name 'openvswitch' are supported"


This is a bit surprising IMO. Why does the basename matter given that we will copy everything we need to OVSInstallDir anyway?

I use base name "openvswitch" as a check here becauseI don't have a good idea on how to check if the local OVS directory is valid or not, and both our opensource OVS files (injected in the container image) and the commercial OVS files are using this name. So I hard-code it here.

I think you have changed it so that it no longer relies on the name? I think it makes more sense that way, because only the contents matter.

hack/windows/Install-OVS.ps1

wenyingd · 2024-06-14T01:56:07Z

/test-windows-all

hack/windows/Install-OVS.ps1

antoninbas · 2024-06-25T22:18:27Z

hack/windows/Install-OVS.ps1

+    # Copy $OVSDriverPath to host path "c:/openvwitch/driver", and then install ovsext.inf with the host path.
+    # This is a workaround for error code "0x80070003" with containerd v1.7+. The error happens when Windows
+    #  utitilty netcfg tries to access the container's mount path "C:/hpc/".
+    $hostDriverPath="${OVSInstallDir}\driver"


I would rename it to driverStagingPath. By the way I assume that we can actually delete ${OVSInstallDir}\driver after running netcfg?

I used to try to remove the ${OVSInstallDir}\driver from the host, but I found that the ovsext driver was shown as "not installed" some time after the Pod is down. This is different from my expectation that OVS driver is supposed to be loaded after installed. So I keep the files on the disk in my final change.

Oh that's interesting. I guess that is one more reason that it needs to be copied to a "host path", as it needs to be somewhat permanent.

antoninbas · 2024-06-25T22:20:57Z

hack/windows/Install-OVS.ps1

+    # This is a workaround for error code "0x80070003" with containerd v1.7+. The error happens when Windows
+    #  utitilty netcfg tries to access the container's mount path "C:/hpc/".
+    $hostDriverPath="${OVSInstallDir}\driver"
+    if ($OVSDriverPath -ne $hostDriverPath){


I think we should remove this test in all the copy functions.
I cannot think of any remaining legitimate case in which $OVSLocalPath and $OVSInstallDir would be the same? In order for this to happen, the user would have to invoke the script with LocalFile == OVSInstallDir, which IMO does not need to be a supported case (we could exit with an error). Maybe I am missing something though.

For the use scenario, sometimes we may need to replace OVS driver on the Windows host for test purpose, e.g., adding logs in the ovsext driver to perform troubleshooting sessions, at this time we can directly replace the driver files in $hostDriverPath, and then call the script.

This is not for production usage, so if you prefer we ignore it, I can remove the compare and this check in the start,

if (($LocalFile -ne "") -and ("$LocalFile" -eq "$OVSInstallDir")) { Log "LocalFile and OVSInstallDir are the same, exiting" exit 1 }

I see. But for troubleshooting wouldn't you also need to somehow overwrite the OVS driver version, or CheckAndInstallOVSDriver would return early?

If this is convenient for debugging and there is no simple alternative, then I think you should keep it, and maybe just log a warning when "$LocalFile" -eq "$OVSInstallDir".

In my last change, I remove the path compare on driver path and usr/bin as you suggested, and add a precheck at the start to reject the input if $LocalFile equals to $OVSInstallDir. For the local troubleshooting (not production env), I can call UnInstall-OVS.ps1 then call Install-OVS.ps1 as an alternative. Please help review the patch again. @antoninbas

hack/windows/Install-OVS.ps1

antoninbas · 2024-06-25T22:23:18Z

hack/windows/Install-OVS.ps1

+    $hostUsrBinPath = "C:\openvswitch\usr\bin"
+    $usrBinPath="${OVSInstallPath}\usr\bin"
+    if ("$usrBinPath" -ne "$hostUsrBinPath") {
+        if (-not $global:updateOVS) {


That doesn't really sound like an issue for an initContainer?

XinShuYang · 2024-06-26T05:47:28Z

docs/windows.md

@@ -208,7 +208,7 @@ your own OVS package with a signed OVS kernel driver, you would run:

 ```powershell
 curl.exe -LO https://raw.githubusercontent.com/antrea-io/antrea/main/hack/windows/Install-OVS.ps1
-.\Install-OVS.ps1 -ImportCertificate $false -Local -LocalFile <PathToOVSPackage>
+.\Install-OVS.ps1 -InstallUserspace $true -LocalFile <PathToOVSPackage>


Since InstallUserspace now is true by default, I think we don't need to specify it?

Also, should we update the guidance of preparing containerized OVS in line 196-204?

I would prefer to highlight "-InstallUserspace $true" with the context expecting to install Windows native Services.

I would prefer to highlight "-InstallUserspace $true" with the context expecting to install Windows native Services.

Got it, I am ok with it.

Also, should we update the guidance of preparing containerized OVS in line 196-204?

@wenyingd Any follow-ups regarding this question?

Updated, remove the original descriptions and example.

antoninbas

Overall LGTM, thanks for making all these changes

I know that as part of CI, we test the containerized case. Is there any validation as part of CI for the "service" case (.\Install-OVS.ps1 -InstallUserspace $true run directly on the host), or alternatively can you confirm that you tested the most recent version on the script manually?

hack/windows/Install-OVS.ps1

antoninbas · 2024-06-27T02:26:14Z

hack/windows/Install-OVS.ps1

+    if ($InstallUserspace -eq $true) {
+        $OVSBinPaths="${OVSBinPaths};${OVSLocalPath}\usr\sbin"
+    }
+    InstallOpenSSLFiles "$OVSBinPaths"


it seems that we copy the SSL files to $OVSBinPaths ("${OVSLocalPath}\usr\bin"), and then we rely on CopyOVSUtilities to copy them to the OVS install directory? Was that intentional? I guess it will work but shouldn't InstallOpenSSLFiles copy the files to their "final" installation location directly?

The current solution is on desired that copy the ssl files to the LocalFile first, then copy the final files to the install dir. This is because the ssl dll files are required by both usr/bin and usr/sbin, and the two directories are copied in different step: usr/bin is copied in step CopyOVSUtilities, and usr/sbin is copied in step InstallOVSServices. If we plan to copy ssl dll files directly to the final path, the coy operations are distributed into different steps, and we also need to record the path where the dll files are extracted until the end.

Ok, sounds good

wenyingd · 2024-06-27T02:43:39Z

I have manually tested the script on my local testbed with both cases to install userspace services and not install services.

XinShuYang · 2024-06-27T03:07:54Z

I know that as part of CI, we test the containerized case. Is there any validation as part of CI for the "service" case (.\Install-OVS.ps1 -InstallUserspace $true run directly on the host), or alternatively can you confirm that you tested the most recent version on the script manually?

I have manually tested the script on my local testbed with both cases to install userspace services and not install services.

@wenyingd For the OVS host service test, We have jenkins-windows-ovs in CI to verify related code changes. In the run_install_windows_ovs function, it tries to run Install-OVS.ps1 and perform a health check for OVS. With the current code changes in this PR, do you think we need to update or add some checks in this job?

antoninbas

LGTM

antoninbas · 2024-06-27T18:09:25Z

/test-windows-all

XinShuYang · 2024-06-28T06:51:18Z

build/charts/antrea-windows/conf/ovs/Install-OVSDriver.ps1

 }
+Write-Host "Completed OVS installation"


XinShuYang

LGTM, only have some comments about logs.

1. Add logic to check the installed OVSext drivers, if only the desired version of driver is already installed, skip the installation; otherwise, remove the existing the drivers and re-install. 2. Add logic to check the installed VC redistributable files, if the existing installd vc_redist version is equal to or higher than the lowest requirement, skip the installation; otherwise re-install with the provided files. 3. Optimize the logic of maintaining the env paths by removing the duplicated paths. 4. Optimize Uninstall-OVS script by removing OVS bin paths from the system path after it is deleted. Signed-off-by: Wenying Dong <wenyingd@vmware.com>

XinShuYang

LGTM, since the latest change is only about log updates, we don't need to rerun windows jenkins jobs.

antoninbas · 2024-06-28T17:23:40Z

/test-windows-all

wenyingd requested a review from XinShuYang May 30, 2024 10:44

XinShuYang reviewed May 30, 2024

View reviewed changes

XinShuYang mentioned this pull request May 30, 2024

Streamlining OVS Driver Installation and Upgrade for Windows Container #6358

Closed

XinShuYang reviewed May 30, 2024

View reviewed changes

hack/windows/OVSHelper.psm1 Outdated Show resolved Hide resolved

wenyingd changed the title ~~[Windows] Optimize the containerized OVS installation steps~~ [Windows] Optimize the containerized OVS installation May 31, 2024

luolanzone added the area/OS/windows Issues or PRs related to the Windows operating system. label Jun 3, 2024

luolanzone reviewed Jun 3, 2024

View reviewed changes

wenyingd force-pushed the issue_6358 branch 5 times, most recently from 417e126 to 93a0b0e Compare June 6, 2024 01:35

luolanzone requested a review from antoninbas June 6, 2024 03:28

wenyingd force-pushed the issue_6358 branch 3 times, most recently from ea32285 to 8e090cf Compare June 6, 2024 16:12

antoninbas reviewed Jun 6, 2024

View reviewed changes

build/yamls/antrea-windows-with-ovs.yml Outdated Show resolved Hide resolved

hack/windows/Install-OVS.ps1 Show resolved Hide resolved

antoninbas reviewed Jun 6, 2024

View reviewed changes

docs/windows.md Outdated Show resolved Hide resolved

rajnkamr reviewed Jun 7, 2024

View reviewed changes

antoninbas reviewed Jun 7, 2024

View reviewed changes

wenyingd force-pushed the issue_6358 branch 4 times, most recently from d1b3224 to 514cf85 Compare June 13, 2024 10:17

wenyingd requested review from antoninbas and XinShuYang June 14, 2024 01:56

antoninbas reviewed Jun 25, 2024

View reviewed changes

wenyingd force-pushed the issue_6358 branch from 912aa9a to 334a610 Compare June 26, 2024 02:39

wenyingd requested a review from antoninbas June 26, 2024 02:40

XinShuYang reviewed Jun 26, 2024

View reviewed changes

antoninbas reviewed Jun 27, 2024

View reviewed changes

wenyingd force-pushed the issue_6358 branch 2 times, most recently from 66be333 to 4647e64 Compare June 27, 2024 07:45

wenyingd requested review from XinShuYang and antoninbas June 27, 2024 07:46

wenyingd force-pushed the issue_6358 branch 4 times, most recently from 2e86758 to a5e9d15 Compare June 27, 2024 11:54

antoninbas previously approved these changes Jun 27, 2024

View reviewed changes

XinShuYang reviewed Jun 28, 2024

View reviewed changes

build/charts/antrea-windows/conf/ovs/Install-OVSDriver.ps1 Outdated

}

Write-Host "Completed OVS installation"

Copy link

Contributor

XinShuYang Jun 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

XinShuYang previously approved these changes Jun 28, 2024

View reviewed changes

wenyingd dismissed stale reviews from XinShuYang and antoninbas via 70ac535 June 28, 2024 07:00

wenyingd force-pushed the issue_6358 branch from a5e9d15 to 70ac535 Compare June 28, 2024 07:00

wenyingd requested review from XinShuYang and antoninbas June 28, 2024 07:01

XinShuYang approved these changes Jun 28, 2024

View reviewed changes

antoninbas approved these changes Jun 28, 2024

View reviewed changes

antoninbas merged commit fc40157 into antrea-io:main Jun 28, 2024
52 of 60 checks passed

antoninbas added the action/release-note Indicates a PR that should be included in release notes. label Jun 28, 2024

[Windows] Optimize the containerized OVS installation #6383

[Windows] Optimize the containerized OVS installation #6383

Conversation

wenyingd commented May 30, 2024 • edited Loading

wenyingd commented May 30, 2024

XinShuYang May 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas commented May 31, 2024

luolanzone commented Jun 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd commented Jun 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd commented Jun 27, 2024

XinShuYang commented Jun 27, 2024 • edited Loading

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Jun 27, 2024

Choose a reason for hiding this comment

XinShuYang left a comment

Choose a reason for hiding this comment

XinShuYang left a comment

Choose a reason for hiding this comment

antoninbas commented Jun 28, 2024

wenyingd commented May 30, 2024 •

edited

Loading

XinShuYang May 30, 2024 •

edited

Loading

XinShuYang commented Jun 27, 2024 •

edited

Loading