liveness: allow testing main heartbeat loop deterministically #107452
Labels
A-testing
Testing tools and infrastructure
C-cleanup
Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior.
C-enhancement
Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
T-kv
KV Team
Comments
tbg
added
C-enhancement
tbg
added a commit
to tbg/cockroach
that referenced
this issue
Jul 24, 2023
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
andrewbaptist
added
the
C-cleanup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Liveness starts a goroutine that heartbeats the own liveness record periodically. This means that lots of tests that interact with liveness need to exert control over this goroutine. It would be easier to test NodeLiveness if the concurrency were externalized. In other words, rather than
NodeLiveness.Startspawning a goroutine that loops and runs code that can only be reached by that goroutine, the goroutine should be a method onNodeLivenessthat can be invoked manually, andStartshould take a suitably defined abstraction over a "periodic runner". In prod, the periodic runner would be the familiar async task with a for loop. In testing, it may just be a no-op, and the harness can call the method directly whenever it wants to pretend the ping interval elapsed.It's a bit tricky to get this right but there is probably an abstraction here that applies similarly to many other subsystems in CRL that start auxiliary goroutines (which are then difficult to test against). We should architect to give tests as much control over concurrency as possible.
Jira issue: CRDB-30050
The text was updated successfully, but these errors were encountered: