Merge #56302

56302: server: simplify init server state handling r=irfansharif a=irfansharif

Previously we had two different "init server state" types floating
around:

- We had the `initState`, which contained all the required "output"
  state the init server was tasked with constructing. It captures what
  our node ID, cluster ID, cluster version, initialized and
  uninitialized engines were.

- We had an `initDiskState` type, which was being used for two
  orthogonal reasons:
  - It was used to capture the on-disk state of a node during server
    restarts. It happened to capture the same details as the init
    server's "output" state. This was used to simply return early from
    the init server in the common case where we were restarting a
    previously bootstrapped node.
  - It was used to safeguard against double bootstrap of clusters,
    where on bootstrap/join/gossip connectivity the init server process
    would mutate the in-memory representation of the "disk state" to
    indicate that we had new initialized engines, and should thus
    prevent future bootstrap attempts.

Our usage of `initDiskState` was somewhat confusing as a result. It
wasn't the case that the in-memory representation of the disk state
would be wholly re-constructed on restart. Given the two state types
shared all the same fields, one also simply embedded the other, further
muddying the distinction between the two.

This PR separates out the three usages above into (hopefully) clearer
forms. We fold bot `initDiskState` and `initState` into a single type
(the latter), and introduce an `inspectedDiskState` field on the init
server to represent the state synthesized from first inspecting the
engines. Like the name suggests, the field is to be read-only. The
safety mechanism for double bootstrap, previously provided by our "disk
state" in-memory, is now powered by a mutex-protected boolean instead.

We now only ever construct `initState` instances after having inspected
disk state. Given that the init server is responsible for generating a
fleshed out `initState`, it does so by persisting relevant bits to disk
first, and the inspecting it to retrieve a reresentative `initState`.
There's no longer a divergence between what's present on disk, and
what the latest `initState` indicates.

We also clarify how the early return in ServeAndWait functions. It was
intended to return early for the common case where we were restarting an
already bootstrapped node. This suggests that we should only be looking
at the inspected disk state. Previously, because we were consulting the
one `initDiskState` field (also mutated during bootstrap), it was
possible for us to race with an in-flight bootstrap attempt to the same
node and read the initState from the bootstrap attempt, rather than the
pre-node-start disk state. This worked "fine" before because by the time
we got to ServeAndWait, we were already bootstrapped, and thus had a
fleshed out `initState` to return.  But still, it's somewhat odd to do
it this way given we have `bootstrapCh` below to listen in on bootstrap
events.

Now it's a bit clearer now what's happening. Our early return will only
look at the inspected state to determine if we were a previously
bootstrapped node, restarting, and if we're racing with an inflight
bootstrap attempt, we'll find out about it by reading off of
`bootstrapCh`.

Release note: None


Co-authored-by: irfan sharif <[email protected]>

pkg/roachpb/api.go

@@ -18,6 +18,7 @@ import (
 	"github.com/aws/aws-sdk-go/aws/credentials"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver/concurrency/lock"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
+	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 )
 
@@ -1466,3 +1467,21 @@ func (rirr *ResolveIntentRangeRequest) AsLockUpdate() LockUpdate {
 		IgnoredSeqNums: rirr.IgnoredSeqNums,
 	}
 }
+
+// CreateStoreIdent creates a store identifier out of the details captured
+// within the join node response (the join node RPC is used to allocate a store
+// ID for the client's first store).
+func (r *JoinNodeResponse) CreateStoreIdent() (StoreIdent, error) {
+	nodeID, storeID := NodeID(r.NodeID), StoreID(r.StoreID)
+	clusterID, err := uuid.FromBytes(r.ClusterID)
+	if err != nil {
+		return StoreIdent{}, err
+	}
+
+	sIdent := StoreIdent{
+		ClusterID: clusterID,
+		NodeID:    nodeID,
+		StoreID:   storeID,
+	}
+	return sIdent, nil
+}