Merge #54758

54758: server: split server pre-start from accepting clients r=irfansharif,tbg a=knz


This will enable the introduction of further cluster initialization
using SQL in `start.go`, before clients become able to connect.

I found this to be a prerequisite to #48647.


Co-authored-by: Raphael 'kena' Poss <[email protected]>

Author: craig[bot]

pkg/cli/start.go

@@ -564,7 +564,7 @@ If problems persist, please see %s.`
 			}
 
 			// Attempt to start the server.
-			if err := s.Start(ctx); err != nil {
+			if err := s.PreStart(ctx); err != nil {
 				if le := (*server.ListenError)(nil); errors.As(err, &le) {
 					const errorPrefix = "consider changing the port via --%s"
 					if le.Addr == serverCfg.Addr {
@@ -596,6 +596,11 @@ If problems persist, please see %s.`
 				return err
 			}
 
+			// Now let SQL clients in.
+			if err := s.AcceptClients(ctx); err != nil {
+				return err
+			}
+
 			// Now inform the user that the server is running and tell the
 			// user about its run-time derived parameters.
 			var buf redact.StringBuilder

pkg/server/server.go

@@ -972,15 +972,32 @@ func getServerEndpointCounter(method string) telemetry.Counter {
 	return telemetry.GetCounter(fmt.Sprintf("%s.%s", counterPrefix, method))
 }
 
-// Start starts the server on the specified port, starts gossip and initializes
-// the node using the engines from the server's context. This is complex since
-// it sets up the listeners and the associated port muxing, but especially since
-// it has to solve the "bootstrapping problem": nodes need to connect to Gossip
-// fairly early, but what drives Gossip connectivity are the first range
-// replicas in the kv store. This in turn suggests opening the Gossip server
-// early. However, naively doing so also serves most other services prematurely,
-// which exposes a large surface of potentially underinitialized services. This
-// is avoided with some additional complexity that can be summarized as follows:
+// Start calls PreStart() and AcceptClient() in sequence.
+// This is suitable for use e.g. in tests.
+func (s *Server) Start(ctx context.Context) error {
+	if err := s.PreStart(ctx); err != nil {
+		return err
+	}
+	return s.AcceptClients(ctx)
+}
+
+// PreStart starts the server on the specified port, starts gossip and
+// initializes the node using the engines from the server's context.
+//
+// It does not activate the pgwire listener over the network / unix
+// socket, which is done by the AcceptClients() method. The separation
+// between the two exists so that SQL initialization can take place
+// before the first client is accepted.
+//
+// PreStart is complex since it sets up the listeners and the associated
+// port muxing, but especially since it has to solve the
+// "bootstrapping problem": nodes need to connect to Gossip fairly
+// early, but what drives Gossip connectivity are the first range
+// replicas in the kv store. This in turn suggests opening the Gossip
+// server early. However, naively doing so also serves most other
+// services prematurely, which exposes a large surface of potentially
+// underinitialized services. This is avoided with some additional
+// complexity that can be summarized as follows:
 //
 // - before blocking trying to connect to the Gossip network, we already open
 //   the admin UI (so that its diagnostics are available)
@@ -990,7 +1007,7 @@ func getServerEndpointCounter(method string) telemetry.Counter {
 //
 // The passed context can be used to trace the server startup. The context
 // should represent the general startup operation.
-func (s *Server) Start(ctx context.Context) error {
+func (s *Server) PreStart(ctx context.Context) error {
 	ctx = s.AnnotateCtx(ctx)
 
 	// Start the time sanity checker.
@@ -1691,7 +1708,7 @@ func (s *Server) Start(ctx context.Context) error {
 	// executes a SQL query, this must be done after the SQL layer is ready.
 	s.node.recordJoinEvent()
 
-	if err := s.sqlServer.start(
+	if err := s.sqlServer.preStart(
 		workersCtx,
 		s.stopper,
 		s.cfg.TestingKnobs,
@@ -1712,6 +1729,24 @@ func (s *Server) Start(ctx context.Context) error {
 	// been run so it is safe to upgrade to the binary's current version.
 	s.startAttemptUpgrade(ctx)
 
+	log.Event(ctx, "server initialized")
+	return nil
+}
+
+// AcceptClients starts listening for incoming SQL clients over the network.
+func (s *Server) AcceptClients(ctx context.Context) error {
+	workersCtx := s.AnnotateCtx(context.Background())
+
+	if err := s.sqlServer.startServeSQL(
+		workersCtx,
+		s.stopper,
+		s.sqlServer.connManager,
+		s.sqlServer.pgL,
+		s.cfg.SocketFile,
+	); err != nil {
+		return err
+	}
+
 	log.Event(ctx, "server ready")
 	return nil
 }

pkg/server/server_sql.go

@@ -92,6 +92,12 @@ type sqlServer struct {
 	stmtDiagnosticsRegistry *stmtdiagnostics.Registry
 	sqlLivenessProvider     sqlliveness.Provider
 	metricsRegistry         *metric.Registry
+
+	// pgL is the shared RPC/SQL listener, opened when RPC was initialized.
+	pgL net.Listener
+	// connManager is the connection manager to use to set up additional
+	// SQL listeners in AcceptClients().
+	connManager netutil.Server
 }
 
 // sqlServerOptionalKVArgs are the arguments supplied to newSQLServer which are
@@ -649,7 +655,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*sqlServer, error) {
 	}, nil
 }
 
-func (s *sqlServer) start(
+func (s *sqlServer) preStart(
 	ctx context.Context,
 	stopper *stop.Stopper,
 	knobs base.TestingKnobs,
@@ -665,6 +671,8 @@ func (s *sqlServer) start(
 			return err
 		}
 	}
+	s.connManager = connManager
+	s.pgL = pgL
 	s.sqlLivenessProvider.Start(ctx)
 	s.execCfg.GCJobNotifier.Start(ctx)
 	s.temporaryObjectCleaner.Start(ctx, stopper)
@@ -752,11 +760,6 @@ func (s *sqlServer) start(
 
 	log.Infof(ctx, "done ensuring all necessary migrations have run")
 
-	// Start serving SQL clients.
-	if err := s.startServeSQL(ctx, stopper, connManager, pgL, socketFile); err != nil {
-		return err
-	}
-
 	// Start the async migration to upgrade namespace entries from the old
 	// namespace table (id 2) to the new one (id 30).
 	if err := migMgr.StartSystemNamespaceMigration(ctx, bootstrapVersion); err != nil {

pkg/server/testserver.go

@@ -711,7 +711,7 @@ func StartTenant(
 		return "", "", err
 	}
 
-	if err := s.start(ctx,
+	if err := s.preStart(ctx,
 		args.stopper,
 		args.TestingKnobs,
 		connManager,
@@ -721,6 +721,13 @@ func StartTenant(
 	); err != nil {
 		return "", "", err
 	}
+	if err := s.startServeSQL(ctx,
+		args.stopper,
+		s.connManager,
+		s.pgL,
+		socketFile); err != nil {
+		return "", "", err
+	}
 
 	return pgLAddr, httpLAddr, nil
 }