[Windows] NoEncap support #2160

lzhecheng · 2021-05-10T12:28:35Z

This PR implements support for Windows Noencap mode. It also includes flow optimization to improve
Pod to Pod performance.
Noencap: If same subnet, destination MAC in OVS is set with peer Node annotation. If not, host routing will
be used to forward traffic.
Hybrid: If same subnet, same as Noencap with same subnet. If not, same as Encap mode.

Supersedes #1901
Fixes #1632

pkg/agent/controller/noderoute/node_route_controller.go

codecov-commenter · 2021-05-10T13:42:59Z

Codecov Report

Merging #2160 (07a1db7) into main (29f4deb) will increase coverage by 3.35%.
The diff coverage is 57.44%.

@@            Coverage Diff             @@
##             main    #2160      +/-   ##
==========================================
+ Coverage   61.63%   64.99%   +3.35%     
==========================================
  Files         273      274       +1     
  Lines       20673    20859     +186     
==========================================
+ Hits        12742    13557     +815     
+ Misses       6594     5922     -672     
- Partials     1337     1380      +43

Flag	Coverage Δ
e2e-tests	`52.85% <48.93%> (?)`
kind-e2e-tests	`52.55% <53.19%> (-0.06%)`	⬇️
unit-tests	`41.28% <46.80%> (+0.07%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/openflow/pipeline.go	`79.02% <50.00%> (+7.87%)`	⬆️
pkg/agent/openflow/client.go	`68.42% <53.84%> (+8.89%)`	⬆️
...gent/controller/noderoute/node_route_controller.go	`60.82% <75.00%> (+14.43%)`	⬆️
pkg/agent/openflow/pipeline_other.go	`100.00% <100.00%> (+50.00%)`	⬆️
...g/controller/networkpolicy/clusternetworkpolicy.go	`72.79% <0.00%> (-14.35%)`	⬇️
pkg/apiserver/handlers/endpoint/handler.go	`58.82% <0.00%> (-11.77%)`	⬇️
pkg/agent/nodeportlocal/rules/iptable_rule.go	`55.93% <0.00%> (-7.71%)`	⬇️
pkg/apiserver/storage/ram/watch.go	`90.38% <0.00%> (-3.85%)`	⬇️
...gent/controller/networkpolicy/status_controller.go	`72.60% <0.00%> (-2.74%)`	⬇️
...kg/controller/networkpolicy/store/networkpolicy.go	`83.58% <0.00%> (-1.42%)`	⬇️
... and 55 more

lzhecheng · 2021-05-10T14:15:21Z

/test-all

pkg/agent/openflow/pipeline.go

pkg/agent/controller/noderoute/node_route_controller.go

lzhecheng · 2021-05-11T03:04:20Z

/test-all

lzhecheng · 2021-05-11T05:37:25Z

/test-all

lzhecheng · 2021-05-11T06:18:06Z

/test-windows-conformance

lzhecheng · 2021-05-11T06:18:57Z

/test-conformance

lzhecheng · 2021-05-11T07:30:06Z

/test-all

lzhecheng · 2021-05-11T10:06:44Z

/test-windows-networkpolicy /test-conformance /test-networkpolicy

lzhecheng · 2021-05-11T11:30:33Z

/test-windows-networkpolicy

lzhecheng · 2021-05-11T11:40:26Z

/test-e2e /test-conformance /test-networkpolicy /test-windows-conformance

lzhecheng · 2021-05-11T14:05:12Z

/test-windows-networkpolicy

lzhecheng · 2021-05-11T15:01:17Z

/test-windows-conformance

pkg/agent/openflow/client.go

lzhecheng · 2021-05-26T06:25:32Z

/test-all

lzhecheng · 2021-05-26T09:30:15Z

/test-networkpolicy

lzhecheng · 2021-05-26T11:42:58Z

/test-networkpolicy

lzhecheng · 2021-05-26T13:05:49Z

/test-networkpolicy

lzhecheng · 2021-05-26T14:53:08Z

/test-windows-networkpolicy

lzhecheng · 2021-05-26T15:08:02Z

@antoninbas if you got no more comments on this PR, could you please merge it when tests are successful? So the new release won't be blocked.

antoninbas · 2021-05-26T15:30:07Z

pkg/agent/controller/noderoute/node_route_controller.go

+	}
+
+	nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName)
+	if installed && nrInfo != nil && nrInfo.(*nodeRouteInfo).nodeMAC != nil && peerNodeMAC != nil && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String() {


it feels like if they are both nil, we should also return early?

if installed && nrInfo != nil { installedMAC := nrInfo.(*nodeRouteInfo).nodeMAC if installedMAC == nil && peerNodeMAC == nil { return nil } if installedMAC != nil && peerNodeMAC != nil && installedMAC.String() == peerNodeMAC.String() { { return nil } }

It's fine to call installedMAC.String() when installedMAC is nil, it will get "".

type HardwareAddr []byte func (a HardwareAddr) String() string { if len(a) == 0 { return "" } buf := make([]byte, 0, len(a)*3-1) for i, b := range a { if i > 0 { buf = append(buf, ':') } buf = append(buf, hexDigit[b>>4]) buf = append(buf, hexDigit[b&0xF]) } return string(buf) }

So this can be simplified to:

if installed && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String()

We don't need to check nrInfo != nil and peerNodeMAC != nil?

No, we don't need to check it. The "String" method checks the length of the struct. You can test it to confirm.

pkg/agent/controller/noderoute/node_route_controller.go

tnqn · 2021-05-26T14:26:13Z

pkg/agent/route/route_windows.go

+	// NoEncap traffic to Node on the different subnet needs underlying routing support.
+	// Use host default route inside the Node.


This is noEncap + different subnet case, we rely on the underlying network to route the Pod traffic to peer Node as we cannot set nexthop to peerNodeIP in such case. We assume the Node has a default gateway which is an external router and user/cloud-controller will configure the routes on the external router.
Maybe the function name NeedsRoutingToPeer is confusing, I was asked several times why we install route when it does NOT need routing to peer. I will make a PR to clarify it.

pkg/agent/controller/noderoute/node_route_controller.go

tnqn · 2021-05-26T15:41:54Z

The old PR #1901
Fixed issue #1632

"Fixed issue" is not a github keyword and won't close the issue. Maybe:

Supersedes #1901
Fixes #1632

tnqn · 2021-05-26T16:20:47Z

pkg/agent/route/route_windows.go

@@ -88,6 +97,10 @@ func (c *Client) Reconcile(podCIDRs []string) error {
 			c.hostRoutes.Store(dst, rt)
 			continue
 		}
+		// If the route is not for uplink interface, ignore it.
+		if c.nodeConfig.UplinkNetConfig != nil && rt.LinkIndex != c.nodeConfig.UplinkNetConfig.Index {


I just found this condition is not correct. listRoutes only returns routes on antrea-gw0 or br-int, so this is going to be always true. And I don't see why we need to check if it's for uplink interface here. We have migrated all routes from uplink to br-int for noEncap case.

I also wonder how we know which route can be deleted as there could be user configured routes on br-int for noEncap case. I checked linux case, it cleans orphaned routes on antrea-gw0 only, which seems working for encap only but at least won't cause problem. I would suggest keep windows the same, i.e. don't try to clean up routes on br-int in Reconcile until there is a way to distinguish user configured routes.

jianjuns

LGTM. Just a few minor comments.

pkg/agent/controller/noderoute/node_route_controller.go

pkg/agent/openflow/client.go

lzhecheng · 2021-05-27T02:36:00Z

/test-conformance

lzhecheng · 2021-05-27T02:54:02Z

/test-networkpolicy

lzhecheng · 2021-05-27T02:55:41Z

/test-networkpolicy

lzhecheng · 2021-05-27T03:53:21Z

/test-e2e

tnqn · 2021-05-27T10:34:44Z

pkg/agent/controller/noderoute/node_route_controller.go

+
+	nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName)
+
+	if installed && nrInfo != nil {


I think installed == true already means nrInfo != nil

tnqn · 2021-05-27T10:38:35Z

pkg/agent/controller/noderoute/node_route_controller.go

+	}
+
+	nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName)
+	if installed && nrInfo != nil && nrInfo.(*nodeRouteInfo).nodeMAC != nil && peerNodeMAC != nil && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String() {


No, we don't need to check it. The "String" method checks the length of the struct. You can test it to confirm.

tnqn · 2021-05-27T10:41:33Z

pkg/agent/route/route_windows.go

@@ -165,7 +191,7 @@ func (c *Client) listRoutes() (map[string]*netroute.Route, error) {
 	rtMap := make(map[string]*netroute.Route)
 	for idx := range routes {
 		rt := routes[idx]
-		if rt.LinkIndex != c.nodeConfig.GatewayConfig.LinkIndex {
+		if rt.LinkIndex != c.nodeConfig.GatewayConfig.LinkIndex && rt.LinkIndex != c.bridgeInfIndex {


You need to remove the second condition too? otherwise some routes on br-int will be removed unexpectedly. We only manage routes on antrea-gw0 so should return them only.

Signed-off-by: Zhecheng Li <[email protected]>

lzhecheng · 2021-05-27T11:28:43Z

/test-all

tnqn

LGTM

lzhecheng · 2021-05-27T14:22:22Z

/test-e2e /test-windows-e2e

vmwclabot added the cla-not-required label May 10, 2021

lzhecheng force-pushed the win-noencap branch from 13a095e to 065bae6 Compare May 10, 2021 12:31

tnqn reviewed May 10, 2021

View reviewed changes

pkg/agent/controller/noderoute/node_route_controller.go Outdated Show resolved Hide resolved

pkg/agent/controller/noderoute/node_route_controller.go Outdated Show resolved Hide resolved

lzhecheng force-pushed the win-noencap branch 2 times, most recently from ee17d65 to f20f2d0 Compare May 10, 2021 14:15

lzhecheng force-pushed the win-noencap branch from f20f2d0 to ba54d73 Compare May 10, 2021 14:36

tnqn reviewed May 10, 2021

View reviewed changes

lzhecheng force-pushed the win-noencap branch from ba54d73 to afa12de Compare May 11, 2021 02:44

lzhecheng force-pushed the win-noencap branch from afa12de to 08b2082 Compare May 11, 2021 05:22

lzhecheng force-pushed the win-noencap branch 2 times, most recently from 77ab28f to b429bf7 Compare May 11, 2021 07:19

lzhecheng force-pushed the win-noencap branch from b62b0d1 to f3d979a Compare May 11, 2021 10:03

lzhecheng force-pushed the win-noencap branch from f3d979a to 42ed700 Compare May 11, 2021 11:18

tnqn reviewed May 12, 2021

View reviewed changes

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved

lzhecheng force-pushed the win-noencap branch 4 times, most recently from 5e6f48d to 6933039 Compare May 19, 2021 04:42

lzhecheng requested a review from antoninbas May 26, 2021 06:25

antoninbas reviewed May 26, 2021

View reviewed changes

tnqn reviewed May 26, 2021

View reviewed changes

jianjuns reviewed May 26, 2021

View reviewed changes

pkg/agent/controller/noderoute/node_route_controller.go Show resolved Hide resolved

pkg/agent/openflow/client.go Outdated Show resolved Hide resolved

pkg/agent/openflow/client.go Show resolved Hide resolved

lzhecheng requested review from antoninbas and tnqn May 27, 2021 03:17

lzhecheng force-pushed the win-noencap branch from 9406670 to 22349ec Compare May 27, 2021 05:32

tnqn reviewed May 27, 2021

View reviewed changes

address comments

07a1db7

Signed-off-by: Zhecheng Li <[email protected]>

lzhecheng force-pushed the win-noencap branch from 22349ec to 07a1db7 Compare May 27, 2021 11:27

tnqn approved these changes May 27, 2021

View reviewed changes

antoninbas merged commit 9547582 into antrea-io:main May 27, 2021

lzhecheng deleted the win-noencap branch May 27, 2021 21:19

tnqn mentioned this pull request May 28, 2021

[Windows] NoEncap support #1901

Closed

tnqn mentioned this pull request Jun 12, 2021

Bypass Windows host network when forwarding Pod egress traffic in noencap mode #2157

Closed

		// NoEncap traffic to Node on the different subnet needs underlying routing support.
		// Use host default route inside the Node.


		nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName)

		if installed && nrInfo != nil {

[Windows] NoEncap support #2160

[Windows] NoEncap support #2160

Conversation

lzhecheng commented May 10, 2021 • edited Loading

codecov-commenter commented May 10, 2021 • edited Loading

Codecov Report

lzhecheng commented May 10, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 11, 2021

lzhecheng commented May 26, 2021

lzhecheng commented May 26, 2021

lzhecheng commented May 26, 2021

lzhecheng commented May 26, 2021

lzhecheng commented May 26, 2021

lzhecheng commented May 26, 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

tnqn commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

lzhecheng commented May 27, 2021

lzhecheng commented May 27, 2021

lzhecheng commented May 27, 2021

lzhecheng commented May 27, 2021

tnqn May 27, 2021 • edited Loading

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

lzhecheng commented May 27, 2021

tnqn left a comment

Choose a reason for hiding this comment

lzhecheng commented May 27, 2021

lzhecheng commented May 10, 2021 •

edited

Loading

codecov-commenter commented May 10, 2021 •

edited

Loading

tnqn May 27, 2021 •

edited

Loading