server: separate "new" from "start" for tenant servers

This peels the call to "start" from the `newTenantServer`
interface and pulls it into the orchestration retry loop.

This change also incidentally reveals an earlier misdesign: we are
calling `newTenantServer` _then_ `start` in the same retry loop. If
`new` succeeds but `start` fails, the next retry will call
`newTenantServer` again *with the same stopper*, which will leak
closers from the previous call to `new`.

Release note: None

pkg/server/server_controller.go

@@ -203,7 +203,12 @@ func (t *tenantServerWrapper) preStart(ctx context.Context) error {
 }
 
 func (t *tenantServerWrapper) acceptClients(ctx context.Context) error {
-	return t.server.AcceptClients(ctx)
+	if err := t.server.AcceptClients(ctx); err != nil {
+		return err
+	}
+	// Show the tenant details in logs.
+	// TODO(knz): Remove this once we can use a single listener.
+	return t.server.reportTenantInfo(ctx)
 }
 
 func (t *tenantServerWrapper) stop(ctx context.Context) {

pkg/server/server_controller_new_server.go

@@ -83,7 +83,7 @@ func (s *Server) newTenantServer(
 	// Apply the TestTenantArgs, if any.
 	baseCfg.TestingKnobs = testArgs.Knobs
 
-	tenantServer, err := startTenantServerInternal(ctx, baseCfg, sqlCfg, tenantStopper, tenantNameContainer)
+	tenantServer, err := newTenantServerInternal(ctx, baseCfg, sqlCfg, tenantStopper, tenantNameContainer)
 	if err != nil {
 		return nil, err
 	}
@@ -124,12 +124,12 @@ func (s *Server) getTenantID(
 	return tenantID, nil
 }
 
-// startTenantServerInternal starts a server for the given target
+// newTenantServerInternal instantiates a server for the given target
 // tenant ID.
 //
-// Note that even if an error is returned, tasks might have been started with
-// the stopper, so the caller needs to Stop() it.
-func startTenantServerInternal(
+// Note that even if an error is returned, closers may have been
+// registered with the stopper, so the caller needs to Stop() it.
+func newTenantServerInternal(
 	ctx context.Context,
 	baseCfg BaseConfig,
 	sqlCfg SQLConfig,
@@ -140,28 +140,13 @@ func startTenantServerInternal(
 	stopper.SetTracer(baseCfg.Tracer)
 
 	// New context, since we're using a separate tracer.
-	startCtx := ambientCtx.AnnotateCtx(context.Background())
+	newCtx := ambientCtx.AnnotateCtx(context.Background())
 
 	// Inform the logs we're starting a new server.
-	log.Infof(startCtx, "starting tenant server")
+	log.Infof(newCtx, "creating tenant server")
 
-	// Now start the tenant proper.
-	tenantServer, err := newSharedProcessTenantServer(startCtx, stopper, baseCfg, sqlCfg, tenantNameContainer)
-	if err != nil {
-		return nil, err
-	}
-
-	if err := tenantServer.Start(startCtx); err != nil {
-		return tenantServer, err
-	}
-
-	// Show the tenant details in logs.
-	// TODO(knz): Remove this once we can use a single listener.
-	if err := tenantServer.reportTenantInfo(startCtx); err != nil {
-		return tenantServer, err
-	}
-
-	return tenantServer, nil
+	// Now instantiate the tenant server proper.
+	return newSharedProcessTenantServer(newCtx, stopper, baseCfg, sqlCfg, tenantNameContainer)
 }
 
 func (s *Server) makeSharedProcessTenantConfig(

pkg/server/server_controller_orchestration.go

@@ -276,7 +276,7 @@ func (c *serverController) startControlledServer(
 		ctx := tenantCtx
 		// We want a context that gets cancelled when the tenant is
 		// shutting down, for the possible few cases in
-		// startServerInternal which are not looking at the
+		// newServerInternal/preStart/acceptClients which are not looking at the
 		// tenant.ShouldQuiesce() channel but are sensitive to context
 		// cancellation.
 		var cancel func()
@@ -291,8 +291,18 @@ func (c *serverController) startControlledServer(
 
 		var s onDemandServer
 		for retry := retry.StartWithCtx(ctx, retryOpts); retry.Next(); {
-			var err error
-			s, err = c.startServerInternal(ctx, entry.nameContainer, tenantStopper)
+			err := func() error {
+				var err error
+				s, err = c.newServerInternal(ctx, entry.nameContainer, tenantStopper)
+				if err != nil {
+					return err
+				}
+				startCtx := s.annotateCtx(context.Background())
+				if err := s.preStart(startCtx); err != nil {
+					return err
+				}
+				return s.acceptClients(startCtx)
+			}()
 			if err != nil {
 				c.logStartEvent(ctx, roachpb.TenantID{}, 0,
 					entry.nameContainer.Get(), false /* success */, err)
@@ -406,7 +416,7 @@ func (c *serverController) startAndWaitForRunningServer(
 	}
 }
 
-func (c *serverController) startServerInternal(
+func (c *serverController) newServerInternal(
 	ctx context.Context, nameContainer *roachpb.TenantNameContainer, tenantStopper *stop.Stopper,
 ) (onDemandServer, error) {
 	tenantName := nameContainer.Get()