diff --git a/docker-compose.yml b/docker-compose.yml index 178318e8ae..12c4bd7c1b 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -336,6 +336,65 @@ services: profiles: - dev + # Opt-in replica nodes, one per master, enabled with the redis-cluster-replicas profile: + # docker compose --profile dev --profile redis-cluster-replicas up -d redis-cluster-configure + # redis-cluster-create.sh picks up whichever of these are running and joins them with + # --cluster-replicas 1, which is what makes read_only=true routing testable locally. Without the + # profile the cluster stays three masters with no replicas. + redis-cluster-node-4: + image: redis:${DC_REDIS_VERSION:-7.2.4}-alpine + command: redis-server /usr/local/etc/redis/redis.conf + networks: + - primary + ports: + - '7004:6379' + - '16374:16379' + volumes: + - ./docker/redis/redis-cluster.conf:/usr/local/etc/redis/redis.conf + healthcheck: + test: ['CMD', 'redis-cli', '-p', '6379', 'ping'] + interval: 10s + timeout: 5s + retries: 3 + profiles: + - redis-cluster-replicas + + redis-cluster-node-5: + image: redis:${DC_REDIS_VERSION:-7.2.4}-alpine + command: redis-server /usr/local/etc/redis/redis.conf + networks: + - primary + ports: + - '7005:6379' + - '16375:16379' + volumes: + - ./docker/redis/redis-cluster.conf:/usr/local/etc/redis/redis.conf + healthcheck: + test: ['CMD', 'redis-cli', '-p', '6379', 'ping'] + interval: 10s + timeout: 5s + retries: 3 + profiles: + - redis-cluster-replicas + + redis-cluster-node-6: + image: redis:${DC_REDIS_VERSION:-7.2.4}-alpine + command: redis-server /usr/local/etc/redis/redis.conf + networks: + - primary + ports: + - '7006:6379' + - '16376:16379' + volumes: + - ./docker/redis/redis-cluster.conf:/usr/local/etc/redis/redis.conf + healthcheck: + test: ['CMD', 'redis-cli', '-p', '6379', 'ping'] + interval: 10s + timeout: 5s + retries: 3 + profiles: + - redis-cluster-replicas + redis-cluster-configure: image: redis:${DC_REDIS_VERSION:-7.2.4}-alpine command: /usr/local/etc/redis/redis-cluster-create.sh @@ -348,6 +407,18 @@ services: condition: service_healthy redis-cluster-node-3: condition: service_healthy + redis-cluster-node-4: + condition: service_healthy + # Absent unless the redis-cluster-replicas profile is enabled. + required: false + redis-cluster-node-5: + condition: service_healthy + # Absent unless the redis-cluster-replicas profile is enabled. + required: false + redis-cluster-node-6: + condition: service_healthy + # Absent unless the redis-cluster-replicas profile is enabled. + required: false volumes: - ./docker/redis/:/usr/local/etc/redis/ restart: on-failure:1 @@ -433,4 +504,7 @@ volumes: redis-cluster-node-1: redis-cluster-node-2: redis-cluster-node-3: + redis-cluster-node-4: + redis-cluster-node-5: + redis-cluster-node-6: plugin-registry: diff --git a/docker/redis/redis-cluster-create.sh b/docker/redis/redis-cluster-create.sh index 3c91a0cc3f..6e5bdbfffc 100755 --- a/docker/redis/redis-cluster-create.sh +++ b/docker/redis/redis-cluster-create.sh @@ -1,23 +1,56 @@ # wait for the docker-compose depends_on to spin up the redis nodes usually takes this long sleep 10 -node_1_ip=$(getent hosts redis-cluster-node-1 | awk '{ print $1 }') -node_2_ip=$(getent hosts redis-cluster-node-2 | awk '{ print $1 }') -node_3_ip=$(getent hosts redis-cluster-node-3 | awk '{ print $1 }') +resolve() { + getent hosts "$1" | awk '{ print $1 }' +} + +master_ips="" +for node in redis-cluster-node-1 redis-cluster-node-2 redis-cluster-node-3; do + ip=$(resolve $node) + if [ -z "$ip" ]; then + echo "$node did not resolve, cannot create the cluster" + exit 1 + fi + master_ips="$master_ips $ip" +done + +# Nodes 4-6 only run when the redis-cluster-replicas compose profile is enabled, so they are +# picked up when they resolve and left out of the cluster otherwise. +replica_ips="" +replica_count=0 +for node in redis-cluster-node-4 redis-cluster-node-5 redis-cluster-node-6; do + ip=$(resolve $node) + if [ -z "$ip" ]; then + continue + fi + replica_ips="$replica_ips $ip" + replica_count=$((replica_count + 1)) +done + +# redis-cli spreads replicas evenly over the masters, so it takes one each or none at all. +replicas_per_master=0 +if [ "$replica_count" -eq 3 ]; then + replicas_per_master=1 +elif [ "$replica_count" -ne 0 ]; then + echo "Only $replica_count of the 3 replica nodes are running, creating the cluster without replicas" + replica_ips="" +fi + +node_ips="$master_ips $replica_ips" # Prepare the nodes for the cluster -for ip in $node_1_ip $node_2_ip $node_3_ip; do +for ip in $node_ips; do echo "Emptying db 0 of Redis node at $ip and resetting cluster" redis-cli -h $ip -p 6379 FLUSHDB redis-cli -h $ip -p 6379 CLUSTER RESET redis-cli -h $ip -p 6379 CONFIG SET cluster-announce-ip "$ip" done -# Create the cluster +# Create the cluster. The masters come first, so the replica nodes that follow become their +# replicas, which is what read_only=true routing needs to be exercised. redis-cli --cluster create \ - $node_1_ip:6379 \ - $node_2_ip:6379 \ - $node_3_ip:6379 \ - --cluster-replicas 0 --cluster-yes + $(for ip in $node_ips; do printf '%s:6379 ' "$ip"; done) \ + --cluster-replicas $replicas_per_master --cluster-yes -echo "Redis Cluster setup complete!" \ No newline at end of file +echo "Redis Cluster setup complete!" diff --git a/docs-website/router/configuration.mdx b/docs-website/router/configuration.mdx index 0a5b720fcc..881adc6169 100644 --- a/docs-website/router/configuration.mdx +++ b/docs-website/router/configuration.mdx @@ -1321,9 +1321,9 @@ These apply when `cluster_enabled: true`. | Parameter | Description | Default Value | | ---------------- | ------------------------------------------------------------------------------------------ | ------------- | | max_redirects | Number of MOVED/ASK redirects to follow before giving up on a command. | 3 | -| read_only | Allow routing read-only commands to replicas. | false | -| route_by_latency | Route read-only commands to the closest node by measured latency. Implies `read_only`. | false | -| route_randomly | Route read-only commands to a random node. Implies `read_only`. | false | +| read_only | Route read-only commands to a replica of the shard that owns the key, instead of its master. Requires a cluster that has replicas, see [Reading from replicas](/router/configuration#reading-from-replicas). | false | +| route_by_latency | Route read-only commands to whichever node of the shard has the lowest measured latency, the master included. Implies `read_only`. | false | +| route_randomly | Route read-only commands to a random node of the shard, the master included. Implies `read_only`. | false | With `cluster_enabled: true`, query parameters are only read from the **first** URL in the list. The router uses the remaining URLs as additional cluster seed addresses and takes only their host and port, so parameters placed on them are silently dropped. Put all connection options on the first URL. @@ -1342,6 +1342,35 @@ These apply when `cluster_enabled: true`. Setting `max_active_conns` lower than `pool_size` therefore admits more concurrent commands than there are connections available for, and the excess fails instead of queueing. Keep `max_active_conns` at or above `pool_size`, or leave it unset. +### Reading from replicas + +`read_only=true` sends read-only commands to a replica of the shard that owns the key instead of to its master, which spreads read traffic over more nodes. It is a Redis Cluster feature, so it needs `cluster_enabled: true`. + +```yaml config.yaml +storage_providers: + redis: + - id: "redis-provider" + urls: + - "redis://localhost:7001?read_only=true" # options read from the first URL only + - "redis://localhost:7002" + - "redis://localhost:7003" + cluster_enabled: true +``` + +Only commands that Redis itself flags as read-only are routed this way. Writes always go to the master, and so do Lua scripts, because `EVALSHA` carries no read-only flag. The rate limiter's counter script therefore keeps running on the master and is unaffected by this setting, while features that read with a plain `GET`, such as automatic persisted queries, start reading from replicas. + +The cluster has to actually have replicas for the setting to do anything. Against a cluster of masters only it changes nothing, and a shard whose replica is unreachable falls back to its master. Reads keep working in both cases, they just are not distributed, and neither case is reported as an error. + + + Replication is asynchronous, so a replica can answer with an older value than the master holds, or miss a key entirely for a moment after it is written. A `GET` that immediately follows its `SET` may return the previous value or nothing at all. For automatic persisted queries that means an operation can look unregistered on the request right after it was registered. Only enable `read_only` where a stale read is acceptable. + + +`route_by_latency` and `route_randomly` both imply `read_only`, but they choose among all nodes of the shard, the master included. They distribute reads without guaranteeing that a replica serves them, and `route_by_latency` in particular often settles on the master when it is the closest node. Plain `read_only` always prefers a replica. + + + These three parameters are only recognised on a cluster URL. Without `cluster_enabled: true` the router fails at startup with `redis: unexpected option: read_only`. + + ### Sizing the pool For a Redis Cluster these limits apply **per node**, not to the cluster as a whole: `pool_size=20` against a six-node cluster permits up to 20 connections to each node. diff --git a/router/internal/rediscloser/read_replica_test.go b/router/internal/rediscloser/read_replica_test.go new file mode 100644 index 0000000000..9ac65592db --- /dev/null +++ b/router/internal/rediscloser/read_replica_test.go @@ -0,0 +1,375 @@ +package rediscloser + +import ( + "context" + "fmt" + "net" + "net/url" + "os" + "strconv" + "strings" + "sync" + "testing" + "time" + + "github.com/redis/go-redis/v9" + "github.com/stretchr/testify/require" + "go.uber.org/zap" +) + +// read_only is one of the query parameters a user can put on the Redis URL (see url_options_test.go +// for the parsing side). Unlike the pool parameters it changes where commands are sent: go-redis +// routes read-only commands to a replica of the owning shard instead of its master. That only +// exists for cluster clients, so these tests need a real cluster with replicas. The local one has +// them behind an opt-in compose profile, six nodes on 7001-7006 joined with --cluster-replicas 1: +// +// docker compose --profile dev --profile redis-cluster-replicas up -d redis-cluster-configure +// +// The assertion is made server side: each node's INFO commandstats says how many GETs it actually +// served, which is the ground truth for "the read was served by a replica". The tests skip when no +// such cluster is reachable, including the default replica-less one, so `make test` stays green +// without infra. +// +// go test ./internal/rediscloser/ -run TestClusterReadOnly -v + +const clusterURLsEnv = "TEST_REDIS_CLUSTER_URLS" + +var defaultTestRedisClusterURLs = []string{ + "redis://localhost:7001", + "redis://localhost:7002", + "redis://localhost:7003", + "redis://localhost:7004", + "redis://localhost:7005", + "redis://localhost:7006", +} + +// TestClusterReadOnlyRoutesReadsToReplicas asserts that read_only=true on the URL makes the +// replicas serve every GET, and that they serve the values the masters were written with. +func TestClusterReadOnlyRoutesReadsToReplicas(t *testing.T) { + const repeats = 3 + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + client := newTestClusterClient(t, ctx, "read_only=true") + nodes := awaitClusterNodesByRole(t, ctx, client) + + // Writes still go to the masters even on a read_only client, so this also covers that the + // flag does not break writes. + keys := writeTestKeys(t, ctx, client) + + // A replica may not have the key yet, and a GET is counted either way, so settle replication + // before measuring rather than reading through a moving target. + require.Eventually(t, func() bool { + for key, want := range keys { + if got, err := client.Get(ctx, key).Result(); err != nil || got != want { + return false + } + } + return true + }, 10*time.Second, 50*time.Millisecond, "keys never became readable through the read_only client") + + nodes.resetStats(t, ctx) + + for range repeats { + for key, want := range keys { + got, err := client.Get(ctx, key).Result() + require.NoError(t, err) + require.Equal(t, want, got, "replica served a stale or wrong value for %s", key) + } + } + + masterGets, replicaGets := nodes.getCalls(t, ctx) + t.Logf("read_only=true: %d GETs on %d masters, %d GETs on %d replicas", + masterGets, len(nodes.masters), replicaGets, len(nodes.replicas)) + + require.Equal(t, int64(len(keys)*repeats), replicaGets, "every read should have been served by a replica") + require.Zero(t, masterGets, "no read should have reached a master") +} + +// TestClusterWithoutReadOnlyRoutesReadsToMasters is the counterpart: without the flag the masters +// serve the reads, so a passing read_only test cannot be explained by the cluster topology alone. +func TestClusterWithoutReadOnlyRoutesReadsToMasters(t *testing.T) { + const repeats = 3 + + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + client := newTestClusterClient(t, ctx, "") + nodes := awaitClusterNodesByRole(t, ctx, client) + + keys := writeTestKeys(t, ctx, client) + + nodes.resetStats(t, ctx) + + for range repeats { + for key, want := range keys { + got, err := client.Get(ctx, key).Result() + require.NoError(t, err) + require.Equal(t, want, got) + } + } + + masterGets, replicaGets := nodes.getCalls(t, ctx) + t.Logf("read_only unset: %d GETs on %d masters, %d GETs on %d replicas", + masterGets, len(nodes.masters), replicaGets, len(nodes.replicas)) + + require.Equal(t, int64(len(keys)*repeats), masterGets, "every read should have been served by a master") + require.Zero(t, replicaGets, "no read should have reached a replica by default") +} + +// newTestClusterClient builds the cluster client under test through NewRedisCloser, so the URL +// handling the router owns is part of what is exercised. params are appended to the first URL, +// which is where go-redis reads cluster options from. +func newTestClusterClient(tb testing.TB, ctx context.Context, params string) *redis.ClusterClient { + tb.Helper() + + urls := testRedisClusterURLs(tb) + if params != "" { + urls[0] = withParams(tb, urls[0], params) + } + + pingCtx, cancel := context.WithTimeout(ctx, 10*time.Second) + defer cancel() + + cl, err := NewRedisCloser(&RedisCloserOptions{ + Logger: zap.NewNop(), + URLs: urls, + ClusterEnabled: true, + Context: pingCtx, + }) + if err != nil { + // The seed port answered but the cluster did not. The usual cause is that the nodes + // announce their container IPs (see redis-cluster-create.sh), which the host can only + // reach with a Docker runtime that routes them, so this is a skip and not a failure. + tb.Skipf("no usable redis cluster at %v, skipping: %v", urls, err) + } + tb.Cleanup(func() { _ = cl.Close() }) + + client, ok := cl.(*redis.ClusterClient) + require.True(tb, ok, "cluster mode must produce a *redis.ClusterClient") + + return client +} + +// testRedisClusterURLs returns the cluster seed URLs, skipping the caller when nothing is +// listening on the first one. +func testRedisClusterURLs(tb testing.TB) []string { + tb.Helper() + + urls := defaultTestRedisClusterURLs + if raw := os.Getenv(clusterURLsEnv); raw != "" { + urls = strings.Split(raw, ",") + } + // Copied because the caller appends query parameters to the first entry. + urls = append([]string(nil), urls...) + + parsed, err := url.Parse(urls[0]) + require.NoError(tb, err, "%s does not hold valid URLs", clusterURLsEnv) + + conn, err := net.DialTimeout("tcp", parsed.Host, 500*time.Millisecond) + if err != nil { + tb.Skipf("no redis cluster reachable at %s, skipping: %v", parsed.Host, err) + } + _ = conn.Close() + + return urls +} + +// writeTestKeys writes one key per attempt to spread them over the shards, and cleans them up. +// The returned map is key -> expected value. +func writeTestKeys(tb testing.TB, ctx context.Context, client *redis.ClusterClient) map[string]string { + tb.Helper() + + const keyCount = 12 + + prefix := fmt.Sprintf("cosmo_read_replica_%d_%s:", time.Now().UnixNano(), tb.Name()) + keys := make(map[string]string, keyCount) + for i := range keyCount { + keys[fmt.Sprintf("%s%d", prefix, i)] = strconv.Itoa(i) + } + + for key, value := range keys { + require.NoError(tb, client.Set(ctx, key, value, 5*time.Minute).Err()) + } + + tb.Cleanup(func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + for key := range keys { + client.Del(cleanupCtx, key) + } + }) + + return keys +} + +// clusterRoles holds a per-node client for every master and every replica the cluster reports. +type clusterRoles struct { + masters []*redis.Client + replicas []*redis.Client +} + +// awaitClusterNodesByRole waits until every shard reports a replica. Right after the cluster is +// created a node can still answer CLUSTER SLOTS with a shard that has no replica yet, and reads for +// that shard fall back to its master, which would make these tests flaky. The check has to be per +// shard rather than a replica count: one master with two replicas and another with none would +// satisfy any count comparison while still leaving a shard whose reads cannot reach a replica. +func awaitClusterNodesByRole(tb testing.TB, ctx context.Context, client *redis.ClusterClient) clusterRoles { + tb.Helper() + + // A cluster with no replicas at all is the default local setup, not a cluster still coming up, + // so it must not cost the poll below its whole deadline before skipping. + if clusterReplicaCount(tb, ctx, client) == 0 { + tb.Skip("redis cluster has no replicas, start them with the redis-cluster-replicas compose profile") + } + + var ( + roles clusterRoles + replicaless []string + ) + for deadline := time.Now().Add(15 * time.Second); ; time.Sleep(250 * time.Millisecond) { + // This reloads the cluster state (ForEachMaster/ForEachSlave go through ReloadOrGet), so + // polling is also what makes the client itself pick up the full topology. + roles = clusterNodesByRole(tb, ctx, client) + + slots, err := client.ClusterSlots(ctx).Result() + require.NoError(tb, err, "CLUSTER SLOTS") + + replicaless = shardsWithoutReplica(slots) + if len(replicaless) == 0 || time.Now().After(deadline) { + break + } + } + + require.Empty(tb, replicaless, + "these shards have no replica, so their reads can only be served by their master") + + return roles +} + +// clusterReplicaCount reports how many replicas the cluster knows about. Replication is set up +// while the cluster is created, so unlike the CLUSTER SLOTS view that routing is built from, this +// answer does not need time to propagate and can be trusted straight away. +func clusterReplicaCount(tb testing.TB, ctx context.Context, client *redis.ClusterClient) int { + tb.Helper() + + // 4f1b1a... 192.168.107.27:6379@16379 slave 1cff53... 0 1787305644000 2 connected + raw, err := client.ClusterNodes(ctx).Result() + require.NoError(tb, err, "CLUSTER NODES") + + var replicas int + for line := range strings.SplitSeq(raw, "\n") { + fields := strings.Fields(line) + if len(fields) < 3 { + continue + } + // The flags field is comma separated ("myself,master"), and redis still calls a replica a + // slave here. + for flag := range strings.SplitSeq(fields[2], ",") { + if flag == "slave" { + replicas++ + } + } + } + + return replicas +} + +// shardsWithoutReplica names the slot ranges that CLUSTER SLOTS reports with a master and nothing +// else. A shard is only usable for these tests when it has at least one replica. +func shardsWithoutReplica(slots []redis.ClusterSlot) []string { + var replicaless []string + for _, slot := range slots { + if len(slot.Nodes) >= 2 { + continue + } + master := "no node" + if len(slot.Nodes) == 1 { + master = slot.Nodes[0].Addr + } + replicaless = append(replicaless, fmt.Sprintf("slots %d-%d (master %s)", slot.Start, slot.End, master)) + } + return replicaless +} + +func clusterNodesByRole(tb testing.TB, ctx context.Context, client *redis.ClusterClient) clusterRoles { + tb.Helper() + + var ( + mu sync.Mutex + roles clusterRoles + ) + + require.NoError(tb, client.ForEachMaster(ctx, func(_ context.Context, node *redis.Client) error { + mu.Lock() + defer mu.Unlock() + roles.masters = append(roles.masters, node) + return nil + })) + require.NoError(tb, client.ForEachSlave(ctx, func(_ context.Context, node *redis.Client) error { + mu.Lock() + defer mu.Unlock() + roles.replicas = append(roles.replicas, node) + return nil + })) + + require.NotEmpty(tb, roles.masters, "cluster reported no masters") + + return roles +} + +// resetStats zeroes the command counters on every node so only the commands the test issues +// afterwards are counted. +func (r clusterRoles) resetStats(tb testing.TB, ctx context.Context) { + tb.Helper() + + nodeClients := make([]*redis.Client, 0, len(r.masters)+len(r.replicas)) + nodeClients = append(nodeClients, r.masters...) + nodeClients = append(nodeClients, r.replicas...) + + for _, node := range nodeClients { + require.NoError(tb, node.ConfigResetStat(ctx).Err(), "CONFIG RESETSTAT on %s", node.Options().Addr) + } +} + +// getCalls returns how many GETs the masters and the replicas served since the last resetStats. +func (r clusterRoles) getCalls(tb testing.TB, ctx context.Context) (masters, replicas int64) { + tb.Helper() + + for _, node := range r.masters { + masters += getCallsOnNode(tb, ctx, node) + } + for _, node := range r.replicas { + replicas += getCallsOnNode(tb, ctx, node) + } + return masters, replicas +} + +func getCallsOnNode(tb testing.TB, ctx context.Context, node *redis.Client) int64 { + tb.Helper() + + info, err := node.Info(ctx, "commandstats").Result() + require.NoError(tb, err, "INFO commandstats on %s", node.Options().Addr) + + // cmdstat_get:calls=6,usec=41,usec_per_call=6.83,rejected_calls=0,failed_calls=0 + for line := range strings.SplitSeq(info, "\n") { + fields, ok := strings.CutPrefix(strings.TrimSpace(line), "cmdstat_get:") + if !ok { + continue + } + for field := range strings.SplitSeq(fields, ",") { + calls, ok := strings.CutPrefix(field, "calls=") + if !ok { + continue + } + parsed, err := strconv.ParseInt(calls, 10, 64) + require.NoError(tb, err, "parsing %q from %s", line, node.Options().Addr) + tb.Logf("%s served %d GETs", node.Options().Addr, parsed) + return parsed + } + } + + // No cmdstat_get line at all means the node served no GET since the reset. + return 0 +} diff --git a/router/internal/rediscloser/url_options_test.go b/router/internal/rediscloser/url_options_test.go index 9f3438846b..4816bdc94a 100644 --- a/router/internal/rediscloser/url_options_test.go +++ b/router/internal/rediscloser/url_options_test.go @@ -64,7 +64,7 @@ func TestURLQueryParamsSurviveClusterURLRewrite(t *testing.T) { "redis://localhost:7001?pool_size=11&min_idle_conns=3&max_idle_conns=6" + "&max_active_conns=13&pool_timeout=5s&conn_max_idle_time=4m&conn_max_lifetime=25m" + "&dial_timeout=3s&read_timeout=8s&write_timeout=9s&max_retries=4" + - "&max_redirects=6&route_by_latency=true", + "&max_redirects=6&route_by_latency=true&read_only=true", "redis://localhost:7002", "redis://localhost:7003", }, @@ -93,6 +93,7 @@ func TestURLQueryParamsSurviveClusterURLRewrite(t *testing.T) { require.Equal(t, 4, clusterOpts.MaxRetries) require.Equal(t, 6, clusterOpts.MaxRedirects) require.True(t, clusterOpts.RouteByLatency) + require.True(t, clusterOpts.ReadOnly) } // TestClusterQueryParamsOnlyReadFromFirstURL documents a sharp edge of the cluster branch: only