kvserver: fix double span Finish on reproposals by pav-kv · Pull Request #108775 · cockroachdb/cockroach

pav-kv · 2023-08-15T14:51:12Z

Since #106750, ProposalData is being copied on superseding reproposals. The caller only knows about the original proposal, and only detaches its context / tracing span when abandoning the request (e.g. on timeout). By the time of abandoning the request, there might have been a few superseding reproposals, and the context / tracing span is being used by multiple ProposalData objects.

In addition to rewriting the original proposal.ctx, we should do the same for all the ProposalData objects corresponding to the same request. This commit unbinds the context for older proposals when reproposing them, and unbinds the latest reproposal context when cleaning up / abandoning the request.

Fixes #107521
Fixes #108534
Fixes #108241
Fixes #108663
Fixes #108696
Fixes #108837

Epic: CRDB-25287
Release note: none

blathers-crl · 2023-08-15T14:51:16Z

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-08-15T14:51:20Z

This change is

pkg/kv/kvserver/replica_application_result.go

erikgrinaker

LGTM in principle, just some minor tweaks.

pkg/kv/kvserver/replica_application_result.go

erikgrinaker · 2023-08-17T15:56:07Z

pkg/kv/kvserver/replica_application_result.go

+		// proposals for post-processing.
+		//
+		// TODO(pavelkalinnikov): prove and assert that origP.reproposal was nil.
+		origP.reproposal = newProposal


This will prevent garbage collection of all the intermediate reproposals. I don't think we hold on to them from anywhere else? Seems like it would be worthwhile to avoid.

I'm guessing the old proposal is discarded after we hit this code path? If so, we really only need the first and last proposal in the chain. We could e.g. have a backreference to the original proposal and update its forward reference to the latest reproposal, or we could have a cancel function that's propagated down to the active reproposal.

Reworked this bit. It became a bit more ugly, but the GC benefit seems worth it. Longer term, we should improve the lifecycle.

Also, had to drop the invariant check.

erikgrinaker

LGTM, but let's squash the chain/seed commits. We should also have a basic test for the proposal linking, if we can easily add one.

pkg/kv/kvserver/replica_raft.go

pkg/kv/kvserver/replica_proposal.go

pav-kv

Addressed inline comments, and squashed commits.

pkg/kv/kvserver/replica_proposal.go

pkg/kv/kvserver/replica_raft.go

pav-kv · 2023-08-21T11:15:10Z

@erikgrinaker Ah hold on, I'm looking if I can add a test.

pkg/kv/kvserver/replica_application_result.go

Since cockroachdb#106750, ProposalData is being copied on superseding reproposals. The caller only knows about the original proposal, and only detaches its context / tracing span when abandoning the request (e.g. on timeout). By the time of abandoning the request, there might have been a few superseding reproposals, and the context / tracing span is being used by multiple ProposalData objects. In addition to rewriting the original proposal.ctx, we should do the same for all the ProposalData objects corresponding to the same request. This commit unbinds the context for older proposals when reproposing them, and unbinds the latest reproposal context when cleaning up / abandoning the request. Epic: CRDB-25287 Release note: none

Epic: none Release note: none

pav-kv · 2023-08-21T12:44:52Z

@erikgrinaker Added a test, PTAL.

erikgrinaker

LGTM, thanks. It's possible that we prematurely clear the seed proposal's context here, which could possibly lose some trace events for the remainder of the failed application, but even if we do we likely don't care about them at this point anyway

pav-kv · 2023-08-21T13:09:32Z

bors r=erikgrinaker

pav-kv · 2023-08-21T13:10:26Z

We don't need to backport this, this refactoring is post-23.1.

craig · 2023-08-21T14:20:46Z

Build succeeded:

Bazel Essential CI (Cockroach)

`TestMultiRegionDataDriven` was skipped in cockroachdb#108107 due to finish (tracing span) being called twice in raft reproposals cockroachdb#107521, which cockroachdb#108775 fixed. Unskip the top level `TestMultiRegionDataDriven` test. Note `/secondary_region` is still skipped due to a known allocator bug. Informs: cockroachdb#98020 Epic: none Release note: None

110063: ccl/multiregionccl: unskip test multi region dd parent test r=rafiss a=kvoli `TestMultiRegionDataDriven` was skipped in #108107 due to finish (tracing span) being called twice in raft reproposals #107521, which #108775 fixed. Unskip the top level `TestMultiRegionDataDriven` test. Note `/secondary_region` is still skipped due to a known allocator bug. Informs: #98020 Epic: none Release note: None Co-authored-by: Austen McClernon <austen@cockroachlabs.com>

pav-kv force-pushed the fix-reproposal-span-finish branch 3 times, most recently from 8d61ee6 to 6e98b9c Compare August 16, 2023 09:00

pav-kv requested a review from erikgrinaker August 16, 2023 09:01

pav-kv marked this pull request as ready for review August 16, 2023 09:01

pav-kv requested a review from a team August 16, 2023 09:01

pav-kv force-pushed the fix-reproposal-span-finish branch from 6e98b9c to 24eb9bb Compare August 16, 2023 09:24

pav-kv commented Aug 16, 2023

View reviewed changes

pkg/kv/kvserver/replica_application_result.go Outdated Show resolved Hide resolved

erikgrinaker mentioned this pull request Aug 16, 2023

kv/kvnemesis: TestKVNemesisSingleNode_ReproposalChaos failed #107860

Closed

yuzefovich mentioned this pull request Aug 16, 2023

kv/kvserver: span use-after-finish in replica proposal buf #108767

Closed

pav-kv commented Aug 17, 2023

View reviewed changes

pkg/kv/kvserver/replica_application_result.go Show resolved Hide resolved

msbutler mentioned this pull request Aug 17, 2023

roachtest: c2c/tpcc/warehouses=1000/duration=60/cutover=30 failed - span reuse after finish #108837

Closed

erikgrinaker reviewed Aug 17, 2023

View reviewed changes

pav-kv force-pushed the fix-reproposal-span-finish branch from afa3a59 to 9becf3b Compare August 18, 2023 15:06

pav-kv requested a review from erikgrinaker August 18, 2023 15:10

erikgrinaker reviewed Aug 21, 2023

View reviewed changes

pkg/kv/kvserver/replica_raft.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/replica_proposal.go Outdated Show resolved Hide resolved

pav-kv force-pushed the fix-reproposal-span-finish branch from 9becf3b to 8a24ab2 Compare August 21, 2023 11:09

pav-kv commented Aug 21, 2023

View reviewed changes

pkg/kv/kvserver/replica_proposal.go Outdated Show resolved Hide resolved

pkg/kv/kvserver/replica_raft.go Outdated Show resolved Hide resolved

pav-kv force-pushed the fix-reproposal-span-finish branch from 8a24ab2 to d3f93d1 Compare August 21, 2023 11:13

pav-kv requested a review from erikgrinaker August 21, 2023 11:14

erikgrinaker reviewed Aug 21, 2023

View reviewed changes

pkg/kv/kvserver/replica_application_result.go Outdated Show resolved Hide resolved

pav-kv added 3 commits August 21, 2023 12:48

kvserver: squash deferred statements into one

788f035

Epic: none Release note: none

kvserv: group "moved" fields together

b83edaf

Epic: none Release note: none

pav-kv force-pushed the fix-reproposal-span-finish branch from d3f93d1 to b83edaf Compare August 21, 2023 11:48

kvserver: add test for reproposal chaining

238c9eb

Epic: none Release note: none

pav-kv requested a review from erikgrinaker August 21, 2023 12:44

erikgrinaker approved these changes Aug 21, 2023

View reviewed changes

pav-kv added the backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only label Aug 21, 2023

pav-kv removed the backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only label Aug 21, 2023

craig bot merged commit 2ac44e6 into cockroachdb:master Aug 21, 2023

pav-kv deleted the fix-reproposal-span-finish branch August 21, 2023 14:22

pav-kv mentioned this pull request Aug 25, 2023

ccl/multiregionccl: TestRegionAddDropWithConcurrentBackupOps failed [span use-after-finish in replicaDecoder] #109097

Closed

kvoli mentioned this pull request Sep 5, 2023

ccl/multiregionccl: unskip test multi region dd parent test #110063

Merged

pav-kv mentioned this pull request Dec 7, 2023

kvserver: runtime assertions for double-application and duplicate proposals #115771

Closed

Conversation

pav-kv commented Aug 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blathers-crl bot commented Aug 15, 2023

Uh oh!

cockroach-teamcity commented Aug 15, 2023

Uh oh!

Uh oh!

Uh oh!

erikgrinaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

erikgrinaker Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

pav-kv Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

pav-kv Aug 18, 2023

Choose a reason for hiding this comment

Uh oh!

erikgrinaker left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pav-kv left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pav-kv commented Aug 21, 2023

Uh oh!

Uh oh!

pav-kv commented Aug 21, 2023

Uh oh!

erikgrinaker left a comment

Choose a reason for hiding this comment

Uh oh!

pav-kv commented Aug 21, 2023

Uh oh!

pav-kv commented Aug 21, 2023

Uh oh!

craig bot commented Aug 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pav-kv commented Aug 15, 2023 •

edited

Loading

erikgrinaker left a comment •

edited

Loading