From d2eb07d74152b405cdcb115e85ba76bb1a1a0488 Mon Sep 17 00:00:00 2001 From: Kevin Binetruy Date: Wed, 14 Oct 2026 10:52:21 +0000 Subject: [PATCH 1/7] MT#55283 redis: add --redis-require-master --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. --- daemon/main.c | 15 +- daemon/redis.c | 27 +++- docs/rtpengine.md | 11 ++ etc/rtpengine.conf | 1 + include/main.h | 1 + include/redis.h | 4 +- t/Makefile | 5 + t/auto-daemon-tests-redis-require-master.pl | 145 ++++++++++++++++++++ 8 files changed, 199 insertions(+), 10 deletions(-) create mode 100755 t/auto-daemon-tests-redis-require-master.pl diff --git a/daemon/main.c b/daemon/main.c index cb1a0a220..cefe9defc 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -812,6 +812,7 @@ static void options(int *argc, char ***argv, charp_ht templates) { { "redis-write",'w', 0, G_OPTION_ARG_STRING, &redisps_write, "Connect to Redis write database", "[PW@]IP:PORT/INT" }, { "redis-subscribe", 0, 0, G_OPTION_ARG_STRING, &redisps_subscribe, "Connect to Redis subscribe database", "[PW@]IP:PORT[/INT]" }, { "redis-resolve-on-reconnect", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_resolve_on_reconnect, "Re-resolve given FQDN on each re-connect to the redis server.", NULL }, + { "redis-require-master", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_require_master, "Write only to a Redis master, and drop the connection when the server stops being one", NULL }, { "redis-num-threads", 0, 0, G_OPTION_ARG_INT, &rtpe_config.redis_num_threads, "Number of Redis restore threads", "INT" }, { "redis-expires", 0, 0, G_OPTION_ARG_INT, &rtpe_config.redis_expires_secs, "Expire time in seconds for redis keys", "INT" }, { "no-redis-required", 'q', 0, G_OPTION_ARG_NONE, &rtpe_config.no_redis_required, "Start no matter of redis connection state", NULL }, @@ -1915,9 +1916,10 @@ static void create_everything(void) { rtpe_config.redis_write_db, rtpe_config.redis_write_hostname, rtpe_config.redis_write_auth, - ANY_REDIS_ROLE, + (rtpe_config.redis_require_master ? MASTER_REDIS_ROLE : ANY_REDIS_ROLE), rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect); + rtpe_config.redis_resolve_on_reconnect, + rtpe_config.redis_require_master); if (!rtpe_redis_write) die("Cannot start up without running Redis %s write database! See also NO_REDIS_REQUIRED parameter.", @@ -1931,7 +1933,8 @@ static void create_everything(void) { rtpe_config.redis_subscribe_auth, ANY_REDIS_ROLE, rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect); + rtpe_config.redis_resolve_on_reconnect, + false); if (!rtpe_redis_notify) die("Cannot start up without running Redis %s subscribe database! See also NO_REDIS_REQUIRED parameter.", @@ -1949,7 +1952,8 @@ static void create_everything(void) { rtpe_config.redis_auth, (rtpe_redis_write ? ANY_REDIS_ROLE : MASTER_REDIS_ROLE), rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect); + rtpe_config.redis_resolve_on_reconnect, + !rtpe_redis_write && rtpe_config.redis_require_master); if (!rtpe_redis) die("Cannot start up without running Redis %s database! " @@ -1963,7 +1967,8 @@ static void create_everything(void) { rtpe_config.redis_auth, (rtpe_redis_write ? ANY_REDIS_ROLE : MASTER_REDIS_ROLE), rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect); + rtpe_config.redis_resolve_on_reconnect, + false); if (!rtpe_redis_notify) die("Cannot start up without running notification Redis %s database! " diff --git a/daemon/redis.c b/daemon/redis.c index 7ea5ef3b2..ad2deac38 100644 --- a/daemon/redis.c +++ b/daemon/redis.c @@ -247,11 +247,21 @@ static void redis_write_result(struct redis *r, const redisReply *rp) { r->write_errors = 0; } +/* an error reply that says the server is not, or no longer, a usable master */ +static bool redis_is_role_error(const redisReply *rp) { + if (rp->type != REDIS_REPLY_ERROR) + return false; + return !strncmp(rp->str, "READONLY", 8) || !strncmp(rp->str, "MASTERDOWN", 10); +} + /* called with r->lock held - * Returns -1 if the replies could not be read (the context is then unusable). - * An error reply keeps the connection: a reconnect would get the same answer. */ + * Returns -1 if the replies could not be read (the context is then unusable), + * or after a role error on a connection that requires a master, so that the + * caller drops it. Other error replies keep the connection: a reconnect would + * get the same answer. */ static int redis_consume(struct redis *r) { redisReply *rp; + bool role_error = false; if (!r->ctx) { ilog(LOG_ERROR, "Unable to consume pipelined replies. No redis context"); @@ -265,8 +275,15 @@ static int redis_consume(struct redis *r) { } r->pipeline--; redis_write_result(r, rp); + if (redis_is_role_error(rp)) + role_error = true; freeReplyObject(rp); } + if (role_error && r->require_master) { + rlog(LOG_WARN, "Redis %s is not a master: dropping the connection", + endpoint_print_buf(&r->endpoint)); + return -1; + } return 0; } @@ -975,7 +992,7 @@ void redis_notify_loop(void *d) { } struct redis *redis_new(const endpoint_t *ep, int db, const char *hostname, const char *auth, - enum redis_role role, int no_redis_required, bool update_resolve) { + enum redis_role role, int no_redis_required, bool update_resolve, bool require_master) { struct redis *r; r = g_new0(struct redis, 1); @@ -990,6 +1007,7 @@ struct redis *redis_new(const endpoint_t *ep, int db, const char *hostname, cons r->restore_tick_us = 0; r->consecutive_errors = 0; r->update_resolve = update_resolve; + r->require_master = require_master; mutex_init(&r->lock); cond_init(&r->reconnect_cond); @@ -1021,7 +1039,8 @@ struct redis *redis_dup(const struct redis *r, int db) { r->auth, r->role, r->no_redis_required, - r->update_resolve); + r->update_resolve, + r->require_master); } void redis_close(struct redis *r) { diff --git a/docs/rtpengine.md b/docs/rtpengine.md index 6662f2b6c..a6737b79a 100644 --- a/docs/rtpengine.md +++ b/docs/rtpengine.md @@ -580,6 +580,17 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- The connection is kept, as reconnecting to the same address would not change the server's answer. +- __\-\-redis-require-master__ + + Requires the master role on the connections that write call data + (__\-\-redis-write__, which otherwise accepts any role, and __\-\-redis__ + when it is also used for writing), and drops such a connection when a + write is refused with __READONLY__ or __MASTERDOWN__. The next operation + reconnects and checks the role; a replica is then a failed attempt (see + __\-\-redis-reconnect-pause__). Other refusals, such as __NOREPLICAS__ + or __LOADING__, keep the connection. It is a boolean value (either + __true__ or __false__), and it is off by default. + - __\-\-redis-subscribe=__\[*PW*@\]*IP*:*PORT*\[/*INT*\] Configures a Redis database for subscribing and receiving notifications. diff --git a/etc/rtpengine.conf b/etc/rtpengine.conf index d5dc2e429..0af57ae8d 100644 --- a/etc/rtpengine.conf +++ b/etc/rtpengine.conf @@ -84,6 +84,7 @@ recording-method = proc # redis-reconnect-pause = 1000 # redis-reconnect-slow = 100 # redis-resolve-on-reconnect = false +# redis-require-master = false # redis-tcp-keepalive-time = 1 # redis-tcp-keepalive-intvl = 1 # redis-tcp-keepalive-probes = 3 diff --git a/include/main.h b/include/main.h index 861e09fa9..c14477bdd 100644 --- a/include/main.h +++ b/include/main.h @@ -139,6 +139,7 @@ enum endpoint_learning { X(player_cache) \ X(poller_per_thread) \ X(redis_resolve_on_reconnect) \ + X(redis_require_master) \ X(measure_rtp) \ X(amr_cn_dtx) \ X(evs_cn_dtx) \ diff --git a/include/redis.h b/include/redis.h index 1c2a2b4e4..6e2b86d0a 100644 --- a/include/redis.h +++ b/include/redis.h @@ -76,6 +76,7 @@ struct redis { int64_t async_last; bool update_resolve; + bool require_master; /* a role error drops the connection */ }; struct redis_hash { @@ -113,7 +114,8 @@ void redis_notify_loop(void *d); void redis_delete_async_loop(void *d); -struct redis *redis_new(const endpoint_t *, int, const char *, const char *, enum redis_role, int, bool); +struct redis *redis_new(const endpoint_t *, int, const char *, const char *, enum redis_role, int, bool, + bool); struct redis *redis_dup(const struct redis *r, int db); void redis_close(struct redis *r); int redis_restore(struct redis *, bool foreign, int db); diff --git a/t/Makefile b/t/Makefile index 4e8c99e45..804122540 100644 --- a/t/Makefile +++ b/t/Makefile @@ -90,6 +90,7 @@ include ../lib/common.Makefile daemon-tests-bundle daemon-tests-dtls daemon-tests-rollback \ daemon-tests-rollback-redis daemon-tests-redis-srtp-index daemon-tests-redis-errors \ daemon-tests-redis-destroyed daemon-tests-redis-reconnect \ + daemon-tests-redis-require-master \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions daemon-tests-alias \ daemon-tests-kernel-api daemon-tests-dtmf-fanout \ daemon-tests-video daemon-tests-inject daemon-tests-dtx-dtmf @@ -144,6 +145,7 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests daemon-tests-bundle daemon-tests-dtls daemon-tests-rollback daemon-tests-rollback-redis \ daemon-tests-redis-srtp-index daemon-tests-redis-errors daemon-tests-redis-destroyed \ daemon-tests-redis-reconnect \ + daemon-tests-redis-require-master \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions \ daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-dtx-no-shift daemon-tests-dtx-dtmf \ daemon-tests-alias \ @@ -252,6 +254,9 @@ daemon-tests-redis-destroyed: daemon-test-deps daemon-tests-redis-reconnect: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-reconnect.pl +daemon-tests-redis-require-master: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-require-master.pl + daemon-tests-audio-player: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-audio-player.pl diff --git a/t/auto-daemon-tests-redis-require-master.pl b/t/auto-daemon-tests-redis-require-master.pl new file mode 100755 index 000000000..ab994c4c7 --- /dev/null +++ b/t/auto-daemon-tests-redis-require-master.pl @@ -0,0 +1,145 @@ +#!/usr/bin/perl + +# --redis-require-master: the write connection (--redis-write) requires the +# master role, and a role error (READONLY, MASTERDOWN) on it drops the +# connection, so that the reconnect checks the role again. The fake Redis +# plays a master that is demoted to a replica, then promoted again. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; + + +my $listener = redis_listen('203.0.113.42'); +my ($write_fd, $read_fd); + +my $sdp = < 'slave', refused => 1); + redis_closed($write_fd, 'replica: connection closed after one INFO'); +}; +offer('write while the node is a replica', { }, $sdp); + +new_call; +$NGCP::Rtpengine::req_cb = sub { + ok(!readable($listener, 0.3), 'no reconnect within the pacing interval') + or die "reconnect attempted within the pacing interval\n"; +}; +offer('write within the pacing interval', { }, $sdp); + +sleep(1); + +new_call; +$NGCP::Rtpengine::req_cb = sub { + $write_fd = redis_conn($listener, 'master again'); + redis_expect_set($write_fd, cid(), "+OK\r\n", 'master again', no_ping => 1); +}; +offer('write after the pacing interval', { }, $sdp); + + +# MASTERDOWN drops the connection too; here the reconnect finds a master + +new_call; +$NGCP::Rtpengine::req_cb = sub { + redis_expect_set($write_fd, cid(), + "-MASTERDOWN Link with MASTER is down and replica-serve-stale-data is set to 'no'.\r\n", + 'MASTERDOWN'); + redis_closed($write_fd, 'MASTERDOWN: connection dropped'); +}; +offer('MASTERDOWN write', { }, $sdp); +is(redis_write_errors(), 3, 'MASTERDOWN counted'); + +new_call; +$NGCP::Rtpengine::req_cb = sub { + $write_fd = redis_conn($listener, 'after MASTERDOWN'); + redis_expect_set($write_fd, cid(), "+OK\r\n", 'after MASTERDOWN', no_ping => 1); +}; +offer('write after MASTERDOWN', { }, $sdp); + + +# the read connection was never touched +ok(!readable($read_fd, 0), 'nothing on the read connection'); + + +undef($NGCP::Rtpengine::req_cb); + +done_testing(); From ec4f55804fd697db842fa4332fdd4e2453a31a06 Mon Sep 17 00:00:00 2001 From: Kevin Binetruy Date: Wed, 14 Oct 2026 10:52:21 +0000 Subject: [PATCH 2/7] MT#55283 redis: find the master through Redis Sentinel 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. --- daemon/Makefile | 2 +- daemon/main.c | 121 ++++-- daemon/redis.c | 83 +++- daemon/redis_sentinel.c | 403 ++++++++++++++++++++ docs/rtpengine.md | 49 ++- etc/rtpengine.conf | 4 + include/main.h | 4 + include/redis.h | 10 +- include/redis_sentinel.h | 32 ++ perl/NGCP/Rtpengine/FakeRedis.pm | 237 +++++++++++- t/Makefile | 10 + t/auto-daemon-tests-redis-require-master.pl | 4 +- t/auto-daemon-tests-redis-sentinel-start.pl | 197 ++++++++++ t/auto-daemon-tests-redis-sentinel.pl | 213 +++++++++++ 14 files changed, 1328 insertions(+), 41 deletions(-) create mode 100644 daemon/redis_sentinel.c create mode 100644 include/redis_sentinel.h create mode 100755 t/auto-daemon-tests-redis-sentinel-start.pl create mode 100755 t/auto-daemon-tests-redis-sentinel.pl diff --git a/daemon/Makefile b/daemon/Makefile index 5b532bc7a..08ad1e6c5 100644 --- a/daemon/Makefile +++ b/daemon/Makefile @@ -82,7 +82,7 @@ endif CFLAGS += $(CFLAGS_MQTT) LDLIBS += $(LDLIBS_MQTT) -SRCS := main.c kernel.c helpers.c control_tcp.c call.c control_udp.c redis.c \ +SRCS := main.c kernel.c helpers.c control_tcp.c call.c control_udp.c redis.c redis_sentinel.c \ cookie_cache.c udp_listener.c control_ng_flags_parser.c control_ng.c sdp.strhash.c stun.c rtcp.c \ crypto.c rtp.c call_interfaces.c dtls.c log.c cli.strhash.c graphite.c ice.c \ media_socket.c homer.c recording.c statistics.c cdr.c ssrc.c iptables.c tcp_listener.c \ diff --git a/daemon/main.c b/daemon/main.c index cefe9defc..f0c671f24 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -33,6 +33,7 @@ #include "call.h" #include "kernel.h" #include "redis.h" +#include "redis_sentinel.h" #include "sdp.h" #include "dtls.h" #include "call_interfaces.h" @@ -570,14 +571,19 @@ static bool if_addr_parse(intf_config_q *q, char *s, struct ifaddrs *ifas) { /** * Sets hostname and auth. + * With the keyword "sentinel" instead of IP:PORT, sets *sentinel and leaves + * the endpoint and hostname unset. */ -static int redis_ep_parse(endpoint_t *ep, int *db, char **hostname, char **auth, const char *auth_env, char *s) { +static int redis_ep_parse(endpoint_t *ep, int *db, char **hostname, char **auth, const char *auth_env, + gboolean *sentinel, char *s) +{ char *sl, *sp; long l; /* ensure to have pointers nulled */ *auth = NULL; *hostname = NULL; + *sentinel = false; sl = strrchr(s, '@'); if (sl) { @@ -602,6 +608,11 @@ static int redis_ep_parse(endpoint_t *ep, int *db, char **hostname, char **auth, *db = l; } + if (!strcmp(s, "sentinel")) { + *sentinel = true; + return 0; + } + /* copy for the case with re-resolve during re-connections */ sp = strrchr(s, ':'); /* make sure to not take port into the value of hostname */ if (sp) @@ -719,6 +730,9 @@ static void options(int *argc, char ***argv, charp_ht templates) { g_autoptr(char) redisps = NULL; g_autoptr(char) redisps_write = NULL; g_autoptr(char) redisps_subscribe = NULL; + g_autoptr(char) redis_sentinels = NULL; + g_autoptr(char) redis_sentinel_master = NULL; + int redis_sentinel_interval = -1; g_autoptr(char) log_facility_cdr_s = NULL; g_autoptr(char) log_facility_rtcp_s = NULL; g_autoptr(char) log_facility_dtmf_s = NULL; @@ -813,6 +827,9 @@ static void options(int *argc, char ***argv, charp_ht templates) { { "redis-subscribe", 0, 0, G_OPTION_ARG_STRING, &redisps_subscribe, "Connect to Redis subscribe database", "[PW@]IP:PORT[/INT]" }, { "redis-resolve-on-reconnect", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_resolve_on_reconnect, "Re-resolve given FQDN on each re-connect to the redis server.", NULL }, { "redis-require-master", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_require_master, "Write only to a Redis master, and drop the connection when the server stops being one", NULL }, + { "redis-sentinel", 0, 0, G_OPTION_ARG_STRING, &redis_sentinels, "Redis Sentinels to ask for the master, for the roles given as 'sentinel'", "[PW@]IP:PORT[,IP:PORT...]" }, + { "redis-sentinel-master", 0, 0, G_OPTION_ARG_STRING, &redis_sentinel_master, "Name of the master monitored by Redis Sentinel", "STRING" }, + { "redis-sentinel-interval", 0, 0, G_OPTION_ARG_INT, &redis_sentinel_interval, "How often to ask Redis Sentinel for the master, in milliseconds", "INT" }, { "redis-num-threads", 0, 0, G_OPTION_ARG_INT, &rtpe_config.redis_num_threads, "Number of Redis restore threads", "INT" }, { "redis-expires", 0, 0, G_OPTION_ARG_INT, &rtpe_config.redis_expires_secs, "Expire time in seconds for redis keys", "INT" }, { "no-redis-required", 'q', 0, G_OPTION_ARG_NONE, &rtpe_config.no_redis_required, "Start no matter of redis connection state", NULL }, @@ -1295,7 +1312,7 @@ static void options(int *argc, char ***argv, charp_ht templates) { if (redisps) { if (redis_ep_parse(&rtpe_config.redis_ep, &rtpe_config.redis_db, &rtpe_config.redis_hostname, - &rtpe_config.redis_auth, "RTPENGINE_REDIS_AUTH_PW", redisps)) + &rtpe_config.redis_auth, "RTPENGINE_REDIS_AUTH_PW", &rtpe_config.redis_via_sentinel, redisps)) { die("Invalid Redis endpoint [IP:PORT/INT] '%s' (--redis)", redisps); } @@ -1303,7 +1320,8 @@ static void options(int *argc, char ***argv, charp_ht templates) { if (redisps_write) { if (redis_ep_parse(&rtpe_config.redis_write_ep, &rtpe_config.redis_write_db, &rtpe_config.redis_write_hostname, - &rtpe_config.redis_write_auth, "RTPENGINE_REDIS_WRITE_AUTH_PW", redisps_write)) + &rtpe_config.redis_write_auth, "RTPENGINE_REDIS_WRITE_AUTH_PW", &rtpe_config.redis_write_via_sentinel, + redisps_write)) { die("Invalid Redis endpoint [IP:PORT/INT] '%s' (--redis-write)", redisps_write); } @@ -1311,14 +1329,38 @@ static void options(int *argc, char ***argv, charp_ht templates) { if (redisps_subscribe) if (redis_ep_parse(&rtpe_config.redis_subscribe_ep, &rtpe_config.redis_subscribe_db, &rtpe_config.redis_subscribe_hostname, - &rtpe_config.redis_subscribe_auth,"RTPENGINE_REDIS_SUBSCRIBE_AUTH_PW", redisps_subscribe)) + &rtpe_config.redis_subscribe_auth,"RTPENGINE_REDIS_SUBSCRIBE_AUTH_PW", &rtpe_config.redis_subscribe_via_sentinel, + redisps_subscribe)) { rtpe_config.redis_subscribe_db = -1; if (redis_ep_parse(&rtpe_config.redis_subscribe_ep, NULL, &rtpe_config.redis_subscribe_hostname, - &rtpe_config.redis_subscribe_auth,"RTPENGINE_REDIS_SUBSCRIBE_AUTH_PW", redisps_subscribe)) + &rtpe_config.redis_subscribe_auth,"RTPENGINE_REDIS_SUBSCRIBE_AUTH_PW", &rtpe_config.redis_subscribe_via_sentinel, + redisps_subscribe)) die("Invalid Redis endpoint [IP:PORT/INT] '%s' (--redis-subscribe)", redisps_subscribe); } + if (rtpe_config.redis_via_sentinel || rtpe_config.redis_write_via_sentinel + || rtpe_config.redis_subscribe_via_sentinel) + { + if (!redis_sentinels) + die("A Redis endpoint is 'sentinel', but --redis-sentinel is not set"); + if (!redis_sentinel_master) + die("--redis-sentinel-master must be set with --redis-sentinel"); + if (redis_sentinel_interval == -1) + redis_sentinel_interval = 1000; + if (redis_sentinel_interval <= 0) + die("Invalid --redis-sentinel-interval (must be a positive number of milliseconds)"); + rtpe_config.redis_sentinel_interval = redis_sentinel_interval; + rtpe_redis_sentinel = redis_sentinel_new(redis_sentinels, redis_sentinel_master, + "RTPENGINE_REDIS_SENTINEL_AUTH_PW"); + if (!rtpe_redis_sentinel) + die("Invalid Redis Sentinel list [PW@]IP:PORT[,IP:PORT...] '%s' (--redis-sentinel)", + redis_sentinels); + } + else if (redis_sentinels || redis_sentinel_master || redis_sentinel_interval != -1) + die("--redis-sentinel, --redis-sentinel-master or --redis-sentinel-interval is set, " + "but no Redis endpoint is 'sentinel'"); + if (rtpe_config.fmt < 0 || rtpe_config.fmt > 2) die("Invalid XMLRPC format"); @@ -1911,30 +1953,54 @@ static void create_everything(void) { create_listeners(&rtpe_config.ng_tcp_listen_ep, &rtpe_control_ng_tcp, (void *(*)(const endpoint_t *)) control_ng_tcp_new, false, "TCP NG control"); create_listeners(&rtpe_config.cli_listen_ep, &rtpe_cli, (void *(*)(const endpoint_t *)) cli_new, false, "CLI"); - if (!is_addr_unspecified(&rtpe_config.redis_write_ep.address)) { + // the roles that use Sentinel require a master, and never re-resolve + bool sent_r = rtpe_config.redis_via_sentinel, sent_w = rtpe_config.redis_write_via_sentinel, + sent_s = rtpe_config.redis_subscribe_via_sentinel; + + if (rtpe_redis_sentinel) { + // the master's password, for the role check on the Sentinels' answer + redis_sentinel_set_probe_auth(rtpe_redis_sentinel, + sent_w ? rtpe_config.redis_write_auth + : sent_r ? rtpe_config.redis_auth + : rtpe_config.redis_subscribe_auth); + ilog(LOG_INFO, "Using Redis Sentinel for the master of '%s'", + redis_sentinel_master_name(rtpe_redis_sentinel)); + if (!rtpe_config.redis_cmd_timeout) + ilog(LOG_WARN, "Redis Sentinel is used without --redis-cmd-timeout: a master that " + "stops answering holds Redis operations until it answers again"); + if (!redis_sentinel_discover(rtpe_redis_sentinel) && !rtpe_config.no_redis_required) + die("Cannot start up without a master of '%s' from Redis Sentinel! " + "See also NO_REDIS_REQUIRED parameter.", + redis_sentinel_master_name(rtpe_redis_sentinel)); + } + + if (!is_addr_unspecified(&rtpe_config.redis_write_ep.address) || sent_w) { rtpe_redis_write = redis_new(&rtpe_config.redis_write_ep, rtpe_config.redis_write_db, rtpe_config.redis_write_hostname, rtpe_config.redis_write_auth, - (rtpe_config.redis_require_master ? MASTER_REDIS_ROLE : ANY_REDIS_ROLE), + ((rtpe_config.redis_require_master || sent_w) + ? MASTER_REDIS_ROLE : ANY_REDIS_ROLE), rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect, - rtpe_config.redis_require_master); + rtpe_config.redis_resolve_on_reconnect && !sent_w, + rtpe_config.redis_require_master || sent_w, + sent_w ? rtpe_redis_sentinel : NULL); if (!rtpe_redis_write) die("Cannot start up without running Redis %s write database! See also NO_REDIS_REQUIRED parameter.", endpoint_print_buf(&rtpe_config.redis_write_ep)); } - if (!is_addr_unspecified(&rtpe_config.redis_subscribe_ep.address)) { + if (!is_addr_unspecified(&rtpe_config.redis_subscribe_ep.address) || sent_s) { rtpe_redis_notify = redis_new(&rtpe_config.redis_subscribe_ep, rtpe_config.redis_subscribe_db, rtpe_config.redis_subscribe_hostname, rtpe_config.redis_subscribe_auth, - ANY_REDIS_ROLE, + (sent_s ? MASTER_REDIS_ROLE : ANY_REDIS_ROLE), rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect, - false); + rtpe_config.redis_resolve_on_reconnect && !sent_s, + sent_s, + sent_s ? rtpe_redis_sentinel : NULL); if (!rtpe_redis_notify) die("Cannot start up without running Redis %s subscribe database! See also NO_REDIS_REQUIRED parameter.", @@ -1945,15 +2011,16 @@ static void create_everything(void) { } } - if (!is_addr_unspecified(&rtpe_config.redis_ep.address)) { + if (!is_addr_unspecified(&rtpe_config.redis_ep.address) || sent_r) { rtpe_redis = redis_new(&rtpe_config.redis_ep, rtpe_config.redis_db, rtpe_config.redis_hostname, rtpe_config.redis_auth, - (rtpe_redis_write ? ANY_REDIS_ROLE : MASTER_REDIS_ROLE), + ((rtpe_redis_write && !sent_r) ? ANY_REDIS_ROLE : MASTER_REDIS_ROLE), rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect, - !rtpe_redis_write && rtpe_config.redis_require_master); + rtpe_config.redis_resolve_on_reconnect && !sent_r, + (!rtpe_redis_write && rtpe_config.redis_require_master) || sent_r, + sent_r ? rtpe_redis_sentinel : NULL); if (!rtpe_redis) die("Cannot start up without running Redis %s database! " @@ -1965,10 +2032,11 @@ static void create_everything(void) { rtpe_config.redis_db, rtpe_config.redis_hostname, rtpe_config.redis_auth, - (rtpe_redis_write ? ANY_REDIS_ROLE : MASTER_REDIS_ROLE), + ((rtpe_redis_write && !sent_r) ? ANY_REDIS_ROLE : MASTER_REDIS_ROLE), rtpe_config.no_redis_required, - rtpe_config.redis_resolve_on_reconnect, - false); + rtpe_config.redis_resolve_on_reconnect && !sent_r, + sent_r, + sent_r ? rtpe_redis_sentinel : NULL); if (!rtpe_redis_notify) die("Cannot start up without running notification Redis %s database! " @@ -2135,12 +2203,15 @@ int main(int argc, char **argv) { thread_create_looper(media_player_expire, rtpe_config.idle_scheduling, rtpe_config.idle_priority, "cache expiry", rtpe_config.expiry_timer * 1000000LL); - if (!is_addr_unspecified(&rtpe_config.redis_ep.address) && initial_rtpe_config.redis_delete_async) + if (rtpe_redis && initial_rtpe_config.redis_delete_async) thread_create_detach(redis_delete_async_loop, NULL, "redis async"); if (rtpe_redis_notify) thread_create_detach(redis_notify_loop, NULL, "redis notify"); + if (rtpe_redis_sentinel) + thread_create_detach(redis_sentinel_loop, rtpe_redis_sentinel, "redis sentinel"); + do_redis_restore(); if (graphite_is_enabled()) @@ -2198,7 +2269,7 @@ int main(int argc, char **argv) { websocket_stop(); - if (!is_addr_unspecified(&rtpe_config.redis_ep.address) && initial_rtpe_config.redis_delete_async) + if (rtpe_redis && initial_rtpe_config.redis_delete_async) redis_async_event_base_action(rtpe_redis_write, EVENT_BASE_LOOPBREAK); if (rtpe_redis_notify) @@ -2206,10 +2277,10 @@ int main(int argc, char **argv) { threads_join_all(true); - if (!is_addr_unspecified(&rtpe_config.redis_ep.address) && initial_rtpe_config.redis_delete_async) + if (rtpe_redis && initial_rtpe_config.redis_delete_async) redis_async_event_base_action(rtpe_redis_write, EVENT_BASE_FREE); - if (!is_addr_unspecified(&rtpe_config.redis_ep.address) && rtpe_redis_notify) + if (rtpe_redis && rtpe_redis_notify) redis_async_event_base_action(rtpe_redis_notify, EVENT_BASE_FREE); ilog(LOG_INFO, "Version %s shutting down", RTPENGINE_VERSION); @@ -2235,6 +2306,8 @@ int main(int argc, char **argv) { if (rtpe_redis_write != rtpe_redis) redis_close(rtpe_redis_write); redis_close(rtpe_redis_notify); + redis_sentinel_free(rtpe_redis_sentinel); + rtpe_redis_sentinel = NULL; free_prefix(); log_free(); diff --git a/daemon/redis.c b/daemon/redis.c index ad2deac38..043f0122b 100644 --- a/daemon/redis.c +++ b/daemon/redis.c @@ -34,6 +34,7 @@ #include "codec.h" #include "sdp.h" #include "statistics.h" +#include "redis_sentinel.h" #define REDIS_CONNECT_ROLE -2 /* redis_connect(): the server has the wrong role */ @@ -97,8 +98,6 @@ static int redisCommandNR(redisContext *r, const char *fmt, ...) #define REDIS_FMT(x) (int) (x)->len, (x)->str -#define rlog(l, x...) ilog(l | LOG_FLAG_RESTORE, x) - // To protect against a restore race condition: Keyspace notifications are set up @@ -277,8 +276,12 @@ static int redis_consume(struct redis *r) { redis_write_result(r, rp); if (redis_is_role_error(rp)) role_error = true; + else if (r->sentinel && rp->type == REDIS_REPLY_ERROR && !strncmp(rp->str, "NOREPLICAS", 10)) + redis_sentinel_wake(r->sentinel, true); // the master may have been replaced freeReplyObject(rp); } + if (role_error && r->sentinel) + redis_sentinel_wake(r->sentinel, false); if (role_error && r->require_master) { rlog(LOG_WARN, "Redis %s is not a master: dropping the connection", endpoint_print_buf(&r->endpoint)); @@ -308,6 +311,36 @@ int redis_set_cmd_timeout(struct redis *r, int64_t timeout) { return redis_set_timeout(r, timeout); } +/* called with r->lock held + * With Sentinel, takes the master that the Sentinel thread published, if it + * is a new one, and drops the connection to the old one: true if so. Not + * while a reconnect is under way: the next check follows. */ +static bool redis_sentinel_follow(struct redis *r) { + if (!r->sentinel || r->reconnecting || redis_sentinel_gen(r->sentinel) == r->gen) + return false; + + endpoint_t ep; + uint64_t gen = redis_sentinel_get(r->sentinel, &ep); + bool moved = !r->gen || !endpoint_eq(&ep, &r->endpoint); + r->gen = gen; + if (!moved) + return false; + + rlog(LOG_NOTICE, "Redis: following the master of '%s' from %s to %s", + redis_sentinel_master_name(r->sentinel), + r->ctx ? endpoint_print_buf(&r->endpoint) : "none", endpoint_print_buf(&ep)); + r->endpoint = ep; + sockaddr_print(&ep.address, r->host, sizeof(r->host)); + // try the new master at once, whatever happened with the old one + r->restore_tick_us = 0; + r->consecutive_errors = 0; + if (r->ctx) + redisFree(r->ctx); + r->ctx = NULL; + r->state = REDIS_STATE_DISCONNECTED; + return true; +} + int redis_reconnect(struct redis* r) { int rval; LOCK(&r->lock); @@ -317,6 +350,10 @@ int redis_reconnect(struct redis* r) { rlog(LOG_WARN, "Redis %s: a reconnect is already in progress", endpoint_print_buf(&r->endpoint)); return -1; } + redis_sentinel_follow(r); + if (r->sentinel && !r->gen) + return -1; + rval = redis_connect(r, 0, r->update_resolve); if (rval) r->state = REDIS_STATE_DISCONNECTED; @@ -680,6 +717,7 @@ int redis_async_event_base_action(struct redis *r, enum event_base_action action rlog(LOG_ERR, "redis_async_event_base_action: NULL r"); return -1; } else { + LOCK(&r->lock); // (Sentinel moves the endpoint) rlog(LOG_DEBUG, "redis_async_event_base_action: Use Redis %s", endpoint_print_buf(&r->endpoint)); } @@ -992,7 +1030,8 @@ void redis_notify_loop(void *d) { } struct redis *redis_new(const endpoint_t *ep, int db, const char *hostname, const char *auth, - enum redis_role role, int no_redis_required, bool update_resolve, bool require_master) { + enum redis_role role, int no_redis_required, bool update_resolve, bool require_master, + struct redis_sentinel *sentinel) { struct redis *r; r = g_new0(struct redis, 1); @@ -1008,9 +1047,23 @@ struct redis *redis_new(const endpoint_t *ep, int db, const char *hostname, cons r->consecutive_errors = 0; r->update_resolve = update_resolve; r->require_master = require_master; + r->sentinel = sentinel; mutex_init(&r->lock); cond_init(&r->reconnect_cond); + if (sentinel) { + r->gen = redis_sentinel_get(sentinel, &r->endpoint); + sockaddr_print(&r->endpoint.address, r->host, sizeof(r->host)); + if (!r->gen) { + if (r->no_redis_required) { + rlog(LOG_WARN, "Starting with no master of '%s' known from Redis Sentinel !", + redis_sentinel_master_name(sentinel)); + return r; + } + goto err; + } + } + if (redis_connect(r, 10, false)) { if (r->no_redis_required) { rlog(LOG_WARN, "Starting with no initial connection to Redis %s !", @@ -1040,7 +1093,8 @@ struct redis *redis_dup(const struct redis *r, int db) { r->role, r->no_redis_required, r->update_resolve, - r->require_master); + r->require_master, + r->sentinel); } void redis_close(struct redis *r) { @@ -1092,6 +1146,18 @@ static int redis_check_conn(struct redis *r) { rtpe_now = now_us(); } + bool moved = redis_sentinel_follow(r); + if (r->sentinel && !r->gen) { + // no master known yet + r->skipped++; + if (!r->skipped_logged) + rlog(LOG_WARNING, "No master of '%s' known from Redis Sentinel yet", + redis_sentinel_master_name(r->sentinel)); + r->skipped_logged = true; + redis_sentinel_wake(r->sentinel, false); + return REDIS_STATE_DISCONNECTED; + } + if ((r->state == REDIS_STATE_DISCONNECTED) && r->restore_tick_us > rtpe_now) { // every Redis operation ends up here: say it once r->skipped++; @@ -1103,8 +1169,9 @@ static int redis_check_conn(struct redis *r) { } if (r->state == REDIS_STATE_DISCONNECTED) { - ilog(LOG_INFO, "RE-Establishing connection for Redis server %s (%u operations skipped)", - endpoint_print_buf(&r->endpoint), r->skipped); + if (!moved) + ilog(LOG_INFO, "RE-Establishing connection for Redis server %s (%u operations skipped)", + endpoint_print_buf(&r->endpoint), r->skipped); r->skipped = 0; r->skipped_logged = false; } @@ -1123,7 +1190,7 @@ static int redis_check_conn(struct redis *r) { // redis is disconnected if (r->state == REDIS_STATE_CONNECTED) { rlog(LOG_ERR, "Lost connection to Redis '%s'", - r->hostname); + r->hostname ? : r->host); r->state = REDIS_STATE_DISCONNECTED; } @@ -1141,6 +1208,8 @@ static int redis_check_conn(struct redis *r) { if (ret) { // redis is disconnected redis_count_err_and_disable(r); + if (r->sentinel && ret == REDIS_CONNECT_ROLE) + redis_sentinel_wake(r->sentinel, false); // After the wrong role, or a slow failure (a timeout), pause (one // second by default): a failover takes longer than that, so the // next attempt would fail the same way. A quick failure diff --git a/daemon/redis_sentinel.c b/daemon/redis_sentinel.c new file mode 100644 index 000000000..5e693454c --- /dev/null +++ b/daemon/redis_sentinel.c @@ -0,0 +1,403 @@ +#include "redis_sentinel.h" + +#include +#include +#include +#include +#include +#include +#include + +#include "helpers.h" +#include "log_d.h" +#include "main.h" +#include "redis.h" + +struct sentinel_node { + char *host; /* as configured, for --redis-resolve-on-reconnect */ + endpoint_t ep; + bool failing; /* last query failed (logged once) */ +}; + +struct redis_sentinel { + struct sentinel_node *nodes; + unsigned int num_nodes; + char *auth; + char *master_name; + const char *probe_auth; /* data node password, for ROLE */ + + /* published master: written by the thread, read by the connections */ + mutex_t lock; + cond_t cond; + bool wake; + endpoint_t master; + uint64_t epoch; + atomic64 gen; /* 0: no master known yet */ + atomic64 limited_wake_us; + + /* thread only */ + bool none_answered; /* logged once */ + bool epoch_back; /* logged once */ + struct thread_waker waker; +}; + +struct redis_sentinel *rtpe_redis_sentinel; + + +/* [PW@]HOST:PORT[,HOST:PORT...] */ +struct redis_sentinel *redis_sentinel_new(const char *list, const char *master_name, + const char *auth_env) +{ + if (!list || !*list || !master_name || !*master_name) + return NULL; + + struct redis_sentinel *s = g_new0(struct redis_sentinel, 1); + mutex_init(&s->lock); + cond_init(&s->cond); + g_autofree char *dup = g_strdup(list); + g_auto(GStrv) items = NULL; + char *hosts = dup; + + char *at = strrchr(hosts, '@'); + if (at) { + *at = '\0'; + if (!*hosts) + goto err; // "@host:port": an empty password is a mistake + s->auth = g_strdup(hosts); + hosts = at + 1; + } + else { + const char *env = getenv(auth_env); + if (env && *env) + s->auth = g_strdup(env); + } + + items = g_strsplit(hosts, ",", -1); + s->num_nodes = g_strv_length(items); + if (!s->num_nodes) + goto err; + s->nodes = g_new0(struct sentinel_node, s->num_nodes); + + for (unsigned int i = 0; i < s->num_nodes; i++) { + struct sentinel_node *n = &s->nodes[i]; + char *item = g_strstrip(items[i]); + char *colon = strrchr(item, ':'); + if (!colon) + goto err; + if (!endpoint_parse_any_getaddrinfo_full(&n->ep, item) || !n->ep.port) + goto err; + n->host = g_strndup(item, colon - item); + } + + s->master_name = g_strdup(master_name); + return s; + +err: + redis_sentinel_free(s); + return NULL; +} + +void redis_sentinel_free(struct redis_sentinel *s) { + if (!s) + return; + for (unsigned int i = 0; s->nodes && i < s->num_nodes; i++) + g_free(s->nodes[i].host); + g_free(s->nodes); + g_free(s->auth); + g_free(s->master_name); + mutex_destroy(&s->lock); + g_free(s); +} + +void redis_sentinel_set_probe_auth(struct redis_sentinel *s, const char *auth) { + s->probe_auth = auth; +} + +const char *redis_sentinel_master_name(const struct redis_sentinel *s) { + return s->master_name; +} + + +uint64_t redis_sentinel_gen(struct redis_sentinel *s) { + return atomic64_get(&s->gen); +} + +uint64_t redis_sentinel_get(struct redis_sentinel *s, endpoint_t *ep) { + LOCK(&s->lock); + *ep = s->master; + return atomic64_get_na(&s->gen); +} + +/* "limited": at most once per poll interval (NOREPLICAS lasts as long as the + * master lacks replicas) */ +void redis_sentinel_wake(struct redis_sentinel *s, bool limited) { + if (limited) { + int64_t now = now_us(); + int64_t last = atomic64_get(&s->limited_wake_us); + if (last && now >= last && now - last < rtpe_config.redis_sentinel_interval * 1000LL) + return; + atomic64_set(&s->limited_wake_us, now); + } + LOCK(&s->lock); + s->wake = true; + cond_signal(&s->cond); +} + + +static redisContext *sentinel_connect(const endpoint_t *ep, const char *auth) { + char host[64]; + sockaddr_print(&ep->address, host, sizeof(host)); + int64_t connect_timeout = atomic_get_na(&rtpe_config.redis_connect_timeout) * 1000LL; + // never without a command timeout ("rediscmdtimeout 0"): a server that + // accepts but never answers would stop the thread for good + int64_t cmd_timeout = atomic_get_na(&rtpe_config.redis_cmd_timeout) * 1000LL ? : 1000000; + + redisContext *ctx = redisConnectWithTimeout(host, ep->port, timeval_from_us(connect_timeout)); + if (!ctx) + return NULL; + const char *err = NULL; + redisReply *rp = NULL; + if (ctx->err || redisSetTimeout(ctx, timeval_from_us(cmd_timeout))) + err = ctx->errstr; + else if (auth && !(rp = redisCommand(ctx, "AUTH %s", auth))) + err = ctx->errstr; + else if (rp && rp->type == REDIS_REPLY_ERROR) + err = "authentication failed"; + if (rp) + freeReplyObject(rp); + if (!err) + return ctx; + rlog(LOG_WARN | LOG_FLAG_LIMIT, "Redis Sentinel: cannot connect to %s: %s", + endpoint_print_buf(ep), err); + redisFree(ctx); + return NULL; +} + +static const char *reply_field(const redisReply *rp, const char *name) { + for (size_t i = 0; i + 1 < rp->elements; i += 2) { + const redisReply *k = rp->element[i], *v = rp->element[i + 1]; + if (k->type == REDIS_REPLY_STRING && v->type == REDIS_REPLY_STRING && !strcmp(k->str, name)) + return v->str; + } + return NULL; +} + +static const char *sentinel_parse(const redisReply *rp, const redisReply *ra, + endpoint_t *master, uint64_t *epoch) +{ + if (rp->type == REDIS_REPLY_ERROR) + return rp->str; + if (ra->type == REDIS_REPLY_ERROR) + return ra->str; + const char *epoch_s = rp->type == REDIS_REPLY_ARRAY ? reply_field(rp, "config-epoch") : NULL; + if (!epoch_s || ra->type != REDIS_REPLY_ARRAY || ra->elements != 2 + || ra->element[0]->type != REDIS_REPLY_STRING + || ra->element[1]->type != REDIS_REPLY_STRING) + return "unexpected reply"; + char *end; + *epoch = strtoull(epoch_s, &end, 10); + if (end == epoch_s || *end) + return "unexpected reply"; + unsigned long port = strtoul(ra->element[1]->str, &end, 10); + if (end == ra->element[1]->str || *end || !port || port > 0xffff) + return "unexpected reply"; + // a host name (announce-hostnames) is resolved here, in the thread + if (!sockaddr_getaddrinfo(&master->address, ra->element[0]->str)) + return "cannot resolve the master's address"; + master->port = port; + return NULL; +} + +/* Epoch first, then address, so that a failover between the two replies can + * only pair an older epoch with the newer address. (The "ip" of SENTINEL + * MASTER switches seconds after the epoch.) */ +static bool sentinel_query(struct redis_sentinel *s, struct sentinel_node *n, + endpoint_t *master, uint64_t *epoch) +{ + if (rtpe_config.redis_resolve_on_reconnect) { + sockaddr_t a; + if (sockaddr_getaddrinfo(&a, n->host)) + n->ep.address = a; + } + + redisContext *ctx = sentinel_connect(&n->ep, s->auth); + if (!ctx) { + n->failing = true; + return false; + } + + redisReply *rp = NULL, *ra = NULL; + const char *err; + redisAppendCommand(ctx, "SENTINEL MASTER %s", s->master_name); + redisAppendCommand(ctx, "SENTINEL GET-MASTER-ADDR-BY-NAME %s", s->master_name); + if (redisGetReply(ctx, (void **) &rp) != REDIS_OK || redisGetReply(ctx, (void **) &ra) != REDIS_OK) + err = ctx->errstr; + else + err = sentinel_parse(rp, ra, master, epoch); + + if (err && !n->failing) + rlog(LOG_WARN, "Redis Sentinel %s: cannot get the master of '%s': %s", + endpoint_print_buf(&n->ep), s->master_name, err); + else if (!err && n->failing) + rlog(LOG_NOTICE, "Redis Sentinel %s answers again", endpoint_print_buf(&n->ep)); + n->failing = err != NULL; + + if (rp) + freeReplyObject(rp); + if (ra) + freeReplyObject(ra); + redisFree(ctx); + return !err; +} + +static bool sentinel_is_master(struct redis_sentinel *s, const endpoint_t *ep, uint64_t epoch) { + redisContext *ctx = sentinel_connect(ep, s->probe_auth); + if (!ctx) + return false; + + bool ret = false; + redisReply *rp = redisCommand(ctx, "ROLE"); + if (rp && rp->type == REDIS_REPLY_ARRAY && rp->elements && rp->element[0]->type == REDIS_REPLY_STRING) { + ret = !strcmp(rp->element[0]->str, "master"); + if (!ret) + rlog(LOG_WARN | LOG_FLAG_LIMIT, "Redis Sentinel names %s as master of '%s' " + "(config epoch %" PRIu64 "), but it is a %s: not used", + endpoint_print_buf(ep), s->master_name, epoch, rp->element[0]->str); + } + else + rlog(LOG_WARN | LOG_FLAG_LIMIT, "Redis Sentinel: cannot check the role of %s: %s", + endpoint_print_buf(ep), !rp ? ctx->errstr + : rp->type == REDIS_REPLY_ERROR ? rp->str : "unexpected reply"); + + if (rp) + freeReplyObject(rp); + redisFree(ctx); + return ret; +} + +static void sentinel_publish(struct redis_sentinel *s, const endpoint_t *m, uint64_t epoch) { + { + LOCK(&s->lock); + s->master = *m; + s->epoch = epoch; + atomic64_inc(&s->gen); + } + rlog(LOG_NOTICE, "Redis Sentinel: master of '%s' is now %s (config epoch %" PRIu64 ")", + s->master_name, endpoint_print_buf(m), epoch); +} + +/* true if the candidate is not a master: ask again soon */ +static bool sentinel_round(struct redis_sentinel *s) { + endpoint_t best = {0}, pub; + uint64_t best_epoch = 0, pub_epoch, pub_gen; + bool found = false; + + { + LOCK(&s->lock); + pub = s->master; + pub_epoch = s->epoch; + pub_gen = atomic64_get_na(&s->gen); + } + + for (unsigned int i = 0; i < s->num_nodes; i++) { + endpoint_t m = {0}; + uint64_t e = 0; + if (!sentinel_query(s, &s->nodes[i], &m, &e)) + continue; + // at the same epoch, keep the master in use (no flapping while + // the Sentinels disagree) + if (!found || e > best_epoch || (e == best_epoch && pub_gen && endpoint_eq(&m, &pub))) { + best = m; + best_epoch = e; + found = true; + } + } + + if (!found) { + if (!s->none_answered) + rlog(LOG_ERR, "Redis Sentinel: no Sentinel answered for '%s'%s%s", + s->master_name, + pub_gen ? "; keeping " : "", + pub_gen ? endpoint_print_buf(&pub) : ""); + s->none_answered = true; + return false; + } + s->none_answered = false; + + // Never back to an older epoch: a demoted master still says "master" + // until Sentinel reconfigures it. The master in use is kept for as long + // as it answers as a master (its connection checks the role). + if (pub_gen && best_epoch < pub_epoch) { + if (!s->epoch_back) + rlog(LOG_ERR, "Redis Sentinel: every Sentinel that answers names %s as master of '%s' " + "at config epoch %" PRIu64 ", older than %" PRIu64 " (%s, in use): not " + "followed. If the Sentinels were redeployed without their state, their " + "epochs went back, and rtpengine must be restarted", + endpoint_print_buf(&best), s->master_name, best_epoch, pub_epoch, + endpoint_print_buf(&pub)); + s->epoch_back = true; + return false; + } + s->epoch_back = false; + + if (pub_gen && endpoint_eq(&best, &pub)) { + LOCK(&s->lock); + s->epoch = best_epoch; + return false; + } + + if (!sentinel_is_master(s, &best, best_epoch)) + return true; + + sentinel_publish(s, &best, best_epoch); + return false; +} + +/* at startup: as for a data node, fails at once if nothing answers, and asks + * again once a second (11 times) while the candidate is not a master */ +bool redis_sentinel_discover(struct redis_sentinel *s) { + for (int i = 0; i < 11; i++) { + if (i) + usleep(1000000); + bool refused = sentinel_round(s); + if (redis_sentinel_gen(s)) + return true; + if (!refused) + break; + } + return false; +} + +void redis_sentinel_loop(struct redis_sentinel *s) { + int64_t interval_us = rtpe_config.redis_sentinel_interval * 1000LL; + int64_t next = now_us() + interval_us, last = 0; + + // a round is bounded by the timeouts; at shutdown, the waker ends the wait + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL); + + s->waker.lock = &s->lock; + s->waker.cond = &s->cond; + thread_waker_add(&s->waker); + + while (!rtpe_shutdown) { + { + LOCK(&s->lock); + // a wake-up comes at least 250 ms after the last round + while (!rtpe_shutdown) { + int64_t until = s->wake ? MIN(next, last + 250000) : next; + if (now_us() >= until) + break; + cond_timedwait(&s->cond, &s->lock, until); + } + s->wake = false; + } + if (rtpe_shutdown) + break; + + bool refused = sentinel_round(s); + last = now_us(); + next = last + (refused ? MIN(250000, interval_us) : interval_us); + } + + thread_waker_del(&s->waker); +} diff --git a/docs/rtpengine.md b/docs/rtpengine.md index a6737b79a..f82bd417d 100644 --- a/docs/rtpengine.md +++ b/docs/rtpengine.md @@ -560,6 +560,11 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- When this option is given, __rtpengine__ will delay startup until the Redis database adopts the master role (but see below). + Instead of *IP*:*PORT*, the keyword __sentinel__ can be given, for + example *foobar@sentinel/12*: the address is then the current master of + a Redis Sentinel group, see __\-\-redis-sentinel__. This is also + possible for __\-\-redis-write__ and __\-\-redis-subscribe__. + - __-w__, __\-\-redis-write=__\[*PW*@\]*IP*:*PORT*/*INT* Configures a second Redis database for write operations. @@ -597,7 +602,10 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- This option takes precedence over __\-\-redis__, if configured. When __\-\-subscribe-keyspace__ is also configured, the keyspace part of __\-\-redis-subscribe=__ is not used, the former takes precedence. - The keyspace number can also be omitted altogether. + The keyspace number can also be omitted altogether. With __sentinel__ + instead of *IP*:*PORT* (see __\-\-redis-sentinel__), the subscription is + made on the current master: a replica is not used, even though + notifications could be read from one. For password protected Redis servers, the environment variable for the password is __RTPENGINE\_REDIS\_SUBSCRIBE\_AUTH\_PW__. @@ -725,6 +733,41 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- remote redis server try to re-resolve if the redis hostname was an FQDN and not IP address. It is a boolean value (either __true__ or __false__). +- __\-\-redis-sentinel=__\[*PW*@\]*IP*:*PORT*\[,*IP*:*PORT*...\] + + The Redis Sentinels to ask for the master, for every Redis option given + with __sentinel__ instead of *IP*:*PORT*; requires + __\-\-redis-sentinel-master__. Their password, usually not the one of + the data nodes, is given before the `@` symbol or in + __RTPENGINE\_REDIS\_SENTINEL\_AUTH\_PW__. Host names are resolved at + startup, and before each query with __\-\-redis-resolve-on-reconnect__. + + The Sentinels are asked every __\-\-redis-sentinel-interval__ and when a + connection finds a replica. The highest configuration epoch wins, and + never an older one than the master in use: if all are older (Sentinels + redeployed without their state), an error is logged and __rtpengine__ + must be restarted. The master is checked with __ROLE__, then every + connection through Sentinel moves to it at its next operation. These + connections require the master role, as with + __\-\-redis-require-master__. Set __\-\-redis-cmd-timeout__ (for example + to 1000): without it, a master that stops answering holds Redis + operations until it answers again. + + If no Sentinel answers at startup, __rtpengine__ does not start unless + __\-\-no-redis-required__ is given; if none answers later, the last + master is kept. + +- __\-\-redis-sentinel-master=__*STRING* + + The name of the master that the Redis Sentinels monitor. + +- __\-\-redis-sentinel-interval=__*INT* + + How often, in milliseconds, the Redis Sentinels are asked for the master + when nothing prompted a query. The default value is 1000. + __\-\-redis-sentinel-master__ and __\-\-redis-sentinel-interval__ are + refused without a Redis option that uses __sentinel__. + - __\-\-redis-format=bencode__\|__JSON__ Selects the format for serialised call data written to Redis or KeyDB. The @@ -2157,6 +2200,10 @@ IPv6. specified, in which case the one specified in __\-\-redis__ will be used for read operations only. +- __RTPENGINE\_REDIS\_SENTINEL\_AUTH\_PW__ + + Redis Sentinel password, for __\-\-redis-sentinel__. + ## FILES - `/etc/rtpengine/rtpengine.conf` diff --git a/etc/rtpengine.conf b/etc/rtpengine.conf index 0af57ae8d..7206561cd 100644 --- a/etc/rtpengine.conf +++ b/etc/rtpengine.conf @@ -85,6 +85,10 @@ recording-method = proc # redis-reconnect-slow = 100 # redis-resolve-on-reconnect = false # redis-require-master = false +# redis-write = password@sentinel/42 +# redis-sentinel = password@10.0.0.11:26379,10.0.0.12:26379,10.0.0.13:26379 +# redis-sentinel-master = mymaster +# redis-sentinel-interval = 1000 # redis-tcp-keepalive-time = 1 # redis-tcp-keepalive-intvl = 1 # redis-tcp-keepalive-probes = 3 diff --git a/include/main.h b/include/main.h index c14477bdd..817e548e2 100644 --- a/include/main.h +++ b/include/main.h @@ -62,6 +62,7 @@ enum endpoint_learning { X(redis_tcp_keepalive_time) \ X(redis_tcp_keepalive_intvl) \ X(redis_tcp_keepalive_probes) \ + X(redis_sentinel_interval) \ X(num_threads) \ X(media_num_threads) \ X(codec_num_threads) \ @@ -140,6 +141,9 @@ enum endpoint_learning { X(poller_per_thread) \ X(redis_resolve_on_reconnect) \ X(redis_require_master) \ + X(redis_via_sentinel) \ + X(redis_write_via_sentinel) \ + X(redis_subscribe_via_sentinel) \ X(measure_rtp) \ X(amr_cn_dtx) \ X(evs_cn_dtx) \ diff --git a/include/redis.h b/include/redis.h index 6e2b86d0a..bb740b771 100644 --- a/include/redis.h +++ b/include/redis.h @@ -15,8 +15,13 @@ #include "crypto.h" #include "dtls.h" +struct redis_sentinel; + #define REDIS_RESTORE_NUM_THREADS 4 +// also logged while calls are being restored +#define rlog(l, x...) ilog(l | LOG_FLAG_RESTORE, x) + enum redis_role { MASTER_REDIS_ROLE = 0, SLAVE_REDIS_ROLE = 1, @@ -77,6 +82,9 @@ struct redis { bool update_resolve; bool require_master; /* a role error drops the connection */ + + struct redis_sentinel *sentinel; /* address from Redis Sentinel, or NULL */ + uint64_t gen; /* generation of the master in endpoint */ }; struct redis_hash { @@ -115,7 +123,7 @@ void redis_delete_async_loop(void *d); struct redis *redis_new(const endpoint_t *, int, const char *, const char *, enum redis_role, int, bool, - bool); + bool, struct redis_sentinel *); struct redis *redis_dup(const struct redis *r, int db); void redis_close(struct redis *r); int redis_restore(struct redis *, bool foreign, int db); diff --git a/include/redis_sentinel.h b/include/redis_sentinel.h new file mode 100644 index 000000000..0a31e1838 --- /dev/null +++ b/include/redis_sentinel.h @@ -0,0 +1,32 @@ +#ifndef __REDIS_SENTINEL_H__ +#define __REDIS_SENTINEL_H__ + +#include +#include + +#include "socket.h" + +/* + * A thread asks the Redis Sentinels for the master and publishes it with a + * generation number; the connections that use it compare the generation (one + * atomic read) and reconnect when it changes. All Sentinel I/O is done in + * that thread. + */ + +struct redis_sentinel; + +extern struct redis_sentinel *rtpe_redis_sentinel; + +struct redis_sentinel *redis_sentinel_new(const char *list, const char *master_name, + const char *auth_env); +void redis_sentinel_set_probe_auth(struct redis_sentinel *, const char *auth); +bool redis_sentinel_discover(struct redis_sentinel *); +void redis_sentinel_loop(struct redis_sentinel *); +void redis_sentinel_free(struct redis_sentinel *); + +uint64_t redis_sentinel_gen(struct redis_sentinel *); +uint64_t redis_sentinel_get(struct redis_sentinel *, endpoint_t *); +void redis_sentinel_wake(struct redis_sentinel *, bool limited); +const char *redis_sentinel_master_name(const struct redis_sentinel *); + +#endif diff --git a/perl/NGCP/Rtpengine/FakeRedis.pm b/perl/NGCP/Rtpengine/FakeRedis.pm index bc15b3f8e..552d409fc 100644 --- a/perl/NGCP/Rtpengine/FakeRedis.pm +++ b/perl/NGCP/Rtpengine/FakeRedis.pm @@ -2,12 +2,15 @@ package NGCP::Rtpengine::FakeRedis; # Fake Redis servers for the daemon tests: listeners on test addresses (made # local by tests-preload.so), and helpers that read the commands that -# rtpengine sends (RESP arrays of bulk strings) and answer them. +# rtpengine sends (RESP arrays of bulk strings) and answer them. Fake Redis +# Sentinels run in a child process (sentinels_start()), so that they answer +# while the test script handles the data nodes. use strict; use warnings; use Socket qw(AF_INET SOCK_STREAM sockaddr_in inet_aton); use File::Temp; +use IO::Handle; use Test::More; use Time::HiRes qw(time sleep); use IO::Select; @@ -18,9 +21,18 @@ use Exporter 'import'; our @EXPORT = qw(redis_listen redis_accept redis_conn redis_read redis_send redis_expect redis_expect_set redis_expect_del redis_close redis_closed readable info_reply redis_write_errors wait_log - redis_serve redis_sets_seen redis_record_after redis_serve_next_get); + redis_serve redis_sets_seen redis_record_after redis_serve_next_get + %role @probes wait_probe serve_probes + sentinels_start sentinels_set sentinel_log sentinel_queries); my %rbuf; # what was read from each socket and not parsed yet +my %node; # IP of each listener, by fileno + +# Role probes of the Sentinel thread (AUTH, then ROLE) on the data nodes are +# answered from %role (by IP, "master" by default), and recorded in @probes +# as [IP, password, role answered]. +our %role; +our @probes; sub redis_listen { @@ -29,6 +41,7 @@ sub redis_listen { socket($l, AF_INET, SOCK_STREAM, 0) or die; bind($l, sockaddr_in($port // 6379, inet_aton($ip))) or die "bind $ip: $!"; listen($l, 10) or die; + $node{fileno($l)} = $ip; return $l; } @@ -132,14 +145,40 @@ sub redis_accept { return $fd; } +sub role_reply { + my $role = shift; + return "*3\r\n\$6\r\nmaster\r\n:0\r\n*0\r\n" if $role eq 'master'; + return "*5\r\n\$5\r\nslave\r\n\$12\r\n203.0.113.44\r\n:6379\r\n\$9\r\nconnected\r\n:0\r\n"; +} + +# after the AUTH of a probe: answer its ROLE +sub probe_answer { + my ($fd, $ip, $auth) = @_; + my $r = $role{$ip} // 'master'; + redis_send($fd, role_reply($r)); + push(@probes, [$ip, $auth, $r]); + redis_close($fd); +} + # A new data connection: AUTH, SELECT, INFO answered with the role (default # "master"), then TYPE unless "refused" (the role is not the one wanted). -# Options: auth, db, role, short, replid, refused. +# Options: auth, db, role, short, replid, refused. Probes that come first +# are answered. sub redis_conn { my ($l, $n, %opt) = @_; - my $fd = redis_accept($l); - redis_expect($fd, ['AUTH', $opt{auth} // 'auth'], "+OK\r\n", "$n AUTH"); - redis_expect($fd, ['SELECT', $opt{db} // 2], "+OK\r\n", "$n SELECT"); + my ($fd, $auth, $c); + while (1) { + $fd = redis_accept($l); + $auth = redis_read($fd); + # (a probe can give up before it is answered) + redis_send($fd, "+OK\r\n") if $auth; + $c = $auth && redis_read($fd); + last if $c && $c->[0] ne 'ROLE'; + $c ? probe_answer($fd, $node{fileno($l)}, $auth->[1]) : redis_close($fd); + } + is(join(' ', @$auth), 'AUTH ' . ($opt{auth} // 'auth'), "$n AUTH"); + is(join(' ', @$c), 'SELECT ' . ($opt{db} // 2), "$n SELECT"); + redis_send($fd, "+OK\r\n"); redis_expect($fd, ['INFO'], info_reply($opt{role} // 'master', %opt), "$n INFO"); redis_expect($fd, ['TYPE', 'calls'], "+none\r\n", "$n TYPE") unless $opt{refused}; return $fd; @@ -318,4 +357,190 @@ sub wait_log { die "not in the log: $re\n"; } + + +# wait for a role probe on this listener +sub wait_probe { + my ($l, $n, $timeout) = @_; + my $fd = redis_accept($l, $timeout); + my $c = redis_expect($fd, ['AUTH', 'auth'], "+OK\r\n", "$n: probe AUTH with the data password"); + redis_expect($fd, ['ROLE'], undef, "$n: probe ROLE"); + probe_answer($fd, $node{fileno($l)}, $c->[1]); +} + +# for a while, answer every probe on these listeners, whatever arrives +sub serve_probes { + my ($secs, @l) = @_; + my $end = time() + $secs; + while ((my $left = $end - time()) > 0) { + my $rin = ''; + vec($rin, fileno($_), 1) = 1 for @l; + next unless select(my $rout = $rin, undef, undef, $left) > 0; + for my $l (grep { vec($rout, fileno($_), 1) } @l) { + my $fd = redis_accept($l); + my $a = eval { redis_read($fd, 1) }; + redis_send($fd, "+OK\r\n") if $a; + my $c = $a && eval { redis_read($fd, 1) }; + $c && $c->[0] eq 'ROLE' ? probe_answer($fd, $node{fileno($l)}, $a->[1]) : redis_close($fd); + } + } +} + + +# Fake Sentinels. The child re-reads a state file every 50 ms and logs every +# command it gets ("IP:PORT COMMAND ARG..."), plus "applied N" for each new +# state. Each Sentinel (IP, port 26379) is either "down" (not listening) or +# "MASTER_IP MASTER_PORT EPOCH [MONITORED_IP]": it names this master +# (GET-MASTER-ADDR-BY-NAME) at this config epoch (SENTINEL MASTER), whose +# "ip" is MONITORED_IP if given: a Sentinel running a failover reports the +# promoted replica in the former and still the old master in the latter. + +my ($sentinel_pid, $sentinel_state, $sentinel_log, $sentinel_password); +my $serial = 0; + +sub sentinel_log { + open(my $fh, '<', "$sentinel_log") or die; + local $/; + return <$fh> // ''; +} + +# how many times this Sentinel was asked for the master +sub sentinel_queries { + my $ip = shift; + my @l = sentinel_log() =~ /^\Q$ip\E:26379 SENTINEL MASTER mymaster$/mg; + return scalar(@l); +} + +# set what the Sentinels answer (IP => state), and wait until it is applied +sub sentinels_set { + my %st = @_; + $serial++; + open(my $fh, '>', "$sentinel_state.new") or die; + print $fh "serial $serial\npassword $sentinel_password\n"; + print $fh "$_:26379 $st{$_}\n" for sort(keys(%st)); + close($fh); + rename("$sentinel_state.new", "$sentinel_state") or die; + return unless $sentinel_pid; + for (1 .. 100) { + return if sentinel_log() =~ /^applied $serial$/m; + sleep(0.05); + } + die "fake Sentinels did not apply state $serial\n"; +} + +sub sentinels_start { + ($sentinel_password, my %st) = @_; + $sentinel_state = File::Temp->new(TEMPLATE => 'rtpe-sentinel-state.XXXXXXXXXX', TMPDIR => 1); + $sentinel_log = File::Temp->new(TEMPLATE => 'rtpe-sentinel-log.XXXXXXXXXX', TMPDIR => 1); + sentinels_set(%st); + $sentinel_pid = fork() // die; + if (!$sentinel_pid) { + exec($^X, (map { "-I$_" } grep { !ref($_) } @INC), '-MNGCP::Rtpengine::FakeRedis', + '-e', 'NGCP::Rtpengine::FakeRedis::sentinel_main(@ARGV)', + "$sentinel_state", "$sentinel_log") or die; + } + sentinels_set(%st); +} + +END { + if ($sentinel_pid) { + kill('TERM', $sentinel_pid); + waitpid($sentinel_pid, 0); + } +} + +# the child +sub sentinel_main { + my ($statefile, $logfile) = @_; + open(my $log, '>>', $logfile) or die; + $log->autoflush(1); + my $done = 0; + $SIG{TERM} = sub { $done = 1 }; + $SIG{PIPE} = 'IGNORE'; + my (%sentinels, %clients, $password); + my $applied = -1; + + my $read_state = sub { + open(my $fh, '<', $statefile) or return; + my ($ser, %want); + while (my $l = <$fh>) { + my @f = split(' ', $l); + if ($f[0] eq 'serial') { $ser = $f[1] } + elsif ($f[0] eq 'password') { $password = $f[1] } + else { $want{$f[0]} = [ @f[1 .. $#f] ] } + } + close($fh); + for my $s (keys(%want)) { + my $st = $sentinels{$s} //= { }; + $st->{state} = $want{$s}; + if ($want{$s}[0] eq 'down') { + close(delete($st->{listener})) if $st->{listener}; + } + elsif (!$st->{listener}) { + my ($ip, $port) = split(/:/, $s); + $st->{listener} = redis_listen($ip, $port); + } + } + # a Sentinel that goes down closes its clients too + for my $fn (keys(%clients)) { + next if $sentinels{$clients{$fn}{sentinel}}{listener}; + close($clients{$fn}{fh}); + delete($clients{$fn}); + } + if (defined($ser) && $ser != $applied) { + $applied = $ser; + print $log "applied $ser\n"; + } + }; + + my $reply = sub { + my ($c, $args) = @_; + my $st = $sentinels{$c->{sentinel}}{state}; + print $log "$c->{sentinel} @$args\n"; + my $cmd = uc($args->[0]) . ' ' . uc($args->[1] // ''); + if ($cmd =~ /^AUTH /) { + return "-WRONGPASS invalid username-password pair\r\n" if $args->[-1] ne $password; + $c->{authed} = 1; + return "+OK\r\n"; + } + return "-NOAUTH Authentication required.\r\n" unless $c->{authed}; + return "-ERR No such master with that name\r\n" if ($args->[2] // '') ne 'mymaster'; + my ($ip, $port, $epoch, $monitored) = @$st; + return "*2\r\n" . bulk($ip) . bulk($port) if $cmd eq 'SENTINEL GET-MASTER-ADDR-BY-NAME'; + my @kv = (name => 'mymaster', ip => $monitored // $ip, port => $port, flags => 'master', + 'num-slaves' => 2, 'num-other-sentinels' => 2, quorum => 2, 'config-epoch' => $epoch); + return '*' . scalar(@kv) . "\r\n" . join('', map { bulk($_) } @kv) if $cmd eq 'SENTINEL MASTER'; + return "-ERR unknown command\r\n"; + }; + + $read_state->(); + while (!$done) { + my $rin = ''; + my %l = map { fileno($_->{listener}) => $_ } grep { $_->{listener} } values(%sentinels); + vec($rin, $_, 1) = 1 for (keys(%l), keys(%clients)); + if (select(my $rout = $rin, undef, undef, 0.05) > 0) { + for my $fn (grep { vec($rout, $_, 1) } keys(%l)) { + my ($s) = grep { $sentinels{$_} == $l{$fn} } keys(%sentinels); + accept(my $fh, $l{$fn}{listener}) or next; + $clients{fileno($fh)} = { fh => $fh, buf => '', sentinel => $s }; + } + for my $fn (grep { vec($rout, $_, 1) } keys(%clients)) { + my $c = $clients{$fn}; + my $data; + if (!sysread($c->{fh}, $data, 65536)) { + close($c->{fh}); + delete($clients{$fn}); + next; + } + $c->{buf} .= $data; + while (my $args = parse_cmd(\$c->{buf})) { + syswrite($c->{fh}, $reply->($c, $args)); + } + } + } + $read_state->(); + } + exit(0); +} + 1; diff --git a/t/Makefile b/t/Makefile index 804122540..ffa89ecf2 100644 --- a/t/Makefile +++ b/t/Makefile @@ -91,6 +91,7 @@ include ../lib/common.Makefile daemon-tests-rollback-redis daemon-tests-redis-srtp-index daemon-tests-redis-errors \ daemon-tests-redis-destroyed daemon-tests-redis-reconnect \ daemon-tests-redis-require-master \ + daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions daemon-tests-alias \ daemon-tests-kernel-api daemon-tests-dtmf-fanout \ daemon-tests-video daemon-tests-inject daemon-tests-dtx-dtmf @@ -146,6 +147,7 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests daemon-tests-redis-srtp-index daemon-tests-redis-errors daemon-tests-redis-destroyed \ daemon-tests-redis-reconnect \ daemon-tests-redis-require-master \ + daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions \ daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-dtx-no-shift daemon-tests-dtx-dtmf \ daemon-tests-alias \ @@ -257,6 +259,12 @@ daemon-tests-redis-reconnect: daemon-test-deps daemon-tests-redis-require-master: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-require-master.pl +daemon-tests-redis-sentinel: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-sentinel.pl + +daemon-tests-redis-sentinel-start: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-sentinel-start.pl + daemon-tests-audio-player: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-audio-player.pl @@ -456,6 +464,7 @@ test-stats: test-stats.o \ ../daemon/ng_client.o \ ../daemon/recording.o \ ../daemon/redis.o \ + ../daemon/redis_sentinel.o \ ../daemon/rtcp.o \ ../daemon/rtp.o \ ../daemon/sdp.strhash.o \ @@ -520,6 +529,7 @@ test-transcode: test-transcode.o \ ../daemon/ng_client.o \ ../daemon/recording.o \ ../daemon/redis.o \ + ../daemon/redis_sentinel.o \ ../daemon/rtcp.o \ ../daemon/rtp.o \ ../daemon/sdp.strhash.o \ diff --git a/t/auto-daemon-tests-redis-require-master.pl b/t/auto-daemon-tests-redis-require-master.pl index ab994c4c7..199482eac 100755 --- a/t/auto-daemon-tests-redis-require-master.pl +++ b/t/auto-daemon-tests-redis-require-master.pl @@ -136,8 +136,10 @@ offer('write after MASTERDOWN', { }, $sdp); -# the read connection was never touched +# the read connection was never touched, and without Sentinel options +# nothing of Sentinel is set up ok(!readable($read_fd, 0), 'nothing on the read connection'); +unlike(rtpe_log(), qr/Sentinel/, 'no Sentinel'); undef($NGCP::Rtpengine::req_cb); diff --git a/t/auto-daemon-tests-redis-sentinel-start.pl b/t/auto-daemon-tests-redis-sentinel-start.pl new file mode 100755 index 000000000..08c2ce3e8 --- /dev/null +++ b/t/auto-daemon-tests-redis-sentinel-start.pl @@ -0,0 +1,197 @@ +#!/usr/bin/perl + +# Redis Sentinel, started with no Sentinel reachable (--no-redis-required), +# and polled rarely (--redis-sentinel-interval=60000), so that every query +# seen here was triggered by rtpengine itself: by an operation with no master +# known, by a NOREPLICAS refusal, by a READONLY refusal, or by a reconnect +# that finds a replica. The Sentinel password comes from the environment. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; +use Time::HiRes qw(sleep); + + +my %l = map { $_ => redis_listen("203.0.113.$_") } (42, 44); + +sentinels_start('spw2', '203.0.113.43' => 'down'); + +# a missing reply fails the test with the daemon's last log lines +local $SIG{ALRM} = sub { die "timeout\n" }; +# a role probe can give up before it is answered +$SIG{PIPE} = 'IGNORE'; + +my $sdp = < '203.0.113.42 6379 1'); +new_queries(); +new_call; +undef($NGCP::Rtpengine::req_cb); +offer('write that wakes the Sentinel thread', { }, $sdp); +wait_query('woken by a write with no master known'); +like(sentinel_log(), qr/^203\.0\.113\.43:26379 AUTH spw2$/m, 'Sentinel password from the environment'); +wait_probe($l{42}, 'first master'); +wait_log(qr/master of 'mymaster' is now 203\.0\.113\.42:6379 \(config epoch 1\)/, 'master published'); + +my $fd; +new_call; +$NGCP::Rtpengine::req_cb = sub { + $fd = redis_conn($l{42}, 'first connection'); + redis_expect_set($fd, cid(), "+OK\r\n", 'first write', no_ping => 1); +}; +offer('first write', { }, $sdp); + + +# NOREPLICAS: the connection is kept, and the Sentinels are asked at once; +# they name .44, where the next write goes + +sentinels_set('203.0.113.43' => '203.0.113.44 6379 2'); +new_queries(); +refused_write($fd, 'NOREPLICAS Not enough good replicas to write.', 'NOREPLICAS write'); +wait_query('woken by NOREPLICAS'); +wait_probe($l{44}, 'after NOREPLICAS'); +wait_log(qr/master of 'mymaster' is now 203\.0\.113\.44:6379 \(config epoch 2\)/, '.44 published'); + +new_call; +$NGCP::Rtpengine::req_cb = sub { + redis_closed($fd, 'connection to .42 closed'); + $fd = redis_conn($l{44}, 'after NOREPLICAS'); + redis_expect_set($fd, cid(), "+OK\r\n", 'write on .44', no_ping => 1); +}; +offer('write on .44', { }, $sdp); +unlike(rtpe_log(), qr/RE-Establishing connection for Redis server 203\.0\.113\.44/, + 'the move is logged once'); + +# NOREPLICAS again, within the poll interval: the connection is kept, and +# the Sentinels are not asked again (a master also says this for as long as +# it lacks replicas: at most one query per interval) + +new_queries(); +refused_write($fd, 'NOREPLICAS Not enough good replicas to write.', 'NOREPLICAS again'); +sleep(0.5); +is(new_queries(), 0, 'NOREPLICAS again: no new query within the interval'); + + +# READONLY: the master role is implied in Sentinel mode, so the connection +# is dropped at once, and the Sentinels are asked + +new_queries(); +new_call; +$NGCP::Rtpengine::req_cb = sub { + redis_expect_set($fd, cid(), "-READONLY You can't write against a read only replica.\r\n", 'READONLY'); + redis_closed($fd, 'READONLY: connection dropped'); +}; +offer('READONLY write', { }, $sdp); +wait_query('woken by READONLY'); + +# The Sentinel still names .44 (same address and epoch: no probe). The +# reconnect finds a replica, gives up after one INFO, asks the Sentinels +# again, and makes no new attempt within the next second. + +sleep(0.3); +new_queries(); +new_call; +$NGCP::Rtpengine::req_cb = sub { + $fd = redis_conn($l{44}, 'replica', role => 'slave', refused => 1); + redis_closed($fd, 'replica: connection closed after one INFO'); +}; +offer('write while .44 is a replica', { }, $sdp); +wait_query('woken by the role mismatch'); +ok(!readable($l{44}, 0.3), 'no probe for an unchanged answer, no reconnect within the pause'); + +new_call; +$NGCP::Rtpengine::req_cb = sub { + ok(!readable($l{44}, 0.2), 'no reconnect within the pacing interval'); +}; +offer('write within the pacing interval', { }, $sdp); + +sleep(1); +new_call; +$NGCP::Rtpengine::req_cb = sub { + $fd = redis_conn($l{44}, 'master again'); + redis_expect_set($fd, cid(), "+OK\r\n", 'master again', no_ping => 1); +}; +offer('write after the pacing interval', { }, $sdp); + + +undef($NGCP::Rtpengine::req_cb); + +done_testing(); diff --git a/t/auto-daemon-tests-redis-sentinel.pl b/t/auto-daemon-tests-redis-sentinel.pl new file mode 100755 index 000000000..db5365b8c --- /dev/null +++ b/t/auto-daemon-tests-redis-sentinel.pl @@ -0,0 +1,213 @@ +#!/usr/bin/perl + +# Redis Sentinel: --redis=PW@sentinel/DB takes the master's address from the +# Sentinels. Two fake Sentinels (203.0.113.43 and .45) name the master; two +# fake data nodes (203.0.113.42 and .44) are scripted here. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; +use Time::HiRes qw(sleep time); + + +my %l = map { $_ => redis_listen("203.0.113.$_") } (42, 44); + +sentinels_start('spw', '203.0.113.43' => '203.0.113.42 6379 1', '203.0.113.45' => '203.0.113.42 6379 1'); + +# a missing reply fails the test with the daemon's last log lines +local $SIG{ALRM} = sub { die "timeout\n" }; +# a role probe can give up before it is answered +$SIG{PIPE} = 'IGNORE'; + +my $sdp = <= $q + $n; + sleep(0.05); + } + return 0; +} +sub write_on { + my ($fd, $n) = @_; + new_call; + $NGCP::Rtpengine::req_cb = sub { redis_expect_set($$fd, cid(), "+OK\r\n", $n) }; + offer($n, { }, $sdp); +} +# the next write moves to the new master, with nothing sent to the old one +sub write_moves { + my ($fd, $to, $n) = @_; + new_call; + $NGCP::Rtpengine::req_cb = sub { + redis_closed($$fd, "$n: old connection closed without a write"); + $$fd = redis_conn($l{$to}, "$n: on .$to"); + redis_expect_set($$fd, cid(), "+OK\r\n", $n, no_ping => 1); + }; + offer($n, { }, $sdp); +} + + +my $main_fd; + +$NGCP::Rtpengine::AutoTest::launch_cb = sub { + # discovery (Sentinels, then a role probe on .42), then the connection + $main_fd = redis_conn($l{42}, 'startup'); + redis_expect($main_fd, ['PING'], "+PONG\r\n", 'restore PING'); + redis_expect($main_fd, ['KEYS', '*'], "*0\r\n", 'restore KEYS'); +}; + +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 + -n 2223 -f -L 7 -E + --redis=auth@sentinel/2 + --redis-sentinel-master=mymaster --redis-sentinel-interval=500 + --redis-cmd-timeout=1000), + '--redis-sentinel=spw@203.0.113.43:26379,203.0.113.45:26379') + or die; + + +# startup: both Sentinels asked, with their password; the master they name +# checked with ROLE and the data password before it was used + +for my $s ('203.0.113.43', '203.0.113.45') { + like(sentinel_log(), qr/^\Q$s\E:26379 AUTH spw\n\Q$s\E:26379 SENTINEL MASTER mymaster\n\Q$s\E:26379 SENTINEL GET-MASTER-ADDR-BY-NAME mymaster$/m, + "Sentinel $s asked at startup"); +} +is_deeply(\@probes, [['203.0.113.42', 'auth', 'master']], 'one role probe at startup, with the data password'); +is(published(qr/203\.0\.113\.42:6379 \(config epoch 1\)/), 1, 'master published'); +like(rtpe_log(), qr/Using Redis Sentinel for the master of 'mymaster'/, 'Sentinel mode'); +write_on(\$main_fd, 'first write on .42'); + + +# Failover to .44. The Sentinel running it (.45) has raised the epoch and +# names the promoted replica in GET-MASTER-ADDR-BY-NAME, while the "ip" of +# its SENTINEL MASTER is still the old master; the other Sentinel (.43) has +# not heard of it yet. The new master is taken at once, once. + +sentinels_set('203.0.113.43' => '203.0.113.42 6379 1', '203.0.113.45' => '203.0.113.44 6379 2 203.0.113.42'); +wait_probe($l{44}, 'failover, leader alone at the new epoch'); +wait_log(qr/master of 'mymaster' is now 203\.0\.113\.44:6379 \(config epoch 2\)/, 'new master published'); +sentinels_set('203.0.113.43' => '203.0.113.44 6379 2', '203.0.113.45' => '203.0.113.44 6379 2'); +ok(queried('203.0.113.43', 2), 'Sentinels asked again'); +ok(!readable($l{44}, 0), 'failover: no second probe'); +is(published(qr/203\.0\.113\.44/), 1, 'failover: published once'); + +write_moves(\$main_fd, 44, 'first write after the failover'); +like(rtpe_log(), qr/following the master of 'mymaster' from 203\.0\.113\.42:6379 to 203\.0\.113\.44:6379/, + 'move logged'); +write_on(\$main_fd, 'next write on .44'); + + +# a Sentinel that missed the failover names .42 with the old epoch: not used +# while the other one names the new master + +sentinels_set('203.0.113.43' => '203.0.113.42 6379 1', '203.0.113.45' => '203.0.113.44 6379 2'); +ok(queried('203.0.113.43', 2), 'stale Sentinel asked'); +ok(!readable($l{42}, 0), 'stale Sentinel: no probe and no connection to .42'); +unlike(rtpe_log(), qr/epochs went back/, 'stale Sentinel, the other one up to date: no error'); + + +# Every Sentinel that answers names an older epoch: never followed, an error +# is logged once, and the master in use is kept. First the stale Sentinel +# alone, then (after a normal state) a Sentinel set redeployed without its +# state, whose epochs start again. + +my $epoch_back = qr/every Sentinel that answers names 203\.0\.113\.42:6379 as master of 'mymaster' at config epoch 1, older than 2 \(203\.0\.113\.44:6379, in use\): not followed\. If the Sentinels were redeployed without their state, their epochs went back, and rtpengine must be restarted/; +sentinels_set('203.0.113.43' => '203.0.113.42 6379 1', '203.0.113.45' => 'down'); +wait_log($epoch_back, 'older epoch from every Sentinel that answers: error'); +ok(queried('203.0.113.43', 2), 'asked again'); +is(scalar(() = rtpe_log() =~ /$epoch_back/g), 1, 'logged once'); + +sentinels_set('203.0.113.43' => '203.0.113.44 6379 2', '203.0.113.45' => '203.0.113.44 6379 2'); +ok(queried('203.0.113.43', 2), 'back to the epoch in use'); +sentinels_set('203.0.113.43' => '203.0.113.42 6379 1', '203.0.113.45' => '203.0.113.42 6379 1'); +ok(queried('203.0.113.43', 2), 'redeployed Sentinels asked'); +is(scalar(() = rtpe_log() =~ /$epoch_back/g), 2, 'logged again after the normal state'); +ok(!readable($l{42}, 0), 'older epoch: no probe and no connection to .42'); +is(published(qr/203\.0\.113\.42:6379 \(config epoch 1\)/), 1, 'older epoch: .42 not published again'); +write_on(\$main_fd, 'write on the master in use'); + + +# a Sentinel names a node at a newer epoch, but it is a replica: not used, +# and asked again soon (before the next poll); then the Sentinels agree on +# .44 at that epoch (same address: no probe) + +$role{'203.0.113.42'} = 'slave'; +sentinels_set('203.0.113.43' => '203.0.113.42 6379 3', '203.0.113.45' => 'down'); +wait_probe($l{42}, 'replica candidate'); +my $t = time(); +wait_probe($l{42}, 'replica candidate again'); +ok(time() - $t < 0.45, 'replica candidate asked again before the next poll'); +sentinels_set('203.0.113.43' => '203.0.113.44 6379 3', '203.0.113.45' => '203.0.113.44 6379 3'); +serve_probes(1, $l{42}); +ok(!readable($l{44}, 0), 'same address at a newer epoch: no probe'); +is(scalar(() = rtpe_log() =~ /names 203\.0\.113\.42:6379 as master of 'mymaster' \(config epoch 3\), but it is a slave: not used/g), + 1, 'replica candidate refused, logged once'); +is(published(qr/203\.0\.113\.42:6379 \(config epoch 3\)/), 0, 'replica never published'); +write_on(\$main_fd, 'write after a replica candidate'); + + +# no Sentinel answers: the last master is kept, logged once + +sentinels_set('203.0.113.43' => 'down', '203.0.113.45' => 'down'); +wait_log(qr/no Sentinel answered for 'mymaster'; keeping 203\.0\.113\.44:6379/, 'no Sentinel answering: logged'); +sleep(1.2); +is(scalar(() = rtpe_log() =~ /no Sentinel answered/g), 1, 'logged once'); +write_on(\$main_fd, 'write without Sentinels'); + +sentinels_set('203.0.113.43' => '203.0.113.44 6379 3', '203.0.113.45' => '203.0.113.44 6379 3'); +wait_log(qr/Redis Sentinel 203\.0\.113\.43:26379 answers again/, 'Sentinel back'); + + +# a second failover, back to .42, seen first from its leader (.45), then +# from both: one move, at once, no flip back + +$role{'203.0.113.42'} = 'master'; +sentinels_set('203.0.113.43' => '203.0.113.44 6379 3', '203.0.113.45' => '203.0.113.42 6379 4 203.0.113.44'); +wait_probe($l{42}, 'second failover'); +wait_log(qr/master of 'mymaster' is now 203\.0\.113\.42:6379 \(config epoch 4\)/, 'new master at once'); +sentinels_set('203.0.113.43' => '203.0.113.42 6379 4', '203.0.113.45' => '203.0.113.42 6379 4'); +ok(queried('203.0.113.43', 2), 'Sentinels asked again'); +is(published(qr/203\.0\.113\.44:6379 \(config epoch 4\)/), 0, 'no flip back to the old master'); +ok(!readable($l{44}, 0), 'no probe of the old master'); +write_moves(\$main_fd, 42, 'first write on the new master'); + + +# throughout: the Sentinels only got their own password and their two +# queries, and every role probe used the data password + +is(scalar(grep { !/^(applied \d+|\S+ (AUTH spw|SENTINEL MASTER mymaster|SENTINEL GET-MASTER-ADDR-BY-NAME mymaster))$/ } split(/\n/, sentinel_log())), 0, + 'Sentinels get nothing else'); +is(scalar(grep { $_->[1] ne 'auth' } @probes), 0, 'role probes only use the data password'); + + +undef($NGCP::Rtpengine::req_cb); + +done_testing(); From 09b775f97417a4726728ca2342a97a337c781608 Mon Sep 17 00:00:00 2001 From: Kevin Binetruy Date: Wed, 14 Oct 2026 10:52:21 +0000 Subject: [PATCH 3/7] MT#55283 redis: keyspace subscription follows the Sentinel master 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. --- daemon/redis.c | 78 +++++++++- docs/rtpengine.md | 10 +- include/redis.h | 3 + t/Makefile | 5 + ...o-daemon-tests-redis-sentinel-subscribe.pl | 147 ++++++++++++++++++ 5 files changed, 232 insertions(+), 11 deletions(-) create mode 100755 t/auto-daemon-tests-redis-sentinel-subscribe.pl diff --git a/daemon/redis.c b/daemon/redis.c index 043f0122b..ec819a499 100644 --- a/daemon/redis.c +++ b/daemon/redis.c @@ -656,6 +656,13 @@ void redis_delete_async_context_disconnect(const redisAsyncContext *redis_delete } void redis_notify_async_context_disconnect(const redisAsyncContext *redis_notify_async_context, int status) { + // forget the context, and let event_base_dispatch() return (the timer + // must not keep it running), so that redis_notify_loop() subscribes again + struct redis *r = rtpe_redis_notify; + if (r && r->notify_timer) + event_del(r->notify_timer); + if (r && r->async_ctx == redis_notify_async_context) + r->async_ctx = NULL; if (status == REDIS_ERR) { if (redis_notify_async_context->errstr) { rlog(LOG_ERROR, "redis_notify_async_context_disconnect error %d on context free: %s", @@ -671,6 +678,12 @@ void redis_notify_async_context_disconnect(const redisAsyncContext *redis_notify } } +// hiredis frees a context that fails to connect without the disconnect callback +static void redis_notify_async_context_connect(const redisAsyncContext *ac, int status) { + if (status != REDIS_OK) + redis_notify_async_context_disconnect(ac, status); +} + // connect_cb = connect callback, disconnect_cb = disconnect callback int redis_async_context_alloc(struct redis *r, void *connect_cb, void *disconnect_cb) { // sanity checks @@ -868,8 +881,35 @@ static int redis_delete_async(struct redis *r) { return 0; } +/* With Sentinel, once a second: on a new master, close the subscription, so + * that event_base_dispatch() returns and redis_notify_loop() subscribes again. + * The timer is one-shot, re-armed only while nothing has changed: it never + * keeps the event loop running by itself. */ +static void redis_notify_follow(evutil_socket_t fd, short what, void *p) { + struct redis *r = p; + struct timeval tv = timeval_from_us(1000000); + + if (redis_sentinel_gen(r->sentinel) == r->notify_gen) { + evtimer_add(r->notify_timer, &tv); + return; + } + + rlog(LOG_NOTICE, "Redis: the master of '%s' has changed: subscribing again; keyspace " + "notifications between the change and the new subscription are missed", + redis_sentinel_master_name(r->sentinel)); + // forget the context first: one still connecting may be freed at once, + // with no callback; without a context, leave the loop + redisAsyncContext *ac = r->async_ctx; + r->async_ctx = NULL; + if (ac) + redisAsyncDisconnect(ac); + else + event_base_loopbreak(r->async_ev); +} + static int redis_notify(struct redis *r) { GList *l; + int ret = 0; if (!r) { rlog(LOG_ERROR, "redis_notify database is NULL on redis_notify()"); @@ -917,13 +957,34 @@ static int redis_notify(struct redis *r) { } rwlock_unlock_r(&rtpe_config.keyspaces_lock); + if (r->sentinel) { + struct timeval tv = timeval_from_us(1000000); + r->notify_timer = evtimer_new(r->async_ev, redis_notify_follow, r); + if (r->notify_timer) + evtimer_add(r->notify_timer, &tv); + } + // dispatch event base => thread blocks here if (event_base_dispatch(r->async_ev) < 0) { rlog(LOG_ERROR, "Fail event_base_dispatch()"); - return -1; + ret = -1; } - return 0; + if (r->notify_timer) { + event_free(r->notify_timer); + r->notify_timer = NULL; + } + + return ret; +} + +/* the subscription's context, with the generation of the master it is for */ +static int redis_notify_context_alloc(struct redis *r) { + LOCK(&r->lock); + redis_sentinel_follow(r); + r->notify_gen = r->gen; + return redis_async_context_alloc(r, redis_notify_async_context_connect, + redis_notify_async_context_disconnect); } /* redis_check_conn() needs the lock: the CLI uses the same connection, and @@ -982,8 +1043,12 @@ void redis_notify_loop(void *d) { return ; } + // with Sentinel and no master known yet, the loop below subscribes once + // there is one + bool now = !r->sentinel || r->gen; + // alloc redis async context - if (redis_async_context_alloc(r, NULL, redis_notify_async_context_disconnect) < 0) { + if (now && redis_notify_context_alloc(r) < 0) { return ; } @@ -993,7 +1058,7 @@ void redis_notify_loop(void *d) { } // initial redis_notify - if (redis_check_conn_locked(r) == REDIS_STATE_CONNECTED) { + if (now && redis_check_conn_locked(r) == REDIS_STATE_CONNECTED) { redis_notify_return = redis_notify(r); } @@ -1010,7 +1075,7 @@ void redis_notify_loop(void *d) { if (redis_check_conn_locked(r) == REDIS_STATE_CONNECTED || redis_notify_return < 0) { r->async_ctx = NULL; // alloc new redis async context upon redis breakdown - if (redis_async_context_alloc(r, NULL, redis_notify_async_context_disconnect) < 0) { + if (redis_notify_context_alloc(r) < 0) { continue; } @@ -1024,7 +1089,8 @@ void redis_notify_loop(void *d) { redis_notify_subscribe_action(r, UNSUBSCRIBE_ALL, 0); // free async context - redisAsyncDisconnect(r->async_ctx); + if (r->async_ctx) + redisAsyncDisconnect(r->async_ctx); r->async_ctx = NULL; } } diff --git a/docs/rtpengine.md b/docs/rtpengine.md index f82bd417d..42b4d5278 100644 --- a/docs/rtpengine.md +++ b/docs/rtpengine.md @@ -747,11 +747,11 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- never an older one than the master in use: if all are older (Sentinels redeployed without their state), an error is logged and __rtpengine__ must be restarted. The master is checked with __ROLE__, then every - connection through Sentinel moves to it at its next operation. These - connections require the master role, as with - __\-\-redis-require-master__. Set __\-\-redis-cmd-timeout__ (for example - to 1000): without it, a master that stops answering holds Redis - operations until it answers again. + connection through Sentinel moves to it at its next operation, a + keyspace subscription within a second. These connections require the + master role, as with __\-\-redis-require-master__. Set + __\-\-redis-cmd-timeout__ (for example to 1000): without it, a master + that stops answering holds Redis operations until it answers again. If no Sentinel answers at startup, __rtpengine__ does not start unless __\-\-no-redis-required__ is given; if none answers later, the last diff --git a/include/redis.h b/include/redis.h index bb740b771..44ccebaf1 100644 --- a/include/redis.h +++ b/include/redis.h @@ -16,6 +16,7 @@ #include "dtls.h" struct redis_sentinel; +struct event; #define REDIS_RESTORE_NUM_THREADS 4 @@ -85,6 +86,8 @@ struct redis { struct redis_sentinel *sentinel; /* address from Redis Sentinel, or NULL */ uint64_t gen; /* generation of the master in endpoint */ + struct event *notify_timer; /* subscriber: checks the generation */ + uint64_t notify_gen; /* generation the subscription was made at */ }; struct redis_hash { diff --git a/t/Makefile b/t/Makefile index ffa89ecf2..ca15e1815 100644 --- a/t/Makefile +++ b/t/Makefile @@ -92,6 +92,7 @@ include ../lib/common.Makefile daemon-tests-redis-destroyed daemon-tests-redis-reconnect \ daemon-tests-redis-require-master \ daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ + daemon-tests-redis-sentinel-subscribe \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions daemon-tests-alias \ daemon-tests-kernel-api daemon-tests-dtmf-fanout \ daemon-tests-video daemon-tests-inject daemon-tests-dtx-dtmf @@ -148,6 +149,7 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests daemon-tests-redis-reconnect \ daemon-tests-redis-require-master \ daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ + daemon-tests-redis-sentinel-subscribe \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions \ daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-dtx-no-shift daemon-tests-dtx-dtmf \ daemon-tests-alias \ @@ -265,6 +267,9 @@ daemon-tests-redis-sentinel: daemon-test-deps daemon-tests-redis-sentinel-start: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-sentinel-start.pl +daemon-tests-redis-sentinel-subscribe: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-sentinel-subscribe.pl + daemon-tests-audio-player: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-audio-player.pl diff --git a/t/auto-daemon-tests-redis-sentinel-subscribe.pl b/t/auto-daemon-tests-redis-sentinel-subscribe.pl new file mode 100755 index 000000000..9bf2f5f6e --- /dev/null +++ b/t/auto-daemon-tests-redis-sentinel-subscribe.pl @@ -0,0 +1,147 @@ +#!/usr/bin/perl + +# Redis Sentinel and keyspace notifications: the subscription follows the +# master named by the Sentinels, and is made again on each new master. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; + + +my %l = map { $_ => redis_listen("203.0.113.$_") } (42, 44); + +sentinels_start('spw', '203.0.113.43' => '203.0.113.42 6379 1', '203.0.113.45' => '203.0.113.42 6379 1'); + +# a missing reply fails the test with the daemon's last log lines +local $SIG{ALRM} = sub { die "timeout\n" }; +# a role probe can give up before it is answered +$SIG{PIPE} = 'IGNORE'; + +my $sdp = <[0] eq 'psubscribe') { + $sub_fd = $fd; + is(join(' ', @$c), 'psubscribe __keyspace@3__:*', 'startup psubscribe'); + redis_send($fd, $psubscribed); + next; + } + is(join(' ', @$c), 'SELECT 2', 'restore copy SELECT'); + redis_send($fd, "+OK\r\n"); + redis_expect($fd, ['INFO'], info_reply('master'), 'restore copy INFO'); + redis_expect($fd, ['TYPE', 'calls'], "+none\r\n", 'restore copy TYPE'); + redis_expect($fd, ['PING'], "+PONG\r\n", 'restore copy PING'); + redis_expect($fd, ['SELECT', '3'], "+OK\r\n", 'restore copy SELECT 3'); + redis_expect($fd, ['KEYS', '*'], "*0\r\n", 'restore copy KEYS'); + redis_expect($fd, ['SELECT', '2'], "+OK\r\n", 'restore copy SELECT back'); + } + # own calls + redis_expect($main_fd, ['PING'], "+PONG\r\n", 'restore PING'); + redis_expect($main_fd, ['KEYS', '*'], "*0\r\n", 'restore KEYS'); +}; + +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 + -n 2223 -f -L 7 -E + --redis=auth@sentinel/2 + --redis-sentinel-master=mymaster --redis-cmd-timeout=1000 + --subscribe-keyspace=3 + --redis-num-threads=1), + '--redis-sentinel=spw@203.0.113.43:26379,203.0.113.45:26379') + or die; + +is_deeply(\@probes, [['203.0.113.42', 'auth', 'master']], 'one role probe at startup'); + +new_call; +$NGCP::Rtpengine::req_cb = sub { redis_expect_set($main_fd, cid(), "+OK\r\n", 'first write') }; +offer('first write on .42', { }, $sdp); + + +# The server closes the subscription, the master is unchanged: the event +# loop must end (nothing, the timer included, may keep it running), and the +# notification thread checks its connection and subscribes again. + +redis_close($sub_fd); +redis_expect($notify_fd, ['PING'], "+PONG\r\n", 'subscription closed: connection checked'); +$sub_fd = subscription($l{42}, 'subscription closed: again on .42'); + + +# Failover, then back: each time, the old subscription and its sync +# connection are closed, both are made again on the new master, and the next +# write goes there, with nothing sent to the old master first. + +for my $step ([44, 2, 'failover'], [42, 3, 'failback']) { + my ($to, $epoch, $n) = @$step; + sentinels_set('203.0.113.43' => "203.0.113.$to 6379 $epoch", '203.0.113.45' => "203.0.113.$to 6379 $epoch"); + wait_probe($l{$to}, $n); + wait_log(qr/master of 'mymaster' is now 203\.0\.113\.$to:6379 \(config epoch $epoch\)/, "$n: published"); + + redis_closed($sub_fd, "$n: old subscription closed"); + redis_closed($notify_fd, "$n: old notification connection closed"); + $notify_fd = redis_conn($l{$to}, "$n: notify"); + $sub_fd = subscription($l{$to}, "$n: subscription"); + + new_call; + $NGCP::Rtpengine::req_cb = sub { + redis_closed($main_fd, "$n: main connection closed without a write"); + $main_fd = redis_conn($l{$to}, "$n: main"); + redis_expect_set($main_fd, cid(), "+OK\r\n", "$n: write", no_ping => 1); + }; + offer("first write after the $n", { }, $sdp); +} +like(rtpe_log(), qr/the master of 'mymaster' has changed: subscribing again/, 'subscriber change logged'); + +# no stray connection: everything above was scripted +ok(!readable($l{$_}, 0.5), "nothing more on .$_") for (42, 44); + + +undef($NGCP::Rtpengine::req_cb); + +done_testing(); From bf3594d7f4cc6c41575c1ef5bed71be49230794b Mon Sep 17 00:00:00 2001 From: Kevin Binetruy Date: Wed, 14 Oct 2026 10:52:22 +0000 Subject: [PATCH 4/7] MT#55283 redis: add --redis-repair-writes 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. --- daemon/main.c | 9 + daemon/redis.c | 347 ++++++++++++++++++++++-- daemon/statistics.c | 3 + docs/ng_control_protocol.md | 1 + docs/rtpengine.md | 14 + etc/rtpengine.conf | 1 + include/counter_stats_fields.inc | 1 + include/main.h | 1 + include/redis.h | 5 + perl/NGCP/Rtpengine/FakeRedis.pm | 10 +- t/Makefile | 8 + t/auto-daemon-tests-redis-repair-ids.pl | 201 ++++++++++++++ t/auto-daemon-tests-redis-repair.pl | 282 +++++++++++++++++++ t/test-stats.c | 42 ++- 14 files changed, 891 insertions(+), 34 deletions(-) create mode 100755 t/auto-daemon-tests-redis-repair-ids.pl create mode 100755 t/auto-daemon-tests-redis-repair.pl diff --git a/daemon/main.c b/daemon/main.c index f0c671f24..9a61c173a 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -827,6 +827,7 @@ static void options(int *argc, char ***argv, charp_ht templates) { { "redis-subscribe", 0, 0, G_OPTION_ARG_STRING, &redisps_subscribe, "Connect to Redis subscribe database", "[PW@]IP:PORT[/INT]" }, { "redis-resolve-on-reconnect", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_resolve_on_reconnect, "Re-resolve given FQDN on each re-connect to the redis server.", NULL }, { "redis-require-master", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_require_master, "Write only to a Redis master, and drop the connection when the server stops being one", NULL }, + { "redis-repair-writes", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_repair_writes, "Write again the calls whose Redis writes were refused or skipped", NULL }, { "redis-sentinel", 0, 0, G_OPTION_ARG_STRING, &redis_sentinels, "Redis Sentinels to ask for the master, for the roles given as 'sentinel'", "[PW@]IP:PORT[,IP:PORT...]" }, { "redis-sentinel-master", 0, 0, G_OPTION_ARG_STRING, &redis_sentinel_master, "Name of the master monitored by Redis Sentinel", "STRING" }, { "redis-sentinel-interval", 0, 0, G_OPTION_ARG_INT, &redis_sentinel_interval, "How often to ask Redis Sentinel for the master, in milliseconds", "INT" }, @@ -2203,6 +2204,9 @@ int main(int argc, char **argv) { thread_create_looper(media_player_expire, rtpe_config.idle_scheduling, rtpe_config.idle_priority, "cache expiry", rtpe_config.expiry_timer * 1000000LL); + if (rtpe_config.redis_repair_writes) + redis_repair_init(); + if (rtpe_redis && initial_rtpe_config.redis_delete_async) thread_create_detach(redis_delete_async_loop, NULL, "redis async"); @@ -2212,6 +2216,10 @@ int main(int argc, char **argv) { if (rtpe_redis_sentinel) thread_create_detach(redis_sentinel_loop, rtpe_redis_sentinel, "redis sentinel"); + if (rtpe_redis_write && rtpe_config.redis_repair_writes) + // not idle scheduling: it holds r->lock, which signalling waits for + thread_create_detach(redis_repair_loop, NULL, "redis repair"); + do_redis_restore(); if (graphite_is_enabled()) @@ -2287,6 +2295,7 @@ int main(int argc, char **argv) { unfill_initial_rtpe_cfg(&initial_rtpe_config); + redis_repair_free(); // it holds calls call_free(); recording_fs_free(); diff --git a/daemon/redis.c b/daemon/redis.c index ec819a499..2b16225a7 100644 --- a/daemon/redis.c +++ b/daemon/redis.c @@ -122,6 +122,11 @@ static int redis_connect(struct redis *r, int wait, bool resolve); static int json_build_ssrc(struct call_media *, parser_arg arg); static void redis_do_delete(call_t *c, struct redis *r); +static void redis_repair_mark(call_t *c); +static void redis_repair_mark_del(const str *callid, int db, call_t *c); +static bool redis_repair_enabled(void); +static void redis_repair_done(call_t *c); +static void redis_lock_waiters_add(struct redis *r, int n); // mutually exclusive multi-A multi-B lock @@ -256,11 +261,12 @@ static bool redis_is_role_error(const redisReply *rp) { /* called with r->lock held * Returns -1 if the replies could not be read (the context is then unusable), * or after a role error on a connection that requires a master, so that the - * caller drops it. Other error replies keep the connection: a reconnect would + * caller drops it; 1 if a reply was an error (the write was refused); 0 if + * all succeeded. Other error replies keep the connection: a reconnect would * get the same answer. */ static int redis_consume(struct redis *r) { redisReply *rp; - bool role_error = false; + bool role_error = false, refused = false; if (!r->ctx) { ilog(LOG_ERROR, "Unable to consume pipelined replies. No redis context"); @@ -274,6 +280,8 @@ static int redis_consume(struct redis *r) { } r->pipeline--; redis_write_result(r, rp); + if (rp->type == REDIS_REPLY_ERROR) + refused = true; if (redis_is_role_error(rp)) role_error = true; else if (r->sentinel && rp->type == REDIS_REPLY_ERROR && !strncmp(rp->str, "NOREPLICAS", 10)) @@ -287,7 +295,7 @@ static int redis_consume(struct redis *r) { endpoint_print_buf(&r->endpoint)); return -1; } - return 0; + return refused ? 1 : 0; } int redis_set_timeout(struct redis* r, int64_t timeout) { @@ -1116,6 +1124,7 @@ struct redis *redis_new(const endpoint_t *ep, int db, const char *hostname, cons r->sentinel = sentinel; mutex_init(&r->lock); cond_init(&r->reconnect_cond); + cond_init(&r->lock_waiters_cond); if (sentinel) { r->gen = redis_sentinel_get(sentinel, &r->endpoint); @@ -3856,30 +3865,33 @@ static bool redis_key_taken(const str *callid, call_t *c, int db) { return taken; } -void redis_update_onekey(call_t *c, struct redis *r) { +/* called with r->lock held: 1 if written, 0 if there was nothing to write, + * -1 if the call is to be written again (it is then marked for + * --redis-repair-writes) */ +static int redis_write_call(call_t *c, struct redis *r) { unsigned int redis_expires_s; - - if (!r) - return; - if (IS_FOREIGN_CALL(c)) - return; - - LOCK(&r->lock); - // not a call destroyed while this waited (a timer can hold one) - if (!redis_call_live(c)) - return; unsigned int released = r->released; + // coverity[sleep : FALSE] - if (redis_check_conn(r) == REDIS_STATE_DISCONNECTED) - return; - // nor while redis_check_conn() let go of the lock (a reconnect) + if (redis_check_conn(r) == REDIS_STATE_DISCONNECTED) { + // not if destroyed meanwhile: its deletion has dealt with its key + if (r->released == released || redis_call_live(c)) + redis_repair_mark(c); + return -1; + } + // not destroyed while redis_check_conn() let go of the lock (a reconnect) if (r->released != released && !redis_call_live(c)) - return; + return 0; atomic64_set_na(&c->last_redis_update_us, rtpe_now); rwlock_lock_r(&c->master_lock); + if (IS_FOREIGN_CALL(c)) { + rwlock_unlock_r(&c->master_lock); + return 0; + } + redis_expires_s = rtpe_config.redis_expires_secs; c->redis_hosted_db = r->db; @@ -3904,19 +3916,42 @@ void redis_update_onekey(call_t *c, struct redis *r) { g_free(to_free); arena_free(ctx.buffer); - if (consume_ret) + if (consume_ret < 0) goto err; + if (consume_ret > 0) + redis_repair_mark(c); + else if (result.len) + redis_repair_done(c); rwlock_unlock_r(&c->master_lock); - return; + if (consume_ret) + return -1; + return result.len ? 1 : 0; err: + redis_repair_mark(c); if (r->ctx && r->ctx->err) rlog(LOG_ERR, "Redis error: %s", r->ctx->errstr); redisFree(r->ctx); r->ctx = NULL; rwlock_unlock_r(&c->master_lock); + return -1; +} + +void redis_update_onekey(call_t *c, struct redis *r) { + if (!r) + return; + if (IS_FOREIGN_CALL(c)) + return; + + redis_lock_waiters_add(r, 1); // the repair pass lets this go first + LOCK(&r->lock); + redis_lock_waiters_add(r, -1); + // not a call destroyed while this waited (a timer can hold one) + if (!redis_call_live(c)) + return; + redis_write_call(c, r); } /* must be called lock-free */ @@ -3928,6 +3963,7 @@ static void redis_do_delete(call_t *c, struct redis *r) { return; if (delete_async) { + redis_repair_done(c); // the DEL is queued if (c->redis_hosted_db < 0) return; LOCK(&r->async_lock); @@ -3937,33 +3973,51 @@ static void redis_do_delete(call_t *c, struct redis *r) { return; } + redis_lock_waiters_add(r, 1); // the repair pass lets this go first LOCK(&r->lock); + redis_lock_waiters_add(r, -1); // never written: nothing to delete. Decided under the lock, so that a // write of the call already under way comes first and is deleted. - if (c->redis_hosted_db < 0) + if (c->redis_hosted_db < 0) { + redis_repair_done(c); // a skipped write: no DEL either return; + } // a reconnect made by another thread is waited for until it ends, as // when it held the lock: skipped, the DEL would leave the key behind while (r->reconnecting) cond_wait(&r->reconnect_cond, &r->lock); // coverity[sleep : FALSE] - if (redis_check_conn(r) == REDIS_STATE_DISCONNECTED) + if (redis_check_conn(r) == REDIS_STATE_DISCONNECTED) { + if (IS_OWN_CALL(c)) + redis_repair_mark_del(&c->callid, c->redis_hosted_db, c); return; + } // a new call with the same ID owns the key now - if (redis_key_taken(&c->callid, c, c->redis_hosted_db)) + if (redis_key_taken(&c->callid, c, c->redis_hosted_db)) { + redis_repair_done(c); return; + } rwlock_lock_r(&c->master_lock); if (redis_select_db(r, c->redis_hosted_db)) goto err; - if (redis_delete_call_json(c, r)) + int del_ret = redis_delete_call_json(c, r); + if (del_ret < 0) goto err; + if (IS_OWN_CALL(c)) { + if (del_ret > 0) + redis_repair_mark_del(&c->callid, c->redis_hosted_db, c); + else + redis_repair_done(c); + } rwlock_unlock_r(&c->master_lock); return; err: + if (IS_OWN_CALL(c)) + redis_repair_mark_del(&c->callid, c->redis_hosted_db, c); if (r->ctx && r->ctx->err) rlog(LOG_ERR, "Redis error: %s", r->ctx->errstr); redisFree(r->ctx); @@ -3974,8 +4028,10 @@ static void redis_do_delete(call_t *c, struct redis *r) { /* must be called lock-free */ void redis_delete(call_t *c, struct redis *r) { - if (IS_FOREIGN_CALL(c)) + if (IS_FOREIGN_CALL(c)) { + redis_repair_done(c); // marked while it was own return; + } redis_do_delete(c, r); } @@ -3994,3 +4050,244 @@ void redis_wipe(struct redis *r) { return; redisCommandNR(r->ctx, "DEL calls"); } + + + +/* --redis-repair-writes: own calls whose last write (SET or DEL) was not + * stored, by call ID. A call still in the call table is held; a key whose + * call is gone is kept by ID only (a deleted call keeps its memory until its + * last reference is dropped). */ +struct redis_repair_entry { + str *id; /* the key: &call->callid, or a copy */ + call_t *call; /* held, or NULL once the call is gone */ + int db; /* DB of a gone call's key */ +}; + +static void redis_repair_entry_free(struct redis_repair_entry *e) { + if (e->call) + obj_put(e->call); + else + str_free(e->id); + g_free(e); +} + +TYPED_GHASHTABLE(redis_repair_ht, str, struct redis_repair_entry, str_hash, str_equal, NULL, + redis_repair_entry_free) + +static mutex_t redis_repair_lock = MUTEX_STATIC_INIT; +static cond_t redis_repair_cond = COND_STATIC_INIT; +static redis_repair_ht redis_repair_pending; +static unsigned int redis_repair_overflow; + +static bool redis_repair_enabled(void) { + return rtpe_config.redis_repair_writes; +} + +/* from main(), before the threads start */ +void redis_repair_init(void) { + redis_repair_pending = redis_repair_ht_new(); +} + +/* called with redis_repair_lock held: the entry for a new key, unless the + * table is full */ +static bool redis_repair_room(void) { + if (t_hash_table_size(redis_repair_pending) < 100000) + return true; + redis_repair_overflow++; + return false; +} + +/* called with r->lock held: a call in the call table, whose SET is to be + * repaired. It supersedes an older DEL of its ID: if it was never written, + * it takes that key over, as in redis_key_taken(). */ +static void redis_repair_mark(call_t *c) { + if (!redis_repair_enabled()) + return; + struct redis_repair_entry *old = NULL; + { + LOCK(&redis_repair_lock); + struct redis_repair_entry *e = t_hash_table_lookup(redis_repair_pending, &c->callid); + if (e && e->call == c) + return; + if (e) + t_hash_table_steal_extended(redis_repair_pending, &c->callid, NULL, &old); + else if (!redis_repair_room()) + return; + if (old && !old->call && c->redis_hosted_db < 0) + c->redis_hosted_db = old->db; + e = g_new(__typeof(*e), 1); + *e = (__typeof(*e)) { .id = &c->callid, .call = obj_get(c), .db = -1 }; + t_hash_table_insert(redis_repair_pending, e->id, e); + cond_signal(&redis_repair_cond); + } + // outside the lock: the last reference to a call gone without a deletion + if (old) + redis_repair_entry_free(old); +} + +/* called with r->lock held: the DEL of a key whose call is gone (deleted is + * that call, or NULL), unless a call that has the ID now has a SET to + * repair, which supersedes it; that call takes the key over if it was never + * written */ +static void redis_repair_mark_del(const str *callid, int db, call_t *deleted) { + if (!redis_repair_enabled() || db < 0) + return; + LOCK(&redis_repair_lock); + struct redis_repair_entry *e = t_hash_table_lookup(redis_repair_pending, callid); + if (e && e->call && e->call != deleted) { + if (e->call->redis_hosted_db < 0) + e->call->redis_hosted_db = db; + return; + } + if (e && !e->call) { + e->db = db; + cond_signal(&redis_repair_cond); + return; + } + if (e) + t_hash_table_remove(redis_repair_pending, e->id); + else if (!redis_repair_room()) + return; + e = g_new(__typeof(*e), 1); + *e = (__typeof(*e)) { .id = str_dup(callid), .call = NULL, .db = db }; + t_hash_table_insert(redis_repair_pending, e->id, e); + cond_signal(&redis_repair_cond); +} + +/* counts the signalling threads waiting for r->lock (see redis_repair_give_way()) */ +static void redis_lock_waiters_add(struct redis *r, int n) { + if (!redis_repair_enabled()) + return; + atomic64_add(&r->lock_waiters, n); + if (n < 0) + cond_signal(&r->lock_waiters_cond); +} + +/* the call's write was stored, or it has none to make: nothing to repair for + * its ID, unless another call that has it now has a SET to repair */ +static void redis_repair_done(call_t *c) { + if (!redis_repair_enabled()) + return; + LOCK(&redis_repair_lock); + struct redis_repair_entry *e = t_hash_table_lookup(redis_repair_pending, &c->callid); + if (e && (!e->call || e->call == c)) + t_hash_table_remove(redis_repair_pending, &c->callid); +} + +/* called with r->lock held: waiting signalling writes go first, for 100 ms at most */ +static void redis_repair_give_way(struct redis *r) { + int64_t until = now_us() + 100000; + while (atomic64_get(&r->lock_waiters) && now_us() < until) + cond_timedwait(&r->lock_waiters_cond, &r->lock, until); +} + +/* unless a new call has the ID, which is in the call table before its first + * write */ +static int redis_repair_del(struct redis *r, const str *callid, int db) { + LOCK(&r->lock); + redis_repair_give_way(r); + if (redis_key_taken(callid, NULL, db)) + return 0; + unsigned int released = r->released; + // coverity[sleep : FALSE] + if (redis_check_conn(r) == REDIS_STATE_DISCONNECTED) + goto fail; + // or since, while redis_check_conn() let go of the lock + if (r->released != released && redis_key_taken(callid, NULL, db)) + return 0; + if (redis_select_db(r, db)) + goto drop; + redis_pipe(r, "DEL " PB, PBSTR(callid)); + int ret = redis_consume(r); + if (ret < 0) + goto drop; + if (ret > 0) + goto fail; + return 1; + +drop: + if (r->ctx && r->ctx->err) + rlog(LOG_ERR, "Redis error: %s", r->ctx->errstr); + redisFree(r->ctx); + r->ctx = NULL; +fail: + redis_repair_mark_del(callid, db, NULL); + return -1; +} + +/* 1 if written, 0 if there was nothing to write, -1 if to be tried again */ +static int redis_repair_one(struct redis *r, struct redis_repair_entry *e) { + if (e->call) { + LOCK(&r->lock); + redis_repair_give_way(r); + // deleted since it was marked: its deletion took care of its key + if (!redis_call_live(e->call) || !IS_OWN_CALL(e->call)) + return 0; + return redis_write_call(e->call, r); + } + return redis_repair_del(r, e->id, e->db); +} + +void redis_repair_loop(void *d) { + struct redis *r = rtpe_redis_write; + struct thread_waker waker = { .lock = &redis_repair_lock, .cond = &redis_repair_cond }; + int64_t next_us = 0, report_us = 0; + unsigned int repaired = 0; + + pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, NULL); + thread_waker_add(&waker); + + while (!rtpe_shutdown) { + rtpe_now = now_us(); + + unsigned int overflow = 0; + struct redis_repair_entry *e = NULL; + { + LOCK(&redis_repair_lock); + unsigned int pending = t_hash_table_size(redis_repair_pending); + if ((repaired || redis_repair_overflow) && rtpe_now - report_us >= 1000000) { + overflow = redis_repair_overflow; + redis_repair_overflow = 0; + rlog(LOG_NOTICE, "Redis: %u writes repaired, %u still pending%s", + repaired, pending, + overflow ? " (some calls could not be queued: queue full)" : ""); + repaired = 0; + report_us = rtpe_now; + } + if (rtpe_now >= next_us && pending) { + __auto_type it = t_hash_table_iter(redis_repair_pending); + str *id; + if (t_hash_table_iter_next(&it, &id, &e)) + t_hash_table_steal_extended(redis_repair_pending, id, NULL, NULL); + } + if (!e) { + int64_t until = rtpe_now + 1000000; + if (next_us > rtpe_now) + until = MIN(until, next_us); + cond_timedwait(&redis_repair_cond, &redis_repair_lock, until); + continue; + } + } + + int ret = redis_repair_one(r, e); + if (ret > 0) { + repaired++; + RTPE_STATS_INC(redis_repaired_writes); + } + else if (ret < 0) + next_us = now_us() + 1000000; // marked again + redis_repair_entry_free(e); + } + + thread_waker_del(&waker); +} + +/* at shutdown, after the threads are gone and before the calls are freed */ +void redis_repair_free(void) { + if (!t_hash_table_is_set(redis_repair_pending)) + return; + unsigned int n = t_hash_table_size(redis_repair_pending); + if (n) + ilog(LOG_WARN, "Redis: %u writes not repaired at shutdown", n); + t_hash_table_destroy_ptr(&redis_repair_pending); +} diff --git a/daemon/statistics.c b/daemon/statistics.c index 75996e055..3095fb1b6 100644 --- a/daemon/statistics.c +++ b/daemon/statistics.c @@ -485,6 +485,9 @@ stats_metric_q *statistics_gather_metrics(struct interface_sampled_rate_stats *i METRIC("rediswriteerrors", "Total Redis writes refused by the server", "%" PRIu64, "%" PRIu64, atomic64_get_na(&rtpe_stats->redis_write_errors)); PROM("redis_write_errors_total", "counter"); + METRIC("redisrepairedwrites", "Total Redis writes repaired", "%" PRIu64, "%" PRIu64, + atomic64_get_na(&rtpe_stats->redis_repaired_writes)); + PROM("redis_repaired_writes_total", "counter"); METRICva("avgcallduration", "Average call duration", "%.6f", "%.6f seconds", (double) avg_us / 1000000.0); PROM("call_duration_avg", "gauge"); diff --git a/docs/ng_control_protocol.md b/docs/ng_control_protocol.md index 25e85e6e0..801f66056 100644 --- a/docs/ng_control_protocol.md +++ b/docs/ng_control_protocol.md @@ -2673,6 +2673,7 @@ command. Sample return dictionary: "zerowaystreams": 0, "onewaystreams": 0, "rediswriteerrors": 0, + "redisrepairedwrites": 0, "avgcallduration": "0.000000" }, "intervalstatistics": { diff --git a/docs/rtpengine.md b/docs/rtpengine.md index 42b4d5278..b42426cd0 100644 --- a/docs/rtpengine.md +++ b/docs/rtpengine.md @@ -733,6 +733,20 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- remote redis server try to re-resolve if the redis hostname was an FQDN and not IP address. It is a boolean value (either __true__ or __false__). +- __\-\-redis-repair-writes__ + + Write again, in the background, the own calls whose last Redis write + was not stored: refused by the server (for example __NOREPLICAS__ after + a failover), lost with the connection, or skipped while the connection + was down or paused. A call that still exists is written with its + current state; for a call that is gone, its key is deleted, unless a new + call uses the same ID. Signalling writes go first. A write that fails + again is retried after one second, and at most 100000 calls are kept. + Each stored write is counted in the __redisrepairedwrites__ statistic. + The deletions assume that this instance is the only writer of its + database, or that call-ids are not reused. It is a boolean value + (either __true__ or __false__), and it is off by default. + - __\-\-redis-sentinel=__\[*PW*@\]*IP*:*PORT*\[,*IP*:*PORT*...\] The Redis Sentinels to ask for the master, for every Redis option given diff --git a/etc/rtpengine.conf b/etc/rtpengine.conf index 7206561cd..e230519fa 100644 --- a/etc/rtpengine.conf +++ b/etc/rtpengine.conf @@ -85,6 +85,7 @@ recording-method = proc # redis-reconnect-slow = 100 # redis-resolve-on-reconnect = false # redis-require-master = false +# redis-repair-writes = false # redis-write = password@sentinel/42 # redis-sentinel = password@10.0.0.11:26379,10.0.0.12:26379,10.0.0.13:26379 # redis-sentinel-master = mymaster diff --git a/include/counter_stats_fields.inc b/include/counter_stats_fields.inc index 3efee75bf..decc947e2 100644 --- a/include/counter_stats_fields.inc +++ b/include/counter_stats_fields.inc @@ -22,3 +22,4 @@ F(rtp_skips) F(rtp_seq_resets) F(rtp_reordered) F(redis_write_errors) +F(redis_repaired_writes) diff --git a/include/main.h b/include/main.h index 817e548e2..945079578 100644 --- a/include/main.h +++ b/include/main.h @@ -141,6 +141,7 @@ enum endpoint_learning { X(poller_per_thread) \ X(redis_resolve_on_reconnect) \ X(redis_require_master) \ + X(redis_repair_writes) \ X(redis_via_sentinel) \ X(redis_write_via_sentinel) \ X(redis_subscribe_via_sentinel) \ diff --git a/include/redis.h b/include/redis.h index 44ccebaf1..9f6dba608 100644 --- a/include/redis.h +++ b/include/redis.h @@ -83,6 +83,8 @@ struct redis { bool update_resolve; bool require_master; /* a role error drops the connection */ + atomic64 lock_waiters; /* signalling writes waiting for lock */ + cond_t lock_waiters_cond; /* signalled when the last one has it */ struct redis_sentinel *sentinel; /* address from Redis Sentinel, or NULL */ uint64_t gen; /* generation of the master in endpoint */ @@ -138,6 +140,9 @@ int redis_notify_subscribe_action(struct redis *r, enum subscribe_action action, int redis_set_timeout(struct redis* r, int64_t timeout); int redis_set_cmd_timeout(struct redis *r, int64_t timeout); int redis_reconnect(struct redis* r); +void redis_repair_init(void); +void redis_repair_loop(void *); +void redis_repair_free(void); #endif diff --git a/perl/NGCP/Rtpengine/FakeRedis.pm b/perl/NGCP/Rtpengine/FakeRedis.pm index 552d409fc..07a5d3b23 100644 --- a/perl/NGCP/Rtpengine/FakeRedis.pm +++ b/perl/NGCP/Rtpengine/FakeRedis.pm @@ -20,7 +20,7 @@ use Exporter 'import'; our @EXPORT = qw(redis_listen redis_accept redis_conn redis_read redis_send redis_expect redis_expect_set redis_expect_del redis_close redis_closed readable info_reply - redis_write_errors wait_log + redis_write_errors redis_stat wait_log redis_serve redis_sets_seen redis_record_after redis_serve_next_get %role @probes wait_probe serve_probes sentinels_start sentinels_set sentinel_log sentinel_queries); @@ -341,9 +341,15 @@ sub redis_serve_next_get { } sub redis_write_errors { + return redis_stat('rediswriteerrors'); +} + +# a counter of the "statistics" command's totals +sub redis_stat { + my ($name) = @_; local $NGCP::Rtpengine::req_cb; my $resp = rtpe_req('statistics', 'statistics', { }); - return $resp->{statistics}{totalstatistics}{rediswriteerrors}; + return $resp->{statistics}{totalstatistics}{$name}; } sub wait_log { diff --git a/t/Makefile b/t/Makefile index ca15e1815..1e439adc1 100644 --- a/t/Makefile +++ b/t/Makefile @@ -93,6 +93,7 @@ include ../lib/common.Makefile daemon-tests-redis-require-master \ daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ daemon-tests-redis-sentinel-subscribe \ + daemon-tests-redis-repair daemon-tests-redis-repair-ids \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions daemon-tests-alias \ daemon-tests-kernel-api daemon-tests-dtmf-fanout \ daemon-tests-video daemon-tests-inject daemon-tests-dtx-dtmf @@ -150,6 +151,7 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests daemon-tests-redis-require-master \ daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ daemon-tests-redis-sentinel-subscribe \ + daemon-tests-redis-repair daemon-tests-redis-repair-ids \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions \ daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-dtx-no-shift daemon-tests-dtx-dtmf \ daemon-tests-alias \ @@ -270,6 +272,12 @@ daemon-tests-redis-sentinel-start: daemon-test-deps daemon-tests-redis-sentinel-subscribe: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-sentinel-subscribe.pl +daemon-tests-redis-repair: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-repair.pl + +daemon-tests-redis-repair-ids: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-repair-ids.pl + daemon-tests-audio-player: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-audio-player.pl diff --git a/t/auto-daemon-tests-redis-repair-ids.pl b/t/auto-daemon-tests-redis-repair-ids.pl new file mode 100755 index 000000000..aeea9c189 --- /dev/null +++ b/t/auto-daemon-tests-redis-repair-ids.pl @@ -0,0 +1,201 @@ +#!/usr/bin/perl + +# --redis-repair-writes and call IDs: a call deleted while its writes are +# refused or skipped, and a new call that takes its ID. The DEL of the old +# key is repaired unless the new call owns the key, which it takes over if +# it was never written. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; +use Time::HiRes qw(time sleep); +use NGCP::Rtpengine; +use Bencode; + + +my $listener = redis_listen('203.0.113.42'); +my $fd; + +# a missing reply fails the test with the daemon's last log lines +local $SIG{ALRM} = sub { die "timeout\n" }; + +my $noreplicas = "-NOREPLICAS Not enough good replicas to write.\r\n"; + +# nothing arrives on the Redis connection for this long +sub quiet { + my ($secs, $n) = @_; + ok(!readable($fd, $secs), $n); +} +# something arrives within this long +sub soon { + my ($secs, $n) = @_; + ok(readable($fd, $secs), $n) or die "nothing on the Redis connection: $n\n"; +} +sub req { + my ($cb, $req, @args) = @_; + $NGCP::Rtpengine::req_cb = $cb; + $req->(@args); + undef($NGCP::Rtpengine::req_cb); +} + +my $sdp = < 0 }); +my %got; +soon(2, 'repairs after the back-off'); +while (readable($fd, 0.5)) { + my $cmd = redis_read($fd) or last; + redis_send($fd, $cmd->[0] eq 'PING' ? "+PONG\r\n" : $cmd->[0] eq 'DEL' ? ":1\r\n" : "+OK\r\n"); + $got{"$cmd->[0] $cmd->[1]"} = 1 if $cmd->[0] ne 'PING'; +} +is_deeply(\%got, { "SET $k_id" => 1, "DEL $h_id" => 1 }, 'K written, and the DEL of H repaired'); + + +# During the back-off after L's refused repair: P is stored, and its +# deletion waits for X's attempt (AUTH held); meanwhile a new call takes P's +# ID, and its write, which waits only 100 ms, is skipped. The old P's DEL +# leaves the key to the new P, and the new P's repair is kept. + +new_call; +my $l_id = cid(); +req(sub { redis_expect_set($fd, $l_id, $noreplicas, 'L refused') }, \&offer, 'L refused', { }, $sdp); +soon(2, 'repair attempt of L'); +redis_expect_set($fd, $l_id, $noreplicas, 'repair attempt of L, refused'); +new_call; +my $p_id = cid(); +req(sub { redis_expect_set($fd, $p_id, "+OK\r\n", 'P stored') }, \&offer, 'P', { }, $sdp); +redis_close($fd); +new_call; +my $x_id = cid(); +req(sub { + $fd = redis_accept($listener, 3); + redis_expect($fd, ['AUTH', 'auth'], undef, 'X reconnect AUTH (reply held)'); + my $del = rtpe_req_async({ command => 'delete', 'call-id' => $p_id, 'delete delay' => 0 }); + sleep(0.05); + # on a third socket, without the request callback + my $c3 = NGCP::Rtpengine->new('127.0.0.1', 2223); + $c3->{socket}->send('p2 ' . Bencode::bencode({ command => 'offer', 'call-id' => $p_id, + 'from-tag' => 'p2', sdp => $offer_sdp }), 0) or die $!; + my $ret; + $c3->{socket}->recv($ret, 65535); + like($ret, qr/result.*ok/, 'new P offered, its write skipped'); + redis_send($fd, "+OK\r\n"); + redis_expect($fd, ['SELECT', '2'], "+OK\r\n", 'X reconnect SELECT'); + redis_expect($fd, ['INFO'], info_reply('master'), 'X reconnect INFO'); + redis_expect($fd, ['TYPE', 'calls'], "+none\r\n", 'X reconnect TYPE'); + redis_expect_set($fd, $x_id, "+OK\r\n", 'X written after the reconnect', no_ping => 1); + my %seen; + my $end = time() + 3; + while (time() < $end && readable($fd, $end - time())) { + my $cmd = redis_read($fd) or last; + redis_send($fd, $cmd->[0] eq 'PING' ? "+PONG\r\n" : $cmd->[0] eq 'DEL' ? ":1\r\n" : "+OK\r\n"); + $seen{"$cmd->[0] $cmd->[1]"} = 1 if $cmd->[0] ne 'PING'; + last if $seen{"SET $p_id"} && $seen{"SET $l_id"}; + } + ok(!$seen{"DEL $p_id"}, 'the old P not deleted: the ID is taken'); + ok($seen{"SET $p_id"}, 'the new P written by the repair'); + ok($seen{"SET $l_id"}, 'L written by the repair'); + is($del->()->{result}, 'ok', 'old P deletion answered'); +}, \&offer, 'X reconnects', { }, $sdp); + + +# A long outage: A's DEL is refused; then, while the connection is paused +# (the server came back as a replica), a new call B takes A's ID and is +# deleted before anything of it is stored. Once the server is a master +# again, the repair deletes A's key and writes nothing of B. + +new_call; +my $ab_id = cid(); +req(sub { redis_expect_set($fd, $ab_id, "+OK\r\n", 'A stored') }, \&offer, 'A', { }, $sdp); +req(sub { redis_expect_del($fd, $ab_id, $noreplicas, 'A DEL refused') }, \&rtpe_req, 'delete', + 'delete A, refused', { 'delete delay' => 0 }); +soon(2, 'A DEL repair attempt'); +redis_expect_del($fd, $ab_id, $noreplicas, 'A DEL repair attempt, refused'); +redis_close($fd); +req(sub { + $fd = redis_conn($listener, 'replica, B', role => 'slave', refused => 1); + redis_closed($fd, 'replica, B: connection closed'); +}, \&offer, 'B with the ID of A, while the node is a replica', { }, $sdp); +rtpe_req('delete', 'delete B, never stored', { 'delete delay' => 0 }); +ok(readable($listener, 4), 'repair reconnects') or die; +$fd = redis_conn($listener, 'master again, A and B'); +redis_expect_del($fd, $ab_id, ":1\r\n", "A's key deleted", no_ping => 1); +quiet(1.2, 'nothing of B written'); + + +# The same, with the pass's attempt of B's entry falling in the pause: B's +# write is skipped again and B is marked afresh before it is deleted, never +# stored. Once the server is a master again, the repair deletes A's key. + +new_call; +my $ac_id = cid(); +req(sub { redis_expect_set($fd, $ac_id, "+OK\r\n", 'A2 stored') }, \&offer, 'A2', { }, $sdp); +req(sub { redis_expect_del($fd, $ac_id, $noreplicas, 'A2 DEL refused') }, \&rtpe_req, 'delete', + 'delete A2, refused', { 'delete delay' => 0 }); +soon(2, 'A2 DEL repair attempt'); +redis_expect_del($fd, $ac_id, $noreplicas, 'A2 DEL repair attempt, refused'); +redis_close($fd); +req(sub { + $fd = redis_conn($listener, 'replica, B2', role => 'slave', refused => 1); + redis_closed($fd, 'replica, B2: connection closed'); +}, \&offer, 'B2 with the ID of A2, while the node is a replica', { }, $sdp); +sleep(1.3); # the pass's attempt of B2's entry falls in the pause +rtpe_req('delete', 'delete B2, never stored', { 'delete delay' => 0 }); +ok(readable($listener, 4), 'repair reconnects for A2') or die "nothing to repair: A2's DEL lost\n"; +$fd = redis_conn($listener, 'master again, A2 and B2'); +redis_expect_del($fd, $ac_id, ":1\r\n", "A2's key deleted", no_ping => 1); +quiet(1.2, 'nothing of B2 written'); +# K, the DEL of H, L, the new P, the DEL of A and the DEL of A2 +is(redis_stat('redisrepairedwrites'), 6, 'repaired writes counted'); + + +done_testing(); diff --git a/t/auto-daemon-tests-redis-repair.pl b/t/auto-daemon-tests-redis-repair.pl new file mode 100755 index 000000000..25ae84bb4 --- /dev/null +++ b/t/auto-daemon-tests-redis-repair.pl @@ -0,0 +1,282 @@ +#!/usr/bin/perl + +# --redis-repair-writes: an own call's write that was not stored (refused by +# the server, or skipped while the connection was down) is written again by +# a background pass, once writes succeed; a refused DEL is retried as a DEL, +# unless the call-id is in use again. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; +use Time::HiRes qw(time sleep); + + +my $listener = redis_listen('203.0.113.42'); +my $fd; + +# a missing reply fails the test with the daemon's last log lines +local $SIG{ALRM} = sub { die "timeout\n" }; + +my $noreplicas = "-NOREPLICAS Not enough good replicas to write.\r\n"; + +# nothing arrives on the Redis connection for this long +sub quiet { + my ($secs, $n) = @_; + ok(!readable($fd, $secs), $n); +} +# something arrives within this long +sub soon { + my ($secs, $n) = @_; + ok(readable($fd, $secs), $n) or die "nothing on the Redis connection: $n\n"; +} +sub req { + my ($cb, $req, @args) = @_; + $NGCP::Rtpengine::req_cb = $cb; + $req->(@args); + undef($NGCP::Rtpengine::req_cb); +} + +my $sdp = <= 0.9, 'second repair after about a second'); +redis_expect_set($fd, $b_id, "+OK\r\n", 'second repair'); +quiet(0.5, 'nothing more'); +is(redis_stat('redisrepairedwrites'), 2, 'only the stored repair counted'); + + +# a newer write of the same call that is stored clears the repair + +new_call; +my $c_id = cid(); +req(sub { redis_expect_set($fd, $c_id, $noreplicas, 'refused offer') }, \&offer, 'refused offer', { }, $sdp); +soon(2, 'repair attempt'); +redis_expect_set($fd, $c_id, $noreplicas, 'repair attempt, refused'); +# within the back-off, the call's own next write is stored +req(sub { redis_expect_set($fd, $c_id, "+OK\r\n", 'answer stored') }, \&answer, 'answer stored', { }, < 0 }); +soon(2, 'DEL repair comes'); +redis_expect_del($fd, $d_id, ":1\r\n", 'repair'); +quiet(0.5, 'nothing more after the DEL'); + + +# a refused DEL, then a new call with the same ID whose write is refused +# too: the repair writes the new call (SET), it never deletes its key + +req(sub { redis_expect_set($fd, $d_id, "+OK\r\n", 'same ID again') }, \&offer, 'call to delete, again', { }, $sdp); +req(sub { redis_expect_del($fd, $d_id, $noreplicas, 'refused again') }, \&rtpe_req, 'delete', + 'refused delete again', { 'delete delay' => 0 }); +soon(2, 'DEL repair attempt'); +redis_expect_del($fd, $d_id, $noreplicas, 'repair attempt, refused'); +req(sub { redis_expect_set($fd, $d_id, $noreplicas, 'new call, same ID, refused') }, \&offer, + 'new call with the same ID', { }, $sdp); +soon(2, 'repair of the new call'); +redis_expect_set($fd, $d_id, "+OK\r\n", 'the key is in use again: SET, not DEL'); +quiet(0.5, 'nothing more'); + + +# writes skipped while the connection is paused (the server came back as a +# replica) are written once it is a master again + +redis_close($fd); +new_call; +my $f_id = cid(); +req(sub { + $fd = redis_conn($listener, 'replica', role => 'slave', refused => 1); + redis_closed($fd, 'replica: connection closed'); +}, \&offer, 'write while the node is a replica', { }, $sdp); +ok(readable($listener, 3), 'repair reconnects') or die; +$fd = redis_conn($listener, 'master again'); +redis_expect_set($fd, $f_id, "+OK\r\n", 'skipped write repaired', no_ping => 1); +quiet(0.5, 'nothing more'); + + +# a call whose only write was skipped, deleted before the repair: nothing to +# repair, not even a DEL (no SET of it was ever sent) + +redis_close($fd); +new_call; +req(sub { + $fd = redis_conn($listener, 'replica, S', role => 'slave', refused => 1); + redis_closed($fd, 'replica, S: connection closed'); +}, \&offer, 'S while the node is a replica', { }, $sdp); +rtpe_req('delete', 'delete S, never stored', { 'delete delay' => 0 }); +ok(!readable($listener, 1.5), 'never-stored call deleted: no repair, no DEL'); + + +# A call that was never stored is deleted while the repair pass reconnects +# to write it (its AUTH is held). The reconnect does not hold the lock, so +# the deletion finds "never stored" at once; back under the lock, the +# repair finds the call gone and writes nothing. + +redis_close($fd); +new_call; +my $r_id = cid(); +req(sub { + $fd = redis_conn($listener, 'replica again', role => 'slave', refused => 1); + redis_closed($fd, 'replica again: connection closed'); +}, \&offer, 'R while the node is a replica', { }, $sdp); +$fd = redis_accept($listener, 3); +redis_expect($fd, ['AUTH', 'auth'], undef, 'repair AUTH (reply held)'); +rtpe_req('delete', 'delete R while the repair reconnects', { 'delete delay' => 0 }); +redis_send($fd, "+OK\r\n"); +redis_expect($fd, ['SELECT', '2'], "+OK\r\n", 'repair SELECT'); +redis_expect($fd, ['INFO'], info_reply('master'), 'repair INFO'); +redis_expect($fd, ['TYPE', 'calls'], "+none\r\n", 'repair TYPE'); +quiet(0.5, 'R neither written nor deleted'); + + +# A refused DEL of Z is retried by the repair pass, whose reconnect is held +# at its AUTH; meanwhile a new call takes Z's ID (its write waits, then is +# skipped). Back under the lock, the repair leaves Z's key to the new call: +# no DEL, then the new call's skipped write is repaired. + +new_call; +my $z_id = cid(); +req(sub { redis_expect_set($fd, $z_id, "+OK\r\n", 'Z stored') }, \&offer, 'Z', { }, $sdp); +req(sub { redis_expect_del($fd, $z_id, $noreplicas, 'Z DEL refused') }, \&rtpe_req, 'delete', + 'delete Z, refused', { 'delete delay' => 0 }); +soon(2, 'Z DEL repair attempt'); +redis_expect_del($fd, $z_id, $noreplicas, 'Z DEL repair attempt, refused'); +redis_close($fd); +$fd = redis_accept($listener, 3); +redis_expect($fd, ['AUTH', 'auth'], undef, 'repair AUTH (reply held)'); +req(undef, \&offer, 'new Z while the repair reconnects', { }, $sdp); +redis_send($fd, "+OK\r\n"); +redis_expect($fd, ['SELECT', '2'], "+OK\r\n", 'repair SELECT'); +redis_expect($fd, ['INFO'], info_reply('master'), 'repair INFO'); +redis_expect($fd, ['TYPE', 'calls'], "+none\r\n", 'repair TYPE'); +soon(2, 'repair of the new Z'); +redis_expect_set($fd, $z_id, "+OK\r\n", 'the new Z written, the key not deleted'); +quiet(0.5, 'nothing more after Z'); + + +# Signalling goes first: an offer that waits for the lock while a repair +# write waits for its reply is written before the next repair write. +# (without the give-way, the order is a race between the two threads) + +redis_close($fd); +my %q; +new_call; +$q{cid()} = 1; +req(sub { + $fd = redis_conn($listener, 'replica, Q', role => 'slave', refused => 1); + redis_closed($fd, 'replica, Q: connection closed'); +}, \&offer, 'Q1 while the node is a replica', { }, $sdp); +new_call; +$q{cid()} = 1; +offer('Q2, skipped in the pause', { }, $sdp); +$fd = redis_conn($listener, 'master again, Q'); +my $c = redis_read($fd); +ok($c && $c->[0] eq 'SET' && delete($q{$c->[1]}), 'first repair write (reply held)'); +new_call; +my $n_id = cid(); +req(sub { + sleep(0.3); # the offer now waits for the lock + redis_send($fd, "+OK\r\n"); + redis_expect_set($fd, $n_id, "+OK\r\n", 'the offer before the next repair write'); +}, \&offer, 'offer during the repair', { }, $sdp); +redis_expect($fd, ['PING'], "+PONG\r\n", 'repair PING'); +$c = redis_read($fd); +ok($c && $c->[0] eq 'SET' && delete($q{$c->[1]}), 'repair after the offer'); +redis_send($fd, "+OK\r\n"); + + +# a call that becomes foreign (CLI "standby") is not repaired + +new_call; +my $g_id = cid(); +req(sub { redis_expect_set($fd, $g_id, $noreplicas, 'G refused') }, \&offer, 'G refused', { }, $sdp); +soon(2, 'repair attempt of G'); +redis_expect_set($fd, $g_id, $noreplicas, 'repair attempt of G, refused'); +my $resp = rtpe_req('cli', 'standby', { body => 'standby' }); +like($resp->{response}, qr/all calls set to 'foreign/, 'all calls foreign'); +quiet(1.2, 'foreign call: no repair'); +# a, b, the DEL of d, the SET of d's new call, f, the new Z, Q1 and Q2 +is(redis_stat('redisrepairedwrites'), 8, 'repaired writes counted'); + + +done_testing(); diff --git a/t/test-stats.c b/t/test-stats.c index affd502c2..736907551 100644 --- a/t/test-stats.c +++ b/t/test-stats.c @@ -548,6 +548,10 @@ int main(void) { "rediswriteerrors\n" "0\n" "0\n" + "Total Redis writes repaired\n" + "redisrepairedwrites\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -1394,7 +1398,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "480\n" + "488\n" "}\n" "]\n" "}\n" @@ -1866,6 +1870,10 @@ int main(void) { "rediswriteerrors\n" "0\n" "0\n" + "Total Redis writes repaired\n" + "redisrepairedwrites\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -2712,7 +2720,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "480\n" + "488\n" "}\n" "]\n" "}\n" @@ -3181,6 +3189,10 @@ int main(void) { "rediswriteerrors\n" "0\n" "0\n" + "Total Redis writes repaired\n" + "redisrepairedwrites\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -4027,7 +4039,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "480\n" + "488\n" "}\n" "]\n" "}\n" @@ -4515,6 +4527,10 @@ int main(void) { "rediswriteerrors\n" "0\n" "0\n" + "Total Redis writes repaired\n" + "redisrepairedwrites\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -5361,7 +5377,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "480\n" + "488\n" "}\n" "]\n" "}\n" @@ -5838,6 +5854,10 @@ int main(void) { "rediswriteerrors\n" "0\n" "0\n" + "Total Redis writes repaired\n" + "redisrepairedwrites\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -6684,7 +6704,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "480\n" + "488\n" "}\n" "]\n" "}\n" @@ -7155,6 +7175,10 @@ int main(void) { "rediswriteerrors\n" "0\n" "0\n" + "Total Redis writes repaired\n" + "redisrepairedwrites\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -8001,7 +8025,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "480\n" + "488\n" "}\n" "]\n" "}\n" @@ -8475,6 +8499,10 @@ int main(void) { "rediswriteerrors\n" "0\n" "0\n" + "Total Redis writes repaired\n" + "redisrepairedwrites\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "93.000000 seconds\n" @@ -9321,7 +9349,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "480\n" + "488\n" "}\n" "]\n" "}\n" From ec0db66884d2cc9593235dba93e9191ff32fe8a8 Mon Sep 17 00:00:00 2001 From: Kevin Binetruy Date: Wed, 14 Oct 2026 10:52:22 +0000 Subject: [PATCH 5/7] MT#55283 redis: add --redis-resync-on-master-change 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. --- daemon/main.c | 31 +++- daemon/redis.c | 128 ++++++++++++++- daemon/statistics.c | 9 ++ docs/ng_control_protocol.md | 3 + docs/rtpengine.md | 31 ++++ etc/rtpengine.conf | 3 + include/counter_stats_fields.inc | 3 + include/main.h | 3 + include/redis.h | 1 + t/Makefile | 5 + t/auto-daemon-tests-redis-resync.pl | 237 ++++++++++++++++++++++++++++ t/test-stats.c | 98 +++++++++++- 12 files changed, 539 insertions(+), 13 deletions(-) create mode 100755 t/auto-daemon-tests-redis-resync.pl diff --git a/daemon/main.c b/daemon/main.c index 9a61c173a..f68b1a7a6 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -733,6 +733,8 @@ static void options(int *argc, char ***argv, charp_ht templates) { g_autoptr(char) redis_sentinels = NULL; g_autoptr(char) redis_sentinel_master = NULL; int redis_sentinel_interval = -1; + int redis_resync_delete_window = -1; + int redis_resync_delete_max = -1; g_autoptr(char) log_facility_cdr_s = NULL; g_autoptr(char) log_facility_rtcp_s = NULL; g_autoptr(char) log_facility_dtmf_s = NULL; @@ -828,6 +830,9 @@ static void options(int *argc, char ***argv, charp_ht templates) { { "redis-resolve-on-reconnect", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_resolve_on_reconnect, "Re-resolve given FQDN on each re-connect to the redis server.", NULL }, { "redis-require-master", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_require_master, "Write only to a Redis master, and drop the connection when the server stops being one", NULL }, { "redis-repair-writes", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_repair_writes, "Write again the calls whose Redis writes were refused or skipped", NULL }, + { "redis-resync-on-master-change", 0,0, G_OPTION_ARG_NONE, &rtpe_config.redis_resync_on_master_change, "After a Redis master change, write own calls and recent deletions again", NULL }, + { "redis-resync-delete-window", 0, 0, G_OPTION_ARG_INT, &redis_resync_delete_window, "How long, in milliseconds, own deletions are remembered for a Redis resync", "INT" }, + { "redis-resync-delete-max", 0, 0, G_OPTION_ARG_INT, &redis_resync_delete_max, "How many own deletions are remembered for a Redis resync, at most", "INT" }, { "redis-sentinel", 0, 0, G_OPTION_ARG_STRING, &redis_sentinels, "Redis Sentinels to ask for the master, for the roles given as 'sentinel'", "[PW@]IP:PORT[,IP:PORT...]" }, { "redis-sentinel-master", 0, 0, G_OPTION_ARG_STRING, &redis_sentinel_master, "Name of the master monitored by Redis Sentinel", "STRING" }, { "redis-sentinel-interval", 0, 0, G_OPTION_ARG_INT, &redis_sentinel_interval, "How often to ask Redis Sentinel for the master, in milliseconds", "INT" }, @@ -1362,6 +1367,28 @@ static void options(int *argc, char ***argv, charp_ht templates) { die("--redis-sentinel, --redis-sentinel-master or --redis-sentinel-interval is set, " "but no Redis endpoint is 'sentinel'"); + // the trigger is the write connection's INFO, which gives the master's ID + // on a replica too: that connection must require the master role + if (rtpe_config.redis_resync_on_master_change && redisps_write + && !rtpe_config.redis_require_master && !rtpe_config.redis_write_via_sentinel) + die("--redis-resync-on-master-change needs a write connection that requires the master " + "role: add --redis-require-master, or use 'sentinel' in --redis-write"); + if (rtpe_config.redis_resync_on_master_change) { + if (redis_resync_delete_window == -1) + redis_resync_delete_window = 30000; + if (redis_resync_delete_max == -1) + redis_resync_delete_max = 4096; + if (redis_resync_delete_window <= 0) + die("Invalid --redis-resync-delete-window (must be a positive number of milliseconds)"); + if (redis_resync_delete_max <= 0) + die("Invalid --redis-resync-delete-max (must be positive)"); + rtpe_config.redis_resync_delete_window = redis_resync_delete_window; + rtpe_config.redis_resync_delete_max = redis_resync_delete_max; + } + else if (redis_resync_delete_window != -1 || redis_resync_delete_max != -1) + die("--redis-resync-delete-window or --redis-resync-delete-max is set, " + "but not --redis-resync-on-master-change"); + if (rtpe_config.fmt < 0 || rtpe_config.fmt > 2) die("Invalid XMLRPC format"); @@ -2204,7 +2231,7 @@ int main(int argc, char **argv) { thread_create_looper(media_player_expire, rtpe_config.idle_scheduling, rtpe_config.idle_priority, "cache expiry", rtpe_config.expiry_timer * 1000000LL); - if (rtpe_config.redis_repair_writes) + if (rtpe_config.redis_repair_writes || rtpe_config.redis_resync_on_master_change) redis_repair_init(); if (rtpe_redis && initial_rtpe_config.redis_delete_async) @@ -2216,7 +2243,7 @@ int main(int argc, char **argv) { if (rtpe_redis_sentinel) thread_create_detach(redis_sentinel_loop, rtpe_redis_sentinel, "redis sentinel"); - if (rtpe_redis_write && rtpe_config.redis_repair_writes) + if (rtpe_redis_write && (rtpe_config.redis_repair_writes || rtpe_config.redis_resync_on_master_change)) // not idle scheduling: it holds r->lock, which signalling waits for thread_create_detach(redis_repair_loop, NULL, "redis repair"); diff --git a/daemon/redis.c b/daemon/redis.c index 2b16225a7..32c11362c 100644 --- a/daemon/redis.c +++ b/daemon/redis.c @@ -127,6 +127,8 @@ static void redis_repair_mark_del(const str *callid, int db, call_t *c); static bool redis_repair_enabled(void); static void redis_repair_done(call_t *c); static void redis_lock_waiters_add(struct redis *r, int n); +static void redis_resync_note_replid(struct redis *r, const char *info); +static void redis_resync_note_del(const str *callid, int db); // mutually exclusive multi-A multi-B lock @@ -495,6 +497,7 @@ static int redis_connect(struct redis *r, int wait, bool resolve) { goto err2; done: + redis_resync_note_replid(r, rp->str); freeReplyObject(rp); redis_check_type(r, "calls", NULL, "set"); return 0; @@ -3982,6 +3985,8 @@ static void redis_do_delete(call_t *c, struct redis *r) { redis_repair_done(c); // a skipped write: no DEL either return; } + if (IS_OWN_CALL(c)) + redis_resync_note_del(&c->callid, c->redis_hosted_db); // a reconnect made by another thread is waited for until it ends, as // when it held the lock: skipped, the DEL would leave the key behind while (r->reconnecting) @@ -4080,7 +4085,7 @@ static redis_repair_ht redis_repair_pending; static unsigned int redis_repair_overflow; static bool redis_repair_enabled(void) { - return rtpe_config.redis_repair_writes; + return rtpe_config.redis_repair_writes || rtpe_config.redis_resync_on_master_change; } /* from main(), before the threads start */ @@ -4097,9 +4102,10 @@ static bool redis_repair_room(void) { return false; } -/* called with r->lock held: a call in the call table, whose SET is to be - * repaired. It supersedes an older DEL of its ID: if it was never written, - * it takes that key over, as in redis_key_taken(). */ +/* a call in the call table, whose SET is to be repaired. It supersedes an + * older DEL of its ID: if it was never written, it takes that key over, as + * in redis_key_taken() (with r->lock held: the resync marks only calls + * written before). */ static void redis_repair_mark(call_t *c) { if (!redis_repair_enabled()) return; @@ -4228,6 +4234,109 @@ static int redis_repair_one(struct redis *r, struct redis_repair_entry *e) { return redis_repair_del(r, e->id, e->db); } +/* --redis-resync-on-master-change: own calls and recent own DELs are queued + * for the repair pass when the master's replication ID changes */ +struct redis_resync_del { + char *callid; + int db; + int64_t when; +}; + +static mutex_t redis_resync_lock = MUTEX_STATIC_INIT; +static GQueue redis_resync_dels = G_QUEUE_INIT; +static unsigned int redis_resync_del_dropped; +static atomic64 redis_resync_requested; +static int64_t redis_resync_started_us; + +static void redis_resync_del_free(struct redis_resync_del *d) { + g_free(d->callid); + g_free(d); +} + +static void redis_resync_note_del(const str *callid, int db) { + if (!rtpe_config.redis_resync_on_master_change || db < 0) + return; + int64_t now = now_us(); + struct redis_resync_del *d = g_new(__typeof(*d), 1); + *d = (__typeof(*d)) { .callid = g_strndup(callid->s, callid->len), .db = db, .when = now }; + LOCK(&redis_resync_lock); + g_queue_push_tail(&redis_resync_dels, d); + // the last --redis-resync-delete-window, at most --redis-resync-delete-max: + // count what is dropped + int64_t since = now - rtpe_config.redis_resync_delete_window * 1000LL; + while ((d = g_queue_peek_head(&redis_resync_dels))) { + if (d->when >= since && redis_resync_dels.length <= rtpe_config.redis_resync_delete_max) + break; + if (d->when >= since) + redis_resync_del_dropped++; + redis_resync_del_free(g_queue_pop_head(&redis_resync_dels)); + } +} + +/* from redis_connect() with the INFO reply, by the thread that connects r */ +static void redis_resync_note_replid(struct redis *r, const char *info) { + char id[sizeof(r->master_replid)] = ""; + const char *p = info ? strstr(info, "master_replid:") : NULL; + if (p) { + p += strlen("master_replid:"); + g_strlcpy(id, p, MIN(sizeof(id), strcspn(p, "\r\n") + 1)); + } + if (!id[0]) + return; + if (r->master_replid[0] && strcmp(id, r->master_replid) + && rtpe_config.redis_resync_on_master_change + && r->role == MASTER_REDIS_ROLE && r == rtpe_redis_write) + { + rlog(LOG_NOTICE, "Redis %s has a new replication ID (%s, was %s): a new master; " + "writing own calls and recent deletions again", + endpoint_print_buf(&r->endpoint), id, r->master_replid); + atomic64_set(&redis_resync_requested, 1); + LOCK(&redis_repair_lock); + cond_signal(&redis_repair_cond); + } + g_strlcpy(r->master_replid, id, sizeof(r->master_replid)); +} + +static void redis_resync_queue(struct redis *r) { + unsigned int dels = 0, calls = 0, dropped; + int64_t now = now_us(); + int64_t since = now - rtpe_config.redis_resync_delete_window * 1000LL; + + { + LOCK(&r->lock); // for the hand-over in redis_repair_mark_del() + LOCK(&redis_resync_lock); + for (GList *l = redis_resync_dels.head; l; l = l->next) { + struct redis_resync_del *d = l->data; + if (d->when < since) + continue; + str id = STR(d->callid); + redis_repair_mark_del(&id, d->db, NULL); + dels++; + } + dropped = redis_resync_del_dropped; + redis_resync_del_dropped = 0; + } + + rwlock_lock_r(&rtpe_callhash_lock); + __auto_type iter = t_hash_table_iter(rtpe_callhash); + call_t *c; + while (t_hash_table_iter_next(&iter, NULL, &c)) { + // own calls stored at least once (not those with no-redis-update) + if (IS_OWN_CALL(c) && c->redis_hosted_db >= 0) { + redis_repair_mark(c); + calls++; + } + } + rwlock_unlock_r(&rtpe_callhash_lock); + + rlog(LOG_NOTICE, "Redis: resync after a master change: %u own calls and %u deletions queued%s", + calls, dels, dropped ? " (some older deletions were dropped: too many)" : ""); + RTPE_STATS_INC(redis_resyncs); + RTPE_STATS_ADD(redis_resync_calls, calls); + RTPE_STATS_ADD(redis_resync_deletes, dels); + redis_resync_started_us = now; +} + void redis_repair_loop(void *d) { struct redis *r = rtpe_redis_write; struct thread_waker waker = { .lock = &redis_repair_lock, .cond = &redis_repair_cond }; @@ -4240,11 +4349,21 @@ void redis_repair_loop(void *d) { while (!rtpe_shutdown) { rtpe_now = now_us(); + if (atomic64_get(&redis_resync_requested)) { + atomic64_set(&redis_resync_requested, 0); + redis_resync_queue(r); + next_us = 0; + } unsigned int overflow = 0; struct redis_repair_entry *e = NULL; { LOCK(&redis_repair_lock); unsigned int pending = t_hash_table_size(redis_repair_pending); + if (redis_resync_started_us && !pending) { + rlog(LOG_NOTICE, "Redis: resync after a master change done in %" PRId64 " ms", + (now_us() - redis_resync_started_us) / 1000); + redis_resync_started_us = 0; + } if ((repaired || redis_repair_overflow) && rtpe_now - report_us >= 1000000) { overflow = redis_repair_overflow; redis_repair_overflow = 0; @@ -4284,6 +4403,7 @@ void redis_repair_loop(void *d) { /* at shutdown, after the threads are gone and before the calls are freed */ void redis_repair_free(void) { + g_queue_clear_full(&redis_resync_dels, (GDestroyNotify) redis_resync_del_free); if (!t_hash_table_is_set(redis_repair_pending)) return; unsigned int n = t_hash_table_size(redis_repair_pending); diff --git a/daemon/statistics.c b/daemon/statistics.c index 3095fb1b6..79287820f 100644 --- a/daemon/statistics.c +++ b/daemon/statistics.c @@ -488,6 +488,15 @@ stats_metric_q *statistics_gather_metrics(struct interface_sampled_rate_stats *i METRIC("redisrepairedwrites", "Total Redis writes repaired", "%" PRIu64, "%" PRIu64, atomic64_get_na(&rtpe_stats->redis_repaired_writes)); PROM("redis_repaired_writes_total", "counter"); + METRIC("redisresyncs", "Total Redis resyncs after a master change", "%" PRIu64, "%" PRIu64, + atomic64_get_na(&rtpe_stats->redis_resyncs)); + PROM("redis_resyncs_total", "counter"); + METRIC("redisresynccalls", "Total calls queued by Redis resyncs", "%" PRIu64, "%" PRIu64, + atomic64_get_na(&rtpe_stats->redis_resync_calls)); + PROM("redis_resync_calls_total", "counter"); + METRIC("redisresyncdeletes", "Total deletions queued by Redis resyncs", "%" PRIu64, "%" PRIu64, + atomic64_get_na(&rtpe_stats->redis_resync_deletes)); + PROM("redis_resync_deletes_total", "counter"); METRICva("avgcallduration", "Average call duration", "%.6f", "%.6f seconds", (double) avg_us / 1000000.0); PROM("call_duration_avg", "gauge"); diff --git a/docs/ng_control_protocol.md b/docs/ng_control_protocol.md index 801f66056..22b6054bb 100644 --- a/docs/ng_control_protocol.md +++ b/docs/ng_control_protocol.md @@ -2674,6 +2674,9 @@ command. Sample return dictionary: "onewaystreams": 0, "rediswriteerrors": 0, "redisrepairedwrites": 0, + "redisresyncs": 0, + "redisresynccalls": 0, + "redisresyncdeletes": 0, "avgcallduration": "0.000000" }, "intervalstatistics": { diff --git a/docs/rtpengine.md b/docs/rtpengine.md index b42426cd0..f0404f0cc 100644 --- a/docs/rtpengine.md +++ b/docs/rtpengine.md @@ -747,6 +747,37 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- database, or that call-ids are not reused. It is a boolean value (either __true__ or __false__), and it is off by default. +- __\-\-redis-resync-on-master-change__ + + After a change of Redis master, write again every own call stored at + least once, and make again the recent deletions of own calls (see + __\-\-redis-resync-delete-window__): a new master may lack writes that + the old one acknowledged but had not replicated. A change is seen when + the write connection reconnects to a server with another replication ID + (__master\_replid__), as after a failover or a restart of the master. + The write connection must require the master role (__\-\-redis__ alone, + __\-\-redis-require-master__, or __sentinel__ in __\-\-redis-write__), + otherwise this option is refused at startup. The writes go through + __\-\-redis-repair-writes__, which this option enables, with the same + assumption about deletions. Resyncs, and the calls and deletions they + queue, are counted in the __redisresyncs__, __redisresynccalls__ and + __redisresyncdeletes__ statistics. It is a boolean value (either + __true__ or __false__), and it is off by default. + +- __\-\-redis-resync-delete-window=__*INT* + + How long, in milliseconds, the deletions of own calls are remembered for + __\-\-redis-resync-on-master-change__. It should be longer than a + failover plus the replication lag. The default value is 30000. Only + valid with __\-\-redis-resync-on-master-change__. + +- __\-\-redis-resync-delete-max=__*INT* + + How many deletions of own calls are remembered at most for + __\-\-redis-resync-on-master-change__; beyond that, the oldest are + dropped, and their number is logged at the next resync. The default + value is 4096. Only valid with __\-\-redis-resync-on-master-change__. + - __\-\-redis-sentinel=__\[*PW*@\]*IP*:*PORT*\[,*IP*:*PORT*...\] The Redis Sentinels to ask for the master, for every Redis option given diff --git a/etc/rtpengine.conf b/etc/rtpengine.conf index e230519fa..b13842a6a 100644 --- a/etc/rtpengine.conf +++ b/etc/rtpengine.conf @@ -86,6 +86,9 @@ recording-method = proc # redis-resolve-on-reconnect = false # redis-require-master = false # redis-repair-writes = false +# redis-resync-on-master-change = false +# redis-resync-delete-window = 30000 +# redis-resync-delete-max = 4096 # redis-write = password@sentinel/42 # redis-sentinel = password@10.0.0.11:26379,10.0.0.12:26379,10.0.0.13:26379 # redis-sentinel-master = mymaster diff --git a/include/counter_stats_fields.inc b/include/counter_stats_fields.inc index decc947e2..d406c78ed 100644 --- a/include/counter_stats_fields.inc +++ b/include/counter_stats_fields.inc @@ -23,3 +23,6 @@ F(rtp_seq_resets) F(rtp_reordered) F(redis_write_errors) F(redis_repaired_writes) +F(redis_resyncs) +F(redis_resync_calls) +F(redis_resync_deletes) diff --git a/include/main.h b/include/main.h index 945079578..e1565ef84 100644 --- a/include/main.h +++ b/include/main.h @@ -63,6 +63,8 @@ enum endpoint_learning { X(redis_tcp_keepalive_intvl) \ X(redis_tcp_keepalive_probes) \ X(redis_sentinel_interval) \ + X(redis_resync_delete_window) \ + X(redis_resync_delete_max) \ X(num_threads) \ X(media_num_threads) \ X(codec_num_threads) \ @@ -142,6 +144,7 @@ enum endpoint_learning { X(redis_resolve_on_reconnect) \ X(redis_require_master) \ X(redis_repair_writes) \ + X(redis_resync_on_master_change) \ X(redis_via_sentinel) \ X(redis_write_via_sentinel) \ X(redis_subscribe_via_sentinel) \ diff --git a/include/redis.h b/include/redis.h index 9f6dba608..58af86ac3 100644 --- a/include/redis.h +++ b/include/redis.h @@ -85,6 +85,7 @@ struct redis { bool require_master; /* a role error drops the connection */ atomic64 lock_waiters; /* signalling writes waiting for lock */ cond_t lock_waiters_cond; /* signalled when the last one has it */ + char master_replid[48]; /* of the master last connected to */ struct redis_sentinel *sentinel; /* address from Redis Sentinel, or NULL */ uint64_t gen; /* generation of the master in endpoint */ diff --git a/t/Makefile b/t/Makefile index 1e439adc1..bf262f41a 100644 --- a/t/Makefile +++ b/t/Makefile @@ -94,6 +94,7 @@ include ../lib/common.Makefile daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ daemon-tests-redis-sentinel-subscribe \ daemon-tests-redis-repair daemon-tests-redis-repair-ids \ + daemon-tests-redis-resync \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions daemon-tests-alias \ daemon-tests-kernel-api daemon-tests-dtmf-fanout \ daemon-tests-video daemon-tests-inject daemon-tests-dtx-dtmf @@ -152,6 +153,7 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests daemon-tests-redis-sentinel daemon-tests-redis-sentinel-start \ daemon-tests-redis-sentinel-subscribe \ daemon-tests-redis-repair daemon-tests-redis-repair-ids \ + daemon-tests-redis-resync \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions \ daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-dtx-no-shift daemon-tests-dtx-dtmf \ daemon-tests-alias \ @@ -278,6 +280,9 @@ daemon-tests-redis-repair: daemon-test-deps daemon-tests-redis-repair-ids: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-repair-ids.pl +daemon-tests-redis-resync: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-resync.pl + daemon-tests-audio-player: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-audio-player.pl diff --git a/t/auto-daemon-tests-redis-resync.pl b/t/auto-daemon-tests-redis-resync.pl new file mode 100755 index 000000000..9a74eee03 --- /dev/null +++ b/t/auto-daemon-tests-redis-resync.pl @@ -0,0 +1,237 @@ +#!/usr/bin/perl + +# --redis-resync-on-master-change: when the master's replication ID changes +# at a reconnect (a new master), every own call is written again, and the +# recent DELs are made again; a reconnect to the same master does nothing. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; +use Time::HiRes qw(sleep); + + +my $listener = redis_listen('203.0.113.42'); +my $fd; +my $replid = 'a' x 40; + +my $sdp = <[0] eq 'DEL' ? ":1\r\n" : "+OK\r\n"); + return "$c->[0] $c->[1]"; +} +# a new call, written at once +sub stored_call { + my $n = shift; + new_call; + my $id = cid(); + $NGCP::Rtpengine::req_cb = sub { is(next_cmd($n), "SET $id", "$n stored") }; + offer("call $n", { }, $sdp); + return $id; +} +sub delete_call { + my ($id, $n) = @_; + $NGCP::Rtpengine::req_cb = sub { is(next_cmd("$n del"), "DEL $id", "$n deleted") }; + rtpe_req('delete', "delete $n", { 'delete delay' => 0 }); +} +# the statistics match the "queued" lines of the log +sub resync_stats { + my ($n) = @_; + my @q = rtpe_log() =~ /resync after a master change: (\d+) own calls and (\d+) deletions queued/g; + my ($calls, $dels) = (0, 0); + while (@q) { + $calls += shift(@q); + $dels += shift(@q); + } + is(redis_stat('redisresyncs'), $n, "$n resyncs counted"); + is(redis_stat('redisresynccalls'), $calls, 'calls queued: as logged'); + is(redis_stat('redisresyncdeletes'), $dels, 'deletions queued: as logged'); +} +# the connection drops; the next call's write reconnects, to a master with +# this replication ID +sub reconnect_call { + my ($id, $n) = @_; + redis_close($fd); + $replid = $id; + new_call; + my $cid = cid(); + $NGCP::Rtpengine::req_cb = sub { + $fd = redis_conn($listener, $n, replid => $replid); + redis_expect_set($fd, $cid, "+OK\r\n", $n, no_ping => 1); + }; + offer($n, { }, $sdp); + undef($NGCP::Rtpengine::req_cb); + return $cid; +} +# the resync's writes, in any order, until nothing comes for 0.5 s; the call +# whose write made the reconnect is in it only if that write was done first +sub resync { + my ($n, $own) = @_; + my %got; + my $i = 0; + while (readable($fd, 0.5)) { + $i++; + $got{next_cmd("$n $i")} = 1; + } + delete($got{"SET $own"}); + return \%got; +} + +# refused at startup: the write connection accepts any role (on a replica, +# INFO gives the master's replication ID, so no change would be seen). The +# address has no listener, and the run is bounded, so that if the refusal is +# lost, rtpengine fails to connect (another message) instead of hanging. +{ + my $out = `timeout -k 1 10 $ENV{RTPE_BIN} --config-file=none -i 203.0.113.1 -n 2223 -f -E --redis-cmd-timeout=1000 --redis=auth\@203.0.113.99:6379/2 --redis-write=auth\@203.0.113.99:6379/2 --redis-resync-on-master-change 2>&1`; + isnt($?, 0, 'write connection with any role: refused'); + like($out, qr/--redis-resync-on-master-change needs a write connection that requires the master role/, + 'refusal explained'); +} + +$NGCP::Rtpengine::AutoTest::launch_cb = sub { + $fd = redis_conn($listener, 'startup', replid => $replid); + redis_expect($fd, ['PING'], "+PONG\r\n", 'startup PING'); + redis_expect($fd, ['KEYS', '*'], "*0\r\n", 'startup KEYS'); +}; + +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 + -n 2223 -f -L 7 -E + --redis=auth@203.0.113.42:6379/2 + --redis-resync-on-master-change --redis-cmd-timeout=1000)) + or die; + + +# a call that asked not to be stored: never written, not even by a resync + +new_call; +undef($NGCP::Rtpengine::req_cb); +offer('call N, not stored', { flags => ['no-redis-update'] }, $sdp); +quiet(0.5, 'N not stored'); + +# two calls, one of them deleted + +my $a_id = stored_call('A'); +my $b_id = stored_call('B'); +delete_call($b_id, 'B'); + + +# the connection drops, and the reconnect finds the same master (same +# replication ID): nothing more than the write itself + +my $c_id = reconnect_call('a' x 40, 'C after a reconnect to the same master'); +quiet(0.5, 'same master: no resync'); + + +# the reconnect finds a new master: the own calls (A, C; D is being written +# by the write that reconnected) are written again, and B's DEL is made again + +my $d_id = reconnect_call('b' x 40, 'D on the new master'); +is_deeply(resync('resync', $d_id), { "SET $a_id" => 1, "SET $c_id" => 1, "DEL $b_id" => 1 }, + 'resync: every own call written, the recent DEL made again'); +like(rtpe_log(), qr/new replication ID \(b{40}, was a{40}\)/, 'master change logged'); +like(rtpe_log(), qr/resync after a master change: [23] own calls and 1 deletions queued/, 'resync queued'); +like(rtpe_log(), qr/resync after a master change done in \d+ ms/, 'resync done'); +resync_stats(1); + + +# CLI "standby": the existing calls are foreign now. At the next master +# change, only the recent DEL (B) is made again (H is written by its own write). + +my $resp = rtpe_req('cli', 'standby', { body => 'standby' }); +like($resp->{response}, qr/all calls set to 'foreign/, 'all calls foreign'); +my $h_id = reconnect_call('c' x 40, 'H on the third master'); +is_deeply(resync('second resync', $h_id), { "DEL $b_id" => 1 }, + 'second resync: foreign calls left out'); +like(rtpe_log(), qr/resync after a master change: [01] own calls and 1 deletions queued/, 'second resync queued'); +resync_stats(2); + + +# --redis-resync-delete-window and --redis-resync-delete-max + +NGCP::Rtpengine::AutoTest::shut_rtpe(); +redis_close($fd); +$replid = 'd' x 40; +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 + -n 2223 -f -L 7 -E + --redis=auth@203.0.113.42:6379/2 + --redis-resync-on-master-change --redis-cmd-timeout=1000 + --redis-resync-delete-window=2000 --redis-resync-delete-max=2)) + or die; + +# P and Q, deleted more than 2 s before the master change, are not deleted +# again; U is + +my $p_id = stored_call('P'); +delete_call($p_id, 'P'); +my $q_id = stored_call('Q'); +delete_call($q_id, 'Q'); +sleep(2.2); +my $u_id = stored_call('U'); +delete_call($u_id, 'U'); +my $r_id = reconnect_call('e' x 40, 'R on a new master'); +is_deeply(resync('third resync', $r_id), { "DEL $u_id" => 1 }, + 'third resync: only the deletion within the window'); +like(rtpe_log(), qr/[01] own calls and 1 deletions queued$/m, + 'third resync: P and Q left the window, none dropped for the size'); + +# of U, V, W and X, only the last two deletions are remembered (R is still +# a call) + +my $v_id = stored_call('V'); +delete_call($v_id, 'V'); +my $w_id = stored_call('W'); +delete_call($w_id, 'W'); +my $x_id = stored_call('X'); +delete_call($x_id, 'X'); +my $y_id = reconnect_call('f' x 40, 'Y on a new master'); +is_deeply(resync('fourth resync', $y_id), { "SET $r_id" => 1, "DEL $w_id" => 1, "DEL $x_id" => 1 }, + 'fourth resync: R, and only the last two deletions'); +like(rtpe_log(), qr/[12] own calls and 2 deletions queued \(some older deletions were dropped: too many\)/, + 'dropped deletions logged'); + +# no deletion for 2.2 s: W and X are still remembered, but older than the +# window at the next master change, so they are not made again + +sleep(2.2); +my $z_id = reconnect_call('0' x 40, 'Z on a new master'); +is_deeply(resync('fifth resync', $z_id), { "SET $r_id" => 1, "SET $y_id" => 1 }, + 'fifth resync: the calls, no deletion older than the window'); +resync_stats(3); + + +done_testing(); diff --git a/t/test-stats.c b/t/test-stats.c index 736907551..369853a4c 100644 --- a/t/test-stats.c +++ b/t/test-stats.c @@ -552,6 +552,18 @@ int main(void) { "redisrepairedwrites\n" "0\n" "0\n" + "Total Redis resyncs after a master change\n" + "redisresyncs\n" + "0\n" + "0\n" + "Total calls queued by Redis resyncs\n" + "redisresynccalls\n" + "0\n" + "0\n" + "Total deletions queued by Redis resyncs\n" + "redisresyncdeletes\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -1398,7 +1410,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "488\n" + "512\n" "}\n" "]\n" "}\n" @@ -1874,6 +1886,18 @@ int main(void) { "redisrepairedwrites\n" "0\n" "0\n" + "Total Redis resyncs after a master change\n" + "redisresyncs\n" + "0\n" + "0\n" + "Total calls queued by Redis resyncs\n" + "redisresynccalls\n" + "0\n" + "0\n" + "Total deletions queued by Redis resyncs\n" + "redisresyncdeletes\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -2720,7 +2744,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "488\n" + "512\n" "}\n" "]\n" "}\n" @@ -3193,6 +3217,18 @@ int main(void) { "redisrepairedwrites\n" "0\n" "0\n" + "Total Redis resyncs after a master change\n" + "redisresyncs\n" + "0\n" + "0\n" + "Total calls queued by Redis resyncs\n" + "redisresynccalls\n" + "0\n" + "0\n" + "Total deletions queued by Redis resyncs\n" + "redisresyncdeletes\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -4039,7 +4075,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "488\n" + "512\n" "}\n" "]\n" "}\n" @@ -4531,6 +4567,18 @@ int main(void) { "redisrepairedwrites\n" "0\n" "0\n" + "Total Redis resyncs after a master change\n" + "redisresyncs\n" + "0\n" + "0\n" + "Total calls queued by Redis resyncs\n" + "redisresynccalls\n" + "0\n" + "0\n" + "Total deletions queued by Redis resyncs\n" + "redisresyncdeletes\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -5377,7 +5425,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "488\n" + "512\n" "}\n" "]\n" "}\n" @@ -5858,6 +5906,18 @@ int main(void) { "redisrepairedwrites\n" "0\n" "0\n" + "Total Redis resyncs after a master change\n" + "redisresyncs\n" + "0\n" + "0\n" + "Total calls queued by Redis resyncs\n" + "redisresynccalls\n" + "0\n" + "0\n" + "Total deletions queued by Redis resyncs\n" + "redisresyncdeletes\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -6704,7 +6764,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "488\n" + "512\n" "}\n" "]\n" "}\n" @@ -7179,6 +7239,18 @@ int main(void) { "redisrepairedwrites\n" "0\n" "0\n" + "Total Redis resyncs after a master change\n" + "redisresyncs\n" + "0\n" + "0\n" + "Total calls queued by Redis resyncs\n" + "redisresynccalls\n" + "0\n" + "0\n" + "Total deletions queued by Redis resyncs\n" + "redisresyncdeletes\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "0.000000 seconds\n" @@ -8025,7 +8097,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "488\n" + "512\n" "}\n" "]\n" "}\n" @@ -8503,6 +8575,18 @@ int main(void) { "redisrepairedwrites\n" "0\n" "0\n" + "Total Redis resyncs after a master change\n" + "redisresyncs\n" + "0\n" + "0\n" + "Total calls queued by Redis resyncs\n" + "redisresynccalls\n" + "0\n" + "0\n" + "Total deletions queued by Redis resyncs\n" + "redisresyncdeletes\n" + "0\n" + "0\n" "Average call duration\n" "avgcallduration\n" "93.000000 seconds\n" @@ -9349,7 +9433,7 @@ int main(void) { "size\n" "16777208\n" "used\n" - "488\n" + "512\n" "}\n" "]\n" "}\n" From 2ef302bd22fbb63a4031be7995f64010862b900e Mon Sep 17 00:00:00 2001 From: Kevin Binetruy Date: Wed, 14 Oct 2026 16:50:44 +0000 Subject: [PATCH 6/7] MT#55283 redis: optional TLS for the Redis connections There is no way to reach a Redis server that only accepts TLS (managed services, or tls-port with port 0); asked on the mailing list in 2022: https://groups.google.com/g/rtpengine/c/gO2hWMKxCNA/m/DNH37Z54BQAJ With --redis-cafile or --redis-capath, every Redis connection uses TLS, and --redis-certfile with --redis-keyfile adds a client certificate. As for MQTT, the options exist only in a build with hiredis_ssl. hiredis checks only the chain, so the name is checked here, in the subjectAltName: the host as configured, the IP address, or --redis-tls-server-name. The handshake is bounded by the command timeout, or by the connect timeout when there is none. Tested by auto-daemon-tests-redis-tls.pl (libio-socket-ssl-perl added to Build-Depends). --- daemon/Makefile | 3 + daemon/main.c | 22 +++ daemon/redis.c | 143 +++++++++++++++- debian/control | 1 + docs/rtpengine.md | 25 +++ etc/rtpengine.conf | 5 + include/main.h | 5 + include/redis.h | 5 + perl/NGCP/Rtpengine/FakeRedis.pm | 59 ++++++- t/Makefile | 8 + t/auto-daemon-tests-redis-tls.pl | 273 +++++++++++++++++++++++++++++++ utils/gen-common-flags | 6 + 12 files changed, 550 insertions(+), 5 deletions(-) create mode 100644 t/auto-daemon-tests-redis-tls.pl diff --git a/daemon/Makefile b/daemon/Makefile index 08ad1e6c5..1ba36f833 100644 --- a/daemon/Makefile +++ b/daemon/Makefile @@ -82,6 +82,9 @@ endif CFLAGS += $(CFLAGS_MQTT) LDLIBS += $(LDLIBS_MQTT) +CFLAGS += $(CFLAGS_LIBHIREDIS_SSL) +LDLIBS += $(LDLIBS_LIBHIREDIS_SSL) + SRCS := main.c kernel.c helpers.c control_tcp.c call.c control_udp.c redis.c redis_sentinel.c \ cookie_cache.c udp_listener.c control_ng_flags_parser.c control_ng.c sdp.strhash.c stun.c rtcp.c \ crypto.c rtp.c call_interfaces.c dtls.c log.c cli.strhash.c graphite.c ice.c \ diff --git a/daemon/main.c b/daemon/main.c index f68b1a7a6..fa701382f 100644 --- a/daemon/main.c +++ b/daemon/main.c @@ -836,6 +836,13 @@ static void options(int *argc, char ***argv, charp_ht templates) { { "redis-sentinel", 0, 0, G_OPTION_ARG_STRING, &redis_sentinels, "Redis Sentinels to ask for the master, for the roles given as 'sentinel'", "[PW@]IP:PORT[,IP:PORT...]" }, { "redis-sentinel-master", 0, 0, G_OPTION_ARG_STRING, &redis_sentinel_master, "Name of the master monitored by Redis Sentinel", "STRING" }, { "redis-sentinel-interval", 0, 0, G_OPTION_ARG_INT, &redis_sentinel_interval, "How often to ask Redis Sentinel for the master, in milliseconds", "INT" }, +#ifdef HAVE_HIREDIS_SSL + { "redis-cafile", 0, 0, G_OPTION_ARG_FILENAME, &rtpe_config.redis_cafile, "CA file for TLS to Redis and Redis Sentinel", "FILE" }, + { "redis-capath", 0, 0, G_OPTION_ARG_FILENAME, &rtpe_config.redis_capath, "CA path for TLS to Redis and Redis Sentinel", "PATH" }, + { "redis-certfile", 0, 0, G_OPTION_ARG_FILENAME, &rtpe_config.redis_certfile, "Client certificate file for TLS to Redis", "FILE" }, + { "redis-keyfile", 0, 0, G_OPTION_ARG_FILENAME, &rtpe_config.redis_keyfile, "Client key file for TLS to Redis", "FILE" }, + { "redis-tls-server-name", 0, 0, G_OPTION_ARG_STRING, &rtpe_config.redis_tls_server_name, "Name to expect in the certificates of the Redis servers, instead of their address", "NAME" }, +#endif { "redis-num-threads", 0, 0, G_OPTION_ARG_INT, &rtpe_config.redis_num_threads, "Number of Redis restore threads", "INT" }, { "redis-expires", 0, 0, G_OPTION_ARG_INT, &rtpe_config.redis_expires_secs, "Expire time in seconds for redis keys", "INT" }, { "no-redis-required", 'q', 0, G_OPTION_ARG_NONE, &rtpe_config.no_redis_required, "Start no matter of redis connection state", NULL }, @@ -1367,6 +1374,17 @@ static void options(int *argc, char ***argv, charp_ht templates) { die("--redis-sentinel, --redis-sentinel-master or --redis-sentinel-interval is set, " "but no Redis endpoint is 'sentinel'"); +#ifdef HAVE_HIREDIS_SSL + if (rtpe_config.redis_cafile || rtpe_config.redis_capath || rtpe_config.redis_certfile + || rtpe_config.redis_keyfile || rtpe_config.redis_tls_server_name) + { + if (!rtpe_config.redis_cafile && !rtpe_config.redis_capath) + die("Redis TLS needs --redis-cafile or --redis-capath"); + if (!rtpe_config.redis_certfile != !rtpe_config.redis_keyfile) + die("--redis-certfile and --redis-keyfile must be given together"); + } +#endif + // the trigger is the write connection's INFO, which gives the master's ID // on a replica too: that connection must require the master role if (rtpe_config.redis_resync_on_master_change && redisps_write @@ -1985,6 +2003,9 @@ static void create_everything(void) { bool sent_r = rtpe_config.redis_via_sentinel, sent_w = rtpe_config.redis_write_via_sentinel, sent_s = rtpe_config.redis_subscribe_via_sentinel; + if (redis_tls_init()) + die("Cannot set up TLS for Redis"); + if (rtpe_redis_sentinel) { // the master's password, for the role check on the Sentinels' answer redis_sentinel_set_probe_auth(rtpe_redis_sentinel, @@ -2344,6 +2365,7 @@ int main(int argc, char **argv) { redis_close(rtpe_redis_notify); redis_sentinel_free(rtpe_redis_sentinel); rtpe_redis_sentinel = NULL; + redis_tls_free(); free_prefix(); log_free(); diff --git a/daemon/redis.c b/daemon/redis.c index 32c11362c..6a3ae0ecb 100644 --- a/daemon/redis.c +++ b/daemon/redis.c @@ -36,6 +36,13 @@ #include "statistics.h" #include "redis_sentinel.h" +#ifdef HAVE_HIREDIS_SSL +#include +#include +#include +#include +#endif + #define REDIS_CONNECT_ROLE -2 /* redis_connect(): the server has the wrong role */ typedef union { @@ -394,6 +401,125 @@ void redis_set_keepalive(int fd) { } } +#ifdef HAVE_HIREDIS_SSL +static SSL_CTX *redis_ssl_ctx; +#endif + +/* with --redis-cafile or --redis-capath */ +int redis_tls_init(void) { +#ifdef HAVE_HIREDIS_SSL + if (!rtpe_config.redis_cafile && !rtpe_config.redis_capath) + return 0; + + const char *err = "cannot create the TLS context"; + redis_ssl_ctx = SSL_CTX_new(TLS_client_method()); + if (!redis_ssl_ctx) + goto err; + SSL_CTX_set_min_proto_version(redis_ssl_ctx, TLS1_2_VERSION); + SSL_CTX_set_verify(redis_ssl_ctx, SSL_VERIFY_PEER, NULL); + err = "cannot load the CA file or path"; + if (!SSL_CTX_load_verify_locations(redis_ssl_ctx, rtpe_config.redis_cafile, + rtpe_config.redis_capath)) + goto err; + if (rtpe_config.redis_certfile) { + err = "cannot load the client certificate or key"; + if (!SSL_CTX_use_certificate_chain_file(redis_ssl_ctx, rtpe_config.redis_certfile) + || !SSL_CTX_use_PrivateKey_file(redis_ssl_ctx, rtpe_config.redis_keyfile, + SSL_FILETYPE_PEM) + || !SSL_CTX_check_private_key(redis_ssl_ctx)) + goto err; + } + ilog(LOG_INFO, "Using TLS for Redis"); + return 0; + +err: + ilog(LOG_ERR, "Redis TLS: %s: %s", err, ERR_reason_error_string(ERR_peek_last_error()) ? : "?"); + redis_tls_free(); + return -1; +#else + return 0; +#endif +} + +void redis_tls_free(void) { +#ifdef HAVE_HIREDIS_SSL + if (redis_ssl_ctx) + SSL_CTX_free(redis_ssl_ctx); + redis_ssl_ctx = NULL; +#endif +} + +/* TLS on a new connection, if configured. The certificate must match "name", + * as an IP address or a host name (subjectAltName only): released hiredis + * (1.2.0) only checks the chain. "timeout_us" bounds the handshake on a + * blocking connection that has no command timeout. "sslp", if given, gets the + * SSL of an async connection, whose handshake ends later. */ +int redis_tls_start(redisContext *c, const char *name, int64_t timeout_us, struct ssl_st **sslp) { +#ifdef HAVE_HIREDIS_SSL + if (!redis_ssl_ctx) + return 0; + SSL *ssl = SSL_new(redis_ssl_ctx); + if (!ssl) + return -1; + X509_VERIFY_PARAM *param = SSL_get0_param(ssl); + // no SNI for an IP address (RFC 6066) + if (!X509_VERIFY_PARAM_set1_ip_asc(param, name)) { + X509_VERIFY_PARAM_set_hostflags(param, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS + | X509_CHECK_FLAG_NEVER_CHECK_SUBJECT); + if (!X509_VERIFY_PARAM_set1_host(param, name, 0) || !SSL_set_tlsext_host_name(ssl, name)) { + SSL_free(ssl); + return -1; + } + } + + if (timeout_us) + redisSetTimeout(c, timeval_from_us(timeout_us)); + int ret = redisInitiateSSL(c, ssl); + if (timeout_us) + redisSetTimeout(c, (struct timeval) {0}); + if (ret == REDIS_OK) { + // hiredis frees ssl with the context + if (sslp) + *sslp = ssl; + return 0; + } + + // hiredis has no setter for errstr, which the callers log; its own text + // has no reason for a refused certificate or a timeout + long v = SSL_get_verify_result(ssl); + if (v != X509_V_OK) + snprintf(c->errstr, sizeof(c->errstr), "TLS: certificate of %s not accepted: %s", + name, X509_verify_cert_error_string(v)); + else if (SSL_want_read(ssl) || SSL_want_write(ssl)) + snprintf(c->errstr, sizeof(c->errstr), "TLS: handshake with %s timed out", name); + SSL_free(ssl); + return -1; +#else + return 0; +#endif +} + +/* the name that the server's certificate must match: --redis-tls-server-name, + * the host as configured, or the address that Sentinel gave */ +static const char *redis_tls_name(const struct redis *r) { + if (rtpe_config.redis_tls_server_name) + return rtpe_config.redis_tls_server_name; + return (!r->sentinel && r->hostname) ? r->hostname : r->host; +} + +/* the reason why the certificate of an async connection was refused, if it + * was: hiredis only reports an I/O error then */ +static const char *redis_async_tls_error(const struct redis *r, const redisAsyncContext *ac) { +#ifdef HAVE_HIREDIS_SSL + if (r && r->async_ctx == ac && r->async_ssl) { + long v = SSL_get_verify_result(r->async_ssl); + if (v != X509_V_OK) + return X509_verify_cert_error_string(v); + } +#endif + return NULL; +} + /* called with r->lock held, or with r->reconnecting set, or single thread * Returns 0 when connected, REDIS_CONNECT_ROLE if the server never had the @@ -439,6 +565,10 @@ static int redis_connect(struct redis *r, int wait, bool resolve) { if (redis_set_timeout(r, cmd_timeout)) goto err2; + // with no command timeout, the connect timeout bounds the handshake + if (redis_tls_start(r->ctx, redis_tls_name(r), cmd_timeout ? 0 : connect_timeout, NULL)) + goto err2; + if (r->auth) { if (redisCommandNR(r->ctx, "AUTH %s", r->auth)) goto err2; @@ -670,12 +800,16 @@ void redis_notify_async_context_disconnect(const redisAsyncContext *redis_notify // forget the context, and let event_base_dispatch() return (the timer // must not keep it running), so that redis_notify_loop() subscribes again struct redis *r = rtpe_redis_notify; + const char *tls_err = redis_async_tls_error(r, redis_notify_async_context); if (r && r->notify_timer) event_del(r->notify_timer); if (r && r->async_ctx == redis_notify_async_context) r->async_ctx = NULL; if (status == REDIS_ERR) { - if (redis_notify_async_context->errstr) { + if (tls_err) { + rlog(LOG_ERROR, "redis_notify_async_context_disconnect: TLS: certificate of %s not " + "accepted: %s", redis_tls_name(r), tls_err); + } else if (redis_notify_async_context->errstr) { rlog(LOG_ERROR, "redis_notify_async_context_disconnect error %d on context free: %s", redis_notify_async_context->err, redis_notify_async_context->errstr); } else { @@ -719,6 +853,13 @@ int redis_async_context_alloc(struct redis *r, void *connect_cb, void *disconnec redis_set_keepalive(r->async_ctx->c.fd); + if (redis_tls_start(&r->async_ctx->c, redis_tls_name(r), 0, &r->async_ssl)) { + rlog(LOG_ERROR, "redis_async_context_alloc: %s", r->async_ctx->errstr); + redisAsyncFree(r->async_ctx); + r->async_ctx = NULL; + return -1; + } + // callbacks async context if (redisAsyncSetConnectCallback(r->async_ctx, connect_cb) != REDIS_OK) { rlog(LOG_ERROR, "redis_async_context_alloc: can't set connect callback"); diff --git a/debian/control b/debian/control index f7f7644f7..e0ae94e04 100644 --- a/debian/control +++ b/debian/control @@ -28,6 +28,7 @@ Build-Depends: libio-multiplex-perl, libio-socket-inet6-perl, libio-socket-ip-perl, + libio-socket-ssl-perl, libiptc-dev, libjson-glib-dev, libjson-perl, diff --git a/docs/rtpengine.md b/docs/rtpengine.md index f0404f0cc..890e987b1 100644 --- a/docs/rtpengine.md +++ b/docs/rtpengine.md @@ -813,6 +813,31 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- __\-\-redis-sentinel-master__ and __\-\-redis-sentinel-interval__ are refused without a Redis option that uses __sentinel__. +- __\-\-redis-cafile=__*FILE* +- __\-\-redis-capath=__*PATH* +- __\-\-redis-certfile=__*FILE* +- __\-\-redis-keyfile=__*FILE* + + Use TLS for every Redis connection, including the keyspace subscription + and the asynchronous deletions. As for the __mqtt__ options, at least + __cafile__ or __capath__ must be given to enable TLS, and both + __certfile__ and __keyfile__ for a client certificate; files are in PEM + format, without password protection, and TLS 1.2 is the minimum. The + server's certificate must come from one of these CAs and match, in its + subjectAltName, the host name as given (a wildcard only as a whole first + label) or the IP address (no SNI is sent then), or + __\-\-redis-tls-server-name__. A refused certificate or a handshake that + times out is a failed connection; with no __\-\-redis-cmd-timeout__, + __\-\-redis-connect-timeout__ bounds the handshake. These options are + only available in a build with __hiredis\_ssl__ (hiredis 1.0 or newer). + +- __\-\-redis-tls-server-name=__*NAME* + + The name to send (as SNI) and to expect in the certificate of every + Redis data node, instead of the host or address of each (for example + the name of a managed service). Requires __\-\-redis-cafile__ or + __\-\-redis-capath__. + - __\-\-redis-format=bencode__\|__JSON__ Selects the format for serialised call data written to Redis or KeyDB. The diff --git a/etc/rtpengine.conf b/etc/rtpengine.conf index b13842a6a..5955173d1 100644 --- a/etc/rtpengine.conf +++ b/etc/rtpengine.conf @@ -93,6 +93,11 @@ recording-method = proc # redis-sentinel = password@10.0.0.11:26379,10.0.0.12:26379,10.0.0.13:26379 # redis-sentinel-master = mymaster # redis-sentinel-interval = 1000 +# redis-cafile = +# redis-capath = +# redis-certfile = +# redis-keyfile = +# redis-tls-server-name = # redis-tcp-keepalive-time = 1 # redis-tcp-keepalive-intvl = 1 # redis-tcp-keepalive-probes = 3 diff --git a/include/main.h b/include/main.h index e1565ef84..5529d079d 100644 --- a/include/main.h +++ b/include/main.h @@ -162,6 +162,11 @@ enum endpoint_learning { X(redis_hostname) \ X(redis_write_hostname) \ X(redis_subscribe_hostname) \ + X(redis_cafile) \ + X(redis_capath) \ + X(redis_certfile) \ + X(redis_keyfile) \ + X(redis_tls_server_name) \ X(moh_attr_name) \ X(spooldir) \ X(rec_method) \ diff --git a/include/redis.h b/include/redis.h index 58af86ac3..64dc92104 100644 --- a/include/redis.h +++ b/include/redis.h @@ -17,6 +17,7 @@ struct redis_sentinel; struct event; +struct ssl_st; #define REDIS_RESTORE_NUM_THREADS 4 @@ -77,6 +78,7 @@ struct redis { struct event_base *async_ev; struct redisAsyncContext *async_ctx; + struct ssl_st *async_ssl; /* its TLS, freed with it */ mutex_t async_lock; GQueue async_queue; int64_t async_last; @@ -144,6 +146,9 @@ int redis_reconnect(struct redis* r); void redis_repair_init(void); void redis_repair_loop(void *); void redis_repair_free(void); +int redis_tls_init(void); +void redis_tls_free(void); +int redis_tls_start(redisContext *, const char *name, int64_t timeout_us, struct ssl_st **); #endif diff --git a/perl/NGCP/Rtpengine/FakeRedis.pm b/perl/NGCP/Rtpengine/FakeRedis.pm index 07a5d3b23..48cd7b2e1 100644 --- a/perl/NGCP/Rtpengine/FakeRedis.pm +++ b/perl/NGCP/Rtpengine/FakeRedis.pm @@ -4,11 +4,13 @@ package NGCP::Rtpengine::FakeRedis; # local by tests-preload.so), and helpers that read the commands that # rtpengine sends (RESP arrays of bulk strings) and answer them. Fake Redis # Sentinels run in a child process (sentinels_start()), so that they answer -# while the test script handles the data nodes. +# while the test script handles the data nodes. With redis_tls(), the fake +# servers speak TLS (IO::Socket::SSL, loaded only then). use strict; use warnings; use Socket qw(AF_INET SOCK_STREAM sockaddr_in inet_aton); +use Scalar::Util qw(blessed); use File::Temp; use IO::Handle; use Test::More; @@ -20,13 +22,14 @@ use Exporter 'import'; our @EXPORT = qw(redis_listen redis_accept redis_conn redis_read redis_send redis_expect redis_expect_set redis_expect_del redis_close redis_closed readable info_reply - redis_write_errors redis_stat wait_log + redis_write_errors redis_stat wait_log redis_tls redis_tls_refused redis_handshake tls_ca tls_cert redis_serve redis_sets_seen redis_record_after redis_serve_next_get %role @probes wait_probe serve_probes sentinels_start sentinels_set sentinel_log sentinel_queries); my %rbuf; # what was read from each socket and not parsed yet my %node; # IP of each listener, by fileno +my $tls; # IO::Socket::SSL server options, if TLS # Role probes of the Sentinel thread (AUTH, then ROLE) on the data nodes are # answered from %role (by IP, "master" by default), and recorded in @probes @@ -49,6 +52,7 @@ sub redis_listen { sub readable { my ($fh, $timeout) = @_; return 1 if length($rbuf{fileno($fh)} // ''); + return 1 if blessed($fh) && $fh->isa('IO::Socket::SSL') && $fh->pending; my $rin = ''; vec($rin, fileno($fh), 1) = 1; return select(my $rout = $rin, undef, undef, $timeout) > 0; @@ -95,7 +99,7 @@ sub redis_read { sub redis_send { my ($fd, $s) = @_; - send($fd, $s, 0) // die "send to fake Redis: $!"; + syswrite($fd, $s) // die "send to fake Redis: $!"; } # the next command must be @$exp; it is answered with $reply, unless undef @@ -136,15 +140,62 @@ sub info_reply { return bulk($i . "master_repl_offset:0\r\n"); } +# with "later", the TLS handshake is left to redis_handshake() sub redis_accept { - my ($l, $timeout) = @_; + my ($l, $timeout, $later) = @_; readable($l, $timeout // 5) or die "no connection to the fake Redis\n"; my $fd; accept($fd, $l) or die; + redis_handshake($fd) unless $later; $rbuf{fileno($fd)} = ''; return $fd; } +sub redis_handshake { + my $fd = shift; + return unless $tls; + IO::Socket::SSL->start_SSL($fd, %$tls) + or die "TLS handshake with rtpengine failed: $IO::Socket::SSL::SSL_ERROR\n"; +} + +# The servers speak TLS with these IO::Socket::SSL options (certificate, +# key, client verification...), or plain TCP if none. +sub redis_tls { + my %opt = @_; + require IO::Socket::SSL; + $tls = %opt ? { SSL_server => 1, Timeout => 3, %opt } : undef; +} + +# rtpengine connects and gives up during the handshake +sub redis_tls_refused { + my ($l, $n) = @_; + readable($l, 5) or die "no connection to the fake Redis\n"; + accept(my $fd, $l) or die; + ok(!IO::Socket::SSL->start_SSL($fd, %$tls), $n); + close($fd); +} + +# Test certificates (EC P-256, valid for a day), made with the openssl tool +# in $dir: a CA, and a certificate signed by it for subjectAltName $san. +sub tls_ca { + my ($dir, $name) = @_; + system(qw(openssl req -x509 -newkey ec -pkeyopt ec_paramgen_curve:prime256v1 -nodes -days 1 + -subj), "/CN=$name", '-keyout', "$dir/$name.key", '-out', "$dir/$name.pem") == 0 or die; +} + +my $cert_serial = 0; +sub tls_cert { + my ($dir, $ca, $name, $san) = @_; + system(qw(openssl req -new -newkey ec -pkeyopt ec_paramgen_curve:prime256v1 -nodes + -subj), "/CN=$name", '-keyout', "$dir/$name.key", '-out', "$dir/$name.csr") == 0 or die; + open(my $fh, '>', "$dir/$name.ext") or die; + print $fh "subjectAltName=$san\n" if $san; + close($fh); + system(qw(openssl x509 -req -days 1 -set_serial), ++$cert_serial, '-in', "$dir/$name.csr", + '-CA', "$dir/$ca.pem", '-CAkey', "$dir/$ca.key", '-extfile', "$dir/$name.ext", + '-out', "$dir/$name.pem") == 0 or die; +} + sub role_reply { my $role = shift; return "*3\r\n\$6\r\nmaster\r\n:0\r\n*0\r\n" if $role eq 'master'; diff --git a/t/Makefile b/t/Makefile index bf262f41a..a73064257 100644 --- a/t/Makefile +++ b/t/Makefile @@ -75,6 +75,9 @@ LDLIBS += $(LDLIBS_CODEC_CHAIN) CFLAGS += $(CFLAGS_MQTT) LDLIBS += $(LDLIBS_MQTT) +CFLAGS += $(CFLAGS_LIBHIREDIS_SSL) +LDLIBS += $(LDLIBS_LIBHIREDIS_SSL) + COMMONOBJS := ../lib/str.o ../lib/auxlib.o ../lib/rtplib.o ../lib/loglib.o ../lib/ssllib.o include ../lib/common.Makefile @@ -95,6 +98,7 @@ include ../lib/common.Makefile daemon-tests-redis-sentinel-subscribe \ daemon-tests-redis-repair daemon-tests-redis-repair-ids \ daemon-tests-redis-resync \ + daemon-tests-redis-tls \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions daemon-tests-alias \ daemon-tests-kernel-api daemon-tests-dtmf-fanout \ daemon-tests-video daemon-tests-inject daemon-tests-dtx-dtmf @@ -154,6 +158,7 @@ daemon-tests: daemon-tests-main daemon-tests-jb daemon-tests-pubsub daemon-tests daemon-tests-redis-sentinel-subscribe \ daemon-tests-redis-repair daemon-tests-redis-repair-ids \ daemon-tests-redis-resync \ + daemon-tests-redis-tls \ daemon-tests-recording daemon-tests-create daemon-tests-created-subscriptions \ daemon-tests-dtx daemon-tests-dtx-cn daemon-tests-dtx-no-shift daemon-tests-dtx-dtmf \ daemon-tests-alias \ @@ -283,6 +288,9 @@ daemon-tests-redis-repair-ids: daemon-test-deps daemon-tests-redis-resync: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-resync.pl +daemon-tests-redis-tls: daemon-test-deps + ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-redis-tls.pl + daemon-tests-audio-player: daemon-test-deps ./auto-test-helper "$@" perl -I../perl auto-daemon-tests-audio-player.pl diff --git a/t/auto-daemon-tests-redis-tls.pl b/t/auto-daemon-tests-redis-tls.pl new file mode 100644 index 000000000..59346d886 --- /dev/null +++ b/t/auto-daemon-tests-redis-tls.pl @@ -0,0 +1,273 @@ +#!/usr/bin/perl + +# TLS to Redis: the certificate must come from the CA and match the address +# (or --redis-tls-server-name), a client certificate is sent, a handshake +# that gets no answer is given up, and the keyspace subscription speaks TLS +# too. Certificates are made here with the openssl tool. + +use strict; +use warnings; +use NGCP::Rtpengine::Test; +use NGCP::Rtpengine::AutoTest; +use NGCP::Rtpengine::FakeRedis; +use Test::More; +use File::Temp; +use IO::Socket::SSL; +use Time::HiRes qw(time sleep); + + +# the refusals at startup; a build without TLS support has no TLS option, +# and nothing else can be tested +sub refused_start { + my ($opts, $re, $n) = @_; + my $out = `timeout -k 1 10 $ENV{RTPE_BIN} --config-file=none -i 203.0.113.1 -n 2223 -f -E --redis=auth\@203.0.113.99:6379/2 $opts 2>&1`; + isnt($?, 0, "$n: refused"); + like($out, $re, "$n: explained"); + return $out; +} + +if (refused_start('--redis-certfile=x', qr/./, 'TLS option') =~ /Unknown option --redis-certfile/) { + pass('no TLS options in a build without TLS support'); + done_testing(); + exit(0); +} +refused_start('--redis-certfile=x', qr/Redis TLS needs --redis-cafile or --redis-capath/, + 'client certificate without a CA'); +refused_start('--redis-cafile=/dev/null --redis-certfile=x', + qr/--redis-certfile and --redis-keyfile must be given together/, 'certificate without a key'); +refused_start('--redis-cafile=/nonexistent', qr/cannot load the CA file or path/, 'unreadable CA file'); + + +my $d = File::Temp->newdir(); +tls_ca($d, $_) for qw(ca ca2); +tls_cert($d, 'ca', 'srv', 'IP:203.0.113.42'); +tls_cert($d, 'ca2', 'srv-ca2', 'IP:203.0.113.42'); +tls_cert($d, 'ca', 'srv-ip', 'IP:203.0.113.99'); +tls_cert($d, 'ca', 'rtpengine'); +tls_cert($d, 'ca', 'name', 'DNS:redis.sub.test'); +tls_cert($d, 'ca', 'name-other', 'DNS:other.sub.test'); +tls_cert($d, 'ca', 'name-wild', 'DNS:*.sub.test'); +tls_cert($d, 'ca', 'name-partial', 'DNS:re*.sub.test'); +tls_cert($d, 'ca', 'redis.sub.test'); # the name in the CN only + +sub cert { + my ($name, %opt) = @_; + redis_tls(SSL_cert_file => "$d/$name.pem", SSL_key_file => "$d/$name.key", %opt); +} +# the first server requires a client certificate from the CA +my %client_cert = (SSL_verify_mode => SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT, + SSL_ca_file => "$d/ca.pem"); + +my $l = redis_listen('203.0.113.42'); + +# a missing reply fails the test with the daemon's last log lines +local $SIG{ALRM} = sub { die "timeout\n" }; +$SIG{PIPE} = 'IGNORE'; + +my $sdp = < $before; + sleep(0.05); + } + fail("$n: logged"); +} + + +# verification of the address, client certificate + +my $fd; +cert('srv', %client_cert); +$NGCP::Rtpengine::AutoTest::launch_cb = sub { + $fd = redis_conn($l, 'startup'); + is($fd->peer_certificate('cn'), 'rtpengine', 'client certificate sent'); + ok(!defined($fd->get_servername()), 'no SNI for an IP address'); + redis_expect($fd, ['PING'], "+PONG\r\n", 'startup PING'); + redis_expect($fd, ['KEYS', '*'], "*0\r\n", 'startup KEYS'); +}; + +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 + -n 2223 -f -L 7 -E + --redis=auth@203.0.113.42:6379/2), + "--redis-cafile=$d/ca.pem", "--redis-certfile=$d/rtpengine.pem", + "--redis-keyfile=$d/rtpengine.key") + or die; + +new_call; +$NGCP::Rtpengine::req_cb = sub { redis_expect_set($fd, cid(), "+OK\r\n", 'write over TLS') }; +offer('write over TLS', { }, $sdp); + +cert('srv-ca2', %client_cert); +refused_cert($fd, qr/Failed to connect to Redis 203\.0\.113\.42:6379, error: TLS: certificate of 203\.0\.113\.42 not accepted: unable to get local issuer certificate/, + 'certificate from another CA'); + +cert('srv-ip', %client_cert); +refused_cert($fd, qr/error: TLS: certificate of 203\.0\.113\.42 not accepted: IP address mismatch/, + 'certificate for another address'); + +# no command timeout: the connect timeout (1 s) bounds the handshake +my $frozen; +new_call; +$NGCP::Rtpengine::req_cb = sub { + readable($l, 5) or die; + accept($frozen, $l) or die; +}; +my $start = time(); +alarm(5); +offer('handshake with no answer', { }, $sdp); +alarm(0); +my $took = time() - $start; +ok($took >= 0.9 && $took < 2.5, 'handshake with no answer given up after the connect timeout') + or diag("took $took s"); +wait_log(qr/error: TLS: handshake with 203\.0\.113\.42 timed out/, 'handshake timeout logged'); +close($frozen); + +# after a slow failure, one second without attempts +sleep(1.1); +cert('srv', %client_cert); +new_call; +$NGCP::Rtpengine::req_cb = sub { + $fd = redis_conn($l, 'reconnect'); + redis_expect_set($fd, cid(), "+OK\r\n", 'write after reconnecting', no_ping => 1); +}; +offer('reconnected over TLS', { }, $sdp); + +# the connect timeout bounded only the handshake: with no command timeout, a +# slow reply is waited for, on the same connection +new_call; +$NGCP::Rtpengine::req_cb = sub { + redis_expect($fd, ['PING'], undef, 'slow reply: PING'); + sleep(1.5); + redis_send($fd, "+PONG\r\n"); + redis_expect_set($fd, cid(), "+OK\r\n", 'slow reply: write', no_ping => 1); +}; +offer('slow reply after a TLS reconnect', { }, $sdp); +ok(!readable($l, 0), 'slow reply: no new connection'); + +NGCP::Rtpengine::AutoTest::shut_rtpe(); +undef($NGCP::Rtpengine::req_cb); + + +# --redis-tls-server-name, and the keyspace subscription + +cert('name'); +my ($notify_fd, $sub_fd); +my $psubscribed = "*3\r\n\$10\r\npsubscribe\r\n\$16\r\n__keyspace\@3__:*\r\n:1\r\n"; + +sub subscription { + my ($n) = @_; + my $s = redis_accept($l); + is($s->get_servername(), 'redis.sub.test', "$n: SNI"); + redis_expect($s, ['AUTH', 'auth'], "+OK\r\n", "$n AUTH"); + redis_expect($s, ['psubscribe', '__keyspace@3__:*'], $psubscribed, "$n psubscribe"); + return $s; +} + +$NGCP::Rtpengine::AutoTest::launch_cb = sub { + $fd = redis_conn($l, 'name: startup main'); + is($fd->get_servername(), 'redis.sub.test', 'name: SNI'); + $notify_fd = redis_conn($l, 'name: startup notify'); + + # two connections from two threads: the subscription, and the restore + # thread's copy of the notification connection. The subscription's + # handshake starts once its thread has checked the notification + # connection. + my @fds = map { redis_accept($l, 5, 1) } (0, 1); + redis_expect($notify_fd, ['PING'], "+PONG\r\n", 'name: startup notify PING'); + for my $s (@fds) { + redis_handshake($s); + redis_expect($s, ['AUTH', 'auth'], "+OK\r\n", 'name: startup AUTH'); + my $c = redis_read($s); + if ($c->[0] eq 'psubscribe') { + $sub_fd = $s; + is(join(' ', @$c), 'psubscribe __keyspace@3__:*', 'name: startup psubscribe'); + is($s->get_servername(), 'redis.sub.test', 'name: subscription SNI'); + redis_send($s, $psubscribed); + next; + } + is(join(' ', @$c), 'SELECT 2', 'name: restore copy SELECT'); + redis_send($s, "+OK\r\n"); + redis_expect($s, ['INFO'], info_reply('master'), 'name: restore copy INFO'); + redis_expect($s, ['TYPE', 'calls'], "+none\r\n", 'name: restore copy TYPE'); + redis_expect($s, ['PING'], "+PONG\r\n", 'name: restore copy PING'); + redis_expect($s, ['SELECT', '3'], "+OK\r\n", 'name: restore copy SELECT 3'); + redis_expect($s, ['KEYS', '*'], "*0\r\n", 'name: restore copy KEYS'); + redis_expect($s, ['SELECT', '2'], "+OK\r\n", 'name: restore copy SELECT back'); + } + redis_expect($fd, ['PING'], "+PONG\r\n", 'name: restore PING'); + redis_expect($fd, ['KEYS', '*'], "*0\r\n", 'name: restore KEYS'); +}; + +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 + -n 2223 -f -L 7 -E + --redis=auth@203.0.113.42:6379/2 + --subscribe-keyspace=3 --redis-num-threads=1 + --redis-tls-server-name=redis.sub.test + --redis-cmd-timeout=5000), + "--redis-cafile=$d/ca.pem") + or die; + +new_call; +$NGCP::Rtpengine::req_cb = sub { redis_expect_set($fd, cid(), "+OK\r\n", 'name: write') }; +offer('name: write over TLS', { }, $sdp); + +cert('name-other'); +refused_cert($fd, qr/error: TLS: certificate of redis\.sub\.test not accepted: hostname mismatch/, + 'name: certificate for another name'); + +cert('name-partial'); +my $name_refused = qr/error: TLS: certificate of redis\.sub\.test not accepted: hostname mismatch/; +refused_cert($fd, $name_refused, 'name: partial wildcard'); + +cert('redis.sub.test'); +refused_cert($fd, $name_refused, 'name: name in the CN only'); + +cert('name-wild'); +new_call; +$NGCP::Rtpengine::req_cb = sub { + $fd = redis_conn($l, 'name: wildcard'); + redis_expect_set($fd, cid(), "+OK\r\n", 'name: wildcard: write', no_ping => 1); +}; +offer('name: wildcard accepted', { }, $sdp); +cert('name-other'); + +# the subscription is made again, over TLS, with the same verification +redis_close($sub_fd); +redis_expect($notify_fd, ['PING'], "+PONG\r\n", 'name: subscription closed: connection checked'); +redis_tls_refused($l, 'name: subscription with a certificate for another name: refused'); +wait_log(qr/redis_notify_async_context_disconnect: TLS: certificate of redis\.sub\.test not accepted: hostname mismatch/, + 'name: subscription refusal logged with the reason'); +cert('name'); +redis_expect($notify_fd, ['PING'], "+PONG\r\n", 'name: subscription refused: connection checked'); +$sub_fd = subscription('name: subscription again'); + + +done_testing(); diff --git a/utils/gen-common-flags b/utils/gen-common-flags index a82022da4..9ba258d9a 100755 --- a/utils/gen-common-flags +++ b/utils/gen-common-flags @@ -99,6 +99,12 @@ if pkg-config --exists libmosquitto; then echo "CFLAGS_MQTT += -DHAVE_MQTT" fi +# hiredis 1.0 or newer: TLS to Redis +if pkg-config --exists hiredis_ssl; then + gen-pkgconf-flags LIBHIREDIS_SSL hiredis_ssl + echo "CFLAGS_LIBHIREDIS_SSL += -DHAVE_HIREDIS_SSL" +fi + # look for libsystemd if pkg-config --exists libsystemd; then gen-pkgconf-flags LIBSYSTEMD libsystemd From 739148c8b276d85f8e65bd71547c853c988f5cc7 Mon Sep 17 00:00:00 2001 From: Kevin Binetruy Date: Wed, 14 Oct 2026 16:58:06 +0000 Subject: [PATCH 7/7] MT#55283 redis: TLS for the Redis Sentinel connections With the TLS options, the Sentinel thread still spoke plain TCP to the Sentinels and for the role check of the master they name, so a Sentinel set with tls-port and port 0 could not be used. sentinel_connect() now starts TLS with the same context and options. A Sentinel's certificate must match its host as given in --redis-sentinel; a master's must match the address the Sentinels gave, or --redis-tls-server-name. The handshake happens in the thread, never under r->lock. Tested by auto-daemon-tests-redis-tls.pl. --- daemon/redis_sentinel.c | 11 ++++--- docs/rtpengine.md | 6 ++++ perl/NGCP/Rtpengine/FakeRedis.pm | 24 ++++++++++++-- t/auto-daemon-tests-redis-tls.pl | 54 ++++++++++++++++++++++++++++++-- 4 files changed, 87 insertions(+), 8 deletions(-) diff --git a/daemon/redis_sentinel.c b/daemon/redis_sentinel.c index 5e693454c..a50a800d4 100644 --- a/daemon/redis_sentinel.c +++ b/daemon/redis_sentinel.c @@ -144,7 +144,8 @@ void redis_sentinel_wake(struct redis_sentinel *s, bool limited) { } -static redisContext *sentinel_connect(const endpoint_t *ep, const char *auth) { +/* "name": what the TLS certificate must match, if not the address */ +static redisContext *sentinel_connect(const endpoint_t *ep, const char *name, const char *auth) { char host[64]; sockaddr_print(&ep->address, host, sizeof(host)); int64_t connect_timeout = atomic_get_na(&rtpe_config.redis_connect_timeout) * 1000LL; @@ -157,7 +158,8 @@ static redisContext *sentinel_connect(const endpoint_t *ep, const char *auth) { return NULL; const char *err = NULL; redisReply *rp = NULL; - if (ctx->err || redisSetTimeout(ctx, timeval_from_us(cmd_timeout))) + if (ctx->err || redisSetTimeout(ctx, timeval_from_us(cmd_timeout)) + || redis_tls_start(ctx, name ? : host, 0, NULL)) err = ctx->errstr; else if (auth && !(rp = redisCommand(ctx, "AUTH %s", auth))) err = ctx->errstr; @@ -220,7 +222,7 @@ static bool sentinel_query(struct redis_sentinel *s, struct sentinel_node *n, n->ep.address = a; } - redisContext *ctx = sentinel_connect(&n->ep, s->auth); + redisContext *ctx = sentinel_connect(&n->ep, n->host, s->auth); if (!ctx) { n->failing = true; return false; @@ -251,7 +253,8 @@ static bool sentinel_query(struct redis_sentinel *s, struct sentinel_node *n, } static bool sentinel_is_master(struct redis_sentinel *s, const endpoint_t *ep, uint64_t epoch) { - redisContext *ctx = sentinel_connect(ep, s->probe_auth); + // a data node: --redis-tls-server-name applies, as for the data connections + redisContext *ctx = sentinel_connect(ep, rtpe_config.redis_tls_server_name, s->probe_auth); if (!ctx) return false; diff --git a/docs/rtpengine.md b/docs/rtpengine.md index 890e987b1..05837b9ac 100644 --- a/docs/rtpengine.md +++ b/docs/rtpengine.md @@ -831,6 +831,12 @@ call to inject-DTMF won't be sent to __\-\-dtmf-log-dest=__ or __\-\-listen-tcp- __\-\-redis-connect-timeout__ bounds the handshake. These options are only available in a build with __hiredis\_ssl__ (hiredis 1.0 or newer). + With __\-\-redis-sentinel__, the Sentinels and the role checks of the + masters they name use TLS too, with the same files: a Sentinel's + certificate must match its host as given in __\-\-redis-sentinel__, a + master's the address that the Sentinels give, or + __\-\-redis-tls-server-name__. + - __\-\-redis-tls-server-name=__*NAME* The name to send (as SNI) and to expect in the certificate of every diff --git a/perl/NGCP/Rtpengine/FakeRedis.pm b/perl/NGCP/Rtpengine/FakeRedis.pm index 48cd7b2e1..9523fff4c 100644 --- a/perl/NGCP/Rtpengine/FakeRedis.pm +++ b/perl/NGCP/Rtpengine/FakeRedis.pm @@ -451,6 +451,8 @@ sub serve_probes { # (GET-MASTER-ADDR-BY-NAME) at this config epoch (SENTINEL MASTER), whose # "ip" is MONITORED_IP if given: a Sentinel running a failover reports the # promoted replica in the former and still the old master in the latter. +# The key "tls" gives "CERT_FILE KEY_FILE": the Sentinels speak TLS with +# them, and log "IP:PORT TLS refused" for a handshake that fails. my ($sentinel_pid, $sentinel_state, $sentinel_log, $sentinel_password); my $serial = 0; @@ -474,7 +476,8 @@ sub sentinels_set { $serial++; open(my $fh, '>', "$sentinel_state.new") or die; print $fh "serial $serial\npassword $sentinel_password\n"; - print $fh "$_:26379 $st{$_}\n" for sort(keys(%st)); + print $fh "tls $st{tls}\n" if $st{tls}; + print $fh "$_:26379 $st{$_}\n" for sort(grep { $_ ne 'tls' } keys(%st)); close($fh); rename("$sentinel_state.new", "$sentinel_state") or die; return unless $sentinel_pid; @@ -514,16 +517,23 @@ sub sentinel_main { my $done = 0; $SIG{TERM} = sub { $done = 1 }; $SIG{PIPE} = 'IGNORE'; - my (%sentinels, %clients, $password); + my (%sentinels, %clients, $password, $tls_files); my $applied = -1; my $read_state = sub { open(my $fh, '<', $statefile) or return; my ($ser, %want); + $tls_files = undef; while (my $l = <$fh>) { my @f = split(' ', $l); if ($f[0] eq 'serial') { $ser = $f[1] } elsif ($f[0] eq 'password') { $password = $f[1] } + elsif ($f[0] eq 'tls') { + # loaded now, not at the first connection: loading takes long + # enough on a busy host for the client's handshake to time out + require IO::Socket::SSL; + $tls_files = [ @f[1, 2] ]; + } else { $want{$f[0]} = [ @f[1 .. $#f] ] } } close($fh); @@ -579,6 +589,16 @@ sub sentinel_main { for my $fn (grep { vec($rout, $_, 1) } keys(%l)) { my ($s) = grep { $sentinels{$_} == $l{$fn} } keys(%sentinels); accept(my $fh, $l{$fn}{listener}) or next; + if ($tls_files) { + if (!IO::Socket::SSL->start_SSL($fh, SSL_server => 1, Timeout => 3, + SSL_cert_file => $tls_files->[0], + SSL_key_file => $tls_files->[1])) + { + print $log "$s TLS refused\n"; + close($fh); + next; + } + } $clients{fileno($fh)} = { fh => $fh, buf => '', sentinel => $s }; } for my $fn (grep { vec($rout, $_, 1) } keys(%clients)) { diff --git a/t/auto-daemon-tests-redis-tls.pl b/t/auto-daemon-tests-redis-tls.pl index 59346d886..e8e08a41b 100644 --- a/t/auto-daemon-tests-redis-tls.pl +++ b/t/auto-daemon-tests-redis-tls.pl @@ -2,8 +2,9 @@ # TLS to Redis: the certificate must come from the CA and match the address # (or --redis-tls-server-name), a client certificate is sent, a handshake -# that gets no answer is given up, and the keyspace subscription speaks TLS -# too. Certificates are made here with the openssl tool. +# that gets no answer is given up, and the keyspace subscription and the +# Redis Sentinel connections speak TLS too. Certificates are made here with +# the openssl tool. use strict; use warnings; @@ -49,6 +50,7 @@ sub refused_start { tls_cert($d, 'ca', 'name-wild', 'DNS:*.sub.test'); tls_cert($d, 'ca', 'name-partial', 'DNS:re*.sub.test'); tls_cert($d, 'ca', 'redis.sub.test'); # the name in the CN only +tls_cert($d, 'ca', 'sentinel', 'IP:203.0.113.43'); sub cert { my ($name, %opt) = @_; @@ -270,4 +272,52 @@ sub subscription { $sub_fd = subscription('name: subscription again'); +NGCP::Rtpengine::AutoTest::shut_rtpe(); +undef($NGCP::Rtpengine::req_cb); + + +# Redis Sentinel: the queries and the role probes speak TLS. The Sentinels +# are checked against their addresses, the data nodes (the role probe +# included) against --redis-tls-server-name. + +my $l44 = redis_listen('203.0.113.44'); +cert('name'); +my $sentinel_tls = "$d/sentinel.pem $d/sentinel.key"; +sentinels_start('spw', tls => $sentinel_tls, '203.0.113.43' => '203.0.113.42 6379 1'); + +$NGCP::Rtpengine::AutoTest::launch_cb = sub { + $fd = redis_conn($l, 'sentinel: startup'); + redis_expect($fd, ['PING'], "+PONG\r\n", 'sentinel: startup PING'); + redis_expect($fd, ['KEYS', '*'], "*0\r\n", 'sentinel: startup KEYS'); +}; + +autotest_start(qw(--config-file=none -t -1 -i 203.0.113.1 + -n 2223 -f -L 7 -E + --redis=auth@sentinel/2 + --redis-sentinel-master=mymaster --redis-sentinel-interval=200 + --redis-cmd-timeout=1000 --redis-tls-server-name=redis.sub.test), + '--redis-sentinel=spw@203.0.113.43:26379', "--redis-cafile=$d/ca.pem") + or die; + +like(sentinel_log(), qr/^203\.0\.113\.43:26379 AUTH spw$/m, 'sentinel: queried over TLS'); +is_deeply(\@probes, [['203.0.113.42', 'auth', 'master']], 'sentinel: role probe over TLS'); + +# a Sentinel whose certificate is for another address is not asked +sentinels_set(tls => "$d/srv-ip.pem $d/srv-ip.key", '203.0.113.43' => '203.0.113.42 6379 1'); +wait_log(qr/Redis Sentinel: cannot connect to 203\.0\.113\.43:26379: TLS: certificate of 203\.0\.113\.43 not accepted: IP address mismatch/, + 'sentinel: certificate for another address refused'); +like(sentinel_log(), qr/^203\.0\.113\.43:26379 TLS refused$/m, 'sentinel: handshake not completed'); + +# a master whose certificate is for another name is not followed +cert('name-other'); +sentinels_set(tls => $sentinel_tls, '203.0.113.43' => '203.0.113.44 6379 2'); +redis_tls_refused($l44, 'sentinel: role probe of a master with the wrong certificate'); +wait_log(qr/Redis Sentinel: cannot connect to 203\.0\.113\.44:6379: TLS: certificate of redis\.sub\.test not accepted: hostname mismatch/, + 'sentinel: role probe refused the certificate'); +new_call; +$NGCP::Rtpengine::req_cb = sub { redis_expect_set($fd, cid(), "+OK\r\n", 'sentinel: write') }; +offer('sentinel: still on the master in use', { }, $sdp); +unlike(rtpe_log(), qr/master of 'mymaster' is now 203\.0\.113\.44/, 'sentinel: not followed'); + + done_testing();