Merge cockroachdb#108010

108010: server: unexport Server r=yuzefovich a=knz

We should really look at top level servers through an interface.

Epic: CRDB-28893

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

Author: craig[bot]

pkg/BUILD.bazel

@@ -1545,6 +1545,7 @@ GO_TARGETS = [
     "//pkg/server/profiler:profiler",
     "//pkg/server/profiler:profiler_test",
     "//pkg/server/rangetestutils:rangetestutils",
+    "//pkg/server/serverctl:serverctl",
     "//pkg/server/serverpb:serverpb",
     "//pkg/server/serverpb:serverpb_test",
     "//pkg/server/serverrules:serverrules",

pkg/cli/BUILD.bazel

@@ -148,6 +148,7 @@ go_library(
         "//pkg/server/autoconfig/acprovider",
         "//pkg/server/pgurl",
         "//pkg/server/profiler",
+        "//pkg/server/serverctl",
         "//pkg/server/serverpb",
         "//pkg/server/status",
         "//pkg/server/status/statuspb",

pkg/cli/mt_start_sql.go

@@ -16,6 +16,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cli/clierrorplus"
 	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/server/serverctl"
 	"github.com/cockroachdb/cockroach/pkg/util/stop"
 	"github.com/cockroachdb/redact"
 	"github.com/spf13/cobra"
@@ -78,7 +79,7 @@ func runStartSQL(cmd *cobra.Command, args []string) error {
 		)
 	}
 
-	newServerFn := func(ctx context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverStartupInterface, error) {
+	newServerFn := func(ctx context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverctl.ServerStartupInterface, error) {
 		// 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

pkg/cli/start.go

@@ -38,6 +38,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security/clientsecopts"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/server/serverctl"
 	"github.com/cockroachdb/cockroach/pkg/server/status"
 	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
@@ -312,45 +313,7 @@ func initTempStorageConfig(
 	return tempStorageConfig, nil
 }
 
-type newServerFn func(ctx context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverStartupInterface, error)
-
-type serverStartupInterface interface {
-	serverShutdownInterface
-
-	// ClusterSettings retrieves this server's settings.
-	ClusterSettings() *cluster.Settings
-
-	// LogicalClusterID retrieves this server's logical cluster ID.
-	LogicalClusterID() uuid.UUID
-
-	// PreStart starts the server on the specified port(s) and
-	// initializes subsystems.
-	// 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(ctx context.Context) error
-
-	// AcceptClients starts listening for incoming SQL clients over the network.
-	AcceptClients(ctx context.Context) error
-	// AcceptInternalClients starts listening for incoming internal SQL clients over the
-	// loopback interface.
-	AcceptInternalClients(ctx context.Context) error
-
-	// InitialStart returns whether this node is starting for the first time.
-	// This is (currently) used when displaying the server status report
-	// on the terminal & in logs. We know that some folk have automation
-	// that depend on certain strings displayed from this when orchestrating
-	// KV-only nodes.
-	InitialStart() bool
-
-	// RunInitialSQL runs the SQL initialization for brand new clusters,
-	// if the cluster is being started for the first time.
-	// The arguments are:
-	// - startSingleNode is used by 'demo' and 'start-single-node'.
-	// - adminUser/adminPassword is used for 'demo'.
-	RunInitialSQL(ctx context.Context, startSingleNode bool, adminUser, adminPassword string) error
-}
+type newServerFn func(ctx context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverctl.ServerStartupInterface, error)
 
 var errCannotUseJoin = errors.New("cannot use --join with 'cockroach start-single-node' -- use 'cockroach start' instead")
 
@@ -398,9 +361,9 @@ func runStartJoin(cmd *cobra.Command, args []string) error {
 func runStart(cmd *cobra.Command, args []string, startSingleNode bool) error {
 	const serverType redact.SafeString = "node"
 
-	newServerFn := func(_ context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverStartupInterface, error) {
+	newServerFn := func(_ context.Context, serverCfg server.Config, stopper *stop.Stopper) (serverctl.ServerStartupInterface, error) {
 		// Beware of not writing simply 'return server.NewServer()'. This is
-		// because it would cause the serverStartupInterface reference to
+		// because it would cause the serverctl.ServerStartupInterface reference to
 		// always be non-nil, even if NewServer returns a nil pointer (and
 		// an error). The code below is dependent on the interface
 		// reference remaining nil in case of error.
@@ -757,10 +720,10 @@ func createAndStartServerAsync(
 	newServerFn newServerFn,
 	startSingleNode bool,
 	serverType redact.SafeString,
-) (srvStatus *serverStatus, serverShutdownReqC <-chan server.ShutdownRequest) {
+) (srvStatus *serverStatus, serverShutdownReqC <-chan serverctl.ShutdownRequest) {
 	var serverStatusMu serverStatus
-	var s serverStartupInterface
-	shutdownReqC := make(chan server.ShutdownRequest, 1)
+	var s serverctl.ServerStartupInterface
+	shutdownReqC := make(chan serverctl.ShutdownRequest, 1)
 
 	log.Ops.Infof(ctx, "starting cockroach %s", serverType)
 
@@ -851,8 +814,8 @@ func createAndStartServerAsync(
 			return reportServerInfo(ctx, tBegin, serverCfg, s.ClusterSettings(),
 				serverType, s.InitialStart(), s.LogicalClusterID())
 		}(); err != nil {
-			shutdownReqC <- server.MakeShutdownRequest(
-				server.ShutdownReasonServerStartupError, errors.Wrapf(err, "server startup failed"))
+			shutdownReqC <- serverctl.MakeShutdownRequest(
+				serverctl.ShutdownReasonServerStartupError, errors.Wrapf(err, "server startup failed"))
 		} else {
 			// Start a goroutine that watches for shutdown requests and notifies
 			// errChan.
@@ -885,7 +848,7 @@ type serverStatus struct {
 	// s is a reference to the server, to be used by the shutdown process. This
 	// starts as nil, and is set by setStarted(). Once set, a graceful shutdown
 	// should use a soft drain.
-	s serverShutdownInterface
+	s serverctl.ServerShutdownInterface
 	// stopper is the server's stopper. This is set in setStarted(), together with
 	// `s`. The stopper is handed out to callers of startShutdown(), who will
 	// Stop() it.
@@ -904,7 +867,9 @@ type serverStatus struct {
 // setStarted returns whether shutdown has been requested already. If it has,
 // then the serverStatus does not take ownership of the stopper; the caller is
 // responsible for calling stopper.Stop().
-func (s *serverStatus) setStarted(server serverShutdownInterface, stopper *stop.Stopper) bool {
+func (s *serverStatus) setStarted(
+	server serverctl.ServerShutdownInterface, stopper *stop.Stopper,
+) bool {
 	s.Lock()
 	defer s.Unlock()
 	if s.shutdownRequested {
@@ -927,21 +892,13 @@ func (s *serverStatus) shutdownInProgress() bool {
 // was started already. If the server started, a reference to the server is also
 // returned, and a reference to the stopper that the caller needs to eventually
 // Stop().
-func (s *serverStatus) startShutdown() (bool, serverShutdownInterface, *stop.Stopper) {
+func (s *serverStatus) startShutdown() (bool, serverctl.ServerShutdownInterface, *stop.Stopper) {
 	s.Lock()
 	defer s.Unlock()
 	s.shutdownRequested = true
 	return s.s != nil, s.s, s.stopper
 }
 
-// serverShutdownInterface is the subset of the APIs on a server
-// object that's sufficient to run a server shutdown.
-type serverShutdownInterface interface {
-	AnnotateCtx(context.Context) context.Context
-	Drain(ctx context.Context, verbose bool) (uint64, redact.RedactableString, error)
-	ShutdownRequested() <-chan server.ShutdownRequest
-}
-
 // waitForShutdown blocks until interrupted by a shutdown signal, which can come
 // in several forms:
 //   - a shutdown request coming from an internal module being signaled on
@@ -953,7 +910,7 @@ type serverShutdownInterface interface {
 // before shutting down.
 func waitForShutdown(
 	stopper *stop.Stopper,
-	shutdownC <-chan server.ShutdownRequest,
+	shutdownC <-chan serverctl.ShutdownRequest,
 	signalCh <-chan os.Signal,
 	serverStatusMu *serverStatus,
 ) (returnErr error) {
@@ -966,7 +923,7 @@ func waitForShutdown(
 	case shutdownRequest := <-shutdownC:
 		returnErr = shutdownRequest.ShutdownCause()
 		// There's no point in draining if the server didn't even fully start.
-		drain := shutdownRequest.Reason != server.ShutdownReasonServerStartupError
+		drain := shutdownRequest.Reason != serverctl.ShutdownReasonServerStartupError
 		startShutdownAsync(serverStatusMu, stopWithoutDrain, drain)
 
 	case sig := <-signalCh:

pkg/server/BUILD.bazel

@@ -186,6 +186,7 @@ go_library(
         "//pkg/server/pgurl",
         "//pkg/server/privchecker",
         "//pkg/server/profiler",
+        "//pkg/server/serverctl",
         "//pkg/server/serverpb",
         "//pkg/server/serverrules",
         "//pkg/server/settingswatcher",

pkg/server/admin.go

@@ -130,7 +130,7 @@ type systemAdminServer struct {
 	*adminServer
 
 	nodeLiveness *liveness.NodeLiveness
-	server       *Server
+	server       *topLevelServer
 }
 
 // noteworthyAdminMemoryUsageBytes is the minimum size tracked by the
@@ -161,7 +161,7 @@ func newSystemAdminServer(
 	distSender *kvcoord.DistSender,
 	grpc *grpcServer,
 	drainServer *drainServer,
-	s *Server,
+	s *topLevelServer,
 ) *systemAdminServer {
 	adminServer := newAdminServer(
 		sqlServer,

pkg/server/auto_upgrade.go

@@ -24,7 +24,7 @@ import (
 )
 
 // startAttemptUpgrade attempts to upgrade cluster version.
-func (s *Server) startAttemptUpgrade(ctx context.Context) error {
+func (s *topLevelServer) startAttemptUpgrade(ctx context.Context) error {
 	return s.stopper.RunAsyncTask(ctx, "auto-upgrade", func(ctx context.Context) {
 		ctx, cancel := s.stopper.WithCancelOnQuiesce(ctx)
 		defer cancel()
@@ -120,7 +120,7 @@ const (
 
 // upgradeStatus lets the main checking loop know if we should do upgrade,
 // keep checking upgrade status, or stop attempting upgrade.
-func (s *Server) upgradeStatus(
+func (s *topLevelServer) upgradeStatus(
 	ctx context.Context, clusterVersion string,
 ) (st upgradeStatus, err error) {
 	nodes, err := s.status.ListNodesInternal(ctx, nil)
@@ -205,7 +205,7 @@ func (s *Server) upgradeStatus(
 // clusterVersion returns the current cluster version from the SQL subsystem
 // (which returns the version from the KV store as opposed to the possibly
 // lagging settings subsystem).
-func (s *Server) clusterVersion(ctx context.Context) (string, error) {
+func (s *topLevelServer) clusterVersion(ctx context.Context) (string, error) {
 	row, err := s.sqlServer.internalExecutor.QueryRowEx(
 		ctx, "show-version", nil, /* txn */
 		sessiondata.RootUserSessionDataOverride,

pkg/server/clock_monotonicity.go

@@ -45,7 +45,7 @@ var (
 
 // startMonitoringForwardClockJumps starts a background task to monitor forward
 // clock jumps based on a cluster setting.
-func (s *Server) startMonitoringForwardClockJumps(ctx context.Context) error {
+func (s *topLevelServer) startMonitoringForwardClockJumps(ctx context.Context) error {
 	forwardJumpCheckEnabled := make(chan bool, 1)
 	s.stopper.AddCloser(stop.CloserFn(func() { close(forwardJumpCheckEnabled) }))
 
@@ -69,7 +69,7 @@ func (s *Server) startMonitoringForwardClockJumps(ctx context.Context) error {
 // checkHLCUpperBoundExists determines whether there's an HLC
 // upper bound that will need to refreshed/persisted after
 // the server has initialized.
-func (s *Server) checkHLCUpperBoundExistsAndEnsureMonotonicity(
+func (s *topLevelServer) checkHLCUpperBoundExistsAndEnsureMonotonicity(
 	ctx context.Context, initialStart bool,
 ) (hlcUpperBoundExists bool, err error) {
 	if initialStart {
@@ -238,7 +238,9 @@ func periodicallyPersistHLCUpperBound(
 //
 // tickCallback is called whenever persistHLCUpperBoundCh or a ticker tick is
 // processed
-func (s *Server) startPersistingHLCUpperBound(ctx context.Context, hlcUpperBoundExists bool) error {
+func (s *topLevelServer) startPersistingHLCUpperBound(
+	ctx context.Context, hlcUpperBoundExists bool,
+) error {
 	tickerFn := time.NewTicker
 	persistHLCUpperBoundFn := func(t int64) error { /* function to persist upper bound of HLC to all stores */
 		return s.node.SetHLCUpperBound(context.Background(), t)

pkg/server/decommission.go

@@ -141,7 +141,7 @@ func getPingCheckDecommissionFn(
 // or remove actions. If maxErrors >0, range checks will stop once maxError is
 // reached.
 // The error returned is a gRPC error.
-func (s *Server) DecommissionPreCheck(
+func (s *topLevelServer) DecommissionPreCheck(
 	ctx context.Context,
 	nodeIDs []roachpb.NodeID,
 	strictReadiness bool,
@@ -314,7 +314,7 @@ func evaluateRangeCheckResult(
 
 // Decommission idempotently sets the decommissioning flag for specified nodes.
 // The error return is a gRPC error.
-func (s *Server) Decommission(
+func (s *topLevelServer) Decommission(
 	ctx context.Context, targetStatus livenesspb.MembershipStatus, nodeIDs []roachpb.NodeID,
 ) error {
 	// If we're asked to decommission ourself we may lose access to cluster RPC,
@@ -396,7 +396,7 @@ func (s *Server) Decommission(
 
 // DecommissioningNodeMap returns the set of node IDs that are decommissioning
 // from the perspective of the server.
-func (s *Server) DecommissioningNodeMap() map[roachpb.NodeID]interface{} {
+func (s *topLevelServer) DecommissioningNodeMap() map[roachpb.NodeID]interface{} {
 	s.decomNodeMap.RLock()
 	defer s.decomNodeMap.RUnlock()
 	nodes := make(map[roachpb.NodeID]interface{})

pkg/server/drain.go

@@ -19,6 +19,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/liveness"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/server/serverctl"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/srverrors"
 	"github.com/cockroachdb/cockroach/pkg/settings"
@@ -197,7 +198,7 @@ func (s *drainServer) maybeShutdownAfterDrain(
 		// away (and who knows whether gRPC-goroutines are tied up in some
 		// stopper task somewhere).
 		s.grpc.Stop()
-		s.stopTrigger.signalStop(ctx, MakeShutdownRequest(ShutdownReasonDrainRPC, nil /* err */))
+		s.stopTrigger.signalStop(ctx, serverctl.MakeShutdownRequest(serverctl.ShutdownReasonDrainRPC, nil /* err */))
 	}()
 
 	select {