Skip to content
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

Update Node's MAC address to the Node's annotation for direct routing #2161

Merged
merged 1 commit into from
May 21, 2021

Conversation

tnqn
Copy link
Member

@tnqn tnqn commented May 10, 2021

To bypass Windows host network when forwarding Pod egress traffic in noencap mode, antrea-agent needs to know peer Nodes' MAC addresses so that it can configure Openflow rules which route the packets to underlay network via uplink interface directly.

To discover the MAC addresses, the PR makes each antrea-agent report its uplink's MAC address to the Node's annotation, then other agents can get it when the NodeRouteController configures route/flows to this Node.

Signed-off-by: Quan Tian qtian@vmware.com

For #2157

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2021

Codecov Report

Merging #2161 (0c83e56) into main (a9c7744) will decrease coverage by 0.11%.
The diff coverage is 78.94%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2161      +/-   ##
==========================================
- Coverage   61.30%   61.18%   -0.12%     
==========================================
  Files         273      273              
  Lines       20646    20658      +12     
==========================================
- Hits        12656    12640      -16     
- Misses       6688     6703      +15     
- Partials     1302     1315      +13     
Flag Coverage Δ
e2e-tests ∅ <ø> (?)
kind-e2e-tests 52.10% <73.68%> (-0.21%) ⬇️
unit-tests 41.18% <78.94%> (+0.14%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/agent.go 50.34% <76.47%> (+2.60%) ⬆️
pkg/util/env/env.go 48.57% <100.00%> (ø)
...gent/controller/networkpolicy/status_controller.go 72.60% <0.00%> (-5.48%) ⬇️
pkg/agent/cniserver/ipam/ipam_service.go 63.15% <0.00%> (-5.27%) ⬇️
pkg/agent/cniserver/ipam/ipam_delegator.go 48.83% <0.00%> (-4.66%) ⬇️
pkg/agent/cniserver/pod_configuration.go 53.90% <0.00%> (-3.52%) ⬇️
pkg/controller/grouping/controller.go 64.64% <0.00%> (-3.04%) ⬇️
pkg/agent/cniserver/server.go 68.91% <0.00%> (-1.93%) ⬇️
pkg/flowaggregator/flowaggregator.go 56.18% <0.00%> (-1.90%) ⬇️
pkg/controller/networkpolicy/status_controller.go 86.45% <0.00%> (-0.65%) ⬇️
... and 5 more

@tnqn tnqn mentioned this pull request May 10, 2021
pkg/agent/agent.go Outdated Show resolved Hide resolved
antoninbas
antoninbas previously approved these changes May 11, 2021
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

pkg/agent/agent_windows.go Outdated Show resolved Hide resolved
antoninbas
antoninbas previously approved these changes May 11, 2021
jianjuns
jianjuns previously approved these changes May 11, 2021
@tnqn
Copy link
Member Author

tnqn commented May 11, 2021

/test-all

@tnqn tnqn dismissed stale reviews from jianjuns and antoninbas via 34effe8 May 11, 2021 10:16
@tnqn
Copy link
Member Author

tnqn commented May 11, 2021

@jianjuns @antoninbas I moved the MAC reporting to a platform generic function initNodeLocalConfig in the latest patch, because a mixture of linux nodes and windows nodes is possible, in which case the windows nodes need to know linux nodes' MAC addresses as well, otherwise win-to-lin pod performance would be worse than win-to-win one.

@tnqn
Copy link
Member Author

tnqn commented May 11, 2021

/test-all

antoninbas
antoninbas previously approved these changes May 11, 2021
@tnqn
Copy link
Member Author

tnqn commented May 13, 2021

/test-all

@tnqn
Copy link
Member Author

tnqn commented May 14, 2021

/test-all

@lzhecheng
Copy link
Contributor

/test-networkpolicy

@tnqn
Copy link
Member Author

tnqn commented May 14, 2021

/test-windows-conformance
/test-e2e
/test-all-features-conformance

@lzhecheng lzhecheng requested review from jianjuns and antoninbas May 19, 2021 04:14
@lzhecheng
Copy link
Contributor

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

@tnqn
Copy link
Member Author

tnqn commented May 19, 2021

@antoninbas @lzhecheng I rebased on main and added unit test for initNodeLocalConfig. Could you approve again?

@tnqn
Copy link
Member Author

tnqn commented May 19, 2021

/test-all

pkg/agent/agent_test.go Outdated Show resolved Hide resolved
@tnqn
Copy link
Member Author

tnqn commented May 19, 2021

/test-all

antoninbas
antoninbas previously approved these changes May 19, 2021
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 230 to 238
expectedNodeConfig: config.NodeConfig{
Name: nodeName,
OVSBridge: ovsBridge,
DefaultTunName: defaultTunInterfaceName,
PodIPv4CIDR: podCIDR,
NodeIPAddr: nodeIPNet,
NodeMTU: 1400,
UplinkNetConfig: new(config.AdapterNetConfig),
},
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it seems that you could have a getExpectedNodeConfig helper function with the expectedMTU as a parameter to avoid repeating this? Don't feel strongly about it though.

Copy link
Member Author

Choose a reason for hiding this comment

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

makes sense, changed to construct an expectedNodeConfig directly in t.Run

TunnelType: tt.tunnelType,
},
}
assert.NoError(t, initializer.initNodeLocalConfig())
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this should probably be a require. If initNodeLocalConfig fails, initializer.nodeConfig could be nil?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, thanks for catching it

To bypass Windows host network when forwarding Pod egress traffic in
noencap mode, antrea-agent needs to know peer Nodes' MAC addresses so
that it can configure Openflow rules which route the packets to underlay
network via uplink interface directly.

To discover the MAC addresses, the PR makes each antrea-agent report its
uplink's MAC address to the Node's annotation, then other agents can get
it when the NodeRouteController configures route/flows to this Node.

Signed-off-by: Quan Tian <qtian@vmware.com>
@tnqn
Copy link
Member Author

tnqn commented May 20, 2021

/test-all

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants