-
Notifications
You must be signed in to change notification settings - Fork 366
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Windows] NoEncap support #2160
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ee17d65
to
f20f2d0
Compare
/test-all |
/test-all |
/test-all |
/test-windows-conformance |
/test-conformance |
77ab28f
to
b429bf7
Compare
/test-all |
/test-windows-networkpolicy /test-conformance /test-networkpolicy |
/test-windows-networkpolicy |
/test-e2e /test-conformance /test-networkpolicy /test-windows-conformance |
/test-windows-networkpolicy |
/test-windows-conformance |
5e6f48d
to
6933039
Compare
/test-all |
/test-networkpolicy |
2 similar comments
/test-networkpolicy |
/test-networkpolicy |
/test-windows-networkpolicy |
@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. |
} | ||
|
||
nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName) | ||
if installed && nrInfo != nil && nrInfo.(*nodeRouteInfo).nodeMAC != nil && peerNodeMAC != nil && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to check nrInfo != nil
and peerNodeMAC != nil
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, we don't need to check it. The "String" method checks the length of the struct. You can test it to confirm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
// NoEncap traffic to Node on the different subnet needs underlying routing support. | ||
// Use host default route inside the Node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
"Fixed issue" is not a github keyword and won't close the issue. Maybe:
|
pkg/agent/route/route_windows.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Just a few minor comments.
/test-conformance |
/test-networkpolicy |
1 similar comment
/test-networkpolicy |
/test-e2e |
|
||
nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName) | ||
|
||
if installed && nrInfo != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think installed == true
already means nrInfo != nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
} | ||
|
||
nrInfo, installed, _ := c.installedNodes.GetByKey(nodeName) | ||
if installed && nrInfo != nil && nrInfo.(*nodeRouteInfo).nodeMAC != nil && peerNodeMAC != nil && nrInfo.(*nodeRouteInfo).nodeMAC.String() == peerNodeMAC.String() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/route/route_windows.go
Outdated
@@ -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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Signed-off-by: Zhecheng Li <lzhecheng@vmware.com>
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-e2e /test-windows-e2e |
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