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

[irfansharif]

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

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

[cockroach-teamcity]

This change is 

[irfansharif]

Will have to change roachprod to start using init, hm, that'll be a bit tedious.

[irfansharif]

Updated roachprod to use explicit init, tested against v19.1-v20.2; cockroachdb/cockroach-go PR is cockroachdb/cockroach-go#81 (and I'll bump examples-orm after that lands).

[tbg]

I'll defer to @knz for final LGTM.

I checked out your PR to see what would happen if I run

./cockroach start --insecure

without a join flag. It does not error out. Do we already consider this a footgun? ./cockroach sql --insecure will simply hang forever. Should we force a join flag to be supplied, even if it is empty (--join=)? I expect without some additional change here we will get lots of users starting a node that waits for init but without users realizing (maybe most of these users will be CRDB engineers but still, it's a potential issue).

Reviewed 18 of 18 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @knz)

---

pkg/acceptance/cluster/dockercluster.go, line 488 at r1 (raw file):

		firstNodeAddr = l.Nodes[0].nodeStr
	}
	cmd = append(cmd, "--join="+net.JoinHostPort(firstNodeAddr, base.DefaultPort))

Why give the first node --join=:26257 instead of l.Nodes[0].nodeStr? Is Nodes[0] still unavailable at this point? Is this join flag useful?

---

pkg/base/test_server_args.go, line 95 at r1 (raw file):

	// their config. If WaitForBootstrap is set, that behavior is disabled
	// and the test becomes responsible for initializing the cluster.
	WaitForBootstrap bool

might be easier to understand what this does if you named it NoAutoInitializeCluster but your call.

---

pkg/cli/init.go, line 62 at r1 (raw file):

	if _, err = c.Bootstrap(ctx, &serverpb.BootstrapRequest{}); err != nil {
		if strings.Contains(err.Error(), server.ErrClusterInitialized.Error()) {

Why did this check go away? Idempotency of init?

---

pkg/cmd/roachprod/install/cockroach.go, line 306 at r1 (raw file):

		var initOut string
		display = fmt.Sprintf("%s: bootstrapping cluster", c.Name)
		c.Parallel(display, 1, 0, func(i int) ([]byte, error) {

Wait, why c.Parallel()? This is just hitting a single node, no?

---

pkg/server/server.go, line 1293 at r1 (raw file):

		switch {
		case err == nil:
			log.Infof(ctx, "**** add additional nodes by specifying --join=%s", s.cfg.AdvertiseAddr)

Maybe just remove this log now that it's not prod code any more.

---

pkg/server/server.go, line 1300 at r1 (raw file):

	}

	log.Info(ctx, "awaiting init command or join with an already initialized node.")

} else {?

[irfansharif]

I'm not sure what to do with the lonecockroach start --insecure, it does seem annoying. But forcing an empty join seems bad too? I wonder if it's possible/good enough to have cockroach sql prompt a warning asking the operator to ensure that the cluster is initialized, or the fact that it's waiting for initialization. @knz, thoughts?

---

I’m not sure how to work around #42083 to make sure my changes in cockroachdb/cockroach-go#81 and cockroachdb/examples-orms#109 all work for this branch, without me pushing to examples-orm master and letting CI pick that up. Is that the only way? Seems unwieldy.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

---

pkg/acceptance/cluster/dockercluster.go, line 488 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Why give the first node --join=:26257 instead of l.Nodes[0].nodeStr? Is Nodes[0] still unavailable at this point? Is this join flag useful?

Done.

---

pkg/base/test_server_args.go, line 95 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

might be easier to understand what this does if you named it NoAutoInitializeCluster but your call.

Done.

---

pkg/cli/init.go, line 62 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Why did this check go away? Idempotency of init?

init is not idempotent, we still want to surface ErrClusterInitialized but I pulled the error message generated to to MaybeDecorateGRPCError, where we seem to do a lot of the same.

---

pkg/cmd/roachprod/install/cockroach.go, line 306 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Wait, why c.Parallel()? This is just hitting a single node, no?

It's hitting a single node, yup. Was cargo culting here from the setting of cluster settings in the block below, it nicely captures command process/failure outputs.

---

pkg/server/server.go, line 1293 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

Maybe just remove this log now that it's not prod code any more.

Done.

---

pkg/server/server.go, line 1300 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

} else {?

Done.

[irfansharif]

I checked out your PR to see what would happen if I run ./cockroach start --insecure without a join flag. It does not error out.

It now errors out. We apparently already emitted a warning earlier deprecating usage of cockroach start without --join, so now we can remove it entirely.

[irfansharif]

Ignore the first commit in this PR, I've sent it out in #51329.

[irfansharif]

Went ahead and merged cockroachdb/cockroach-go#81 + cockroachdb/examples-orms#109, which clears out the example-orm test suite run against this branch (and master). Should be reflected in the now green CI.

[knz]

code LGTM, just minor concerns on the error objects

Reviewed 1 of 18 files at r1, 18 of 18 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @tbg)

---

pkg/cli/start.go, line 544 at r2 (raw file):

	// Check the --join flag.
	if !flagSetForCmd(cmd).Lookup(cliflags.Join.Name).Changed {
		err := errors.New("no --join flags provided to 'cockroach start'\n" +

Please consider using an error hint for the second sentence instead. This presents better on the console.

---

pkg/server/init.go, line 34 at r2 (raw file):

// ErrClusterInitialized is reported when the Bootstrap RPC is run on
// a node that is already part of an initialized cluster.
var ErrClusterInitialized = fmt.Errorf("cluster has already been initialized")

Can you add the error hint here too. Especially if the CLI code now returns ErrClusterInitialized without adding the hint itself.

[irfansharif]

TFTRs!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz and @tbg)

---

pkg/cli/start.go, line 544 at r2 (raw file):

Previously, knz (kena) wrote…

Please consider using an error hint for the second sentence instead. This presents better on the console.

Done.

---

pkg/server/init.go, line 34 at r2 (raw file):

Previously, knz (kena) wrote…

Can you add the error hint here too. Especially if the CLI code now returns ErrClusterInitialized without adding the hint itself.

Discussed offline, decided to skip the hint here as the error is self-explanatory.

[irfansharif]

bors r+

[craig]

Canceled

[irfansharif]

bors r+

[irfansharif]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)