liveness: remove one-time init fields from the mu lock #103601

andrewbaptist · 2023-05-18T16:05:24Z

Seperate out fields that don't need to be under the mu lock since they are initialized either prior or part of start.
This simplifies later PRs that refactor liveness.

blathers-crl · 2023-05-18T16:05:28Z

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 · 2023-05-18T16:05:33Z

This change is

andrewbaptist · 2023-05-19T19:39:51Z

This also fixes some "race" conditions related to node startup and notification of liveness changes. I don't think any of them were particularly important, but it could have caused test flakiness. The goal is to be able to cleanly split the 3 main concepts of liveness: persistence, gossip cache, and liveness publishing as they are currently somewhat overlapping.

erikgrinaker

It seems like it would be better to set this up during construction, instead of having to do all this non-obvious synchronization around Start(). Is there a compelling reason not to? The engines are already available during construction, and it seems like it should be possible to propagate the various callbacks as well.

Reviewable status: complete! 0 of 0 LGTMs obtained

andrewbaptist

TFTR:
In theory, this should be possible, but it was a larger refactor than I wanted to take on. I started down this path, but soon realized that it required major surgery to the NewServer and PreStart methods in server. It is pretty difficult with the current code structure.

I gave up on this refactor because I kept hitting issues with the "first node start" and "gossip vs health". One problem was that the liveness is read in the cache from a couple of unexpected places, specifically the replica_range_lease for when it first tries to get an epoch lease (

cockroach/pkg/kv/kvserver/replica_range_lease.go

Line 302 in fc277ea

l, ok := p.repl.store.cfg.NodeLiveness.GetLiveness(nextLeaseHolder.NodeID)

). So that needs to happen before Start is called otherwise we can't get the lease on this range. I am going to add a note that if we ever completely remove epoch leases this should be possible, but I'm not sure how to do it right now.

I moved the one comment from the next commit back into this commit.

Reviewable status: complete! 0 of 0 LGTMs obtained

erikgrinaker

it was a larger refactor than I wanted to take on

Yeah, that's fair -- the startup process is a bit gnarly. But thanks for moving Engines to construction-time! Looks like it should be straightforward to move onSelfLive to construction rather than Start() too, since it only depends on clock and stores which are also available at construction time.

I suppose onSelfLive could also be registered as a regular onIsLive callback to avoid the duplicate logic. But there might be some subtlety around which ones are called when. Just an idea if you feel like it.

Reviewed 2 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andrewbaptist)

pkg/kv/kvserver/liveness/liveness.go line 226 at r2 (raw file):

	// onIsLive is a callback registered by stores prior to starting liveness.
	// It fires when a node transitions from not live to live.
	onIsLive []IsLiveCallback // see NodeLivenessOptions.OnSelfLive

nit: NodeLivenessOptions.OnSelfLive should be RegisterCallback

pkg/kv/kvserver/liveness/liveness.go line 227 at r2 (raw file):

		syncutil.RWMutex
		onIsLive []IsLiveCallback // see NodeLivenessOptions.OnSelfLive
		// nodes is an in-memory cache of liveness records that NodeLiveness

This paragraph was wrongfully deleted.

pkg/kv/kvserver/liveness/liveness.go line 764 at r2 (raw file):

	for _, l := range nl.GetLivenesses() {
		for _, fn := range nl.onIsLive {
			fn(l)

This needs to check that the nodes are actually live, i.e. l.IsLive().

pkg/kv/kvserver/liveness/liveness.go line 770 at r2 (raw file):

	if nl.onSelfLive != nil {
		if _, found := nl.Self(); found {
			nl.onSelfLive(ctx)

This also needs to check that we're actually live, i.e. that we were able to heartbeat.

pkg/kv/kvserver/liveness/liveness.go line 1310 at r2 (raw file):

		// different replica and nudge it into acquiring the lease. This can leak a
		// goroutine in the case of a stalled disk.
		engines := nl.engines

nit: now that this is no longer mutable, we don't need to copy it into a local variable anymore.

pkg/kv/kvserver/node_liveness_test.go line 310 at r2 (raw file):

	verifyLiveness(t, tc)
	pauseNodeLivenessHeartbeatLoops(tc)
	tc.Servers[0].NodeLiveness().(*liveness.NodeLiveness).TestingStart(false)

Seems like it'd be better to just set up the callbacks before starting liveness, and check the time in the callback. We can propagate the callback down via a testing knob, unless we already have a suitable knob.

pkg/kv/kvserver/replica_range_lease_test.go line 131 at r2 (raw file):

					zonepb.DefaultZoneConfigRef()),
			})
			l.TestingStart(true)

This doesn't appear necessary.

andrewbaptist

I cleaned up the onSelfLive (renamed to onSelfHeartbeat to clarify) and had it injected at construction time.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @erikgrinaker)

pkg/kv/kvserver/liveness/liveness.go line 226 at r2 (raw file):