From d9f3d9818c4a7dfa31443a01916fb8b618c70d13 Mon Sep 17 00:00:00 2001 From: Yahor Yuzefovich Date: Wed, 1 Nov 2023 02:04:21 +0000 Subject: [PATCH] cli: create new tracing span in mt-start-sql 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 --- pkg/cli/mt_start_sql.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/pkg/cli/mt_start_sql.go b/pkg/cli/mt_start_sql.go index f6c59339f42c..9e05258e976f 100644 --- a/pkg/cli/mt_start_sql.go +++ b/pkg/cli/mt_start_sql.go @@ -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" ) @@ -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