Fix delete inconsistencies in read buffer #17195

siyuanfoundation · 2024-01-03T18:24:38Z

Please read https://github.com/etcd-io/etcd/blob/main/CONTRIBUTING.md#contribution-flow.

The PR includes fix for

UnsafeDelete (*batchTx) UnsafeDelete may break the linearizability #17158

siyuanfoundation · 2024-01-03T19:33:41Z

cc @ahrtr @serathius

server/storage/backend/tx_buffer.go

server/storage/backend/batch_tx_test.go

server/storage/backend/batch_tx.go

ahrtr · 2024-01-04T20:14:55Z

The PR includes fix for

UnsafeDelete (*batchTx) UnsafeDelete may break the linearizability #17158

I found another problem if there is duplicate keys in the same transaction (overwrite).

This PR is changing one of the critical parts of etcd, please let's breakdown it into two separate PRs. Each only fixes one problem. For each PR, the first commit only includes test cases to reproduce the issue you are going to fix in the 2nd or 3rd commits. Ensure you can reproduce the issue before you adding the 2nd or 3rd commit/patch, and can't reproduce it anymore after adding the patch.

siyuanfoundation · 2024-01-05T02:52:49Z

/retest

fuweid

LGTM

serathius · 2024-01-05T08:52:42Z

This PR is changing one of the critical parts of etcd, please let's breakdown it into two separate PRs. Each only fixes one problem. For each PR, the first commit only includes test cases to reproduce the issue you are going to fix in the 2nd or 3rd commits. Ensure you can reproduce the issue before you adding the 2nd or 3rd commit/patch, and can't reproduce it anymore after adding the patch.

Agree with that, we need to make sure that each of those issues are properly understood and tested. Having a single PR for both adds unnecessary cognitive overhead.

server/storage/backend/batch_tx.go

serathius · 2024-01-05T10:52:38Z

server/storage/backend/batch_tx.go

-		if t.pending >= t.backend.batchLimit {
+		// We need to commit the transaction if
+		// 1. t.pending >= t.backend.batchLimit for performance,
+		// 	  because We don't want buffer to grow too big as it has worse complexity.


That's my current understanding, however would be good to confirm. Could you check PRs when the batchlimit was introduced and see what was the motivation back then. Referencing PR would be also useful.

cc @ahrtr

this is one relevant pr #10283

serathius · 2024-01-05T11:01:33Z

server/storage/backend/batch_tx_test.go

+	checkForEachResponseMatch(t, b.BatchTx(), b.ReadTx(), schema.Test, nil, nil)
+}
+
+func checkRangeResponseMatch(t *testing.T, tx backend.BatchTx, rtx backend.ReadTx, key, endKey []byte, limit int64, expectedKeys, expectedValues [][]byte) {


By adding validation of expected values you mixed assertion based testing with property based testing. Let's pick one of them.

Originally I implemented the test as property testing. The property I was testing was "ReadTx should always return the same response as BatchTX". This property should always hold, no matter what operations are executed before. I did it to allow make validation independent from test scenario and potentially randomize operations before validation.

Recent changes to the PR added expectedKeys and expectedValues testing, this means you hardeded the validation to specific cases. This is still correct, however the code can be much more simplified.

In unit test, the test cases are hard coded. I think it is important to test the actual key and values in case ReadTx and BatchTX make the same mistake.
Changed the code to just check the results in checkForEach

server/storage/backend/batch_tx.go

ahrtr · 2024-01-06T17:00:06Z

tests/integration/clientv3/user_test.go

@@ -55,6 +55,66 @@ func TestUserError(t *testing.T) {
 	}
 }

+func TestAddUserAfterDelete(t *testing.T) {


Have you confirmed that the test case can reproduce the issue?

Confirmed the test case if the defensive commit is removed. Because auth force commits for every change, this is not actually a problem for auth right now.

I am adding this test for completeness.

Wow, nice find. This defensive commit seems scary, similar to #17119 we noticed some inconsistency and didn't root cause it.

I think we should consider a followup where we add adding verifiers #17158 (comment), remove the defensive commit and see if we find anything.

tests/integration/clientv3/user_test.go

tests/integration/member_test.go

siyuanfoundation · 2024-01-09T20:51:40Z

/retest

server/etcdserver/api/cluster.go

Signed-off-by: Siyuan Zhang <sizhang@google.com>

siyuanfoundation · 2024-01-10T18:53:07Z

/retest

ahrtr

lgtm

Could you backport the fix to 3.5 and 3.4? thx

siyuanfoundation force-pushed the txBuf1 branch from fa874cb to c6fc7bb Compare January 3, 2024 18:32

fuweid reviewed Jan 4, 2024

View reviewed changes

server/storage/backend/tx_buffer.go Outdated Show resolved Hide resolved

server/storage/backend/batch_tx_test.go Outdated Show resolved Hide resolved

serathius reviewed Jan 4, 2024

View reviewed changes

server/storage/backend/batch_tx_test.go Outdated Show resolved Hide resolved

serathius reviewed Jan 4, 2024

View reviewed changes

server/storage/backend/batch_tx.go Outdated Show resolved Hide resolved

serathius reviewed Jan 4, 2024

View reviewed changes

server/storage/backend/batch_tx.go Outdated Show resolved Hide resolved

serathius reviewed Jan 4, 2024

View reviewed changes

server/storage/backend/batch_tx.go Outdated Show resolved Hide resolved

siyuanfoundation force-pushed the txBuf1 branch from c6fc7bb to 9600592 Compare January 4, 2024 19:47

siyuanfoundation force-pushed the txBuf1 branch 2 times, most recently from 2828c9e to 656fae1 Compare January 5, 2024 00:51

siyuanfoundation changed the title ~~Fix some inconsistencies in read buffer~~ Fix delete inconsistencies in read buffer Jan 5, 2024

fuweid approved these changes Jan 5, 2024

View reviewed changes

siyuanfoundation requested review from ahrtr and serathius January 5, 2024 03:37

serathius reviewed Jan 5, 2024

View reviewed changes

server/storage/backend/batch_tx.go Outdated Show resolved Hide resolved

serathius reviewed Jan 5, 2024

View reviewed changes

siyuanfoundation force-pushed the txBuf1 branch from 656fae1 to 8c14477 Compare January 6, 2024 00:02

ahrtr reviewed Jan 6, 2024

View reviewed changes

ahrtr mentioned this pull request Jan 8, 2024

Plans to release 3.4.29 #17207

Closed

4 tasks

siyuanfoundation force-pushed the txBuf1 branch 3 times, most recently from 923a035 to 1a249b1 Compare January 9, 2024 00:48

serathius reviewed Jan 9, 2024

View reviewed changes

tests/integration/member_test.go Outdated Show resolved Hide resolved

siyuanfoundation requested a review from ahrtr January 9, 2024 19:29

siyuanfoundation force-pushed the txBuf1 branch from 1a249b1 to 906e962 Compare January 9, 2024 19:39

siyuanfoundation requested a review from serathius January 9, 2024 20:52

serathius approved these changes Jan 10, 2024

View reviewed changes

ahrtr reviewed Jan 10, 2024

View reviewed changes

server/etcdserver/api/cluster.go Outdated Show resolved Hide resolved

siyuanfoundation added 2 commits January 10, 2024 09:33

add tests to test tx delete consistency.

b3a5307

Signed-off-by: Siyuan Zhang <sizhang@google.com>

commit bbolt transaction if there is any pending deleting operations

db61c96

Signed-off-by: Siyuan Zhang <sizhang@google.com>

siyuanfoundation force-pushed the txBuf1 branch from 906e962 to db61c96 Compare January 10, 2024 17:42

siyuanfoundation requested a review from ahrtr January 10, 2024 17:51

ahrtr approved these changes Jan 11, 2024

View reviewed changes

ahrtr merged commit b3bf59a into etcd-io:main Jan 11, 2024
39 checks passed

This was referenced Jan 11, 2024

Commit bbolt transaction if there is any pending deleting operations #17157

Closed

Commit transaction for each configuration change #17119

Closed

Reproduce readTxn breaks read commited on delete #17124

Closed

This was referenced Jan 11, 2024

[3.5] Fix delete inconsistencies in read buffer #17230

Merged

[3.4] Fix delete inconsistencies in read buffer #17238

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delete inconsistencies in read buffer #17195

Fix delete inconsistencies in read buffer #17195

siyuanfoundation commented Jan 3, 2024 •

edited

Loading

siyuanfoundation commented Jan 3, 2024

ahrtr commented Jan 4, 2024 •

edited

Loading

siyuanfoundation commented Jan 5, 2024

fuweid left a comment

serathius commented Jan 5, 2024

serathius Jan 5, 2024

siyuanfoundation Jan 6, 2024

serathius Jan 5, 2024

siyuanfoundation Jan 9, 2024

ahrtr Jan 6, 2024

siyuanfoundation Jan 8, 2024

serathius Jan 9, 2024 •

edited

Loading

siyuanfoundation commented Jan 9, 2024

siyuanfoundation commented Jan 10, 2024

ahrtr left a comment

Fix delete inconsistencies in read buffer #17195

Fix delete inconsistencies in read buffer #17195

Conversation

siyuanfoundation commented Jan 3, 2024 • edited Loading

siyuanfoundation commented Jan 3, 2024

ahrtr commented Jan 4, 2024 • edited Loading

siyuanfoundation commented Jan 5, 2024

fuweid left a comment

Choose a reason for hiding this comment

serathius commented Jan 5, 2024

serathius Jan 5, 2024

Choose a reason for hiding this comment

siyuanfoundation Jan 6, 2024

Choose a reason for hiding this comment

serathius Jan 5, 2024

Choose a reason for hiding this comment

siyuanfoundation Jan 9, 2024

Choose a reason for hiding this comment

ahrtr Jan 6, 2024

Choose a reason for hiding this comment

siyuanfoundation Jan 8, 2024

Choose a reason for hiding this comment

serathius Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

siyuanfoundation commented Jan 9, 2024

siyuanfoundation commented Jan 10, 2024

ahrtr left a comment

Choose a reason for hiding this comment

siyuanfoundation commented Jan 3, 2024 •

edited

Loading

ahrtr commented Jan 4, 2024 •

edited

Loading

serathius Jan 9, 2024 •

edited

Loading