liveness: allow registering callbacks after start #107265

The two removed fields are nil. This made a test failure during the refactors in this PR more annoying. If we're going to set up a half-inited NodeLiveness, let's at least be honest about it.

This will help with testing.

We'll give this a proper interface soon.

So that it can implement a public interface.

It's needed to implement the liveness storage (once it exists).

We still want a Storage to be passed into NewNodeLiveness as opposed to a `*kv.DB`, but so far, so good.

Now Liveness is constructed using a `Storage` as opposed to a `*kv.DB`.

I discovered[^1] a deadlock scenario when multiple nodes in the cluster restart with additional stores that need to be bootstrapped. In that case, liveness must be running when the StoreIDs are allocated, but it is not. Trying to address this problem, I realized that when an auxiliary Store is bootstrapped, it will create a new replicateQueue, which will register a new callback into NodeLiveness. But if liveness must be started at this point to fix cockroachdb#106706, we'll run into the assertion that checks that we don't register callbacks on a started node liveness. Something's got to give: we will allow registering callbacks at any given point in time, and they'll get an initial set of notifications synchronously. I audited the few users of RegisterCallback and this seems OK with all of them. [^1]: cockroachdb#106706 (comment) Epic: None Release note: None

I think there was a bug here. This method was previously invoked in `updateLiveness`, but that method is the general workhorse for updating anyone's liveness. In particular, it is called by `IncrementEpoch`. So we were invoking `onSelfHeartbeat` when we would increment other nodes' epochs. This doesn't seem great. Additionally, the code was trying to avoid invoking this callback before liveness was officially "started". Heartbeating yourself before liveness is started is unfortunately a thing due to the tangled start-up initialization sequence; we may see heartbeats triggered by lease requests. Avoid both complications by invoking `onSelfCallback` from the actual main heartbeat loop, whose only job is to heartbeat the own liveness record. I tried to adopt `TestNodeHeartbeatCallback` to give better coverage, but it's a yak shave. A deterministic node liveness (i.e. a way to invoke the main heartbeat loop manually) would make this a lot simpler. I filed an issue to that effect: cockroachdb#107452

Helps with testing.

This tests that regardless of when a callback is registered, it gets called.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

liveness: allow registering callbacks after start #107265

liveness: allow registering callbacks after start #107265

Commits on Jul 24, 2023