Repository navigation
Conversation
Call updates (SET) and deletions (DEL) are pipelined, and redis_consume() freed their replies unread: a write refused with READONLY or NOREPLICAS, as during a Sentinel failover, left no trace in rtpengine. Check every reply. An error reply is counted in a new statistic, rediswriteerrors, and logged at most once a second per connection; the connection is kept, since a reconnect would get the same answer. A read error or timeout on the replies drops the connection at once instead of at the next PING. An encoding failure no longer drops the connection. Tested by auto-daemon-tests-redis-errors.pl, with a new fake Redis module, NGCP::Rtpengine::FakeRedis.
On a role mismatch, the reconnect in redis_check_conn() sleeps one second, asks INFO again and sleeps once more: two seconds under r->lock, which every Redis update holds before the ng reply (sipwise#1865). After a failover, with --redis on the old master's address, every update did this, and ng requests timed out. Make one attempt, with no sleep. After a role failure, or a failure that took --redis-reconnect-slow (100 ms) or more, skip Redis operations on that connection for --redis-reconnect-pause (1000 ms), through restore_tick_us as --redis-allowed-errors does. A quick failure (connection refused) is retried at once, as before. As with a failed attempt, a skipped operation is not written; skips are logged once per pause. The startup connection keeps its eleven attempts. Tested by auto-daemon-tests-redis-errors.pl.
cli_handle() takes a str, but most "set" handlers parsed their value
with strtol(), strtod() or atoi() on instr->s and printed it with "%s",
reading up to the next NUL. The ng "cli" command passes a slice of the
request: "set maxsessions 1000" set 10007, "set rediscmdtimeout 0" set
7 ms, and error messages quoted the rest of the request.
Parse with the str_to_X helpers, plus a new str_to_d for maxcpu and
maxload, and print with STR_FORMAT. As with the str_to_X family, a
value with trailing characters ("10x") is refused, and so is a
non-number in the seven handlers that read it as 0. "terminate" now
matches all, own and foreign exactly (str_cmp()), not as a prefix.
Tested by auto-daemon-tests-redis-errors.pl and the new t/test-str.c.
The timer that rewrites never-stored SRTP calls can hold a call while it waits for r->lock. If the call is deleted meanwhile, the deletion, deciding without the lock that the call was never stored, deletes nothing, and the timer then writes it: the call is back at the next restore. Any update racing with a deletion can do the same. Under r->lock, redis_update_onekey() writes a call only while it is still in the call table, and redis_do_delete() decides "never stored", so that a write already past that check is followed by the DEL. Tested by auto-daemon-tests-redis-destroyed.pl.
Behind Redis Sentinel, rtpengine stays on the address it was given: after a failover its writes go to the old master, which refuses them until it is demoted, then serves them as a replica. New options --redis-sentinel, --redis-sentinel-master and --redis-sentinel-interval; the keyword "sentinel" replaces IP:PORT in --redis, --redis-write or --redis-subscribe. A "redis sentinel" thread asks every Sentinel for the master, takes the highest config epoch (never an older one than in use), checks the node with ROLE and publishes it with a generation number. It also runs at once when a connection meets a replica. redis_check_conn() compares the generation and reconnects on a change; these connections require the master role. Without "sentinel", none of this runs. Tested by auto-daemon-tests-redis-sentinel.pl and -sentinel-start.pl.
A subscription made through "sentinel" stayed on the old master after a failover, and missed the notifications of the new one. With Sentinel, the notification event loop gets a one-second timer: when the master changed, it closes the subscription, and redis_notify_loop() subscribes again on the new master. The subscription's context also gets a connect callback, since hiredis frees a context that fails to connect without calling the disconnect callback. Notifications missed during the switch are not replayed. Tested by auto-daemon-tests-redis-sentinel-subscribe.pl.
After a failover the new master refuses writes with NOREPLICAS until it has a replica. Those writes are counted and logged, but never made again: a call created then is missing at the next restore, and a call deleted then comes back. With --redis-repair-writes (off by default), an own call whose write was not stored (an error reply, a connection lost before the reply, a skipped write) is marked, and a stored write clears the mark. A "redis repair" thread writes the marked calls again, one at a time, through redis_write_call(), now shared with redis_update_onekey(): a live call gets its current state, a call that is gone a DEL, unless a new call uses its ID. Signalling writes waiting for r->lock go first, for 100 ms at most. A failed write is retried after a second; at most 100000 calls are kept. New statistic: redisrepairedwrites. A repaired DEL assumes one writer per database. Tested by auto-daemon-tests-redis-repair.pl.
Replication is asynchronous, so a master that is replaced takes with it the writes it acknowledged but had not replicated: a call created then is missing on the new master, and a call deleted then comes back. --redis-resync-on-master-change (off by default) keeps the master_replid of the INFO that redis_connect() already sends. When the write connection reconnects to another ID, every own call stored at least once, and the own DELs of the last --redis-resync-delete-window (at most --redis-resync-delete-max), are queued for the repair thread of --redis-repair-writes. The write connection must require the master role. No key is scanned. New statistics: redisresyncs, redisresynccalls and redisresyncdeletes. Tested by auto-daemon-tests-redis-resync.pl.
A reconnect attempt runs under r->lock, which every ng request that writes a call waits for (sipwise#1865). Against a server that drops packets it takes the full connect timeout, so signalling stalls for up to that long at each attempt, for as long as the outage lasts. Make the attempt without the lock: the thread that reconnects sets r->reconnecting and owns r->ctx until it clears the flag under the lock. Other threads wait for it for up to --redis-reconnect-slow, then skip their operation, as during the pause. After the lock was released, redis_update_onekey() checks again that the call is live, and redis_do_delete() leaves the key to a new call with the same ID. The CLI, the DEL after a failed restore and the notification thread leave a connection being made alone. A command on an established connection still holds the lock. Tested by auto-daemon-tests-redis-reconnect.pl.
redis, redis-json and redis-subscribe compared raw bytes on their own sockets, and redis-srtp-index and rollback-redis each carried the same forked server. They now use NGCP::Rtpengine::FakeRedis, and the forked server moves into it as redis_serve(). The checks, their names and their counts are unchanged.
--redis-write accepts any role, so after a failover that demotes its node, rtpengine reconnects to the same address, accepts the replica, and every later write is refused with READONLY. --redis-require-master (off by default) makes the connections that write call data require the master role at connect, and drop the connection on a write refused with READONLY or MASTERDOWN, so that the next operation reconnects and checks the role. Other refusals keep the connection. Tested by auto-daemon-tests-redis-require-master.pl.
kbinetruy
force-pushed
the
redis-sentinel
branch
2 times, most recently
from
October 20, 2026 09:52
82d140f to
f8d776b
Compare
rfuchs
reviewed
Oct 20, 2026
| return; | ||
| LOCK(&redis_repair_lock); | ||
| if (!redis_repair_pending) | ||
| redis_repair_pending = g_hash_table_new_full(g_str_hash, g_str_equal, g_free, NULL); |
Member
There was a problem hiding this comment.
Consider using str objects as key type for the hash table (str_hash + str_equal) to avoid having to dup/copy the key strings all the time.
| static mutex_t redis_repair_lock = MUTEX_STATIC_INIT; | ||
| static cond_t redis_repair_cond = COND_STATIC_INIT; | ||
| static GHashTable *redis_repair_pending; | ||
| static atomic64 redis_repair_count; /* entries, for a lock-free test */ |
Member
There was a problem hiding this comment.
repair_count seems to be effectively just a bool flag? Also since it's used as a condition it should always be accessed under the lock. AFAICT it can probably be removed altogether?
| } | ||
| } | ||
| g_hash_table_replace(redis_repair_pending, g_strndup(callid->s, callid->len), | ||
| GINT_TO_POINTER(db + 1)); |
Member
There was a problem hiding this comment.
Why + 1 and then - 1 below? Is the LLM assuming that this is needed to distinguish absent keys from keys with zero values?
Comment on lines
+4191
to
+4195
| if (g_hash_table_iter_next(&it, &k, &v)) { | ||
| g_hash_table_iter_steal(&it); | ||
| id = k; | ||
| db = GPOINTER_TO_INT(v) - 1; | ||
| } |
Member
There was a problem hiding this comment.
How about keeping call_t objects directly in the hash table (by reference) instead of using their call IDs and then having to look them up? Could even use a queue instead of hash table. Just an idea.
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
2 participants
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.
Behind Redis Sentinel, rtpengine stays on the address it was given. After a failover its writes go to the demoted master, which refuses them (with
--redis-write, for good), and the new master refuses writes withNOREPLICASuntil it has a replica. Refused or skipped writes are never made again. These five opt-in commits follow the master through Sentinel and write back what the failover lost; without the new options, nothing changes.This PR depends on #2211. It contains those six commits (11 on the branch). Only the last five are new:
MT#55283 redis: add --redis-require-masterMT#55283 redis: find the master through Redis SentinelMT#55283 redis: keyspace subscription follows the Sentinel masterMT#55283 redis: add --redis-repair-writesMT#55283 redis: add --redis-resync-on-master-changeThe five commits on their own: kbinetruy/rtpengine@redis-failover-fixes...redis-sentinel (21 files, +2898 −74; 1611 of the added lines are tests).
min-replicas-to-write 1, ~21 writes/sWith Sentinel, rtpengine used the new master 1.26–1.66 s after
SENTINEL FAILOVER. Valkey 8.1 gave the same results.1.
--redis-require-master. The connections that write call data require the master role at connect, and drop the connection on a write refused withREADONLYorMASTERDOWN. The reconnect then checks the role, paced by the fixes PR.NOREPLICASkeeps it.2. Sentinel. The keyword
sentinelreplacesIP:PORTin--redis,--redis-writeor--redis-subscribe. A "redis sentinel" thread asks every Sentinel each--redis-sentinel-interval, and at once on a role error. It takes the highest config epoch, never an older one than in use, checks the candidate withROLE, and publishes it with a generation number.redis_check_conn()compares that number (one atomic read) and reconnects on a change. No Sentinel I/O runs underr->lock.3. Subscription. With
sentinel, a one-second timer in the notification loop closes the subscription when the master changed, and the loop subscribes again on the new master. Missed notifications are not replayed.4.
--redis-repair-writes. An own call whoseSETorDELwas refused, lost with the connection or skipped is queued (at most 100000). A "redis repair" thread writes it again, one call at a time: the call's current state, or aDELunless a new call has taken the ID. After taking the lock it lets waiting signalling writes go first (a condition wait, at most 100 ms), and waits 1 s after a failed write.5.
--redis-resync-on-master-change.redis_connect()keepsmaster_replidfrom itsINFO. When the write connection reconnects to a master with another ID (Sentinel switch, moved address, restart), every own call stored at least once and the recent ownDELs are queued for the repair. No key is scanned.--redis-require-master--redis-sentinel=[PW@]IP:PORT[,…]--redis-sentinel-master=NAME--redis-sentinel-interval=MS--redis-repair-writes--redis-resync-on-master-change--redis-resync-delete-window=MSDELs are remembered for the resync.--redis-resync-delete-max=INTNew statistics:
redisrepairedwrites,redisresyncs,redisresynccallsandredisresyncdeletes(the last two count what is queued).Testing. Six new daemon tests (
redis-require-master,redis-sentinel,redis-sentinel-start,redis-sentinel-subscribe,redis-repair,redis-resync), with fake Redis and fake Sentinel servers inFakeRedis.pm; each fails on its parent commit; about 45 s in all. The existing Redis tests pass unchanged.make asan-checkon master3865f896with the TLS follow-up: exit 0 on Debian trixie and Ubuntu noble, 9503ok, 0not ok. The patches apply withgit amon3865f896.Questions:
sentinelkeyword in the existing endpoint options right, or would you prefer per-role flags or one switch?Lab method, failure modes and locking notes: https://github.com/kbinetruy/rtpengine/blob/redis-notes/redis-sentinel-notes.md