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

pkg: Add Bitwise LPM Trie Library #29717

Merged
merged 1 commit into from Feb 18, 2024
Merged

pkg: Add Bitwise LPM Trie Library #29717

merged 1 commit into from Feb 18, 2024

Conversation

nathanjsweet
Copy link
Member

This bitwise lpm trie is a non-thread-safe binary
trie that indexes arbitrarily long bit-based keys
with associated prefixes indexed from most
significant bit to least significant bit using
the longest prefix match algorithm.

Documenting the behavior of the data structure is
localized around the method calls in the trie.go
file.

The tests specifically test boundary cases for the various methods.

Fixes: #29519

@nathanjsweet nathanjsweet added the release-note/misc This PR makes changes that have no direct user impact. label Dec 7, 2023
@nathanjsweet nathanjsweet requested review from a team as code owners December 7, 2023 22:09
@nathanjsweet nathanjsweet mentioned this pull request Dec 7, 2023
Closed
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-bit-lpm-library branch from c116566 to 3e06323 Compare December 8, 2023 03:10
@nathanjsweet nathanjsweet added backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. and removed backport/1.13 This PR represents a backport for Cilium 1.13.x of a PR that was merged to main. backport/1.14 This PR represents a backport for Cilium 1.14.x of a PR that was merged to main. labels Dec 8, 2023
bimmlerd
bimmlerd reviewed Dec 8, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

Probably good to solicit review from @cilium/ipcache and @cilium/sig-policy given they will own this pkg. The CODEOWNERs change LGTM, but I have not reviewed the trie in detail.

For me, I think the biggest question is whether we really need/want to roll our own trie implementation when we already have a radix tree vendored.

pkg/bitlpm/unsigned.go Outdated Show resolved Hide resolved
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
@squeed
Copy link
Contributor

squeed commented Dec 8, 2023

Nice, I was just lamenting the lack of a good bitwise lpm trie in go.

@nathanjsweet nathanjsweet requested review from a team, jrajahalme and squeed and removed request for a team December 8, 2023 20:17
@nathanjsweet nathanjsweet force-pushed the pr/nathanjsweet/add-bit-lpm-library branch 2 times, most recently from 000d788 to ec259ec Compare December 11, 2023 21:26
bimmlerd
bimmlerd approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

CODEOWNERS lgtm

squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/unsigned_test.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
squeed
squeed reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
@joestringer joestringer added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Dec 12, 2023
joestringer
joestringer reviewed Dec 12, 2023
View reviewed changes
pkg/bitlpm/trie.go Outdated Show resolved Hide resolved
@nathanjsweet nathanjsweet added the needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch label Feb 15, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.14.7 Feb 15, 2024
@nathanjsweet nathanjsweet added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Feb 16, 2024
@aanm aanm disabled auto-merge February 18, 2024 14:31
@aanm aanm merged commit 27430d4 into main Feb 18, 2024
208 checks passed
@aanm aanm deleted the pr/nathanjsweet/add-bit-lpm-library branch February 18, 2024 14:31
@tklauser tklauser mentioned this pull request Feb 20, 2024
Merged
7 tasks
@tklauser tklauser 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 Feb 20, 2024
@tklauser tklauser mentioned this pull request Feb 20, 2024
Merged
6 tasks
@tklauser tklauser added backport-pending/1.14 The backport for Cilium 1.14.x for this PR is in progress. and removed needs-backport/1.14 This PR / issue needs backporting to the v1.14 branch labels Feb 20, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.14 in 1.14.7 Feb 20, 2024
@tklauser tklauser mentioned this pull request Feb 20, 2024
Merged
5 tasks
@tklauser tklauser added backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. and removed needs-backport/1.13 This PR / issue needs backporting to the v1.13 branch labels Feb 20, 2024
@tklauser
Copy link
Member

Looks like this cannot be backported to v1.13 because it doesn't build using Go 1.20 which is still the default Go version on that branch:

Error: pkg/container/bitlpm/trie.go:147:14: min requires go1.21 or later (-lang was set to go1.20; check go.mod)
Error: pkg/container/bitlpm/trie.go:198:14: min requires go1.21 or later (-lang was set to go1.20; check go.mod)
Error: pkg/container/bitlpm/trie.go:282:14: min requires go1.21 or later (-lang was set to go1.20; check go.mod)
Error: pkg/container/bitlpm/trie.go:416:14: min requires go1.21 or later (-lang was set to go1.20; check go.mod)
Error: pkg/container/bitlpm/trie.go:520:11: min requires go1.21 or later (-lang was set to go1.20; check go.mod)

@tklauser tklauser added backport/author The backport will be carried out by the author of the PR. and removed backport-pending/1.13 The backport for Cilium 1.13.x for this PR is in progress. labels Feb 20, 2024
@nathanjsweet
Copy link
Member Author

Looks like this cannot be backported to v1.13

That's fine. 1.14 is far enough.

@tklauser tklauser removed the backport/author The backport will be carried out by the author of the PR. label Feb 21, 2024
@github-actions github-actions bot added backport-done/1.14 The backport for Cilium 1.14.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. labels Feb 21, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot removed this from Backport pending to v1.14 in 1.14.7 Feb 21, 2024
@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 Feb 21, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Backport done to v1.14 in 1.14.7 Feb 21, 2024
@joestringer joestringer mentioned this pull request Feb 26, 2024
Open
@jrajahalme jrajahalme mentioned this pull request Mar 13, 2024
Merged
@thorn3r thorn3r mentioned this pull request Mar 13, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki left review comments

@ti-mo ti-mo ti-mo left review comments

@joestringer joestringer joestringer approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@jrajahalme jrajahalme Awaiting requested review from jrajahalme jrajahalme is a code owner automatically assigned from cilium/ipcache

@squeed squeed Awaiting requested review from squeed

Assignees
No one assigned
Labels
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/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.14.7
Backport done to v1.14
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add a userspace bitwise LPM trie data structure
8 participants
@nathanjsweet @squeed @joamaki @ti-mo @tklauser @joestringer @bimmlerd @aanm