kvcoord: test additional ambiguous failure states #108001

AlexTalks · 2023-08-02T03:27:21Z

This change builds on the testing introduced in #107323 with additional
subtests evaluating the behavior of different race outcomes with
contended transactions where the first transaction experiences an RPC
failure (i.e. an ambiguous write).

Depends on #107323, #108154.

Part of: #103817

Release note: None

cockroach-teamcity · 2023-08-02T03:27:30Z

This change is

While previously, RPC failures were assumed to be retriable, as write operations (with the notable exception of `EndTxn`) were assumed to be idempotent, it has been seen in cockroachdb#67765 and documented in cockroachdb#103817 that RPC failures on write operations that occur in parallel with a commit (i.e. a partial batch where `withCommit==true`), it is not always possible to assume idempotency and retry the "ambiguous" writes. This is due to the fact that the retried write RPC could result in the transaction's `WriteTimestamp` being bumped, changing the commit timestamp of the transaction that may in fact already be implicitly committed if the initial "ambiguous" write actually succeeded. This change modifies the protocol of the DistSender to flag in subsequent retries that a batch with a commit has previously experienced ambiguity, as well as the handling of the retried write in the MVCC layer to detect this previous ambiguity and reject retries that change the write timestamp as a non-idempotent replay. The flag allows subsequent retries to "remember" the earlier ambiguous write and evaluate accordingly. The flag allows us to properly handle RPC failures (i.e. ambiguous writes) that occur on commit, as a transaction that is implicitly committed is eligible to be marked as explicitly committed by contending transactions via the `RecoverTxn` operation, resulting in a race between retries by the transaction coordinator and recovery by contending transactions that could result in either incorrectly reporting a transaction as having failed with a `RETRY_SERIALIZABLE` error (despite the possibility that it already was or could be recovered and successfully committed), or in attempting to explicitly commit an already-recovered and committed transaction, resulting in seeing an assertion failure due to `transaction unexpectedly committed`. The replay protection introduced here allows us to avoid both of these situations by detecting a replay that should be considered non-idempotent and returning an error, causing the original RPC error remembered by the DistSender to be propagated as an `AmbiguousResultError`. As such, this can be handled by application code by validating the success/failure of a transaction when receiving this error. Depends on cockroachdb#107680, cockroachdb#107323, cockroachdb#108154, cockroachdb#108001 Fixes: cockroachdb#103817 Release note (bug fix): Properly handles RPC failures on writes using the parallel commit protocol that execute in parallel to the commit operation, avoiding incorrect retriable failures and `transaction unexpectedly committed` assertions by detecting when writes cannot be retried idempotently, instead returning an `AmbiguousResultError`.

arulajmani

Nice job with the thorough tests here!

It seems like there's a bit of copy pasting that's going on between each subtest. I'm wondering if we can get rid of some of it, as it'll make these tests much more readable. One opportunity, like I mentioned below, that comes to mind is to use a table driven test where we inject different "txn2"s for each subtest. That'll allow us to condense 3 variants (writer-reader conflict, writer-writer conflict, and workload) into one table driven test.

Maybe there's something we can do for some of the other variants as well, by grouping some test cases together? What do you think?

Reviewed all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @AlexTalks)

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 558 at r3 (raw file):

			tCtx := context.Background()

			// txn2 just performs a simple GetForUpdate on a conflicting key, causing

Curious, is there a reason for the locking Get? Or would a non-locking Get have worked just as well?

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 692 at r3 (raw file):

			tCtx := context.Background()

			// txn2 performs simple Puts on conflicting keys, causing it to issue a

There's a bit of copy pasting between this subtest, the writer-reader subtest, and the workload conflict subtest.

I think the only thing that's changing between these 3 subtests is txn2. Would it be possible to inject txn2 into each of these 3 subtests, instead of copy pasting the entire subtest 3 times? Maybe a table driven test like:

  
testCases := []struct{  
 name string  
 txn2 func(wg sync.WaitGroup, txn2Ready chan struct{})  
}{  
 ...  
}

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 967 at r3 (raw file):

			tCtx := context.Background()
			txn := db.NewTxn(tCtx, "txn1")
			vals := getInBatch(tCtx, txn, keyA, keyB, keyAPrime)

Could this test be fit into the table driven testCases approach if we made txn1 a parameter as well?

AlexTalks

Reviewed 1 of 1 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 558 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Curious, is there a reason for the locking Get? Or would a non-locking Get have worked just as well?

Done - I don't think a GetForUpdate is required here, this was just so I could reuse the function that does the GetForUpdates in a batch, but we can certainly change this to be a simple txn.Get...

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 692 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

There's a bit of copy pasting between this subtest, the writer-reader subtest, and the workload conflict subtest.

I think the only thing that's changing between these 3 subtests is txn2. Would it be possible to inject txn2 into each of these 3 subtests, instead of copy pasting the entire subtest 3 times? Maybe a table driven test like:
  
testCases := []struct{  
 name string  
 txn2 func(wg sync.WaitGroup, txn2Ready chan struct{})  
}{  
 ...  
}

Done - was able to do this, but only for the first three, which have identical concurrent operations (txn1/txn2/lease mover), and (nearly) identical request schedules.

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 967 at r3 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Could this test be fit into the table driven testCases approach if we made txn1 a parameter as well?

Unfortunately no, the schedule of intercepted requests is different in each of the cases apart from the first three, as are the signals/checkpoints in each test.

Keep in mind, the goal is to eventually transition to a data-driven test framework - this is meant to provide intermediary coverage for the immediately following fix PR. So while it is important for these tests to be stable, not flakey, and cover the various scenarios possible, the framework itself should be considered a work in progress (and should hopefully not block the fix).

All that is to say, I'm not sure rewriting the framework again now should be the priority.

AlexTalks

Refactored the ones with identical schedules, but I'm not sure it's reasonable for the other ones at this stage (see comments inline).

The maybeWait functions have some similarities, but each are distinct as each test examines the result of a slightly different operation schedule. As such, the framework was rewritten in this way from earlier feedback that said following the request interception flow was too confusing when it was separated from the test itself, and it would be much easier to follow if we could trace the flow of operations in order for each test.

Hence, refactoring this to fit into a table driven test would require allowing declaration of generalized named checkpoints (ie channels), and defining a way to write the order of operations as scheduled in the maybeWait functions into the table-driven test - which may still end up making these things more confusing. I'd like to defer further enhancements like this to our discussed work on a higher-level data-driven test framework, if that makes sense.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)

While previously, RPC failures were assumed to be retriable, as write operations (with the notable exception of `EndTxn`) were assumed to be idempotent, it has been seen in cockroachdb#67765 and documented in cockroachdb#103817 that RPC failures on write operations that occur in parallel with a commit (i.e. a partial batch where `withCommit==true`), it is not always possible to assume idempotency and retry the "ambiguous" writes. This is due to the fact that the retried write RPC could result in the transaction's `WriteTimestamp` being bumped, changing the commit timestamp of the transaction that may in fact already be implicitly committed if the initial "ambiguous" write actually succeeded. This change modifies the protocol of the DistSender to flag in subsequent retries that a batch with a commit has previously experienced ambiguity, as well as the handling of the retried write in the MVCC layer to detect this previous ambiguity and reject retries that change the write timestamp as a non-idempotent replay. The flag allows subsequent retries to "remember" the earlier ambiguous write and evaluate accordingly. The flag allows us to properly handle RPC failures (i.e. ambiguous writes) that occur on commit, as a transaction that is implicitly committed is eligible to be marked as explicitly committed by contending transactions via the `RecoverTxn` operation, resulting in a race between retries by the transaction coordinator and recovery by contending transactions that could result in either incorrectly reporting a transaction as having failed with a `RETRY_SERIALIZABLE` error (despite the possibility that it already was or could be recovered and successfully committed), or in attempting to explicitly commit an already-recovered and committed transaction, resulting in seeing an assertion failure due to `transaction unexpectedly committed`. The replay protection introduced here allows us to avoid both of these situations by detecting a replay that should be considered non-idempotent and returning an error, causing the original RPC error remembered by the DistSender to be propagated as an `AmbiguousResultError`. As such, this can be handled by application code by validating the success/failure of a transaction when receiving this error. Depends on cockroachdb#107680, cockroachdb#107323, cockroachdb#108154, cockroachdb#108001 Fixes: cockroachdb#103817 Release note (bug fix): Properly handles RPC failures on writes using the parallel commit protocol that execute in parallel to the commit operation, avoiding incorrect retriable failures and `transaction unexpectedly committed` assertions by detecting when writes cannot be retried idempotently, instead returning an `AmbiguousResultError`.

arulajmani

modulo comments.

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @AlexTalks)

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 692 at r3 (raw file):

Previously, AlexTalks (Alex Sarkesian) wrote…

Done - was able to do this, but only for the first three, which have identical concurrent operations (txn1/txn2/lease mover), and (nearly) identical request schedules.

Ack, thanks for pulling the ones we could here. It's a huge readability win 💯

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 705 at r4 (raw file):

	}

	for _, variant := range basicVariants {

nit: let's pull the basicVariants definition right above this loop.

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 917 at r4 (raw file):

			}

			// 5. txn3->n2: Put(c), EndTxn(parallel commit)

nit: I think txn3 should be able to hit 1PC, so maybe txn3->n2: 1PC.

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 998 at r4 (raw file):

			"expected no AssertionFailedError due to sanity check")
	})
	t.Run("recovery after transfer lease", func(t *testing.T) {

Is there something special about this test case (over the transfer lease happens after recovery variants) or are we just adding it for completeness? Either way is fine, but maybe let's add a comment explaining why for future readers.

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 1099 at r4 (raw file):

			"expected AssertionFailedError due to sanity check on transaction already committed")
	})
	t.Run("retry sees other intent", func(t *testing.T) {

Similar to my comment above, a high level comment explaining why this test case is interesting would be helpful.

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 1146 at r4 (raw file):

			// 9. txn2->n1: Put(a), EndTxn(parallel commit) -- Writes intent.
			// 10. txn2->n1: Put(b)
			// 11. txn2->n1: EndTxn(commit)

This is a bit subtle -- we only expect this to happen if the parallel commit attempt failed, but we were able to retry it. Should we add a comment explaining this?

This change builds on the testing introduced in cockroachdb#107323 with additional subtests evaluating the behavior of different race outcomes with contended transactions where the first transaction experiences an RPC failure (i.e. an ambiguous write). Part of: cockroachdb#103817 Release note: None

AlexTalks

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani)

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 917 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: I think txn3 should be able to hit 1PC, so maybe txn3->n2: 1PC.

I think you're right, good catch ;-) Added comment

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 998 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Is there something special about this test case (over the transfer lease happens after recovery variants) or are we just adding it for completeness? Either way is fine, but maybe let's add a comment explaining why for future readers.

This one was specifically because you asked about it actually, heh - I'll add a comment but if we don't think it's necessary I can remove it.

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 1099 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

Similar to my comment above, a high level comment explaining why this test case is interesting would be helpful.

Done.

pkg/kv/kvclient/kvcoord/dist_sender_ambiguous_test.go line 1146 at r4 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

This is a bit subtle -- we only expect this to happen if the parallel commit attempt failed, but we were able to retry it. Should we add a comment explaining this?

I don't think it's super relevant - even though it's asynchronous, it still happens - but I added a comment.

AlexTalks · 2023-08-31T22:45:09Z

bors r+

craig · 2023-08-31T23:54:33Z

Build succeeded:

Bazel Essential CI (Cockroach)

While previously, RPC failures were assumed to be retryable, as write operations (with the notable exception of `EndTxn`) were assumed to be idempotent, it has been seen in cockroachdb#67765 and documented in cockroachdb#103817 that RPC failures on write operations that occur in parallel with a commit (i.e. a partial batch where `withCommit==true`), it is not always possible to assume idempotency and retry the "ambiguous" writes. This is due to the fact that the retried write RPC could result in the transaction's `WriteTimestamp` being bumped, changing the commit timestamp of the transaction that may in fact already be implicitly committed if the initial "ambiguous" write actually succeeded. This change modifies the protocol of the DistSender to flag in subsequent retries that a batch with a commit has previously experienced ambiguity, as well as the handling of the retried write in the MVCC layer to detect this previous ambiguity and reject retries that change the write timestamp as a non-idempotent replay. The flag allows subsequent retries to "remember" the earlier ambiguous write and evaluate accordingly. The flag allows us to properly handle RPC failures (i.e. ambiguous writes) that occur on commit, as a transaction that is implicitly committed is eligible to be marked as explicitly committed by contending transactions via the `RecoverTxn` operation, resulting in a race between retries by the transaction coordinator and recovery by contending transactions that could result in either incorrectly reporting a transaction as having failed with a `RETRY_SERIALIZABLE` error (despite the possibility that it already was or could be recovered and successfully committed), or in attempting to explicitly commit an already-recovered and committed transaction, resulting in seeing an assertion failure due to `transaction unexpectedly committed`. The replay protection introduced here allows us to avoid both of these situations by detecting a replay that should be considered non-idempotent and returning an error, causing the original RPC error remembered by the DistSender to be propagated as an `AmbiguousResultError`. As such, this can be handled by application code by validating the success/failure of a transaction when receiving this error. Depends on cockroachdb#107680, cockroachdb#107323, cockroachdb#108154, cockroachdb#108001 Fixes: cockroachdb#103817 Release note (bug fix): Properly handles RPC failures on writes using the parallel commit protocol that execute in parallel to the commit operation, avoiding incorrect retryable failures and `transaction unexpectedly committed` assertions by detecting when writes cannot be retried idempotently, instead returning an `AmbiguousResultError`.

107658: kv: enable replay protection for ambiguous writes on commits r=AlexTalks a=AlexTalks While previously, RPC failures were assumed to be retriable, as write operations (with the notable exception of `EndTxn`) were assumed to be idempotent, it has been seen in #67765 and documented in #103817 that RPC failures on write operations that occur in parallel with a commit (i.e. a partial batch where `withCommit==true`), it is not always possible to assume idempotency and retry the "ambiguous" writes. This is due to the fact that the retried write RPC could result in the transaction's `WriteTimestamp` being bumped, changing the commit timestamp of the transaction that may in fact already be implicitly committed if the initial "ambiguous" write actually succeeded. This change modifies the protocol of the DistSender to flag in subsequent retries that a batch with a commit has previously experienced ambiguity, as well as the handling of the retried write in the MVCC layer to detect this previous ambiguity and reject retries that change the write timestamp as a non-idempotent replay. The flag allows subsequent retries to "remember" the earlier ambiguous write and evaluate accordingly. The flag allows us to properly handle RPC failures (i.e. ambiguous writes) that occur on commit, as a transaction that is implicitly committed is eligible to be marked as explicitly committed by contending transactions via the `RecoverTxn` operation, resulting in a race between retries by the transaction coordinator and recovery by contending transactions that could result in either incorrectly reporting a transaction as having failed with a `RETRY_SERIALIZABLE` error (despite the possibility that it already was or could be recovered and successfully committed), or in attempting to explicitly commit an already-recovered and committed transaction, resulting in seeing an assertion failure due to `transaction unexpectedly committed`. The replay protection introduced here allows us to avoid both of these situations by detecting a replay that should be considered non-idempotent and returning an error, causing the original RPC error remembered by the DistSender to be propagated as an `AmbiguousResultError`. As such, this can be handled by application code by validating the success/failure of a transaction when receiving this error. Depends on #107680, #107323, #108154, #108001 Fixes: #103817 Release note (bug fix): Properly handles RPC failures on writes using the parallel commit protocol that execute in parallel to the commit operation, avoiding incorrect retriable failures and `transaction unexpectedly committed` assertions by detecting when writes cannot be retried idempotently, instead returning an `AmbiguousResultError`. Co-authored-by: Alex Sarkesian <[email protected]>

While previously, RPC failures were assumed to be retryable, as write operations (with the notable exception of `EndTxn`) were assumed to be idempotent, it has been seen in cockroachdb#67765 and documented in cockroachdb#103817 that RPC failures on write operations that occur in parallel with a commit (i.e. a partial batch where `withCommit==true`), it is not always possible to assume idempotency and retry the "ambiguous" writes. This is due to the fact that the retried write RPC could result in the transaction's `WriteTimestamp` being bumped, changing the commit timestamp of the transaction that may in fact already be implicitly committed if the initial "ambiguous" write actually succeeded. This change modifies the protocol of the DistSender to flag in subsequent retries that a batch with a commit has previously experienced ambiguity, as well as the handling of the retried write in the MVCC layer to detect this previous ambiguity and reject retries that change the write timestamp as a non-idempotent replay. The flag allows subsequent retries to "remember" the earlier ambiguous write and evaluate accordingly. The flag allows us to properly handle RPC failures (i.e. ambiguous writes) that occur on commit, as a transaction that is implicitly committed is eligible to be marked as explicitly committed by contending transactions via the `RecoverTxn` operation, resulting in a race between retries by the transaction coordinator and recovery by contending transactions that could result in either incorrectly reporting a transaction as having failed with a `RETRY_SERIALIZABLE` error (despite the possibility that it already was or could be recovered and successfully committed), or in attempting to explicitly commit an already-recovered and committed transaction, resulting in seeing an assertion failure due to `transaction unexpectedly committed`. The replay protection introduced here allows us to avoid both of these situations by detecting a replay that should be considered non-idempotent and returning an error, causing the original RPC error remembered by the DistSender to be propagated as an `AmbiguousResultError`. As such, this can be handled by application code by validating the success/failure of a transaction when receiving this error. Depends on cockroachdb#107680, cockroachdb#107323, cockroachdb#108154, cockroachdb#108001 Fixes: cockroachdb#103817 Release note (bug fix): Properly handles RPC failures on writes using the parallel commit protocol that execute in parallel to the commit operation, avoiding incorrect retryable failures and `transaction unexpectedly committed` assertions by detecting when writes cannot be retried idempotently, instead returning an `AmbiguousResultError`.

AlexTalks requested a review from a team as a code owner August 2, 2023 03:27

AlexTalks force-pushed the test_followup_txn_unexpectedly_committed branch 2 times, most recently from e6c42f4 to 1622488 Compare August 2, 2023 16:36

AlexTalks mentioned this pull request Aug 2, 2023

kvcoord: add test for transaction unexpectedly committed #107323

Merged

AlexTalks force-pushed the test_followup_txn_unexpectedly_committed branch 4 times, most recently from c157520 to 5cb8374 Compare August 4, 2023 06:14

AlexTalks changed the title ~~kvserver: test additional ambiguous failure states~~ kvcoord: test additional ambiguous failure states Aug 4, 2023

AlexTalks mentioned this pull request Aug 7, 2023

kv: enable replay protection for ambiguous writes on commits #107658

Merged

AlexTalks requested review from arulajmani and nvanbenschoten August 8, 2023 00:19

AlexTalks force-pushed the test_followup_txn_unexpectedly_committed branch from 5cb8374 to 974c752 Compare August 8, 2023 00:28

AlexTalks force-pushed the test_followup_txn_unexpectedly_committed branch from 974c752 to 4836efc Compare August 17, 2023 23:38

nvanbenschoten removed their request for review August 28, 2023 17:44

arulajmani reviewed Aug 28, 2023

View reviewed changes

AlexTalks force-pushed the test_followup_txn_unexpectedly_committed branch from 4836efc to 69a94bf Compare August 29, 2023 23:39

AlexTalks commented Aug 29, 2023

View reviewed changes

arulajmani approved these changes Aug 31, 2023

View reviewed changes

AlexTalks force-pushed the test_followup_txn_unexpectedly_committed branch from 69a94bf to cab234a Compare August 31, 2023 21:35

AlexTalks commented Aug 31, 2023

View reviewed changes

craig bot merged commit 12eb1bd into cockroachdb:master Aug 31, 2023

AlexTalks mentioned this pull request Sep 19, 2023

release-23.1: kv, storage: fix handling of ambiguous failures on commit #110757

Closed

AlexTalks mentioned this pull request Sep 21, 2023

release-23.1: kv, storage: fix handling of ambiguous failures on commit #111017

Closed

AlexTalks mentioned this pull request Oct 5, 2023

release-23.1: kvcoord: add ambiguous commit tests #111871

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvcoord: test additional ambiguous failure states #108001

kvcoord: test additional ambiguous failure states #108001

AlexTalks commented Aug 2, 2023 •

edited

Loading

cockroach-teamcity commented Aug 2, 2023

arulajmani left a comment

AlexTalks left a comment

AlexTalks left a comment

arulajmani left a comment

AlexTalks left a comment

AlexTalks commented Aug 31, 2023

craig bot commented Aug 31, 2023

kvcoord: test additional ambiguous failure states #108001

kvcoord: test additional ambiguous failure states #108001

Conversation

AlexTalks commented Aug 2, 2023 • edited Loading

cockroach-teamcity commented Aug 2, 2023

arulajmani left a comment

Choose a reason for hiding this comment

AlexTalks left a comment

Choose a reason for hiding this comment

AlexTalks left a comment

Choose a reason for hiding this comment

arulajmani left a comment

Choose a reason for hiding this comment

AlexTalks left a comment

Choose a reason for hiding this comment

AlexTalks commented Aug 31, 2023

craig bot commented Aug 31, 2023

AlexTalks commented Aug 2, 2023 •

edited

Loading