Skip to content

Commit

Permalink
kvserver: don't use caller's context for ProposalData
Browse files Browse the repository at this point in the history
Previously, for local proposals, we were threading the caller's context
down into Raft. This has caused a few bugs related to context
cancellation sensitivity - the execution of a raft command should not be
impacted by whether or not the client's context has already finished.

This commit derives the proposal context from the Replica's annotated
context, using a tracing span derived from the client context. That
way, the derived context is not cancelable, but contains the same
tracing Span (thus providing similar utility) as before. Notably,
the trace span should also already contain the log tags present
in the client's context.

The tracing has coverage through (at least) a few tests that make
assertions about trace events from below-raft showing up in a trace
collected by the client, which I verified by omitting the Span:

```
TestRaftSSTableSideloadingProposal
TestReplicaAbandonProposal
TestReplicaRetryRaftProposal
TestReplicaBurstPendingCommandsAndRepropose
TestReplicaReproposalWithNewLeaseIndexError
TestFailureToProcessCommandClearsLocalResult
TestDBAddSSTable
```

See #75656.

Release note: None
  • Loading branch information
tbg committed Mar 22, 2022
1 parent b3525c9 commit 825a4ee
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 5 deletions.
4 changes: 1 addition & 3 deletions pkg/kv/kvserver/replica_application_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ type replicatedCmd struct {
// Replica's proposal map.
proposal *ProposalData

// ctx is a context that follows from the proposal's context if it was
// proposed locally. Otherwise, it will follow from the context passed to
// ApplyCommittedEntries.
// ctx is a non-cancelable context used to apply the command.
ctx context.Context
// sp is the tracing span corresponding to ctx. It is closed in
// finishTracingSpan. This span "follows from" the proposer's span (even
Expand Down
19 changes: 18 additions & 1 deletion pkg/kv/kvserver/replica_application_decoder.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,10 @@ package kvserver

import (
"context"
"fmt"

"github.com/cockroachdb/cockroach/pkg/kv/kvserver/apply"
"github.com/cockroachdb/cockroach/pkg/util"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/quotapool"
"github.com/cockroachdb/cockroach/pkg/util/tracing"
Expand Down Expand Up @@ -144,8 +146,19 @@ func (d *replicaDecoder) createTracingSpans(ctx context.Context) {
var it replicatedCmdBufSlice
for it.init(&d.cmdBuf); it.Valid(); it.Next() {
cmd := it.cur()

if cmd.IsLocal() {
cmd.ctx, cmd.sp = tracing.ChildSpan(cmd.proposal.ctx, opName)
// We intentionally don't propagate the client's cancellation policy (in
// cmd.ctx) onto the request. See #75656.
propCtx := ctx // raft scheduler's ctx
var propSp *tracing.Span
// If the client has a trace, put a child into propCtx.
if sp := tracing.SpanFromContext(cmd.proposal.ctx); sp != nil {
propCtx, propSp = sp.Tracer().StartSpanCtx(
propCtx, "local proposal", tracing.WithParent(sp),
)
}
cmd.ctx, cmd.sp = propCtx, propSp
} else if cmd.raftCmd.TraceData != nil {
// The proposal isn't local, and trace data is available. Extract
// the remote span and start a server-side span that follows from it.
Expand All @@ -167,6 +180,10 @@ func (d *replicaDecoder) createTracingSpans(ctx context.Context) {
} else {
cmd.ctx, cmd.sp = tracing.ChildSpan(ctx, opName)
}

if util.RaceEnabled && cmd.ctx.Done() != nil {
panic(fmt.Sprintf("cancelable context observed during raft application: %+v", cmd))
}
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/kv/kvserver/replica_proposal.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ import (
// be returned to the caller.
type ProposalData struct {
// The caller's context, used for logging proposals, reproposals, message
// sends, and command application. In order to enable safely tracing events
// sends, but not command application. In order to enable safely tracing events
// beneath, modifying this ctx field in *ProposalData requires holding the
// raftMu.
ctx context.Context
Expand Down

0 comments on commit 825a4ee

Please sign in to comment.