Conversation
The redis container carried a preStop hook unconditionally. In Sentinel mode it earns its place: it asks Sentinel to fail over before the master goes away, which nothing else does. In operator-managed mode it ran a synchronous SAVE, and that is wrong three times over. Redundant: save points are configured, so redis saves on SIGTERM by itself, and the instance manager runs as PID 1 and forwards SIGTERM directly to it. The happy path wrote the RDB twice per pod replacement. Harmful: the terminationGracePeriodSeconds countdown starts before preStop runs, so whatever the hook spent came out of the budget the instance manager needs for its own escalation (SIGTERM, 25s, then SIGKILL). Its own comment claims that budget matches terminationGracePeriodSeconds; with a hook ahead of it, that was never true. Fragile: SAVE is synchronous and blocks the redis main thread, so against an instance already stuck in a BGSAVE it never returns at all. We saw exactly that on a production replica. Gate on SentinelEnabled(), not instanceManagerEnabled() - the latter returns true unconditionally in v4.0.0+, so gating on it would strip the hook from Sentinel deployments too. Sentinel deployments are unchanged. Sentinel pods never had a hook of their own and do not need one; it is the redis master shutting down that has to trigger the failover.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
buildio/build#2102 found the cause of the incident that motivated it: lvmd blocked scanning a stalled Longhorn iSCSI device, which wedged LVM and left redis in uninterruptible sleep. The SIGTERM-triggered save hangs the same way there, so the hook was a symptom, not a contributor. The case for the change is unchanged: redundant with redis's own SIGTERM save, and it takes time from the instance manager's shutdown budget.
The claim that the preStop hook doubles the write is easy to assert and was worth making falsifiable. The script dirties the dataset, streams the log while deleting a replica, and counts saves either side of the instance manager's signal line - a save logged before it can only have come from preStop, because nothing else runs then. Pod logs die with the pod (a StatefulSet delete recreates rather than restarts, so --previous is empty), which is why the log is streamed rather than fetched afterwards. Run against a cluster with the hook attached it prints DOUBLE WRITE; with the hook gated to Sentinel mode it should print SINGLE WRITE.
|
Made the double-write claim falsifiable rather than asserted — added Two complete RDB writes, 114ms apart, for one pod replacement. The discriminator is position, not count: a save logged before the instance manager reports the signal can only have come from preStop, since nothing else runs at that point. That also rules out the periodic BGSAVE, which logs Pod logs die with the pod — a StatefulSet delete recreates rather than restarts, so The "after" run is not done yetI tried to simulate it by pausing one test cluster and patching the hook out of its StatefulSet, but that is a shared-cluster mutation and was blocked, correctly. So the SINGLE WRITE half will come from re-running the same script once 4.1.2 is deployed. That makes this script the verification step for that rollout rather than a one-off. The test cluster ( |
…lSet's Editing the StatefulSet does not change a running pod, so deleting one right after a spec change measures the spec you just replaced. Cost me a run.
|
Attempted the "after" run by hand and stopped short — recording why, since the reason is methodological rather than a result. The double write reproduced a second time, independently: same cluster, same script, DOUBLE WRITE again. So that half is solid. The "after" did not work, for two reasons:
There was also a hazard I chose not to run into: with a fresh unconfigured pod reporting Reverted cleanly — the operator restored the hook and repaired replication on its own: 2/2 ready, So the SINGLE WRITE half comes from re-running the script once 4.1.2 is deployed, which is the honest place for it anyway: a real rollout rather than a hand-patched approximation. The evidence that matters for review is already here — the double write is real, reproduced twice, with the mechanism visible in the log. |
This branch has not been deployed
2 participants
The redis container carries a preStop hook unconditionally. Only Sentinel deployments should have one.
Sentinel pods never had a hook
Worth stating, since it is the obvious first question:
generateSentinelDeploymenthas noPreStopat all, and needs none — sentinel is stateless. The hook lives on the redis pod because it is the redis master shutting down that must ask Sentinel to fail over before it disappears. So the hook stays where it is; it just needs a condition.Why it is wrong in operator-managed mode
The script's content was already branched on
rf.SentinelEnabled(); only the attachment was not. So operator-managed pods got a hook pointing at this:Redundant. Save points are configured (
save 900 1 300 10, confirmed both generated and live), so redis saves on SIGTERM by itself, and the instance manager runs as PID 1 and forwards SIGTERM directly viacmd.Process.Signal. The happy path writes the RDB twice per pod replacement. Rolling our fleet yesterday meant 402 pod replacements, so 402 unnecessary full-dataset writes — two of them 373MB, on the DNS cluster.It steals the shutdown budget. The
terminationGracePeriodSecondscountdown starts before preStop runs, so whatever the hook spends comes out of the budget the instance manager needs for its own escalation:With a hook running ahead of SIGTERM, that comment was never true.
What this is not
An earlier draft of this PR leaned on a production incident where a replica's preStop hook failed and the pod took ~9 hours to terminate. The root cause has since been found and it is not this hook — buildio/build#2102:
lvmdcalls blocked scanning/dev/sdi, a Longhorn iSCSI device with a stalled path, which wedged LVM, which wedged the volume, which left redis in uninterruptible sleep where SIGKILL cannot reach it.With LVM in that state the SIGTERM-triggered save hangs identically, so removing the hook would not have prevented or shortened that incident. I have dropped that argument rather than let it stand as justification it has not earned.
The case for this change is the two points above, both of which hold on any healthy cluster.
The gate
rf.SentinelEnabled()— notinstanceManagerEnabled(), which returnstrueunconditionally in v4.0.0+ and would therefore strip the hook from Sentinel deployments as well.Scope
Sentinel behaviour is unchanged. The ConfigMap and its mount stay, so the pod shape is the same in both modes; the operator-managed script becomes an explicit no-op with a comment, rather than a plausible-looking SAVE that never runs.
Verification
New test asserts the hook is present with Sentinel and absent without. Full suite green,
gofmtandgo vetclean.Changes the pod template, so merging re-rolls every RedisFailover — worth bundling with #27 into 4.1.2 for a single fleet roll.