replica redirect read&write to master in standalone mode #325
Conversation
Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
soloestoy
force-pushed
the
replica-redirect
branch
from
106cac7
to
8fce41e
Compare
src/server.c
Outdated
| addReplyErrorSds(c,sdscatprintf(sdsempty(), "-MOVED -1 %s:%d", | ||
| server.masterhost, server.masterport)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #325 +/- ##
============================================
+ Coverage 69.81% 69.83% +0.02%
============================================
Files 109 109
Lines 61792 61794 +2
============================================
+ Hits 43139 43155 +16
+ Misses 18653 18639 -14
|
| # 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" |
There was a problem hiding this comment.
| # When enabled, a replica instance will reply "-MOVED -1 master-ip:port" | |
| # When enabled, a replica instance will reply "-REDIRECT -1 master-ip:port" |
| !mustObeyClient(c) && | ||
| (is_write_command || | ||
| (is_read_command && !(c->flags & CLIENT_READONLY)))) { | ||
| addReplyErrorSds(c,sdscatprintf(sdsempty(), "-REDIRECT %s:%d", |
There was a problem hiding this comment.
Why is -1 missing here? Keep the same format as MOVED and the client can reuse the parsing code.
There was a problem hiding this comment.
-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.)
There was a problem hiding this comment.
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.
|
@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 |
To implement #319
replica-enable-redirectto control whether to redirect or not, with the default setting being offreadonlyandreadwritecommand in standalone mode, may be a breaking changereplica-enable-redirectenabled