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

Switch to cilium/statedb #32125

Merged
merged 6 commits into from
May 10, 2024
Merged

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Apr 22, 2024

This PR removes pkg/statedb, pulls in cilium/statedb and fixes up StateDB usage in few places as cilium/statedb had evolved a bit.

Here's a summary of changes:

  • Remove pkg/statedb
  • Vendor cilium/statedb
  • Remove the StateDB Swagger API endpoints in favor of StateDB's built-in HTTP handler
  • Add the StateDB handler under /statedb/ in the UNIX socket HTTP API
  • Fix up uses of StateDB reconciler (StatusPendingDelete is gone, WithStatus is now SetStatus+Clone)
  • statedb.NewDB is now statedb.New
  • DeleteTracker() is now Changes()
  • Get() is List(), and First() is Get()
  • Add metrics implementations for StateDB and the reconciler. Minor change to metrics, "package" label is now "handle" (extracting the package name with runtime.Callers was way too expensive). The metrics have been disabled by default and there should be no users yet depending on stability of the metrics.
  • Add module provider for statedb.Handle. Using statedb.Handle instead of *statedb.DB will account metrics to the module id instead of the generic "DB" handle.
  • Fix health status JSON marshalling by using string instead of error type for Status field

Additionally small boy scout cleanups that I encounted when testing:

  • Increase sysctl full reconciliation to 10 mins from 10 secs (leftover from copy-pasting example code)
  • Change sysctl table name from "sysctl-settings" to just "sysctl"
  • Added status to sysctl table output
  • Add missing ipsets statedb command.

The major functional change in cilium/statedb is the switch to our own adaptive radix tree implementation from go-immutable-radix which doubles the insert throughput of StateDB and almost halves memory usage as the tree nodes are now more compact.

Benchmarks (insert of 1000 objects in various txn sizes):

# Before:
BenchmarkDB_WriteTxn_1-32      220941	     5345 ns/op	   187094 objects/sec
BenchmarkDB_WriteTxn_10-32     779038	     1513 ns/op	   660740 objects/sec
BenchmarkDB_WriteTxn_100-32    845493	     1224 ns/op	   816714 objects/sec
BenchmarkDB_WriteTxn_1000-32  1000000	     1276 ns/op	   783520 objects/sec

## reconciler/benchmark:
1000000 objects reconciled in 2.72 seconds (batch size 1000)
Throughput 367442.46 objects per second
Allocated 11020638 objects, 774545kB bytes, 947656kB bytes still in use

# After 
BenchmarkDB_WriteTxn_1-32        599227	      1986 ns/op	   503406 objects/sec
BenchmarkDB_WriteTxn_10-32      1719872	      711.2 ns/op	  1406167 objects/sec
BenchmarkDB_WriteTxn_100-32     2134458	      564.6 ns/op	  1771128 objects/sec
BenchmarkDB_WriteTxn_1000-32    2393395	      523.6 ns/op	  1909838 objects/sec

## reconciler/benchmark:
1000000 objects reconciled in 1.33 seconds (batch size 1000)
Throughput 751258.11 objects per second
Allocated 6011578 objects, 393555kB bytes, 533768kB bytes still in use

Thank you for reviewing 🥰 . It's a lot!

The StateDB in-memory database library was switched to github.com/cilium/statedb with a new much faster radix tree implementation. This is used internally in the cilium-agent for storing and accessing, among others, the network devices and local node IP addresses. This state can be inspected with the "cilium-dbg statedb" commands.
cilium-dbg: Added "statedb ipsets" command
cilium-dbg: "statedb sysctl-settings" is now "statedb sysctl"

@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 Apr 22, 2024
@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch from ac4c439 to 85257f3 Compare April 23, 2024 09:24
@maintainer-s-little-helper

This comment was marked as outdated.

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Apr 23, 2024
@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch from 85257f3 to 5b595a6 Compare April 25, 2024 14:56
@maintainer-s-little-helper

This comment was marked as outdated.

@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch from 5b595a6 to 2cf631b Compare April 29, 2024 14:45
@maintainer-s-little-helper

This comment was marked as outdated.

@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch from 2cf631b to bae5261 Compare April 29, 2024 15:25
@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 Apr 29, 2024
@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch from bae5261 to 3c2bed3 Compare April 29, 2024 15:48
@joamaki joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Apr 29, 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 Apr 29, 2024
@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch 2 times, most recently from cf4f30c to 96c858f Compare May 1, 2024 10:57
@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch 3 times, most recently from 1477f04 to 2033049 Compare May 8, 2024 11:28
@joamaki
Copy link
Contributor Author

joamaki commented May 8, 2024

/test

@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch 4 times, most recently from a6c45f9 to 6dc538c Compare May 9, 2024 08:24
- Remove pkg/statedb and vendor cilium/statedb
- Rename imports of pkg/statedb to cilium/statedb
- Replace the StateDB Swagger API endpoints with the HTTP handler
  implemented in StateDB.
- Fix up the change from  DeleteTracker() to Changes() in StateDB
  API.
- Add metrics adapters for statedb and reconciler

Signed-off-by: Jussi Maki <jussi@isovalent.com>
Use the statedb.Handle instead of statedb.DB in devices controller
so that the DB related metrics are correctly accounted for in
the devices controller.

This serves as an initial example for how to use the named handles.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/joamaki/relocate-statedb branch from 6dc538c to 759f76b Compare May 9, 2024 08:57
@joamaki joamaki changed the title [DRAFT] Switch to cilium/statedb Switch to cilium/statedb May 9, 2024
@joamaki
Copy link
Contributor Author

joamaki commented May 9, 2024

/test

StateDB has switched from gob encoding to JSON encoding
in order to support wider range of different types.
This allows writing custom marshalling to for example transfer
private fields etc.

The "error" type is not JSON marshable (it's an interface type),
so switch to string instead.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki requested review from a team as code owners May 9, 2024 09:49
@joamaki joamaki added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. and removed release-note/misc This PR makes changes that have no direct user impact. labels May 9, 2024
Copy link
Contributor

@marseel marseel left a comment

Choose a reason for hiding this comment

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

approving vendor changes.

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member

@pippolo84 pippolo84 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! 🚀

@joamaki joamaki enabled auto-merge May 9, 2024 14:18
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM for ci-structure

@joamaki joamaki added this pull request to the merge queue May 10, 2024
@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 May 10, 2024
Merged via the queue into cilium:main with commit b15f5dc May 10, 2024
63 of 64 checks passed
@joamaki joamaki deleted the pr/joamaki/relocate-statedb branch May 10, 2024 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants