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

statedb: Fix read-after-write bug #31164

Merged

Conversation

joamaki
Copy link
Contributor

@joamaki joamaki commented Mar 5, 2024

statedb: Fix read-after-write bug

The optimization in *txn to hold the last used index read transaction
to avoid a hash map lookup was broken as the write txn used for reading
was cloned, leading to not being able to read any of the future writes
in the same transaction.

  txn := db.WriteTxn(table)
  table.Insert(txn, Foo{ID: 1})
  table.First(txn, IDIndex.Query(1)) // works as it caches the clone of just created indexWriteTxn
  table.Insert(txn, Foo{ID: 2})
  table.First(txn, IDIndex.Query(2)) // fails as the cloned transaction doesn't see the insert

Benchmark show that this optimization does not actually help, so solve
the problem by removing the optimization.

Before:
BenchmarkDB_RandomLookup-8 1000000 1471 ns/op

After:
BenchmarkDB_RandomLookup-8 1000000 1485 ns/op

Fixes: d0d4d46 ("statedb: Store index unique info in the index tree entry")

Note that v1.14 does not have the "StateDB v2.0", so it is not affected by this.

Fix a bug in the StateDB library that may have caused stale read after write. This may have potentially affected the L2 announcements feature and the node address selection.

The optimization to reuse the last used "indexReadTxn" was broken
when a WriteTxn was first used for a read and then for a write
followed by a read (the last read re-used an old transaction).

Add test to observe this bug.

Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki added release-note/bug This PR fixes an issue in a previous release of Cilium. needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch labels Mar 5, 2024
@joamaki joamaki requested a review from bimmlerd March 5, 2024 15:33
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from main in 1.15.2 Mar 5, 2024
@joamaki
Copy link
Contributor Author

joamaki commented Mar 5, 2024

/test

The optimization in *txn to hold the last used index read transaction
to avoid a hash map lookup was broken as the write txn used for reading
was cloned, leading to not being able to read any of the future writes
in the same transaction.

Benchmark show that this optimization does not actually help, so solve
the problem by removing the optimization.

Before:
BenchmarkDB_RandomLookup-8       1000000              1471 ns/op
After:
BenchmarkDB_RandomLookup-8       1000000              1485 ns/op

Fixes: d0d4d46 ("statedb: Store index unique info in the index tree entry")
Signed-off-by: Jussi Maki <jussi@isovalent.com>
@joamaki joamaki force-pushed the pr/joamaki/fix-statedb-read-after-write branch from cb3839f to a3cf05e Compare March 5, 2024 16:29
@joamaki
Copy link
Contributor Author

joamaki commented Mar 6, 2024

/test

@joamaki joamaki marked this pull request as ready for review March 6, 2024 08:01
@joamaki joamaki requested a review from a team as a code owner March 6, 2024 08:01
@joamaki joamaki enabled auto-merge March 6, 2024 08:31
@joamaki joamaki added this pull request to the merge queue Mar 6, 2024
Merged via the queue into cilium:main with commit 52944a0 Mar 6, 2024
61 of 63 checks passed
@joamaki joamaki deleted the pr/joamaki/fix-statedb-read-after-write branch March 6, 2024 13:23
@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 6, 2024
@jibi jibi mentioned this pull request Mar 12, 2024
16 tasks
@jibi jibi 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 12, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from main to Backport pending to v1.15 in 1.15.2 Mar 12, 2024
@jibi jibi added needs-backport/1.15 This PR / issue needs backporting to the v1.15 branch and removed backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. labels Mar 13, 2024
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.15 to Needs backport from main in 1.15.2 Mar 13, 2024
@jrajahalme jrajahalme added this to Needs backport from main in 1.15.3 Mar 13, 2024
@jrajahalme jrajahalme removed this from Needs backport from main in 1.15.2 Mar 13, 2024
@jibi jibi 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 13, 2024
@github-actions github-actions bot removed the backport-pending/1.15 The backport for Cilium 1.15.x for this PR is in progress. label Mar 16, 2024
@github-actions github-actions bot added the backport-done/1.15 The backport for Cilium 1.15.x for this PR is done. label Mar 16, 2024
@jrajahalme jrajahalme moved this from Needs backport from main to Backport done to v1.15 in 1.15.3 Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
No open projects
1.15.3
Backport done to v1.15
Development

Successfully merging this pull request may close these issues.

None yet

3 participants