Conversation
|
Commit 8372713aeee883bda04ba3e913b30b127b0c1794 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 |
0568840 to
73333ae
Compare
|
Commit 8372713aeee883bda04ba3e913b30b127b0c1794 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 |
73333ae to
7f6c412
Compare
To allow control over where a table can be modified, split the Table[Obj] interface into reader (Table[Obj]) and writer (RWTable[Obj]) parts. RWTable[Obj] is a superset of Table[Obj], and can thus be used everywhere where a Table[Obj] is required. For convenience, implement NewProtectedTableCell() to provide the RWTable[Obj] only in the hive module's scope, and Table[Obj] globally. With this, the cell defining the table can choose to either not allow modifications at all to the table (tests can still go via NewTable() to create a modifiable table), or only allow modifications under very controller and validated ways. Signed-off-by: Jussi Maki <[email protected]>
Make RWTable[*Device] and RWTable[*Route] private to the devices controller to make sure they're not modified by anyone else. The DeviceTableCell and RouteTableCell are still available in the tables package for tests to use. Signed-off-by: Jussi Maki <[email protected]>
These allow efficiently doing optimistic concurrency control in
cases where a long-running computation is performed against the
data and then results written back to a table, e.g.
1. With ReadTxn compute results for each object (with revision number)
2. With WriteTxn commit the results using CompareAndDelete. If the
object has changed, the stale result for that object is ignored.
3. Wait for changes and repeat
Signed-off-by: Jussi Maki <[email protected]>
Refresh of statedb-split-write-methods
Benchmark the delay and throughput for propagating changes from one table
to another. This simulates the expected use-case for statedb where we would
be reading from one or more tables and computing desired state that is written
to another table.
Results on my machine:
goos: linux
goarch: amd64
pkg: github.com/cilium/cilium/pkg/statedb
cpu: AMD Ryzen 9 5950X 16-Core Processor
BenchmarkDB_PropagationDelay 224287 5071 ns/op
--- BENCH: BenchmarkDB_PropagationDelay
benchmarks_test.go:349: Propagation delay (N: 224287) 50p: 37.12µs, 90p: 91.633µs, 99p: 123.924µs
On a single core, 197k objects per second with batch size of 10. For batch size of 1
I got 61k objects per second, so even small increase in batch size oversets the
cost of waiting for the watch channel to close. Propagating the batch
took ~123 microseconds (99 percentile).
Signed-off-by: Jussi Maki <[email protected]>
Having access to the module identifier is useful for writing generic cells that are meant to be embedded into a module. This provides the module identifier as 'cell.ModuleId' scoped to the module. Signed-off-by: Jussi Maki <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
Signed-off-by: Jussi Maki <[email protected]>
9cea8ff to
5361786
Compare
Signed-off-by: Jussi Maki <[email protected]>
c6f3bf7 to
550cfd8
Compare
The device name hacks in DesiredRoute makes it impossible to compare actual and desired routes. Instead resolve index in InsertLegacy/DeleteLegacy. Signed-off-by: Jussi Maki <[email protected]>
aanm
left a comment
There was a problem hiding this comment.
I've reviewed on a per-commit basis.
For some of my comments please leave the reply in the source code as well since people that will use the code examples as a base won't have the same questions.
| SysctlKeyIndex = statedb.Index[*SysctlSetting, string]{ | ||
| Name: "key", | ||
| FromObject: func(s *SysctlSetting) index.KeySet { | ||
| return index.NewKeySet(index.String(s.Key)) | ||
| }, | ||
| FromKey: index.String, | ||
| Unique: true, | ||
| } |
There was a problem hiding this comment.
Since this is an example, can we have comments for each field of this struct initialization for better understanding?
There was a problem hiding this comment.
Yes! Also made me realize that the Index struct itself needs much better docs.
| FromKey: index.String, | ||
| Unique: true, | ||
| } | ||
| SysctlStatusIndex = reconciler.NewStatusIndex[*SysctlSetting]() |
There was a problem hiding this comment.
Can we also have a comment of why we need an "indexer"? Or why do we need this indexer?
There was a problem hiding this comment.
Yes definitely! This is still a very messy draft so many things like that missing. I’ll make sure whatever the final ”canonical example” ends up being will include these.
| // afterwards. If the object has changed in the meanwhile the status update for | ||
| // the old object is skipped. |
There was a problem hiding this comment.
The reconciliation status is per object does this mean that if the commit fails it will fail for all objects or just for the object that has changed?
There was a problem hiding this comment.
Just for the object. CompareAndSwap does nothing if the revision doesn’t match
pkg/reconciler/reconciler.go
Outdated
|
|
||
| var err error | ||
| if status.Delete { | ||
| err = r.Target.Delete(obj) |
There was a problem hiding this comment.
Please add a comment in this line stating that this is the business logic for "deletes". It was not obvious on a first sight.
pkg/reconciler/reconciler.go
Outdated
| updateResults[obj] = result{rev, StatusError(true, err)} | ||
| } | ||
| } else { | ||
| err = r.Target.Update(obj) |
There was a problem hiding this comment.
Please add a comment in this line stating that this is the business logic for "updates". It was not obvious on a first sight.
| } | ||
|
|
||
| func modifyIPTables(db *statedb.DB, t statedb.RWTable[*Rule]) { | ||
| go func() { |
There was a problem hiding this comment.
Because one can’t block in Invoke. I’ll make this example proper by using pkg/hive/job for these.
pkg/reconciler/example/iptables.go
Outdated
| // TODO: | ||
| // - "iptables-save" and parse? Or "-C" on each rule? | ||
| // - Check that cilium chains exist | ||
| // - Find unexpected rules in cilium chains? | ||
| return false, nil |
There was a problem hiding this comment.
Ah, I was really curious to see this one 😅
| // | ||
| // Rule | ||
| // |
There was a problem hiding this comment.
Do we need to define all types of "rules" "jump"? Can't the state be the full iptables command? For example, the desired state is -A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE and the realized state is basically a execution of iptables -A POSTROUTING -s 172.17.0.0/16 ! -o docker0 -j MASQUERADE.
There was a problem hiding this comment.
I think I shouldn’t have left this very unfinished example in as there’s too many open questions in it..
pkg/reconciler/example/route.go
Outdated
| var errs []error | ||
|
|
||
| for desired, _, ok := iter.Next(); ok; desired, _, ok = iter.Next() { | ||
| actual, _, ok := t.routes.First(txn, tables.RouteIDIndex.Query(desired.RouteID())) |
There was a problem hiding this comment.
Because we’re only interested in the first match. StateDB supports both unique and non-unique indexes and doesn’t differentiate the query API for these.
pkg/reconciler/example/route.go
Outdated
| @@ -0,0 +1,419 @@ | |||
| package main | |||
There was a problem hiding this comment.
The routes reconciler seems to be the hardest to understand because I couldn't find the "reconciliation" logic to fetch the realized state and do the comparison between the desired and the realized state. The sysctl example was easier on this aspect.
There was a problem hiding this comment.
Look again, it’s looking up the ”actual” route from the route table maintained by the devices controller (which pulls routes over netlink). This way it can efficiently reconcile a lot of routes without having to do individual syscalls to fetch routes one by one.
But indeed, it makes it harder to follow as it depends on this outside realized state table that’s populated elsewhere. The final version will need to document this well. Especially how it deals with acting on potentially stale ”realized state” data (that’s where full reconciliation is needed).
Signed-off-by: Jussi Maki <[email protected]>
- Replace Sync with Update&Prune. This allows reusing Update() so most reconcilers don't have to implement things twice. Prune for most reconcilers is a no-op. - Use string for error in Status as 'error' doesn't serialize. Likely should fix with a custom JSON marshalling instead, so that Status.Error can be used with e.g. errors.Is. - Update desired object status also in full reconciliation Signed-off-by: Jussi Maki <[email protected]>
550cfd8 to
e88757c
Compare
|
This pull request has been automatically marked as stale because it |
|
This pull request has not seen any activity since it was marked stale. |
Builds on top of #28140.
This implements a "generic reconciler" which takes the desired state as a StateDB table
and a "reconciliation target" that defines the reconciliation operations. It performs
fast incremental reconciliation of changes and periodic full reconciliation. Metrics
and health reporting are included.
pkg/reconciler/examplehas some rough early examples for sysctl and iptables.By wrapping the reconciliation into a reusable generic utility we gain:
Summary of the API:
Usage:
Health reporting:
Metrics: