Skip to content

Commit

Permalink
server: simplify init server state handling
Browse files Browse the repository at this point in the history
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
  • Loading branch information
irfansharif committed Nov 6, 2020
1 parent 3765351 commit fbd447d
Show file tree
Hide file tree
Showing 5 changed files with 233 additions and 209 deletions.
19 changes: 19 additions & 0 deletions pkg/roachpb/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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
}
Loading

0 comments on commit fbd447d

Please sign in to comment.