replica redirect read&write to master in standalone mode #325

soloestoy · 2024-04-16T07:59:25Z

To implement #319

replica is able to redirect read and write commands to it's master in standalone mode
- reply with "-MOVED -1 master-ip:port"
add a config replica-enable-redirect to control whether to redirect or not, with the default setting being off
- when enabled, the data access commands(read and write) will be redirected
allow readonly and readwrite command in standalone mode, may be a breaking change
- use READONLY command can allow read commands on a replica when replica-enable-redirect enabled

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

zuiderkwast

High level LGTM.

(I didn't review the documentation of the config yet.)

zuiderkwast · 2024-04-16T09:46:50Z

src/server.c

+        addReplyErrorSds(c,sdscatprintf(sdsempty(), "-MOVED -1 %s:%d",
+                                                    server.masterhost, server.masterport));


For cluster, we return the TLS port if the client is TLS and the non-TLS port if the client is non-TLS.

How can we handle this in standalone mode? Maybe we can only handle it if client connection and replication connection are both TLS or none of them is TLS? Otherwise return an error?

Let's not coupling client connection and replication connection together (just like client connection and cluster bus). Maybe we need a new dedicated port to do the replicatlication, and this is a breaking change.

@zuiderkwast yes, this is a problem if users use both TLS port and non-TLS port. And now in standalone mode replicas and master doesn't know the port's info each other.

Maybe we can use REPLCONF or something to send the port's info, I incline to open another issue to discuss it. I think it's better to discuss them separately to avoid mixing them up and making the problem more complicated.

At the beginning of TLS design, the server adopted a two port model, but many related issues were not well resolved. For example, cluster can provide two different services, TLS and non TLS, until redis/redis#12233 fixed by @CharlesChen888 . TLS is a more macro level issue I think.

If we don't implement something now to to make the replica know both ports of the primary, then at least we should only use the redirect when the client can use it. Otherwise we should just return an error. We can't return a non-TLS port to a TLS client and vice versa.

Another problem that @enjoy-binbin pointed out is the announced-ip and announced-port. If they are in use, then probably the primary is using them too and the client can't use the redirect.

The actual scenario is more complex, involving not only non-TLS and TLS port issues but also concerns regarding network isolation and NAT. There are four distinct scenarios in general:

Both the client and the master/replica nodes are deployed in the same network, and both nodes utilize a single port (non-TLS or TLS port). In this case, replica and client access the master through the same port.

Both the client and the master/replica nodes are deployed in the same network, and nodes provides two services ports (non-TLS and TLS port). In this situation, replica and client may access master's different ports.

Client and master/replica nodes are deployed in separate networks. During this time the master/replica nodes need to provide service to client through NAT. Therefore, the master ip returned by REDIRECT also needs to be configured as the NAT IP.

A more complex scenario involves the master and replica being deployed in separate networks as well. In this case, not only would the master/replica need to provide NAT IPs to the client, but they would also require additional NAT IPs for communication between themselves.

I understand all of these issues as we have encountered them in the production environment as well. We can thoroughly discuss these scenarios, but I would like to address the above issues step by step. I do not favor complicating things right from the start as it increases the complexity of engineering implementation.

This PR can resolve issue 1, achieving smooth switchover when it is within the same network and the same port. Then, we can proceed to solve the remaining issues.

enjoy-binbin

generally LGTM. I think the slot number -1 is acceptable, and i suppose that many clients will find that it is easy to add the support. (but we seem to be lacking the most support from clients right now.)

The other one is announced-ip and announced-port, i'm not sure about this.

src/server.c

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

codecov · 2024-04-22T03:15:57Z

Codecov Report

Attention: Patch coverage is 92.85714% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 69.84%. Comparing base (fdd023f) to head (be838bc).
Report is 18 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #325      +/-   ##
============================================
+ Coverage     69.81%   69.84%   +0.03%     
============================================
  Files           109      109              
  Lines         61792    61809      +17     
============================================
+ Hits          43139    43173      +34     
+ Misses        18653    18636      -17

Files	Coverage Δ
src/cluster.c	`87.05% <ø> (+0.77%)`	⬆️
src/commands.def	`100.00% <ø> (ø)`
src/server.c	`88.63% <100.00%> (+0.02%)`	⬆️
src/networking.c	`85.03% <83.33%> (+0.13%)`	⬆️

... and 15 files with indirect coverage changes

yangbodong22011 · 2024-04-29T06:54:55Z

valkey.conf

+# to its master or not, excluding commands such as:
+# INFO, AUTH, ROLE, SUBSCRIBE, UNSUBSCRIBE, PUBLISH.
+#
+# When enabled, a replica instance will reply "-MOVED -1 master-ip:port"


Suggested change

# When enabled, a replica instance will reply "-MOVED -1 master-ip:port"

# When enabled, a replica instance will reply "-REDIRECT -1 master-ip:port"

yangbodong22011 · 2024-04-29T07:09:14Z

src/server.c

+        !mustObeyClient(c) &&
+        (is_write_command ||
+         (is_read_command && !(c->flags & CLIENT_READONLY)))) {
+        addReplyErrorSds(c,sdscatprintf(sdsempty(), "-REDIRECT %s:%d",


Why is -1 missing here? Keep the same format as MOVED and the client can reuse the parsing code.

-1 is not part of MOVED.

MOVED is for a slot. REDIRECT is for all writes to the node, without slot. (Only cluster has slots.)

MOVED is for a slot. REDIRECT is for all writes to the node, without slot. (Only cluster has slots.)

ok, that's reasonable from valkey developer.

hwware · 2024-05-01T17:44:16Z

@soloestoy Now it looks like we agree to add "-REDIRECT ip port", I think you could update the top description to match the latest decision. Thanks

madolson · 2024-05-13T18:40:26Z

Highlighting my two concerns:

How much complexity are we signing up standalone clients to implement by having them understand the concept of "redirects", that was already built for cluster aware clients. Clients need to understand that when getting this new error message, they should update their internal endpoint to point to this instead, which is not what clients are expected to do today. Since this logic already basically exists for Valkey cluster, my preference is we still wait to understand if that is what we want longterm for standalone Valkey, but could be convinced otherwise. @PingXie Since we discussed this.
I don't like that this is adding a server configuration that causes backwards incompatible state, which is basically adding a type of configuration that clients have to know and understand. Specifically, in the happy path, when a client was previously able to directly talk to a replica client and send read commands, it will now get the -REDIRECT error unless it also sends the READONLY command. If an operator is unsure of this behavior, it's a very painful end user experience to start seeing unexpected errors and their only recourse is to flip a server configuration back. I think we should bias towards having clients negotiate this new capability, by sending either a new command (like CLIENT REDIRECT ON) or using the READONLY READWRITE command as flags. This should allow us to remove the config.

soloestoy · 2024-05-14T01:44:29Z

After the discussion I think we have an agreement about the redirections value, and need a bit change that this feature should not be a config in server. To make it as a flag of per client would be better (CLIENT REDIRECT ON seems not a good choice, since it can be easily confused with CLIENT TRACKING REDIRECT), I'm thinking which command or a new command is better.

madolson · 2024-05-14T02:43:13Z

CLIENT TRACKING REDIRECT I did think about that but was hoping nobody really uses is. I think basically everyone that uses server assisted client side caching uses RESP3 which doesn't require that command. We could maybe do CLIENT NODE REDIRECT.

PingXie · 2024-05-14T06:12:33Z

Summarizing @valkey-io/core-team's discussion and thought process:

This is a breaking change. Standalone replicas can serve reads just fine today. This change when enabled would break the read-replica scenario for unenlightened clients
Breaking changes should be not introduced via server-side knobs, such as configs/etc
Breaking changes should be opt'ed in by the client explicitly
We don't have a systematic way to support client opt-ins today but there have been precedents of client opt-in via special commands like READONLY

PingXie · 2024-05-14T06:41:09Z

Even if we allow the client to opt-in via a new command like CLIENT NODE REDIRECT, we should not enable this particular behavior server-wide. This is because a Valkey server can be shared by multiple applications. It would be really confusing to application B if application A changes a global setting unilaterally. This brings in another problem however. I can imagine some other behavior changes in the future would only make sense at the server level. It feels like we might need to think through what a "capability" model would look like first or I am worried about getting into another discussion like the half done function support #58.

madolson · 2024-05-14T18:42:08Z

I can imagine some other behavior changes in the future would only make sense at the server level.

Can you elaborate more, because I don't think there are that many real cases like this.

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

soloestoy · 2024-05-16T06:13:33Z

I removed the server config, and make it as a per client opt-in feature via CLIENT CAPA REDIRECT (we can append more capabilities in future), WDYT?

replica redirect read&write to master in standalone mode

8fce41e

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

soloestoy force-pushed the replica-redirect branch from 106cac7 to 8fce41e Compare April 16, 2024 09:36

zuiderkwast reviewed Apr 16, 2024

View reviewed changes

enjoy-binbin linked an issue Apr 17, 2024 that may be closed by this pull request

Support smooth swithover in standalone mode by reusing MOVED #319

Open

enjoy-binbin reviewed Apr 17, 2024

View reviewed changes

src/server.c Outdated Show resolved Hide resolved

change to REDIRECT

0d83765

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

madolson mentioned this pull request Apr 24, 2024

Inconsistent command behavior for keyless read commands #369

Open

yangbodong22011 reviewed Apr 29, 2024

View reviewed changes

hwware added the major-decision-pending Needs decision by core team label May 1, 2024

Merge remote-tracking branch 'upstream/unstable' into replica-redirect

15163c9

redirect by per client's capability

be838bc

Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>

soloestoy force-pushed the replica-redirect branch from 0c67122 to be838bc Compare May 16, 2024 05:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replica redirect read&write to master in standalone mode #325

replica redirect read&write to master in standalone mode #325

soloestoy commented Apr 16, 2024

zuiderkwast left a comment

zuiderkwast Apr 16, 2024

CharlesChen888 Apr 17, 2024

soloestoy Apr 17, 2024

zuiderkwast Apr 22, 2024

soloestoy Apr 22, 2024 •

edited

enjoy-binbin left a comment

codecov bot commented Apr 22, 2024 •

edited

yangbodong22011 Apr 29, 2024

yangbodong22011 Apr 29, 2024 •

edited

zuiderkwast Apr 29, 2024

yangbodong22011 Apr 29, 2024

hwware commented May 1, 2024 •

edited

madolson commented May 13, 2024 •

edited

soloestoy commented May 14, 2024 •

edited

madolson commented May 14, 2024

PingXie commented May 14, 2024

PingXie commented May 14, 2024

madolson commented May 14, 2024

soloestoy commented May 16, 2024 •

edited

		addReplyErrorSds(c,sdscatprintf(sdsempty(), "-MOVED -1 %s:%d",
		server.masterhost, server.masterport));

	# When enabled, a replica instance will reply "-MOVED -1 master-ip:port"
	# When enabled, a replica instance will reply "-REDIRECT -1 master-ip:port"

replica redirect read&write to master in standalone mode #325

Are you sure you want to change the base?

replica redirect read&write to master in standalone mode #325

Conversation

soloestoy commented Apr 16, 2024

zuiderkwast left a comment

Choose a reason for hiding this comment

zuiderkwast Apr 16, 2024

Choose a reason for hiding this comment

CharlesChen888 Apr 17, 2024

Choose a reason for hiding this comment

soloestoy Apr 17, 2024

Choose a reason for hiding this comment

zuiderkwast Apr 22, 2024

Choose a reason for hiding this comment

soloestoy Apr 22, 2024 • edited

Choose a reason for hiding this comment

enjoy-binbin left a comment

Choose a reason for hiding this comment

codecov bot commented Apr 22, 2024 • edited

Codecov Report

yangbodong22011 Apr 29, 2024

Choose a reason for hiding this comment

yangbodong22011 Apr 29, 2024 • edited

Choose a reason for hiding this comment

zuiderkwast Apr 29, 2024

Choose a reason for hiding this comment

yangbodong22011 Apr 29, 2024

Choose a reason for hiding this comment

hwware commented May 1, 2024 • edited

madolson commented May 13, 2024 • edited

soloestoy commented May 14, 2024 • edited

madolson commented May 14, 2024

PingXie commented May 14, 2024

PingXie commented May 14, 2024

madolson commented May 14, 2024

soloestoy commented May 16, 2024 • edited

soloestoy Apr 22, 2024 •

edited

codecov bot commented Apr 22, 2024 •

edited

yangbodong22011 Apr 29, 2024 •

edited

hwware commented May 1, 2024 •

edited

madolson commented May 13, 2024 •

edited

soloestoy commented May 14, 2024 •

edited

soloestoy commented May 16, 2024 •

edited