Graceful stop database heartbeats in ICS

[GavinFrazar]

Part of: #50237

• There is no way to gracefully stop a single inventory control stream heartbeat #50237

Support stopping an individual database heartbeat in the inventory control stream instead of waiting for multiple keepalives to fail.

[espadolini]

Is this going to stop heartbeating individual databases one by one at shutdown time?

[espadolini]

Shouldn't it be an error to stop a heartbeat that wasn't running?

[GavinFrazar]

The client handle may have been lost and re-established, but the heartbeat may not have been re-announced prior to client sending a "Stop" message, so there's no keepalives running for it but the heartbeat still exists in backend.

We could just log an error message here and return early, letting the heartbeat expire on its own, but why not just delete it instead?

[espadolini]

Is it possible that the heartbeat will get stopped while another copy of the heartbeat has just started heartbeating? I'm not seeing "ownership" of a particular name on the HeartbeatV2 side or the db.Server side. Could this also happen if two competing instances of Teleport are running, say, during an upgrade?

[GavinFrazar]

Is it possible that the heartbeat will get stopped while another copy of the heartbeat has just started heartbeating? I'm not seeing "ownership" of a particular name on the HeartbeatV2 side or the db.Server side.

It's not possible in a single process, because the stop(name) message is only sent when a dynamic db is unregistered.
If a db is updated, we don't send stop(name).
If it's unregistered and then immediately re-registered, the message order would still be stop(name) -> start(name) because gRPC client streams guarantee message ordering of scheduled messages, and we block until the message is scheduled, and our reconciliation loop will handle deletion and recreation in a single goroutine.

This is all non-obvious and somewhat fragile, so I'll just log a debug message (I see no reason to return an error and close the ICS handle) and let the heartbeat expire instead of deleting it

I doubt this edge case will even happen in practice anyway.

Could this also happen if two competing instances of Teleport are running, say, during an upgrade?

I don't think that this edge case is relevant in the code we're discussing, because I think child and parent processes will each have their own control stream.

However, I think you could have a scenario where a forked process is running concurrent with its parent and both are running dynamic db reconciliation loops.
In that case they could both observe events OpDelete(name) ... OpPut(name).
Both would try to send stop(name) -> start(name) messages.
But it's possible that one of the processes exits before sending start(name), so you could get a message order like this:

child: stop(name)
child: start(name)
parent: stop(name)
// parent exits

If this edge case is actually possible, I think we would have to use a conditional delete to handle it, which I doubt we want to do.

It's also a self-healing edge case, so we can just let it happen: the db would disappear temporarily until the child re-announces it after a few minutes.

[GavinFrazar]

changed it to just log a message and skip deleting the heartbeat: d682d06

[GavinFrazar]

Is this going to stop heartbeating individual databases one by one at shutdown time?

No I intentionally omitted that from the db service shutdown code since we already have the graceful "goodbye" to delete all its heartbeats with one message.

Graceful individual heartbeat stop is used for one case: a dynamic database is unregistered. This is important when buffering rate-limited heartbeats in a follow up PR, to avoid a rate-limit delayed heartbeat from being kept alive forever after a database is unregistered.

[espadolini]

Can't we get rid of the fallback announce at this point, so we'd only need to deal with upstreams not supporting in-band delete?

[GavinFrazar]

yeah we can, db heartbeatv2 support was released in 17.0.0 so I was being conservative when I added DELETE IN 19 for the fallback announcer.

[GavinFrazar]

actually we can rip out the announcer from all heartbeat v2 types it seems.

To keep this PR focused and small, I'll just stop passing a fallback announcer in NewDatabaseServerHeartbeat.
Then in another PR I'll rip out the announcer entirely.

[GavinFrazar]

done

edit: and the comment too: a2acaee

[espadolini]

Is it possible that the heartbeat will get stopped while another copy of the heartbeat has just started heartbeating? I'm not seeing "ownership" of a particular name on the HeartbeatV2 side or the db.Server side. Could this also happen if two competing instances of Teleport are running, say, during an upgrade?