[Core] GCS FT with redis sentinel #47335

kanwang · 2024-08-26T13:27:08Z

Why are these changes needed?

We want to use redis sentinel to support Ray GCS FT. I opened a ticket here: #46983. Redis sentinel should provide high availability without worrying too much about redis cluster operations.

Related issue number

Closes #46983

Checks

I've signed off every commit(by using the -s flag, i.e., git commit -s) in this PR.
I've run scripts/format.sh to lint the changes in this PR.
I've included any doc changes needed for https://docs.ray.io/en/master/.
- I've added any new APIs to the API Reference. For example, if I added a
  method in Tune, I've added it in doc/source/tune/api/ under the
  corresponding .rst file.
I've made sure the tests are passing. Note that there might be a few flaky tests, see the recent failures at https://flakey-tests.ray.io/
Testing Strategy
- Unit tests
- Release tests
- This PR is not tested :(

Signed-off-by: Kan Wang <[email protected]>

kanwang · 2024-08-26T13:35:58Z

@jjyao @rkooo567 would you be able to help review this?

Signed-off-by: Kan Wang <[email protected]>

kanwang · 2024-09-08T20:34:38Z

hey @rkooo567 gentle ping on this PR.

kanwang · 2024-09-24T16:41:01Z

Hey,
Gentle ping on this. Could I get some review please? thank you!

anyscalesam · 2024-10-09T19:39:17Z

@kanwang per Ray Slack; aiming to get to this early next week

kanwang · 2024-10-17T13:28:59Z

hey @anyscalesam
Any movement here? sorry for keep bugging you guys!

jjyao · 2024-10-17T15:12:59Z

Hi @kanwang sorry, the team is a bit busy right now. We will review it ASAP.

kanwang · 2024-10-25T13:55:31Z

Hey team, any chance that we can get this reviewed this week or early next week? Thank you! cc @jjyao

jjyao

Very sorry for the late review. I'll actively review this PR from now on.

doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

jjyao · 2024-10-29T06:21:14Z

src/ray/gcs/redis_context.cc

@@ -506,6 +564,14 @@ Status RedisContext::Connect(const std::string &address,
  // Ray has some restrictions for RedisDB. Validate it here.
  ValidateRedisDB(*this);


If it's Redis Sentinel, what will INFO CLUSTER returns?

It will return empty - basically that section doesn't exist. Here's a full result of INFO on redis sentinel:

# Server redis_version:7.0.7 redis_git_sha1:00000000 redis_git_dirty:0 redis_build_id:5dad631ce1b0fc10 redis_mode:sentinel os:Linux 6.8.0-1017-aws x86_64 arch_bits:64 monotonic_clock:POSIX clock_gettime multiplexing_api:epoll atomicvar_api:c11-builtin gcc_version:9.4.0 process_id:7 process_supervised:no run_id:de8c925350decfbb0abd5940bf718855eb191f8d tcp_port:26379 server_time_usec:1730346466295003 uptime_in_seconds:728236 uptime_in_days:8 hz:14 configured_hz:10 lru_clock:2293218 executable:/redis-sentinel config_file:/etc/redis/sentinel.conf io_threads_active:0 # Clients connected_clients:3 cluster_connections:0 maxclients:10000 client_recent_max_input_buffer:20480 client_recent_max_output_buffer:0 blocked_clients:0 tracking_clients:0 clients_in_timeout_table:0 # Stats total_connections_received:121374 total_commands_processed:2334908 instantaneous_ops_per_sec:3 total_net_input_bytes:135052866 total_net_output_bytes:163753996 total_net_repl_input_bytes:0 total_net_repl_output_bytes:0 instantaneous_input_kbps:0.17 instantaneous_output_kbps:0.02 instantaneous_input_repl_kbps:0.00 instantaneous_output_repl_kbps:0.00 rejected_connections:0 sync_full:0 sync_partial_ok:0 sync_partial_err:0 expired_keys:0 expired_stale_perc:0.00 expired_time_cap_reached_count:0 expire_cycle_cpu_milliseconds:12828 evicted_keys:0 evicted_clients:0 total_eviction_exceeded_time:0 current_eviction_exceeded_time:0 keyspace_hits:0 keyspace_misses:0 pubsub_channels:0 pubsub_patterns:0 pubsubshard_channels:0 latest_fork_usec:0 total_forks:0 migrate_cached_sockets:0 slave_expires_tracked_keys:0 active_defrag_hits:0 active_defrag_misses:0 active_defrag_key_hits:0 active_defrag_key_misses:0 total_active_defrag_time:0 current_active_defrag_time:0 tracking_total_keys:0 tracking_total_items:0 tracking_total_prefixes:0 unexpected_error_replies:0 total_error_replies:97084 dump_payload_sanitizations:0 total_reads_processed:2502068 total_writes_processed:2380696 io_threaded_reads_processed:0 io_threaded_writes_processed:0 reply_buffer_shrinks:29 reply_buffer_expands:0 # CPU used_cpu_sys:1282.061897 used_cpu_user:931.763629 used_cpu_sys_children:0.032320 used_cpu_user_children:0.026019 used_cpu_sys_main_thread:1282.061999 used_cpu_user_main_thread:931.762826 # Sentinel sentinel_masters:1 sentinel_tilt:0 sentinel_tilt_since_seconds:-1 sentinel_running_scripts:0 sentinel_scripts_queue_length:0 sentinel_simulate_failure_flags:0 master0:name=redis-ha,status=ok,address=10.112.187.90:6379,slaves=2,sentinels=3

jjyao · 2024-10-29T06:23:56Z

src/ray/gcs/redis_context.cc

+    // directly. continue otherwise.
+    return sentinel_status;
+  }
+
  // Find the true leader


This is needed only for Redis Cluster.

I feel our high level code for RedisContext::Connect should be

if (redis_cluster) { ValidateRedisCluster() // make sure it only has 1 shard FindMasterUsingMOVED() } else { // redis sentinel FindMasterUsingSentinel() }

thanks for the suggestion! refactored a little bit. so now the logic in RedisContext::Connect should be mostly untouched: added one validation of redis_sentienel and an else branch to handle sentinel.

this should be ready for re-review.

Co-authored-by: Jiajun Yao <[email protected]> Signed-off-by: kanwang <[email protected]>

Signed-off-by: Kan Wang <[email protected]> Signed-off-by: Kan Wang <[email protected]>

Signed-off-by: Kan Wang <[email protected]>

Signed-off-by: Kan Wang <[email protected]> Signed-off-by: Kan Wang <[email protected]>

kanwang · 2024-11-06T14:35:25Z

hey @jjyao gentle ping on this. thank you!

kanwang · 2024-11-13T18:01:45Z

hey @jjyao checking on this again since all tests passed now.

jjyao · 2024-11-13T19:36:37Z

Hi @kanwang I'll take another review this week.

jjyao · 2024-11-18T20:58:09Z

src/ray/gcs/redis_context.cc

@@ -431,6 +431,69 @@ void ValidateRedisDB(RedisContext &context) {
  }
 }

+Status ValidateRedisSentinel(RedisContext &context) {


bool IsRedisSentinel(RedisContext &context) { }

jjyao · 2024-11-18T21:58:36Z

src/ray/gcs/redis_context.cc

+    // if type error, this is a redis cluster. continue to validate and connect
+
+    // Ray has some restrictions for RedisDB. Validate it here.
+    ValidateRedisDB(*this);
+
+    // Find the true leader
+    std::vector<const char *> argv;
+    std::vector<size_t> argc;
+    std::vector<std::string> cmds = {"DEL", "DUMMY"};
+    for (const auto &arg : cmds) {
+      argv.push_back(arg.data());
+      argc.push_back(arg.size());
+    }

-  // Find the true leader
-  std::vector<const char *> argv;
-  std::vector<size_t> argc;
-  std::vector<std::string> cmds = {"DEL", "DUMMY"};
-  for (const auto &arg : cmds) {
-    argv.push_back(arg.data());
-    argc.push_back(arg.size());
-  }
+    auto redis_reply = reinterpret_cast<redisReply *>(
+        ::redisCommandArgv(context_.get(), cmds.size(), argv.data(), argc.data()));
+
+    if (redis_reply->type == REDIS_REPLY_ERROR) {
+      // This should be a MOVED error
+      // MOVED 14946 10.xx.xx.xx:7001
+      std::string error_msg(redis_reply->str, redis_reply->len);
+      freeReplyObject(redis_reply);
+      auto maybe_ip_port = ParseIffMovedError(error_msg);
+      RAY_CHECK(maybe_ip_port.has_value())
+          << "Setup Redis cluster failed in the dummy deletion: " << error_msg;
+      Disconnect();
+      const auto &[ip, port] = maybe_ip_port.value();
+      // Connect to the true leader.
+      RAY_LOG(INFO) << "Redis cluster leader is " << ip << ":" << port
+                    << ". Reconnect to it.";
+      return Connect(ip, port, password, enable_ssl);
+    } else {
+      RAY_LOG(INFO) << "Redis cluster leader is " << ip_addresses[0] << ":" << port;
+      freeReplyObject(redis_reply);
+    }

-  auto redis_reply = reinterpret_cast<redisReply *>(
-      ::redisCommandArgv(context_.get(), cmds.size(), argv.data(), argc.data()));
-
-  if (redis_reply->type == REDIS_REPLY_ERROR) {
-    // This should be a MOVED error
-    // MOVED 14946 10.xx.xx.xx:7001
-    std::string error_msg(redis_reply->str, redis_reply->len);
-    freeReplyObject(redis_reply);
-    auto maybe_ip_port = ParseIffMovedError(error_msg);
-    RAY_CHECK(maybe_ip_port.has_value())
-        << "Setup Redis cluster failed in the dummy deletion: " << error_msg;
-    Disconnect();
-    const auto &[ip, port] = maybe_ip_port.value();
-    // Connect to the true leader.
-    RAY_LOG(INFO) << "Redis cluster leader is " << ip << ":" << port
-                  << ". Reconnect to it.";
-    return Connect(ip, port, password, enable_ssl);
+    return Status::OK();


Let's move this into a private function as well so we can have

if (IsRedisSentinel(*this)) { return ConnectRedisSentinel(); } else { return ConnectRedisCluster(); }

jjyao · 2024-11-18T22:00:31Z

src/ray/gcs/redis_context.cc

+
+  RAY_CHECK(redis_reply && redis_reply->type == REDIS_REPLY_ARRAY)
+      << "failed to get redis sentinel masters info";
+  RAY_CHECK(redis_reply->elements == 1)


RAY_CHECK_EQ

jjyao · 2024-11-18T22:00:55Z

src/ray/gcs/redis_context.cc

+  auto redis_reply = reinterpret_cast<redisReply *>(
+      ::redisCommandArgv(context.sync_context(), cmds.size(), argv.data(), argc.data()));
+
+  RAY_CHECK(redis_reply && redis_reply->type == REDIS_REPLY_ARRAY)


Can we split into two RAY_CHECKs so that if it fails we know which one fails

jjyao · 2024-11-18T22:01:49Z

src/ray/gcs/redis_context.cc

+    RAY_LOG(FATAL)
+        << "failed to get the ip and port of the primary node from redis sentinel";


FATAL will exit the process, I think you want ERROR

Signed-off-by: Kan Wang <[email protected]>

kanwang · 2024-11-18T22:57:52Z

Thanks! Addressed the feedbacks.

Signed-off-by: Kan Wang <[email protected]>

jjyao

LG. I'll review the tests later.

Have you tested with real Redis Sentinel?

jjyao · 2024-11-19T01:37:15Z

src/ray/gcs/redis_context.cc

+bool isRedisSentinel(RedisContext &context) {
+  auto reply = context.RunArgvSync(std::vector<std::string>{"INFO", "SENTINEL"});
+  if (reply->IsNil() || reply->IsError() || reply->ReadAsString().length() == 0) {
+    RAY_LOG(INFO) << "failed to get redis sentinel info, continue as a regular redis.";


I'd remove this log and add logs inside ConnectRedisCluster and ConnectRedisSentinel like

RAY_LOG(INFO) << "Connect to Redis cluster/sentinel";

src/ray/gcs/redis_context.cc

jjyao · 2024-11-19T01:39:43Z

src/ray/gcs/redis_context.cc

+      ::redisCommandArgv(context.sync_context(), cmds.size(), argv.data(), argc.data()));
+
+  RAY_CHECK(redis_reply) << "failed to get redis sentinel masters info";
+  RAY_CHECK(redis_reply->type == REDIS_REPLY_ARRAY)


RAY_CHECK_EQ

src/ray/gcs/redis_context.cc

jjyao · 2024-11-19T01:40:27Z

src/ray/gcs/redis_context.cc

+      << "redis sentinel master info should be REDIS_REPLY_ARRAY but got "
+      << redis_reply->type;
+  RAY_CHECK_EQ(redis_reply->elements, 1)
+      << "expecting only one primary behind the redis sentinel";


Suggested change

<< "expecting only one primary behind the redis sentinel";

<< "There should be only one primary behind the Redis sentinel";

jjyao · 2024-11-19T01:42:28Z

src/ray/gcs/redis_context.cc

+    RAY_LOG(INFO) << "connecting to the redis primary node behind sentinel: " << actual_ip
+                  << ":" << actual_port;
+    context.Disconnect();
+    return context.Connect(actual_ip, std::stoi(actual_port), password, enable_ssl);


When we connect to the primary, IsRedisSentinel will return false now?

Co-authored-by: Jiajun Yao <[email protected]> Signed-off-by: kanwang <[email protected]>

kanwang added 2 commits August 23, 2024 09:05

[Core] Ray GCS FT support redis sentinel

c6758bb

Signed-off-by: Kan Wang <[email protected]>

update docs and comments

053383f

Signed-off-by: Kan Wang <[email protected]>

kanwang changed the title ~~Kan.wang/gcs with sentinel~~ [Core] GCS FT with redis sentinel Aug 26, 2024

kanwang and others added 2 commits August 26, 2024 10:39

lint

3451698

Signed-off-by: Kan Wang <[email protected]>

Merge branch 'master' into kan.wang/gcs-with-sentinel

7c22b35

kanwang marked this pull request as ready for review August 26, 2024 15:05

kanwang requested review from architkulkarni, maxpumperla, pcmoritz, kevin85421 and a team as code owners August 26, 2024 15:05

anyscalesam added triage Needs triage (eg: priority, bug/not-bug, and owning component) core Issues that should be addressed in Ray Core labels Aug 26, 2024

anyscalesam assigned rkooo567 Aug 26, 2024

anyscalesam added P1 Issue that should be fixed within a few weeks and removed triage Needs triage (eg: priority, bug/not-bug, and owning component) labels Aug 26, 2024

jjyao added the go add ONLY when ready to merge, run all tests label Oct 29, 2024

jjyao reviewed Oct 29, 2024

View reviewed changes

jjyao assigned jjyao and unassigned rkooo567 Oct 29, 2024

kanwang and others added 2 commits October 30, 2024 23:45

Update doc/source/cluster/kubernetes/user-guides/kuberay-gcs-ft.md

5d5bf40

Co-authored-by: Jiajun Yao <[email protected]> Signed-off-by: kanwang <[email protected]>

refactor based on feedback

b21dbf1

Signed-off-by: Kan Wang <[email protected]> Signed-off-by: Kan Wang <[email protected]>

kanwang and others added 5 commits October 31, 2024 22:48

format

ef9885b

Signed-off-by: Kan Wang <[email protected]> Signed-off-by: Kan Wang <[email protected]>

update

6502e39

Signed-off-by: Kan Wang <[email protected]>

Merge branch 'master' into kan.wang/gcs-with-sentinel

936edf0

fix build

7263186

Signed-off-by: Kan Wang <[email protected]> Signed-off-by: Kan Wang <[email protected]>

lint

ea64ebc

Signed-off-by: Kan Wang <[email protected]> Signed-off-by: Kan Wang <[email protected]>

jjyao reviewed Nov 18, 2024

View reviewed changes

kanwang added 2 commits November 18, 2024 17:51

update based on feedback

56f8c38

Signed-off-by: Kan Wang <[email protected]>

update

68c4a2b

Signed-off-by: Kan Wang <[email protected]>

fix build

7743033

Signed-off-by: Kan Wang <[email protected]>

jjyao reviewed Nov 19, 2024

View reviewed changes

kanwang and others added 2 commits November 18, 2024 22:20

Update src/ray/gcs/redis_context.cc

8db8208

Co-authored-by: Jiajun Yao <[email protected]> Signed-off-by: kanwang <[email protected]>

Update src/ray/gcs/redis_context.cc

a404a05

Co-authored-by: Jiajun Yao <[email protected]> Signed-off-by: kanwang <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] GCS FT with redis sentinel #47335

[Core] GCS FT with redis sentinel #47335

kanwang commented Aug 26, 2024 •

edited

Loading

kanwang commented Aug 26, 2024

kanwang commented Sep 8, 2024

kanwang commented Sep 24, 2024

anyscalesam commented Oct 9, 2024

kanwang commented Oct 17, 2024

jjyao commented Oct 17, 2024

kanwang commented Oct 25, 2024

jjyao left a comment

jjyao Oct 29, 2024

kanwang Oct 31, 2024

jjyao Oct 29, 2024

kanwang Nov 1, 2024

kanwang Nov 1, 2024

kanwang commented Nov 6, 2024

kanwang commented Nov 13, 2024

jjyao commented Nov 13, 2024

jjyao Nov 18, 2024

jjyao Nov 18, 2024

jjyao Nov 18, 2024

jjyao Nov 18, 2024

jjyao Nov 18, 2024

kanwang commented Nov 18, 2024

jjyao left a comment

jjyao Nov 19, 2024

jjyao Nov 19, 2024

jjyao Nov 19, 2024

jjyao Nov 19, 2024

		@@ -506,6 +564,14 @@ Status RedisContext::Connect(const std::string &address,
		// Ray has some restrictions for RedisDB. Validate it here.
		ValidateRedisDB(*this);

		RAY_LOG(FATAL)
		<< "failed to get the ip and port of the primary node from redis sentinel";

	<< "expecting only one primary behind the redis sentinel";
	<< "There should be only one primary behind the Redis sentinel";

[Core] GCS FT with redis sentinel #47335

Are you sure you want to change the base?

[Core] GCS FT with redis sentinel #47335

Conversation

kanwang commented Aug 26, 2024 • edited Loading

Why are these changes needed?

Related issue number

Checks

kanwang commented Aug 26, 2024

kanwang commented Sep 8, 2024

kanwang commented Sep 24, 2024

anyscalesam commented Oct 9, 2024

kanwang commented Oct 17, 2024

jjyao commented Oct 17, 2024

kanwang commented Oct 25, 2024

jjyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanwang commented Nov 6, 2024

kanwang commented Nov 13, 2024

jjyao commented Nov 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanwang commented Nov 18, 2024

jjyao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kanwang commented Aug 26, 2024 •

edited

Loading