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

daemon: use netlink for managed neighbor support probe #25134

Merged
merged 2 commits into from May 17, 2023

Conversation

rgo3
Copy link
Contributor

@rgo3 rgo3 commented Apr 26, 2023

With this commit we refactor the probe logic that checks for the existence of a certain helper as this could break on older distro kernels that have backported the commit we probed for.

Using actual netlink calls to probe for managed neighbor support will only succeed if the underlying kernel actually supports it.

Fixes #20694.

@rgo3 rgo3 added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels Apr 26, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Apr 26, 2023
@rgo3 rgo3 added release-note/misc This PR makes changes that have no direct user impact. and removed area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels Apr 26, 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 Apr 26, 2023
@rgo3
Copy link
Contributor Author

rgo3 commented Apr 26, 2023

Tested this probe logic manually on 6.1 and 5.10. It didn't error out on either version, probing managed neighbor support conclusively (available on 6.1, not available on 5.10).

@rgo3
Copy link
Contributor Author

rgo3 commented Apr 26, 2023

/test

@rgo3 rgo3 force-pushed the rewrite-neighbor-support-probe branch from aef3117 to 5887743 Compare May 2, 2023 09:16
@rgo3
Copy link
Contributor Author

rgo3 commented May 2, 2023

/test

@rgo3
Copy link
Contributor Author

rgo3 commented May 2, 2023

runtime hit #22373, rerunning. Also marking as ready to review.

@rgo3 rgo3 marked this pull request as ready for review May 2, 2023 15:26
@rgo3 rgo3 requested review from a team as code owners May 2, 2023 15:26
@rgo3 rgo3 requested a review from ldelossa May 2, 2023 15:26
@rgo3
Copy link
Contributor Author

rgo3 commented May 2, 2023

/test-runtime

@rgo3 rgo3 force-pushed the rewrite-neighbor-support-probe branch from 5887743 to 2f0cc75 Compare May 12, 2023 10:13
@rgo3 rgo3 requested review from a team as code owners May 12, 2023 10:13
@rgo3 rgo3 requested a review from tklauser May 12, 2023 10:13
@rgo3 rgo3 force-pushed the rewrite-neighbor-support-probe branch from 2f0cc75 to ddfbaa5 Compare May 12, 2023 10:16
daemon/cmd/daemon.go Outdated Show resolved Hide resolved
Copy link
Contributor

@ti-mo ti-mo 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! Just a few nits.

pkg/datapath/linux/probes/managed_neighbors.go Outdated Show resolved Hide resolved
pkg/datapath/linux/probes/managed_neighbors.go Outdated Show resolved Hide resolved
pkg/datapath/linux/probes/managed_neighbors.go Outdated Show resolved Hide resolved
@rgo3 rgo3 force-pushed the rewrite-neighbor-support-probe branch from ddfbaa5 to b2bfd37 Compare May 16, 2023 12:48
Copy link
Contributor

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

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

LGTM! One last nit on the docstring.

ti-mo and others added 2 commits May 16, 2023 18:18
This helper allows tests for feature probes to be skipped on kernel versions
that are known to lack the feature.

Signed-off-by: Timo Beckers <timo@isovalent.com>
With this commit we refactor the probe logic that checks for
the existence of a certain helper as this could break on older
distro kernel that have backported the commit we probed for.

Using actual netlink calls to probe for managed neighbor support
will only succeed if the underlying kernel actually supports it.

Fixes cilium#20694.

Signed-off-by: Robin Gögge <r.goegge@isovalent.com>
@rgo3 rgo3 force-pushed the rewrite-neighbor-support-probe branch from b2bfd37 to e7d8d4a Compare May 16, 2023 16:22
@rgo3
Copy link
Contributor Author

rgo3 commented May 16, 2023

/test

@ti-mo ti-mo requested review from tklauser and lmb May 17, 2023 09:30
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

Nice, thanks for adding the explanatory comment in haveManagedNeighbors.

@tklauser tklauser merged commit 0ada4e8 into cilium:main May 17, 2023
58 checks passed
@ti-mo ti-mo deleted the rewrite-neighbor-support-probe branch July 10, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Probes: NTF_EXT_LEARNED and NTF_EXT_MANAGED flags using netlink
4 participants