Skip to content

Commit

Permalink
cli: create new tracing span in mt-start-sql
Browse files Browse the repository at this point in the history
This commit fixes a possible misuse of the tracing infrastructure with
`mt start-sql` command. In particular, previously it was possible for
a tracing span created (and finished) in `runStartInternal` to be used
by a couple of things in `makeTenantSQLServerArgs` (main one being
`rpc.Context`) that hold on to the context after the corresponding span
has been finished. This is disallowed (since we reuse spans after
`Finish` has been called on them) and was recently discovered. This
problem is now fixed by deriving a child span only in `mt start-sql`
code path that is never finished (since those users can be active
throughout the whole lifetime of the tenant server).

The bug is pretty minor (I think only serverless is affected), so there
is no release note.

Release note: None
  • Loading branch information
yuzefovich committed Nov 1, 2023
1 parent bc1b4e2 commit d9f3d98
Showing 1 changed file with 12 additions and 0 deletions.
12 changes: 12 additions & 0 deletions pkg/cli/mt_start_sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/server"
"github.com/cockroachdb/cockroach/pkg/server/serverctl"
"github.com/cockroachdb/cockroach/pkg/util/stop"
"github.com/cockroachdb/cockroach/pkg/util/tracing"
"github.com/cockroachdb/redact"
"github.com/spf13/cobra"
)
Expand Down Expand Up @@ -63,6 +64,17 @@ func runStartSQL(cmd *cobra.Command, args []string) error {
}

newServerFn := func(ctx context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverctl.ServerStartupInterface, error) {
// The context passed into NewSeparateProcessTenantServer will be
// captured in makeTenantSQLServerArgs by a few users that will hold on
// to it. Those users can log something into the current tracing span
// (created in runStartInternal) after the span has been Finish()'ed
// which is disallowed. To go around this issue, in this code path we
// create a new tracing span that is never finished since those users
// stay alive until the tenant server is up.
// TODO(yuzefovich): consider changing NewSeparateProcessTenantServer to
// not take in a context and - akin to NewServer - to construct its own
// from context.Background.
ctx, _ = tracing.ChildSpan(ctx, "mt-start-sql" /* opName */)
// Beware of not writing simply 'return server.NewServer()'. This is
// because it would cause the serverStartupInterface reference to
// always be non-nil, even if NewServer returns a nil pointer (and
Expand Down

0 comments on commit d9f3d98

Please sign in to comment.