liveness: allow registering callbacks after start

[tbg]

I discovered¹ 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 #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.

Epic: None
Release note (bug fix): it was possible for a node status to reflect a "last up" timestamp that lead the actual last liveness heartbeat of the node. This has been fixed.

Footnotes

1. https://github.com/cockroachdb/cockroach/issues/106706#issuecomment-1640254715 ↩

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

[cockroach-teamcity]

This change is 

[erikgrinaker]

Right, this is probably fallout from #103601.

[andrewbaptist]

Thanks Tobi!

I see what you mean about this test, and it was tricky to get some of tests, including this one, to work without doing what you did by allowing later registration.

The approach you have works, but I had considered two other options as well (and could code them up if you want)

1. Don't allow disks/stores to be added to a running instance, instead require a restart. I wasn't sure if this was something that we support in a field deployment but I should have checked first. If we don't support this, then it would be possible to rearrange the code to allow this to work

2. Have a "single" node level callback that then fans out to the stores on liveness changes. This simplifies the liveness code as there is only a single callback, and the node already has locking in place for adding stores.

Your approach also works and is probably the most straightforward.

[tbg]

Don't allow disks/stores to be added to a running instance, instead require a restart. I wasn't sure if this was something that we support in a field deployment but I should have checked first. If we don't support this, then it would be possible to rearrange the code to allow this to work

The new stores are being added following a restart. However, due to the need to assign StoreIDs, KV needs to be up and running. So these new stores are added "async". I don't see a good way around that.

Have a "single" node level callback that then fans out to the stores on liveness changes. This simplifies the liveness code as there is only a single callback, and the node already has locking in place for adding stores.

That's not a bad idea, though I won't be able to do this until I leave (and it might be crowded out anyway). So even though I like that idea I'd like to stick with what I have, so that we can at least address the bug.

[andrewbaptist]

[erikgrinaker]

We'll want a targeted backport here too, since it seems like prior versions are vulnerable to the same issue (but I'm not sure with all the refactoring in this area).

[tbg]

We'll want a targeted backport here too, since it seems like prior versions are vulnerable to the same issue (but I'm not sure with all the refactoring in this area).

I'm not sure we should fix this in a backport. The chances of messing something up are much higher than anyone hitting this bug. Folks very very rarely add stores to existing nodes (multi-storage in itself is rare) and doing so in a way that loses quorum on the storeIDGen is even rarer. Also, there is a workaround - restart a quorum without the extra store.

If you disagree, let's file an issue because I am not sure I would get to this backport even if we tried.

[erikgrinaker]

Ok, I can buy that. Was thinking of surrounding issues like the self callback too though, but they're fairly minor.

[tbg]

The self-callback I can fix in a backport - we can just add a conditional (if we don't insist on also adding a test).

The hang at startup, btw, I think has existed in every past version of CRDB, and to the best of my knowledge was never hit by anyone in a real cluster. Even the test that caught it only did so after I refactored it and made it use in-mem engines. So I am fairly confident that not backporting is the reasonable strategy here.

[tbg]

bors r=erikgrinaker

[tbg]

Backports:

#107605
#107606

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)