Repository navigation
Conversation
--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 alone requires the master at connect, but keeps the connection when its writes are refused. The write connection now requires the master role, as --redis alone does, and a connection that requires it is dropped on a write refused with READONLY or MASTERDOWN, so that the next operation reconnects and checks the role. Other refusals keep the connection. The read database of --redis, when --redis-write is given, still accepts any role. Tested by auto-daemon-tests-redis-master-only.pl; the READONLY cases of auto-daemon-tests-redis-errors.pl, which expected the connection to be kept, now use NOREPLICAS.
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 in a table keyed by call ID: the call itself, held, while it is in the call table; its ID and database once it is deleted, as a deleted call keeps its memory until its last reference goes. A stored write clears the mark. A marked SET supersedes a marked DEL of the same ID; if its call was never written, it takes that key over, so that its own deletion deletes it. 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, the key of a deleted call 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 and auto-daemon-tests-redis-repair-ids.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 calls through the call iterator, one at a time. No key is scanned. New statistics: redisresyncs, redisresynccalls and redisresyncdeletes. Tested by auto-daemon-tests-redis-resync.pl.
82d140f to
f8d776b
Compare
| 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); |
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.
There was a problem hiding this comment.
Addressed together with the other comments on this table; see the reply under the call_t comment.
| 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 */ |
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?
There was a problem hiding this comment.
Addressed together with the other comments on this table; see the reply under the call_t comment.
| } | ||
| } | ||
| g_hash_table_replace(redis_repair_pending, g_strndup(callid->s, callid->len), | ||
| GINT_TO_POINTER(db + 1)); |
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?
There was a problem hiding this comment.
Addressed together with the other comments on this table; see the reply under the call_t comment.
| if (g_hash_table_iter_next(&it, &k, &v)) { | ||
| g_hash_table_iter_steal(&it); | ||
| id = k; | ||
| db = GPOINTER_TO_INT(v) - 1; | ||
| } |
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.
There was a problem hiding this comment.
Thanks, all four are right. Reworked: one str-keyed table whose entries hold the call itself while it is in the call table, and only its ID and DB once it is gone. A destroyed call_t keeps its memory until its last reference: about 54 KB for a plain call, about 20 KB more per re-INVITE, so about 5 GB at the 100000 cap. Its deletion converts or removes the entry. No lookup and no copies for live calls; repair_count and the +1 are gone, and the table is created once. A queued SET of an unwritten call that supersedes a queued DEL of its ID takes that key over, as in #2211. I kept a table rather than a queue because the cap relies on collapsing duplicates by ID.
693942b to
ec0db66
Compare
rfuchs
left a comment
rfuchs
left a comment
There was a problem hiding this comment.
Overall seems reasonable. A few more comments. The IPC logic between the threads seems a bit shaky, but OK.
Writing to a master only is now the default (--redis-require-master is gone); the resync walks the calls with the call iterator; lock order, test names and counts, and make asan-check on master f950a9f as in the reworked series. sipwise#2211 is merged.
ec0db66 to
e8d29ff
Compare
kbinetruy
changed the title
rfuchs
left a comment
rfuchs
left a comment
There was a problem hiding this comment.
One more hint for your LLM. If no interest then I will try to merge as it is.
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. The first commit makes the write connection require a master, by default; the other four, opt-in, follow the master through Sentinel and write back what the failover lost.MT#55283 redis: write call data to a master onlyMT#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-change23 files, +3185 −85; 1840 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. Master only.
--redis-writenow requires the master role at connect, as--redisalone already did, and a connection that requires it is dropped on a write refused withREADONLYorMASTERDOWN. The reconnect then checks the role, paced by--redis-reconnect-pause. Other refusals, such asNOREPLICAS, keep the connection. The read database of--redisstill accepts any role when--redis-writeis given.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 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 live call by reference, a deleted call's key by ID and database. 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 (an unwritten one then takes the key over). After taking the lock it lets waiting signalling writes go first (at most 100 ms), and waits 1 s after a failed write. The lock order is in a comment at the top of the repair code.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 (walked with the call iterator) and the recent ownDELs are queued for the repair. No key is scanned.--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.Testing. Seven new daemon tests (
redis-master-only,redis-sentinel,redis-sentinel-start,redis-sentinel-subscribe,redis-repair,redis-repair-ids,redis-resync), with fake Redis and fake Sentinel servers; each fails on its parent commit; about 56 s in all. Inredis-errors, the refusals that keep the connection are nowNOREPLICASandLOADING(aREADONLYdrops it).make asan-checkon masterf950a9f7with the TLS follow-up: exit 0 on Debian trixie and Ubuntu noble, 10235–10237ok, 0not ok.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