[Windows] Use Windows management virtual network adapter for OVS bridge #3067

wenyingd · 2021-11-30T07:25:42Z

Rename the vnic created by Windows host when creating the HNS Network as
the OVS bridge name, and add OVS internal port for the bridge port. Then
OVS uses the vnic created by Windows host directly, and IP/MAC/route
migrations is not needed. With this workflow, the time of losing IP on
the Windows host is cut off.

Fixes #3057

Signed-off-by: wenying wenyingd@vmware.com

codecov-commenter · 2021-11-30T07:31:24Z

Codecov Report

Merging #3067 (5de6594) into main (c2f3670) will decrease coverage by 8.07%.
The diff coverage is 0.00%.

❗ Current head 5de6594 differs from pull request most recent head 55c9640. Consider uploading reports for the commit 55c9640 to get more accurate results

@@            Coverage Diff             @@
##             main    #3067      +/-   ##
==========================================
- Coverage   58.88%   50.81%   -8.08%     
==========================================
  Files         302      293       -9     
  Lines       25864    35110    +9246     
==========================================
+ Hits        15230    17841    +2611     
- Misses       8950    15579    +6629     
- Partials     1684     1690       +6

Flag	Coverage Δ
e2e-tests	`50.81% <0.00%> (?)`
kind-e2e-tests	`?`
unit-tests	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/interfacestore/types.go	`75.00% <0.00%> (+2.27%)`	⬆️
pkg/ipfix/ipfix_intermediate.go	`0.00% <0.00%> (-89.48%)`	⬇️
pkg/controller/egress/controller.go	`0.00% <0.00%> (-88.45%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`4.58% <0.00%> (-86.85%)`	⬇️
pkg/ipfix/ipfix_collector.go	`0.00% <0.00%> (-81.82%)`	⬇️
pkg/agent/util/iptables/lock.go	`0.00% <0.00%> (-81.82%)`	⬇️
pkg/controller/ipam/antrea_ipam_controller.go	`0.00% <0.00%> (-80.29%)`	⬇️
pkg/controller/ipam/validate.go	`0.00% <0.00%> (-80.00%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go	`0.00% <0.00%> (-79.77%)`	⬇️
pkg/flowaggregator/certificate.go	`0.00% <0.00%> (-76.58%)`	⬇️
... and 293 more

antoninbas

I don't have enough context to be the sole reviewer, but this LGTM.
Out of curiosity, did you test your change and did you observe a significant improvement in downtime?

pkg/agent/util/net_windows.go

antoninbas · 2021-11-30T18:29:44Z

pkg/agent/agent_windows.go

+		// Rename the vnic created by Windows host with the OVS bridge name, then it can be used by OVS directly.
+		if err := util.RenameVMNetworkAdapter(hnsNetwork.Name, uplinkMACStr, brName); err != nil {
+			return err
+		}


I assume we never need to undo this change? deleting the HNSNetwork will still take care of all the clean-up?

You are correct, the vnic created by Windows host is removed even if we have renamed it when the HNSNetwork is removed.

jianjuns

I am not sure if I understood all the changes. Would you update Windows design doc too reflect the changes?

wenyingd · 2021-12-01T02:30:12Z

I don't have enough context to be the sole reviewer, but this LGTM. Out of curiosity, did you test your change and did you observe a significant improvement in downtime?

With our original implementation, the Windows IP address of the uplink address (e.g., we name it as Ethernet0) is lost when we created the HNSNetwork, and then came back after the virtual adapter is created by Windows, then it dismissed again when we removed the vnic until we configured it on the OVS bridge. Using this PR, the IP address is lost only when creating the HNS Network. I have tried to ping the IP address inside the Windows host, and its lost time can be reduced to 1s with this PR, while it lost about 20-30 seconds in the original implementation.

antoninbas

LGTM

docs/design/windows-design.md

jianjuns

Probably we should change "vnic" in the commit message to "virtual network adapter".

docs/design/windows-design.md

wenyingd · 2021-12-02T10:41:12Z

/test-integration
/test-all
/test-windows-all
/skip-ipv6-all
/skip-ipv6-only-all

tnqn · 2021-12-02T17:03:47Z

pkg/agent/route/route_windows_test.go

@@ -67,6 +67,9 @@ func TestRouteOperation(t *testing.T) {
 		},
 	}
 	called := false
+	getOVSBridgeInterfaceNameFunc = func(bridge string) string {
+		return bridge


why the test needs to change getOVSBridgeInterfaceNameFunc in this way?

The test case is actually using the network adapter name directly, which doesn't need a prefix "vEthernet"

pkg/agent/agent_windows.go

tnqn · 2021-12-02T17:21:35Z

pkg/agent/util/net_windows.go

-		return err
-	}
-	if hypervEnabled {
-		if err := RemoveManagementInterface(LocalHNSNetwork); err != nil {


Is this the step that removes the vnic we use since this PR?
Do we still need the two functions? WindowsHyperVEnabled and RemoveManagementInterface

RemoveManagementInterface is not needed any more (and I would remove it), but WindowsHyperVEnabled is used in the integration test for HyperV features check on the setup.

tnqn · 2021-12-02T17:22:46Z

pkg/agent/agent_windows.go

@@ -142,6 +142,10 @@ func (i *Initializer) prepareOVSBridge() error {
 	if _, err = i.ovsBridgeClient.GetOFPort(brName, false); err == nil {
 		klog.Infof("OVS bridge local port %s already exists, skip the configuration", brName)
 	} else {
+		// Rename the vnic created by Windows host with the OVS bridge name, then it can be used by OVS directly.
+		if err = util.RenameVMNetworkAdapter(hnsNetwork.Name, uplinkMACStr, brName); err != nil {


So we rename it to br-int, it will actually get vEthernet (br-int)?

br-int is actually a name of VMNetworkAdapter, but the name of the NetAdapter is "vEthernet (br-int)".

wenyingd · 2021-12-03T11:41:46Z

/test-windows-all
/test-all
/skip-ipv6-all
/skip-ipv6-all

wenyingd · 2021-12-09T10:41:20Z

/test-integration
/test-windows-all
/test-all
/skip-ipv6-all
/skip-ipv6-only-all

wenyingd · 2021-12-16T05:53:15Z

/test-windows-all

wenyingd · 2021-12-17T01:16:47Z

@tnqn @jianjuns Any other comments on this PR?

jianjuns · 2021-12-17T01:58:44Z

LGTM

tnqn

LGTM overall, some nits

pkg/agent/util/net_windows.go

wenyingd · 2022-01-07T00:46:22Z

/test-windows-all

wenyingd · 2022-01-18T02:28:42Z

Rebase code on TOT and resolve the conflict.
Code change is only about Windows, so skip IPv6 relavant tests. Used to manually verified on Windows setup.

/test-windows-all
/test-windows-proxyall-e2e
/test-all
/skip-ipv6-all
/skip-ipv6-only-all

wenyingd · 2022-01-18T06:34:33Z

/test-windows-all
/test-all
/test-windows-proxyall-e2e

wenyingd · 2022-01-18T08:46:54Z

/test-e2e
/test-networkpolicy

tnqn

LGTM

Rename the vnic created by Windows host when creating the HNS Network as the OVS bridge name, and add OVS internal port for the bridge port. Then OVS uses the vnic created by Windows host directly, and IP/MAC/route migrations is not needed. With this workflow, the time of losing IP on the Windows host is reduced. Signed-off-by: wenying <wenyingd@vmware.com>

wenyingd · 2022-01-18T10:07:30Z

/test-windows-all
/test-all
/test-windows-proxyall-e2e
/test-integration
/skip-ipv6-all
/skip-ipv6-only-all

wenyingd · 2022-01-18T11:25:47Z

/test-windows-conformance

wenyingd · 2022-01-19T02:13:54Z

/test-e2e

tnqn · 2022-01-19T10:44:25Z

/skip-e2e failure is unrelated

…ge (antrea-io#3067) Rename the vnic created by Windows host when creating the HNS Network as the OVS bridge name, and add OVS internal port for the bridge port. Then OVS uses the vnic created by Windows host directly, and IP/MAC/route migrations is not needed. With this workflow, the time of losing IP on the Windows host is reduced. Signed-off-by: wenyingd <wenyingd@vmware.com>

wenyingd requested a review from lzhecheng November 30, 2021 07:25

wenyingd assigned antoninbas, tnqn and jianjuns and unassigned antoninbas, tnqn and jianjuns Nov 30, 2021

wenyingd requested review from tnqn, antoninbas and jianjuns November 30, 2021 07:26

wenyingd force-pushed the ovs_host_port branch 2 times, most recently from b1a26a9 to b3b18e3 Compare November 30, 2021 09:10

antoninbas reviewed Nov 30, 2021

View reviewed changes

jianjuns reviewed Nov 30, 2021

View reviewed changes

wenyingd force-pushed the ovs_host_port branch from b3b18e3 to e1e6020 Compare December 1, 2021 06:44

antoninbas reviewed Dec 1, 2021

View reviewed changes

docs/design/windows-design.md Outdated Show resolved Hide resolved

docs/design/windows-design.md Outdated Show resolved Hide resolved

jianjuns reviewed Dec 2, 2021

View reviewed changes

docs/design/windows-design.md Outdated Show resolved Hide resolved

docs/design/windows-design.md Show resolved Hide resolved

wenyingd force-pushed the ovs_host_port branch 2 times, most recently from 44fb592 to b5f9e41 Compare December 2, 2021 07:13

wenyingd changed the title ~~[Windows] Use Windows management vnic for OVS bridge~~ [Windows] Use Windows management virual network adapter for OVS bridge Dec 2, 2021

wenyingd changed the title ~~[Windows] Use Windows management virual network adapter for OVS bridge~~ [Windows] Use Windows management virtual network adapter for OVS bridge Dec 2, 2021

wenyingd force-pushed the ovs_host_port branch from b5f9e41 to 8f451a5 Compare December 2, 2021 07:58

tnqn reviewed Dec 2, 2021

View reviewed changes

wenyingd force-pushed the ovs_host_port branch 2 times, most recently from 1dd7803 to 14ae92e Compare December 3, 2021 01:47

wenyingd force-pushed the ovs_host_port branch from 14ae92e to 3c340ce Compare December 8, 2021 02:12

wenyingd requested a review from tnqn December 9, 2021 10:41

wenyingd requested a review from jianjuns December 17, 2021 01:20

tnqn reviewed Dec 24, 2021

View reviewed changes

pkg/agent/util/net_windows.go Outdated Show resolved Hide resolved

pkg/agent/util/net_windows.go Show resolved Hide resolved

pkg/agent/util/net_windows.go Outdated Show resolved Hide resolved

wenyingd force-pushed the ovs_host_port branch from 3c340ce to c8b6827 Compare January 6, 2022 02:43

wenyingd force-pushed the ovs_host_port branch 2 times, most recently from 71353b1 to 55c9640 Compare January 18, 2022 02:22

wenyingd requested a review from tnqn January 18, 2022 02:29

tnqn added this to the Antrea v1.5 release milestone Jan 18, 2022

tnqn previously approved these changes Jan 18, 2022

View reviewed changes

wenyingd dismissed tnqn’s stale review via b4b2a98 January 18, 2022 09:44

wenyingd force-pushed the ovs_host_port branch from 55c9640 to b4b2a98 Compare January 18, 2022 09:44

wenyingd requested a review from tnqn January 18, 2022 11:26

tnqn approved these changes Jan 19, 2022

View reviewed changes

tnqn merged commit 7dd8d86 into antrea-io:main Jan 19, 2022

wenyingd deleted the ovs_host_port branch August 15, 2022 03:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Use Windows management virtual network adapter for OVS bridge #3067

[Windows] Use Windows management virtual network adapter for OVS bridge #3067

wenyingd commented Nov 30, 2021

codecov-commenter commented Nov 30, 2021 •

edited

Loading

antoninbas left a comment

antoninbas Nov 30, 2021

wenyingd Dec 1, 2021

jianjuns left a comment •

edited

Loading

wenyingd commented Dec 1, 2021

antoninbas left a comment

jianjuns left a comment

wenyingd commented Dec 2, 2021

tnqn Dec 2, 2021

wenyingd Dec 3, 2021

tnqn Dec 2, 2021

wenyingd Dec 3, 2021

tnqn Dec 2, 2021

wenyingd Dec 3, 2021

wenyingd commented Dec 3, 2021

wenyingd commented Dec 9, 2021

wenyingd commented Dec 16, 2021

wenyingd commented Dec 17, 2021

jianjuns commented Dec 17, 2021

tnqn left a comment

wenyingd commented Jan 7, 2022

wenyingd commented Jan 18, 2022

wenyingd commented Jan 18, 2022

wenyingd commented Jan 18, 2022

tnqn left a comment

wenyingd commented Jan 18, 2022

wenyingd commented Jan 18, 2022

wenyingd commented Jan 19, 2022

tnqn commented Jan 19, 2022

[Windows] Use Windows management virtual network adapter for OVS bridge #3067

[Windows] Use Windows management virtual network adapter for OVS bridge #3067

Conversation

wenyingd commented Nov 30, 2021

codecov-commenter commented Nov 30, 2021 • edited Loading

Codecov Report

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

jianjuns left a comment • edited Loading

Choose a reason for hiding this comment

wenyingd commented Dec 1, 2021

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

wenyingd commented Dec 2, 2021

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

wenyingd commented Dec 3, 2021

wenyingd commented Dec 9, 2021

wenyingd commented Dec 16, 2021

wenyingd commented Dec 17, 2021

jianjuns commented Dec 17, 2021

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Jan 7, 2022

wenyingd commented Jan 18, 2022

wenyingd commented Jan 18, 2022

wenyingd commented Jan 18, 2022

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Jan 18, 2022

wenyingd commented Jan 18, 2022

wenyingd commented Jan 19, 2022

tnqn commented Jan 19, 2022

codecov-commenter commented Nov 30, 2021 •

edited

Loading

jianjuns left a comment •

edited

Loading