[Fix][GCS] Implement reconnection for RedisContext

[MortalHappiness]

Why are these changes needed?

When Redis is configured with an idle timeout, a connection that remains idle for too long will be closed by the server. Previously, we used fixed connections to communicate with the server, specifically the sync context and async context.

This PR implements reconnection to resolve this issue.

Additionally, for error replies, the async context already implements exponential retry, whereas the sync context does not. This PR also adds exponential retry for the sync context using the same connection.

Related issue number

Closes #47419

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 :(

[jjyao]

Could you update the PR description to describe why GCS crashes when redis idle timeout is configured and what's the fix

[MortalHappiness]

@jjyao I found it a bit complicated, so I want to discuss the design and implementation here. I drew a simple class diagram. As you can see, RedisClient composes RedisContext and RedisAsioClient. Upon connection, RedisAsioClient will replace the callback function of redisAsyncContext in RedisAsyncContext. redisAsyncContext implicitly uses RedisAsioClient via the callback function. The original implementation assumes that these objects will not be recreated.

However, I now need to implement reconnection. If a connection loss is detected in RedisContext, I need to recreate both RedisAsyncContext and redisContext. But there is a dependency cycle: the lifetimes of RedisAsyncContext and RedisAsioClient are linked. This means I also need to recreate RedisAsioClient, which effectively requires me to call the Connect function of RedisClient again (this is because what RedisClient::Connect does is to call RedisContext::Connect and create a RedisAsioClient object)

Here are the problems:

1. There is a comment in RedisClient stating that Connect and Disconnect are not thread-safe. So it may not be a good idea to call it multiple times.

   ray/src/ray/gcs/redis_client.h

   Lines 58 to 68 in 5896b3f

   	/// Connect to Redis. Non-thread safe.
   	/// Call this function before calling other functions.
   	///
   	/// \param io_service The event loop for this client.
   	/// This io_service must be single-threaded. Because `RedisAsioClient` is
   	/// non-thread safe.
   	/// \return Status
   	Status Connect(instrumented_io_context &io_service);
   	/// Disconnect with Redis. Non-thread safe.
   	void Disconnect();

   )
   • I'm unsure whether this comment is accurate. Since all members in RedisClient share the same instrumented_io_context &io_service, they should be thread-safe because they operate within the same event loop. Please correct me if I am wrong.

2. Disconnection is detected in RedisContext, but RedisClient composes RedisContext. Calling RedisClient::Connect from RedisContext would make RedisContext dependent on RedisClient, which I believe is poor design.

[jjyao]

why?

[MortalHappiness]

This was used to prevent infinite reconnection during startup because the failure in the first connection is usually due to user misconfiguration of the Redis server, such as an incorrect password. However, in #48781 (comment), I've changed it to a fatal error in the Connect function if we fail to connect to the saved RAY_REDIS_ADDRESS. Therefore, this is no longer needed.

[jjyao]

Are we going to have infinite reconnect if address_ is down?

[MortalHappiness]

Change it to a fatal error, such that the GCS server will crash if it fails to connect to the saved address RAY_REDIS_ADDRESS in the Connect function.

ray/src/ray/gcs/redis_context.cc

Lines 706 to 708 in 2db1939

	// If we failed to connect to the saved address RAY_REDIS_ADDRESS, then it's a fatal
	// error.
	RAY_CHECK_OK(ConnectToIPAddress(ip_address, port_));

[jjyao]

Let's do check failure first which matches the current behavior. Later on we can add retry different ip (since DNS may return multiple ips and currently we only try the first one).

[MortalHappiness]

Does "later on" here mean implementing it in this PR or in a separate PR?

[jjyao]

Separate PR.

[jjyao]

Actually this can cause nullptr exception since there might be some RedisRequestContext that hold the raw pointer to redis async context?

[MortalHappiness]

Moved nullptr check for redis async context to RedisContext::async_context in this commit. RedisRequestContext always calls RedisContext::async_context to get the redis async context so this can prevent nullptr exception.

9779e4c

[MortalHappiness]

Also, I changed the deleter for redis async context to use redisAsyncDisconnect instead of redisAsyncFree. redisAsyncDisconnect calls redisAsyncFree, but it tries to execute callbacks for all remaining replies before freeing the context.

ec1e835

ref:

• https://github.com/redis/hiredis/blob/8d8703ee6149ffd3b38cebedd71ad8d1fac63cc7/async.c#L454-L468
• https://github.com/redis/hiredis/blob/8d8703ee6149ffd3b38cebedd71ad8d1fac63cc7/async.c#L426-L452

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. Thank you for your contributions.

• If you'd like to keep this open, just leave any comment, and the stale label will be removed.