[Windows] Improvement in cleanup script #4722

wenyingd · 2023-03-17T08:51:39Z

The existing script would reset ovs-vswitchd when stuck in "starting" status. In some corner cases, ovsdb-server/ovs-ovswitchd services may be removed unexpectedly.

This change includes,

add improvement to recover ovsdb-server/ovs-vswitched service if if they do not exist when running the cleanup script.
remove vNICs using Windows VMSwitch API rather than calling ovs-vsctl commands, this can remove the dependency on the running OVS userspace process (ovs-vswitchd).

Fix: #4721

hack/windows/Clean-AntreaNetwork.ps1

XinShuYang · 2023-03-17T10:19:16Z

hack/windows/Clean-AntreaNetwork.ps1

+    sc.exe failure ovs-vswitchd reset= 0 actions= restart/0/restart/0/restart/0
+    start-service ovs-vswitchd
+    $OVS_VERSION=$(Get-Item $OVSInstallDir\driver\OVSExt.sys).VersionInfo.ProductVersion
+    ovs-vsctl --no-wait set Open_vSwitch . ovs_version=$OVS_VERSION


Why do we need to set ovs_version here? Will restarting ovs service remove version info in ovsdb?

The data is not lost if only restart ovs-vswitchd process/service. But if we re-create the OVS schema file ( called in line 92 when ovs-vswitchd failed ), this data is lost as we have deleted db.conf . This version is needed by in antrea-agent monitoring CR, and it may cause antrea-agent crash if not exists.

luolanzone · 2023-03-17T12:26:02Z

@wenyingd @XinShuYang you can use the label area/OS/Windows to track Windows related PRs.

codecov · 2023-03-19T14:37:20Z

Codecov Report

Merging #4722 (bc07e92) into main (6a7dd77) will decrease coverage by 0.05%.
The diff coverage is n/a.

❗ Current head bc07e92 differs from pull request most recent head faa6b0c. Consider uploading reports for the commit faa6b0c to get more accurate results

@@            Coverage Diff             @@
##             main    #4722      +/-   ##
==========================================
- Coverage   71.40%   71.36%   -0.05%     
==========================================
  Files         409      406       -3     
  Lines       61208    63087    +1879     
==========================================
+ Hits        43706    45020    +1314     
- Misses      14547    15089     +542     
- Partials     2955     2978      +23

Flag	Coverage Δ		*Carryforward flag
e2e-tests	`38.34% <ø> (-0.01%)`	⬇️	Carriedforward from eec86e0
integration-tests	`33.81% <ø> (-0.01%)`	⬇️
kind-e2e-tests	`45.89% <ø> (+5.99%)`	⬆️
unit-tests	`62.72% <ø> (-0.36%)`	⬇️

*This pull request uses carry forward flags. Click here to find out more.

see 96 files with indirect coverage changes

hack/windows/Clean-AntreaNetwork.ps1

XinShuYang

LGTM

XinShuYang · 2023-04-18T06:12:14Z

hack/windows/Clean-AntreaNetwork.ps1

-}
-
-RemoveNetworkAdapter $OVS_BR_ADAPTER
-RemoveNetworkAdapter "antrea-gw0"


Could you add a commit message to clarify why vNic was removed from Windows VMSwitch and why netadapter such as antrea-gw0 doesn't need to be removed in script any more?

antoninbas

in the commit message:

s/other than calling ovs-vsctl commands/rather than calling ovs-vsctl commands

antoninbas · 2023-04-28T17:47:25Z

hack/windows/Clean-AntreaNetwork.ps1

+    stop-service ovs-vswitchd
+    sc.exe delete ovs-vswitchd
+    stop-service ovsdb-server
+    sc.exe delete ovsdb-server


will this fail if one of the services has already been stopped or deleted?

It would not fail if the service is already stopped. For a nonexist service ( deleted ), yes, it may fail, but it does not block the following commands

antoninbas · 2023-04-28T17:50:17Z

hack/windows/Clean-AntreaNetwork.ps1

        return
    }
    $ovsStatus = $(Get-Service ovs-vswitchd).Status
-    if ("$ovsStatus" -EQ "StartPending") {
+    if ("$ovsStatus" -ne "Running") {
        sc.exe delete ovs-vswitchd


would it be safer to add stop-service ovs-vswitchd before this line? Or is this guaranteed to succeed?

I don't think it is not necessary, as the current of the status is not "running" and stop-service does not valid impact.

antoninbas · 2023-04-28T17:52:15Z

hack/windows/Clean-AntreaNetwork.ps1

        return
    }
    $ovsStatus = $(Get-Service ovs-vswitchd).Status
-    if ("$ovsStatus" -EQ "StartPending") {
+    if ("$ovsStatus" -ne "Running") {


is the status case-insensitive? I ask because I notice that you use "running" instead later in the script.

Windows is case-insensitive.

antoninbas · 2023-04-28T17:52:49Z

hack/windows/Clean-AntreaNetwork.ps1

+    if ($vmSwitch -ne $null) {
+        Write-Host "Remove vNICs"
+        Remove-VMNetworkAdapter -SwitchName $AntreaHnsNetworkName -ManagementOS -Confirm:$false -ErrorAction SilentlyContinue
+        $hnsNetwork = Get-HnsNetwork | ? Name -eq $AntreaHnsNetworkName


I notice that we use a slightly different command in another script:

hack/windows/Prepare-AntreaAgent.ps1:$AntreaHnsNetwork = Get-HnsNetwork | Where-Object {$_.Name -eq "antrea-hnsnetwork"}

The two commands are the same, ? is an alias of Where-Object.
I would update to keep consistent.

antoninbas · 2023-04-28T17:54:24Z

hack/windows/Clean-AntreaNetwork.ps1

+    # This might happen after the Windows host is restarted abnormally, in which case some stale configurations block
+    # ovs-vswitchd running, like the pid file and the misconfigurations in OVSDB.


in which case some stale configurations can prevent ovs-vswitchd from running, like a stale pid file or misconfigurations in OVSDB.

hack/windows/Clean-AntreaNetwork.ps1

antoninbas · 2023-04-28T17:56:15Z

hack/windows/Clean-AntreaNetwork.ps1

@@ -6,10 +6,16 @@
  OVS installation directory. It is the path argument when using Install-OVS.ps1. The default path is "C:\openvswitch".
  .PARAMETER RenewIPConfig
  Renew the ipconfig on the host. The default value is $false.
+  .PARAMETER RemoveOVS
+  Remove ovsdb-server and ovs-vswitchd services on the host. The default value is $false. If this argument is set as


s/on the host/from the host

hack/windows/Clean-AntreaNetwork.ps1

luolanzone · 2023-05-08T02:11:56Z

Typo in summary and commit messages:
s/when running the cleaup script./when running the cleanup script.

antoninbas

In PR description / commit message:

s/The existing script would reset ovs-vswitchd is stucking in "starting" status/The existing script would reset ovs-vswitchd when stuck in "starting" status - I think that's what you meant, not totally sure
s/if they are not existing/if they do not exist
s/remove the dependency of/remove the dependency on

Otherwise, LGTM

The existing script would reset ovs-vswitchd when stuck in "starting" status. In some corner cases, ovsdb-server/ovs-ovswitchd services may be removed unexpectedly. This change includes, - add improvement to recover ovsdb-server/ovs-vswitched service if they do not exist when running the cleanup script. - remove vNICs using Windows VMSwitch API rather than calling ovs-vsctl commands, this can remove the dependency on the running OVS userspace process (ovs-vswitchd). Signed-off-by: wenyingd <wenyingd@vmware.com>

antoninbas · 2023-05-11T01:08:30Z

@wenyingd @XinShuYang is there any Jenkins test that uses this script and that we should run before merging?

XinShuYang · 2023-05-11T01:34:32Z

@antoninbas No, all windows testbeds use snapshot to clean the environment.

antoninbas · 2023-05-11T02:02:28Z

/skip-all

The existing script would reset ovs-vswitchd when stuck in "starting" status. In some corner cases, ovsdb-server/ovs-ovswitchd services may be removed unexpectedly. This change includes, - add improvement to recover ovsdb-server/ovs-vswitched service if they do not exist when running the cleanup script. - remove vNICs using Windows VMSwitch API rather than calling ovs-vsctl commands, this can remove the dependency on the running OVS userspace process (ovs-vswitchd). Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd requested a review from XinShuYang March 17, 2023 08:53

wenyingd force-pushed the issue_4721 branch from 10a565c to ebf7d1a Compare March 17, 2023 09:10

XinShuYang reviewed Mar 17, 2023

View reviewed changes

hack/windows/Clean-AntreaNetwork.ps1 Outdated Show resolved Hide resolved

XinShuYang reviewed Mar 17, 2023

View reviewed changes

luolanzone added the area/OS/windows Issues or PRs related to the Windows operating system. label Mar 17, 2023

wenyingd force-pushed the issue_4721 branch from ebf7d1a to e68025b Compare March 17, 2023 14:50

wenyingd force-pushed the issue_4721 branch from e68025b to 1a54ab0 Compare March 20, 2023 02:24

XinShuYang reviewed Mar 20, 2023

View reviewed changes

hack/windows/Clean-AntreaNetwork.ps1 Show resolved Hide resolved

XinShuYang reviewed Mar 20, 2023

View reviewed changes

hack/windows/Clean-AntreaNetwork.ps1 Show resolved Hide resolved

wenyingd force-pushed the issue_4721 branch from 1a54ab0 to a6b4bf2 Compare March 21, 2023 01:54

XinShuYang reviewed Mar 21, 2023

View reviewed changes

hack/windows/Clean-AntreaNetwork.ps1 Show resolved Hide resolved

wenyingd force-pushed the issue_4721 branch from a6b4bf2 to eec86e0 Compare March 21, 2023 08:50

XinShuYang previously approved these changes Mar 21, 2023

View reviewed changes

wenyingd requested a review from tnqn March 27, 2023 02:33

wenyingd added this to the Antrea v1.12 release milestone Mar 27, 2023

wenyingd dismissed XinShuYang’s stale review via bc07e92 April 14, 2023 09:15

wenyingd requested a review from XinShuYang April 18, 2023 05:38

XinShuYang reviewed Apr 18, 2023

View reviewed changes

wenyingd requested a review from antoninbas April 28, 2023 04:15

antoninbas reviewed Apr 28, 2023

View reviewed changes

wenyingd force-pushed the issue_4721 branch 2 times, most recently from ece97c5 to faa6b0c Compare May 8, 2023 03:20

antoninbas reviewed May 8, 2023

View reviewed changes

tnqn added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 9, 2023

wenyingd force-pushed the issue_4721 branch from faa6b0c to 741b81f Compare May 9, 2023 10:04

wenyingd requested a review from antoninbas May 10, 2023 07:50

wenyingd requested a review from XinShuYang May 10, 2023 07:50

antoninbas approved these changes May 11, 2023

View reviewed changes

antoninbas merged commit 7866681 into antrea-io:main May 11, 2023

This was referenced May 12, 2023

Automated cherry pick of #4722: Improvement in cleanup script #4970

Merged

Automated cherry pick of #4722: Improvement in cleanup script #4971

Merged

wenyingd deleted the issue_4721 branch May 30, 2023 06:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Improvement in cleanup script #4722

[Windows] Improvement in cleanup script #4722

wenyingd commented Mar 17, 2023 •

edited

Loading

XinShuYang Mar 17, 2023 •

edited

Loading

wenyingd Mar 17, 2023

luolanzone commented Mar 17, 2023

codecov bot commented Mar 19, 2023 •

edited

Loading

XinShuYang left a comment

XinShuYang Apr 18, 2023

wenyingd Apr 28, 2023

antoninbas left a comment

antoninbas Apr 28, 2023

wenyingd May 8, 2023

antoninbas Apr 28, 2023

wenyingd May 8, 2023

antoninbas Apr 28, 2023

wenyingd May 8, 2023

antoninbas Apr 28, 2023

wenyingd May 8, 2023

antoninbas Apr 28, 2023

wenyingd May 8, 2023

antoninbas Apr 28, 2023

wenyingd May 8, 2023

luolanzone commented May 8, 2023 •

edited

Loading

antoninbas left a comment

antoninbas commented May 11, 2023

XinShuYang commented May 11, 2023

antoninbas commented May 11, 2023

		# This might happen after the Windows host is restarted abnormally, in which case some stale configurations block
		# ovs-vswitchd running, like the pid file and the misconfigurations in OVSDB.

[Windows] Improvement in cleanup script #4722

[Windows] Improvement in cleanup script #4722

Conversation

wenyingd commented Mar 17, 2023 • edited Loading

XinShuYang Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luolanzone commented Mar 17, 2023

codecov bot commented Mar 19, 2023 • edited Loading

Codecov Report

XinShuYang left a 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

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

luolanzone commented May 8, 2023 • edited Loading

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented May 11, 2023

XinShuYang commented May 11, 2023

antoninbas commented May 11, 2023

wenyingd commented Mar 17, 2023 •

edited

Loading

XinShuYang Mar 17, 2023 •

edited

Loading

codecov bot commented Mar 19, 2023 •

edited

Loading

luolanzone commented May 8, 2023 •

edited

Loading