sqlliveness: no deadlines on heartbeats

[joshimhoff]

Describe the problem
From what I can see, there are no deadlines on sqlliveness heartbeats. This means a single hung heartbeat will expire a sqliveness session. A hung heartbeat can happen for various reasons, e.g. temporary disk overload. Multi-tenant SQL pods crash if a sqliveness session expires right now. So a single hung heartbeat can actually crash a multi-tenant SQL pod. I believe we have observed this in practice, tho I'm only medium confident about that claim (link to internal support issue: https://github.com/cockroachlabs/support/issues/1736).

To Reproduce
N/A

Expected behavior
There should be deadlines on sqlliveness heartbeats.

Additional data / screenshots
Link to internal support issue: https://github.com/cockroachlabs/support/issues/1736

Jira issue: CRDB-18302

[andreimatei]

@joshimhoff , the timeout that was introduced as a result of this issue having been filed has been removed because its implementation was buggy. Further, we couldn't convince ourselves that timeouts over there serve any purpose - why would one want to fail the respective query that's trying to heartbeat the session record, only for it to be retried, as opposed to letting it hang?
There's a link above to a slack conversation that suggests that perhaps observability would be better if these queries failed instead of hanging, but that seems unconvincing - there is a metric for the success of these queries, and the absence of success tells a story.

If you have thoughts, feel free to reopen the issue.

[joshimhoff]

I don't think it's about o11y. I think it's about reliability. The ticket description re: this is admittedly a bit vague. I include it down below to see if we can make the argument more concrete.

This means a single hung heartbeat will expire a sqliveness session. A hung heartbeat can happen for various reasons, e.g. temporary disk overload. Multi-tenant SQL pods crash if a sqliveness session expires right now. So a single hung heartbeat can actually crash a multi-tenant SQL pod.

I think we can rephrase ^^ as a question: Are there situations that happen in production where without deadlines & retries a hung heartbeat will stay hung forever leading to a SQL pod crash? Unless we are very sure the answer is no, I think setting a deadline plus doing retries is a good pattern, in particular since the consequence of session expiration is currently a SQL pod crashing. CC @ajwerner in case he has thoughts.

[andreimatei]

Are there situations that happen in production where without deadlines & retries a hung heartbeat will stay hung forever leading to a SQL pod crash?

A heartbeat query can hang when faced with various outages, but in those situations the retries will hang as well - so I don't think they serve any purpose other than to muddy the code. You can imagine bugs that would cause a retry to succeed where the original query is hanging, but I don't think we want to protect against such things.

[joshimhoff]

You can imagine bugs that would cause a retry to succeed where the original query is hanging, but I don't think we want to protect against such things.

By "bug" do you mean a bug in the CRDB code, or do you mean certain poor infra conditions, e.g. blips in the network? I think we certainly want to be robust to the latter. Re: the former, I think we do too, but I can see that as being a more contentious view.

[andreimatei]

I mean a bug in CRDB code. I don't think sprinkling timeouts in more or less random places to protect against hypothetical bugs is a good general policy. To go to pedantry, in my opinion, as far as most code is concerned, the only thing that matters when a bug is present is how easy it is to find the bug - so things that help debugging are good; things that hide bugs are bad; and things that transparently recover from bugs are generally not worth it. In this case, the timeout we're talking about, at least in the straight-forward way to implement it, would destroy the state of the stuck operations (i.e. the stacks traces involved are gone).

[joshimhoff]

We don't have the same view about the utility of higher level controls reducing the impact of bugs in lower level code, but the main reason I want a timeout in place is NOT to protect against bugs in the CRDB code. I want it to keep poor infra conditions e.g. in the network from leading to SQL pod crashes. I guess the basic idea here is that there are infra conditions that happen in practice that lead to a hung heartbeat that succeeds on retry. I don't know if that is actually a real thing (e.g. TCP is already doing retries). As I think more about it, I realize I haven't introspected deeply on why I want timeouts until this discussion, so thanks for having it with me. At same time, there is a more basic argument tho which is that we should not crash the server unless that actually benefits the user in some way. In the case of a stuck SQL liveness heartbeat, crashing the SQL pod is doing nothing useful for the user, and it does reduce reliability, at the very least by slowing down recovery (restart takes time esp. in some environments) and breaking a bunch of stateful SQL connections.

I am interested in hearing what some other people think about this, as I realize my view is a bit uncertain.

CC @ajwerner @bdarnell

[andreimatei]

I guess the basic idea here is that there are infra conditions that happen in practice that lead to a hung heartbeat that succeeds on retry. I don't know if that is actually a real thing (e.g. TCP is already doing retries).

I don't think it's a real thing.

---

I agree that killing the process on session expiration is a bad idea. The only tiny written about it that I can find is here. I have failed to impress others to rethink this, and I didn't see it within my power to change when I was touching this code (and I was touching it, btw, to fix bugs partially introduced by this timeout having been added). Killing the process is unfortunately tied to other decisions about how the sql instance IDs are allocated - other instances are quick to steal the ID corresponding to an expired session, and having multiple servers use the same ID is not good for various reasons.

[ajwerner]

We can definitely do more to avoid killing the process. I don't think that work is particularly related to the discussion on timeouts.

[bdarnell]

I guess the basic idea here is that there are infra conditions that happen in practice that lead to a hung heartbeat that succeeds on retry. I don't know if that is actually a real thing (e.g. TCP is already doing retries).

I think this is possible if the network is acting up and one TCP connection is horked but the packets are still flowing and new connections can be created. This is why we have RPC-level heartbeats on our inter-node connections. Are those being used for SQL pods? I think they should be. Without them we're reliant on TCP keepalives which IIRC have limited configurability and long timeouts.

Other than network-level issues which are better addressed with network-level heartbeats and timeouts, I think most things that would cause one heartbeat to hang would also cause the next attempt to hang, so there's little value in using a timeout to retry the exact same request.

However, AIUI, it's not the exact same request - the update includes an expiration time which was in the future when the attempt started. If that time is now in the past, it's better to cancel the previous attempt and start over. I don't think that's going to actually make much difference because the new attempt will still hang, but at least it means that things have a chance of working as soon as the blockage is cleared, instead of leaving the now-useless original request in flight.

We implemented timeouts for this reason in node liveness, and I think it's turned out to be important there. So it seems like it's probably going to be worthwhile to do something similar for sqlliveness.

[ajwerner]

Are those being used for SQL pods?

Yes, the connections with the sql pods use the same heartbeat logic.

[andreimatei]

However, AIUI, it's not the exact same request - the update includes an expiration time which was in the future when the attempt started. If that time is now in the past, it's better to cancel the previous attempt and start over. I don't think that's going to actually make much difference because the new attempt will still hang, but at least it means that things have a chance of working as soon as the blockage is cleared, instead of leaving the now-useless original request in flight.

We implemented timeouts for this reason in node liveness, and I think it's turned out to be important there. So it seems like it's probably going to be worthwhile to do something similar for sqlliveness.

The context is different between this sql liveness session that's being heartbeated here, and node liveness. For node liveness, recovering as soon as possible is pretty important: if the liveness range is only able to process one hb every, say, 10s, that one hb that it eventually processes better be one that's not already expired. (Although flapping between alive and expired is probably worse than being consistently expired, so I question even that timeout).
Here, there's two differences:

1. The expiration of sql liveness is much slower - 40s. So the respective range has much more time to process the heartbeats before sessions expire.
2. Once the session expires, the process shuts down. So, given the 40s liveness expiration, there's quite a limited window where new requests are better than the original, stuck request.
   Do you buy that this color makes a difference?

How I feel about this timeout is also influenced by the fact that, when we added this timeout, we also introduced a fat bug. I take that as proof that this kind of complications to the code should not be done gratuitously.

[ajwerner]

It seems to me like the order of operations here should be to make it unlikely that the process needs to shut down upon expiration first, and then to talk about adding a timeout.

[joshimhoff]

Appreciate the color from everyone; it's clarifying.

It seems to me like the order of operations here should be to make it unlikely that the process needs to shut down upon expiration first, and then to talk about adding a timeout.

Seems reasonable esp. given IIUC the point Andrei is making about there being little time post expiration but pre server shutdown.

How I feel about this timeout is also influenced by the fact that, when we added this timeout, we also introduced a fat bug. I take that as proof that this kind of complications to the code should not be done gratuitously.

Alternatively, that code is too complex, with the proof being that we introduced a bug by adding timeouts ;)

[bdarnell]

Once the session expires, the process shuts down. So, given the 40s liveness expiration, there's quite a limited window where new requests are better than the original, stuck request.
Do you buy that this color makes a difference?

Yes. We'd need to make session expiration non-fatal before adding a timeout would make sense.

[andreimatei]

Thanks all. Closing...