Skip to content

Conversation

liuyuan10
Copy link
Contributor

NodeCleanNeighbors is also called when enable-l2-neigh-discovery with migrateOnly==false. In that case, all existing arp entries can still be removed because neighLastPingByNextHop is still empty (https://github.com/cilium/cilium/blob/main/pkg/datapath/linux/node.go#L2013). The neighbor link state file then is removed https://github.com/cilium/cilium/blob/main/pkg/datapath/linux/node.go#L2113

The file is only resotred again when next time
NodeConfigurationChanged() is called.

This can break arp entry cleanup when next time cilium is started without enable-l2-neigh-discovery

@liuyuan10 liuyuan10 requested review from a team as code owners October 17, 2023 21:06
@liuyuan10 liuyuan10 requested a review from brb October 17, 2023 21:06
@maintainer-s-little-helper
Copy link

Commit c342806 does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Oct 17, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Oct 17, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Oct 17, 2023
@brb brb requested review from borkmann and removed request for brb October 27, 2023 08:35
@borkmann borkmann added the release-note/misc This PR makes changes that have no direct user impact. label Oct 27, 2023
@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 Oct 27, 2023
@borkmann borkmann added the area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. label Oct 27, 2023
@dylandreimerink
Copy link
Member

/test

NodeCleanNeighbors is also called when enable-l2-neigh-discovery with
migrateOnly==false. In that case, all existing arp entries can still be
removed because neighLastPingByNextHop is still empty
(https://github.com/cilium/cilium/blob/main/pkg/datapath/linux/node.go#L2013).
The neighbor link state file then is removed https://github.com/cilium/cilium/blob/main/pkg/datapath/linux/node.go#L2113

The file is only resotred again when next time
NodeConfigurationChanged() is called.

This can break arp entry cleanup when next time cilium is started
without enable-l2-neigh-discovery

Signed-off-by: Yuan Liu <liuyuan@google.com>
@aanm aanm force-pushed the upstream_fixneigh branch from 5569404 to 83f9911 Compare October 31, 2023 09:53
@aanm
Copy link
Member

aanm commented Oct 31, 2023

/test

@squeed squeed removed the request for review from a team November 8, 2023 10:39
@squeed
Copy link
Contributor

squeed commented Nov 8, 2023

IPSec review no longer needed (in fact, it was removed from this file in a subsequent PR). All required tests are passing. Marking as ready-to-merge.

@squeed squeed added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 8, 2023
@squeed squeed merged commit 8f4df6d into cilium:main Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants