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

bpf, maps: Don't set nodeid to 0 when nodeid allocation fails. #31380

Merged
merged 1 commit into from Mar 14, 2024
Merged

bpf, maps: Don't set nodeid to 0 when nodeid allocation fails. #31380

merged 1 commit into from Mar 14, 2024

Conversation

marseel
Copy link
Contributor

@marseel marseel commented Mar 13, 2024 •
edited

Don't set nodeid to 0 when nodeid allocation fails.

When we run out of IDs to allocate for nodes, we returned zero value before that was later on propagated to bpf map.

Fixed issue with assigning 0 nodeID when corresponding bpf map run out of space. 
Potentially it could have impacted connectivity in large clusters (>4k nodes) with IPSec or Mutual Auth enabled.
Otherwise, it was merely generating unnecessary error log messages.

@marseel marseel requested a review from a team as a code owner March 13, 2024 15:49
@marseel marseel requested a review from aspsk March 13, 2024 15:49
@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 Mar 13, 2024
@marseel
Copy link
Contributor Author

marseel commented Mar 13, 2024

/cc @pchaigno

@pchaigno pchaigno requested review from pchaigno and removed request for aspsk March 13, 2024 16:26
@pchaigno pchaigno added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Mar 13, 2024
@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 Mar 13, 2024
pchaigno
pchaigno reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this Marcel! 🙏

pkg/datapath/linux/node_ids.go Outdated Show resolved Hide resolved
pkg/datapath/linux/node_ids.go Show resolved Hide resolved
pkg/datapath/linux/node_ids.go Show resolved Hide resolved
pkg/datapath/linux/node_ids.go Show resolved Hide resolved
@marseel marseel force-pushed the fix_nodeid_issue branch 2 times, most recently from 137cdfb to 2bc22ae Compare March 13, 2024 17:13
pchaigno
pchaigno approved these changes Mar 13, 2024
View reviewed changes
pkg/datapath/linux/node_ids.go Outdated Show resolved Hide resolved
@marseel
bpf, maps: Don't propagate nodeID to bpf map when allocation fails.
c198503 
When we run out of IDs to allocate for nodes, we were propagating zero
ID to bpf map.
Now we just simply return error and not modify bpf map instead.
Also clean up incorrectly mapped nodeids on startup in case that
happened.

Signed-off-by: Marcel Zieba <marcel.zieba@isovalent.com>
@tommyp1ckles
Copy link
Contributor

/test

@tommyp1ckles tommyp1ckles added needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Mar 13, 2024
@tommyp1ckles tommyp1ckles mentioned this pull request Mar 13, 2024
Closed
@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 Mar 14, 2024
@tommyp1ckles tommyp1ckles added this pull request to the merge queue Mar 14, 2024
Merged via the queue into cilium:main with commit 4f4b0a7 Mar 14, 2024
62 checks passed
@gandro gandro mentioned this pull request Mar 19, 2024
Merged
21 tasks
@gandro gandro added backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. and removed needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Mar 19, 2024
@gandro
Copy link
Member

gandro commented Mar 19, 2024

The error handling of allocateIDForNode works slightly differently in v1.14 and older. Since I don't have any context and this fix seems rather subtle, I'm deferring the backport to those branches to the authors.

@gandro gandro added the backport/author The backport will be carried out by the author of the PR. label Mar 19, 2024
@gandro gandro mentioned this pull request Mar 19, 2024
Merged
10 tasks
@github-actions github-actions bot added backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Mar 21, 2024
@jrajahalme jrajahalme mentioned this pull request Mar 26, 2024
Merged
This was referenced Mar 28, 2024
Merged
Merged
@julianwiedmann julianwiedmann added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Mar 28, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Apr 5, 2024
This was referenced Apr 11, 2024
Open
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pchaigno pchaigno pchaigno approved these changes

@tommyp1ckles tommyp1ckles tommyp1ckles approved these changes

Assignees
No one assigned
Labels
backport/author The backport will be carried out by the author of the PR. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. backport-done/1.14 The backport for Cilium 1.14.x for this PR is done. backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@marseel @tommyp1ckles @gandro @pchaigno @julianwiedmann