cli: remove auto-init with cockroach start without --join

Fixes cockroachdb#44116. Informs cockroachdb#24118. Reviving cockroachdb#44112.

We remove the deprecated behavior of crdb where we previously
auto-initialized the cluster when `cockroach start` was invoked without
a corresponding `--join` flag. We update a few tests along the way that
made use of this behavior by changing them to either explicitly init, or
now lean on `roachprod` changes (from cockroachdb#51329) that transparently manage
initialization.

Release note (backward-incompatible change): A CockroachDB node
started with cockroach start without the --join flag no
longer automatically initializes the cluster. The `cockroach init`
command is now mandatory. The auto-initialization behavior had
been deprecated in version 19.2.

Release note (backward-incompatible change): `cockroach start` without
`--join` is no longer supported. It was deprecated in a previous
release.

pkg/acceptance/cluster/dockercluster.go

@@ -480,10 +480,9 @@ func (l *DockerCluster) startNode(ctx context.Context, node *testNode) {
 		}
 		cmd = append(cmd, fmt.Sprintf("--store=%s", storeSpec))
 	}
-	// Append --join flag (for all nodes except first in bootstrap-node-zero mode)
-	if node.index > 0 || l.config.InitMode != INIT_BOOTSTRAP_NODE_ZERO {
-		cmd = append(cmd, "--join="+net.JoinHostPort(l.Nodes[0].nodeStr, base.DefaultPort))
-	}
+	// Append --join flag for all nodes.
+	firstNodeAddr := l.Nodes[0].nodeStr
+	cmd = append(cmd, "--join="+net.JoinHostPort(firstNodeAddr, base.DefaultPort))
 
 	dockerLogDir := "/logs/" + node.nodeStr
 	localLogDir := filepath.Join(l.volumesDir, "logs", node.nodeStr)

pkg/acceptance/cluster/testconfig.proto

@@ -22,9 +22,7 @@ enum InitMode {
   // init command.
   INIT_COMMAND = 0;
 
-  // INIT_BOOTSTRAP_NODE_ZERO uses the legacy protocol of omitting the
-  // join flag from node zero.
-  INIT_BOOTSTRAP_NODE_ZERO = 1;
+  reserved 1;
 
   // INIT_NONE starts every node with a join flag and leaves the
   // cluster uninitialized.

pkg/base/config.go

@@ -221,6 +221,15 @@ type Config struct {
 	// Enables the use of an PTP hardware clock user space API for HLC current time.
 	// This contains the path to the device to be used (i.e. /dev/ptp0)
 	ClockDevicePath string
+
+	// AutoInitializeCluster, if set, causes the server to bootstrap the
+	// cluster. Note that if two nodes are started with this flag set
+	// and also configured to join each other, each node will bootstrap
+	// its own unique cluster and the join will fail.
+	//
+	// The flag exists mostly for the benefit of tests, and for
+	// `cockroach start-single-node`.
+	AutoInitializeCluster bool
 }
 
 // HistogramWindowInterval is used to determine the approximate length of time

pkg/base/test_server_args.go

@@ -91,6 +91,11 @@ type TestServerArgs struct {
 	SQLMemoryPoolSize           int64
 	CacheSize                   int64
 
+	// By default, test servers have AutoInitializeCluster=true set in
+	// their config. If NoAutoInitializeCluster is set, that behavior is disabled
+	// and the test becomes responsible for initializing the cluster.
+	NoAutoInitializeCluster bool
+
 	// If set, this will be appended to the Postgres URL by functions that
 	// automatically open a connection to the server. That's equivalent to running
 	// SET DATABASE=foo, which works even if the database doesn't (yet) exist.

pkg/cli/context.go

@@ -92,6 +92,7 @@ func setServerContextDefaults() {
 
 	serverCfg.KVConfig.GoroutineDumpDirName = ""
 	serverCfg.KVConfig.HeapProfileDirName = ""
+	serverCfg.AutoInitializeCluster = false
 	serverCfg.KVConfig.ReadyFn = nil
 	serverCfg.KVConfig.DelayedBootstrapFn = nil
 	serverCfg.KVConfig.JoinList = nil

pkg/cli/demo_cluster.go

@@ -121,11 +121,15 @@ func setupTransientCluster(
 	for i := 0; i < demoCtx.nodes; i++ {
 		// All the nodes connect to the address of the first server created.
 		var joinAddr string
-		if c.s != nil {
+		if i != 0 {
 			joinAddr = c.s.ServingRPCAddr()
 		}
 		nodeID := roachpb.NodeID(i + 1)
 		args := testServerArgsForTransientCluster(c.sockForServer(nodeID), nodeID, joinAddr, c.demoDir)
+		if i == 0 {
+			// The first node also auto-inits the cluster.
+			args.NoAutoInitializeCluster = false
+		}
 
 		// servRPCReadyCh is used if latency simulation is requested to notify that a test server has
 		// successfully computed its RPC address.
@@ -144,7 +148,6 @@ func setupTransientCluster(
 		}
 
 		serv := serverFactory.New(args).(*server.TestServer)
-
 		if i == 0 {
 			c.s = serv
 		}
@@ -294,14 +297,15 @@ func testServerArgsForTransientCluster(
 	storeSpec.StickyInMemoryEngineID = fmt.Sprintf("demo-node%d", nodeID)
 
 	args := base.TestServerArgs{
-		SocketFile:        sock.filename(),
-		PartOfCluster:     true,
-		Stopper:           stop.NewStopper(),
-		JoinAddr:          joinAddr,
-		DisableTLSForHTTP: true,
-		StoreSpecs:        []base.StoreSpec{storeSpec},
-		SQLMemoryPoolSize: demoCtx.sqlPoolMemorySize,
-		CacheSize:         demoCtx.cacheSize,
+		SocketFile:              sock.filename(),
+		PartOfCluster:           true,
+		Stopper:                 stop.NewStopper(),
+		JoinAddr:                joinAddr,
+		DisableTLSForHTTP:       true,
+		StoreSpecs:              []base.StoreSpec{storeSpec},
+		SQLMemoryPoolSize:       demoCtx.sqlPoolMemorySize,
+		CacheSize:               demoCtx.cacheSize,
+		NoAutoInitializeCluster: true,
 	}
 
 	if demoCtx.localities != nil {

pkg/cli/demo_test.go

@@ -39,11 +39,12 @@ func TestTestServerArgsForTransientCluster(t *testing.T) {
 			sqlPoolMemorySize: 2 << 10,
 			cacheSize:         1 << 10,
 			expected: base.TestServerArgs{
-				PartOfCluster:     true,
-				JoinAddr:          "127.0.0.1",
-				DisableTLSForHTTP: true,
-				SQLMemoryPoolSize: 2 << 10,
-				CacheSize:         1 << 10,
+				PartOfCluster:           true,
+				JoinAddr:                "127.0.0.1",
+				DisableTLSForHTTP:       true,
+				SQLMemoryPoolSize:       2 << 10,
+				CacheSize:               1 << 10,
+				NoAutoInitializeCluster: true,
 			},
 		},
 		{
@@ -52,11 +53,12 @@ func TestTestServerArgsForTransientCluster(t *testing.T) {
 			sqlPoolMemorySize: 4 << 10,
 			cacheSize:         4 << 10,
 			expected: base.TestServerArgs{
-				PartOfCluster:     true,
-				JoinAddr:          "127.0.0.1",
-				DisableTLSForHTTP: true,
-				SQLMemoryPoolSize: 4 << 10,
-				CacheSize:         4 << 10,
+				PartOfCluster:           true,
+				JoinAddr:                "127.0.0.1",
+				DisableTLSForHTTP:       true,
+				SQLMemoryPoolSize:       4 << 10,
+				CacheSize:               4 << 10,
+				NoAutoInitializeCluster: true,
 			},
 		},
 	}

pkg/cli/error.go

@@ -21,6 +21,7 @@ import (
 	"strings"
 
 	"github.com/cockroachdb/cockroach/pkg/security"
+	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
@@ -348,6 +349,16 @@ func MaybeDecorateGRPCError(
 					"The CockroachDB CLI does not support GSSAPI authentication; use 'psql' instead")
 		}
 
+		// Are we trying to re-initialize an initialized cluster?
+		if strings.Contains(err.Error(), server.ErrClusterInitialized.Error()) {
+			// We really want to use errors.Is() here but this would require
+			// error serialization support in gRPC.
+			// This is not yet performed in CockroachDB even though the error
+			// library now has infrastructure to do so, see:
+			// https://github.com/cockroachdb/errors/pull/14
+			return server.ErrClusterInitialized
+		}
+
 		// Nothing we can special case, just return what we have.
 		return err
 	}

pkg/cli/init.go

@@ -14,10 +14,8 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"strings"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/util/contextutil"
 	"github.com/cockroachdb/cockroach/pkg/util/retry"
@@ -34,14 +32,10 @@ Perform one-time-only initialization of a CockroachDB cluster.
 
 After starting one or more nodes with --join flags, run the init
 command on one node (passing the same --host and certificate flags
-you would use for the sql command). The target of the init command
-must appear in the --join flags of other nodes.
-
-A node started without the --join flag initializes itself as a
-single-node cluster, so the init command is not used in that case.
+you would use for the sql command).
 `,
 	Args: cobra.NoArgs,
-	RunE: maybeShoutError(MaybeDecorateGRPCError(runInit)),
+	RunE: MaybeDecorateGRPCError(runInit),
 }
 
 func runInit(cmd *cobra.Command, args []string) error {
@@ -59,15 +53,6 @@ func runInit(cmd *cobra.Command, args []string) error {
 	c := serverpb.NewInitClient(conn)
 
 	if _, err = c.Bootstrap(ctx, &serverpb.BootstrapRequest{}); err != nil {
-		if strings.Contains(err.Error(), server.ErrClusterInitialized.Error()) {
-			// We really want to use errors.Is() here but this would require
-			// error serialization support in gRPC.
-			// This is not yet performed in CockroachDB even though
-			// the error library now has infrastructure to do so, see:
-			// https://github.com/cockroachdb/errors/pull/14
-			return errors.WithHint(err,
-				"Please ensure all your start commands are using --join.")
-		}
 		return err
 	}
 

pkg/cli/interactive_tests/test_disable_replication.tcl

@@ -36,8 +36,9 @@ stop_server $argv
 start_test "Check that start-single-node on a regular cluster does not reset the replication factor"
 # make a fresh server but using the regular 'start'
 system "rm -rf logs/db"
-system "$argv start --insecure --pid-file=server_pid --background -s=path=logs/db >>logs/expect-cmd.log 2>&1;
-        $argv sql -e 'select 1'"
+system "$argv start --insecure --pid-file=server_pid --background -s=path=logs/db --join=:26257 >>logs/expect-cmd.log 2>&1"
+system "$argv init --insecure"
+system "$argv sql -e 'select 1'"
 # restart with start-single-node
 stop_server $argv
 start_server $argv

pkg/cli/interactive_tests/test_dump_sig.tcl

@@ -8,7 +8,7 @@ send "PS1='\\h:''/# '\r"
 eexpect ":/# "
 
 start_test "Check that the server emits a goroutine dump upon receiving signal"
-send "$argv start --insecure --pid-file=server_pid --store=path=logs/db --logtostderr\r"
+send "$argv start-single-node --insecure --pid-file=server_pid --store=path=logs/db --logtostderr\r"
 eexpect "CockroachDB node starting"
 
 system "kill -QUIT `cat server_pid`"

pkg/cli/interactive_tests/test_flags.tcl

@@ -72,15 +72,13 @@ eexpect {Failed running "cockroach"}
 eexpect ":/# "
 end_test
 
-start_test "Check that start without --join reports a deprecation warning"
+start_test "Check that start without --join errors out"
 send "$argv start --insecure\r"
-eexpect "running 'cockroach start' without --join is deprecated."
-eexpect "node starting"
-interrupt
-eexpect ":/# "
+eexpect "ERROR: no --join flags provided to 'cockroach start'"
+eexpect "HINT: Consider using 'cockroach init' or 'cockroach start-single-node' instead"
+eexpect {Failed running "start"}
 end_test
 
-
 start_server $argv
 
 start_test "Check that a client can connect using the URL env var"