raft: use RawNode for node's event loop; clean up bootstrap

[tbg]

raft: use RawNode for node's event loop

It has always bugged me that any new feature essentially needed to be
tested twice due to the two ways in which apps can use raft (`*node` and
`*RawNode`). Due to upcoming testing work for joint consensus, now is a
good time to rectify this somewhat.

This commit removes most logic from `(*node).run` and uses `*RawNode`
internally. This simplifies the logic and also lead (via debugging) to some
insight on how the semantics of the approaches differ, which is now
documented in the comments.

raft: clean up bootstrap

This is the first (maybe not last) step in cleaning up the bootstrap code
around StartNode.

Initializing a Raft group for the first time is awkward, since a
configuration has to be pulled from thin air. The way this is solved today
is unclean: The app is supposed to pass peers to StartNode(), we add
configuration changes for them to the log, immediately pretend that they
are applied, but actually leave them unapplied (to give the app a chance to
observe them, though if the app did decide to not apply them things would
really go off the rails), and then return control to the app. The app will
then process the initial Readys and as a result the configuration will be
persisted to disk; restarts of the node then use RestartNode which doesn't
take any peers.

The code that did this lived awkwardly in two places fairly deep down the
callstack, though it was really only necessary in StartNode(). This commit
refactors things to make this more obvious: only StartNode does this dance
now. In particular, RawNode does not support this at all any more; it
expects the app to set up its Storage correctly.

Future work may provide helpers to make this "preseeding" of the Storage
more user-friendly. It isn't entirely straightforward to do so since the
Storage interface doesn't provide the right accessors for this purpose.
Briefly speaking, we want to make sure that a non-bootstrapped node can
never catch up via the log so that we can implicitly use one of the
"skipped" log entries to represent the configuration change into the
bootstrap configuration. This is an invasive change that affects all
consumers of raft, and it is of lower urgency since the code (post this
commit) already encapsulates the complexity sufficiently.

[bdarnell]

The first commit LGTM. I agree in spirit with the second commit, but worry about backwards compatibility. For better or worse, RawNode has supported the same auto-initialization as Node, so someone other than CockroachDB could be depending on it (at least the signature change on NewRawNode will break their build so the failure can't pass silently).

I think that since the first commit reduces the duplication so the bootstrapping only lives in one place, it's probably best if that one place continues to be in RawNode instead of StartNode, at least until the anticipated helpers are available.

[tbg]

I think that since the first commit reduces the duplication so the bootstrapping only lives in one place, it's probably best if that one place continues to be in RawNode instead of StartNode, at least until the anticipated helpers are available.

Makes sense. I didn't want to bring this mostly-useless extra arg back though, so I added RawNode.Bootstrap which is now called from StartNode. A comment on NewRawNode indicates to callers that calling Bootstrap(peers) replaces the previous peers parameter.