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.

Sign up for GitHub

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

Jump to bottom

node: Handle arpinging when remote node is in different L2 #14201

Merged
merged 2 commits into from
Dec 1, 2020
Merged

node: Handle arpinging when remote node is in different L2 #14201

merged 2 commits into from
Dec 1, 2020

Conversation

brb
Copy link
Member

@brb brb commented Nov 27, 2020

See commit msgs

Fix: #12824

@brb brb added pending-review sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Nov 27, 2020
@brb brb requested review from a team and kkourt November 27, 2020 14:20
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 27, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Nov 27, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.1 Nov 27, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.6 Nov 27, 2020
@brb brb added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Nov 27, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Nov 27, 2020
@borkmann borkmann added the feature/lb-only Impacts cilium running in lb-only datapath mode label Nov 27, 2020
@brb
Copy link
Member Author

brb commented Nov 27, 2020

test-me-please

This was referenced Nov 27, 2020
Closed
Closed
kkourt
kkourt approved these changes Nov 30, 2020
View reviewed changes
Copy link
Contributor

@kkourt kkourt left a comment

Choose a reason for hiding this comment

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

Looks good to me.

One thing that is not obvious to me is whether the refcount optimization for not pinging gateways can lead to a situation where the entry for the gateway has expired in the ARP cache (I'm assuming this is why we arpping) and there is no way to reinsert this entry because its refcount is >0.

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 30, 2020
@brb
Copy link
Member Author

brb commented Nov 30, 2020

Thanks for the reviews!

One thing that is not obvious to me is whether the refcount optimization for not pinging gateways can lead to a situation where the entry for the gateway has expired in the ARP cache (I'm assuming this is why we arpping) and there is no way to reinsert this entry because its refcount is >0.

@kkourt The ARP entries cannot expire in the local cache, because we set NUD_PERMANENT which makes them permanent.

We are arpinging because when BPF NodePort running in XDP or TC gets a request which needs to be forwarded to another node (because of a backend selection), it needs an L2 address of a nexthop to that node (gw or the node itself). If the L2 addr is not present in the neighbor system, then we have to drop such request, as it's not possible to drive the arp resolution from the BPF program. To avoid that, we do arping from cilium-agent once it learns about a new k8s node.

@brb
node: Handle case when remote node is in different L2
5a4ba1f 
Previously, insertNeighbor() was assuming that a remote node is in the
same L2 subnet, i.e. directly reachable w/o a gateway. However, this is
not the case for all deployments.

This commit adds a check for detecting whether the remote node is in the
same L2. If it's not, then a gateway IP addr (nexthop) is going to be
arpinged instead of the remote node IP addr.

The missing bit in this commit is a refcounting to avoid redundant arpings
and neigh removals when the gateway is used to access more than one
remote node.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
node: Refcount neighbour entries
bec216a 
To avoid redundant pings and neigh entry removals when the entry is
still used by other node (happens when two or more nodes can be accessed
through the same gateway).

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented Nov 30, 2020

Previously, all checks were green. I've just pushed the comment change to address https://github.com/cilium/cilium/pull/14201/files/4ce23957a6661d8f985651aca1dc00e3a7c0833a#diff-14a5efc51ce7ea3959385022d4800b355c766c60e8b7ba0e6052afced05ddbf0.

@joestringer joestringer merged commit b78b3b7 into master Dec 1, 2020
@joestringer joestringer deleted the pr/brb/arping-gw branch December 1, 2020 00:31
@borkmann borkmann mentioned this pull request Dec 2, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.1 Dec 2, 2020
@nathanjsweet nathanjsweet mentioned this pull request Dec 2, 2020
Merged
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.6 Dec 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.6 Dec 2, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.1 Dec 3, 2020
@aanm aanm mentioned this pull request Dec 4, 2020
Merged
@aanm aanm mentioned this pull request Dec 4, 2020
Merged
@jaffcheng jaffcheng mentioned this pull request Dec 9, 2020
Closed
@pchaigno pchaigno mentioned this pull request Dec 10, 2020
Closed
@borkmann borkmann mentioned this pull request Sep 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno left review comments

@kkourt kkourt kkourt approved these changes

@borkmann borkmann borkmann approved these changes

Assignees

@kkourt kkourt

Labels
feature/lb-only Impacts cilium running in lb-only datapath mode ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.10.0
Done
1.8.6
Backport pending to v1.8
1.9.1
Backport done to v1.9
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Connectivity issues - eBPF - FIB lookup failed
7 participants
@brb @kkourt @borkmann @pchaigno @nathanjsweet @joestringer @aanm