feat: add an opt-in write probe to the health checks - #32
Merged
Conversation
check_async_redis_health and check_redis_health now take a keyword-only write_key. Given one, the check still pings first and then runs SET <write_key> 1 EX 60, so a read-only replica and a primary at maxmemory under noeviction -- both of which answer PONG and refuse every write -- come back False instead of True. ReadOnlyError, OutOfMemoryError and anything else the write raises go through the branch that already turns connection trouble into False plus a warning, so the check still never raises. Without write_key nothing changes: the check is the PING it has always been. The key is the caller's to name and to prefix, since this package applies key_prefix to nothing, and it is left to expire rather than deleted.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
check_async_redis_healthandcheck_redis_healthare aPING, andPINGanswersPONGfrom a server that cannot take a write. A primary that has reachedmaxmemoryundernoevictionrefuses everySETfrom that moment on; a replica that a failover or a DNS mistake put in front of the client refuses every write and serves stale reads. Both are states in which a service that uses Redis for anything but reads is down, and both came backTrue, so readiness stayed green and the pods kept taking traffic they could not serve. Rule 10 promised a check that "never says maybe", and it was saying yes when the answer was no.What changed
One keyword-only argument on both health functions,
write_key.Given a key, the check still pings first, and then runs
SET <write_key> 1 EX 60(WRITE_PROBE_TTL_S, now exported fromredis_client_kit.utils). It keeps the ping because on a cluster that is one answer per node, and opting into the write should not cost the all-nodes coverage; the write itself lands on the one node that owns the key's slot, which the docs say.Trueonly when both answer.ReadOnlyError,OutOfMemoryErrorand anything else the write raises go through the branch that already turns connection trouble intoFalseplus a warning in the log, so the check still never raises. Each command has its ownsocket_timeout, so the worst case is two of them, and a paused server is still decided by the ping alone.No
GETand noDEL:OKfromSETis the proof that the server takes writes, andEXlets the key expire on its own rather than adding one more command that can fail. The key is the caller's to name and to prefix — this package applieskey_prefixto nothing (rule 7).The default is untouched. Without
write_keythe check runs the same singlePINGit has always run, and no existing caller sees a change.Rule 10, the two API rows, the
redis_client_kit.utilsrow, a "common mistakes" block and the errors note on the agents page, the health sections of the quick start and the cluster guide, and the README example all say what the code does.What I rejected
probe="ping" | "write", which is what the issue proposed. The write needs a key either way, so a mode string plus a key is two arguments where the key alone carries the decision, and it leavesprobe="write"with no key as a state to reject at run time. A key present is the opt-in.A callable
probe=. General enough to also take the cluster-state check the issue mentions as a follow-up, but that one is not a write, and I would rather add it on its own terms when there is a design for it than open a hook now.Threading
key_prefixin through the settings.RedisSettingsProtocolhas nokey_prefix— it is aBaseRedisSettingsfield this package reads nowhere — so building the key inside the check would mean either a new protocol member or a second argument. The caller already owns prefixing.The Dishka provider's startup check stays a
PING. It has no key to write and no settings member to build one from; giving it one is a separate decision.Tests
Unit, in both flavours: no
write_keypings and never touchesset;write_keycallsset(key, "1", ex=WRITE_PROBE_TTL_S)once, after the ping; aReadOnlyErroror anOutOfMemoryErrorfrom the write returnsFalse; a ping that answers falsy returnsFalsewithout attempting the write; a cluster ping of every node followed by the write returnsTrue.Integration, in both flavours, against
redis:7-alpinecontainers: a healthy server returnsTrueand leaves the key with a TTL insideWRITE_PROBE_TTL_S; a container started asredis-server --maxmemory 1mb --maxmemory-policy noevictionand then filled until it refuses a write answers the pingTrueand the probeFalse; a container started asredis-server --replicaof primary 6379, waited on until its log saysMASTER <-> REPLICA sync: Finished with successagainst a live primary on the same network, does the same. Each of those asserts both answers, so the ping-only default is pinned in the same test as the new behaviour.With master's
aio/lifecycle.pyandsync/lifecycle.pyswapped back in, all six new integration tests fail withTypeError: check_redis_health() got an unexpected keyword argument 'write_key', and the six that were there before pass.Before and after
The reporter's
health_lab.pyservers, each asked both ways,socket_timeout=0.5.The
PING healthcolumn is master's answer, unchanged. The two middle rows are the issue.Gate
make check: ruff checkAll checks passed!, ruff format54 files already formatted, mypySuccess: no issues found in 25 source files.make test:162 passed, coverage 99.65% against the 90% floor.uv.lockuntouched.CHANGELOG.mdis release-please's.Closes #31