replica redirect read&write to primary in standalone mode

[soloestoy]

To implement #319

1. replica is able to redirect read and write commands to it's primary in standalone mode
   • reply with "-REDIRECT primary-ip:port"
2. add a subcommand CLIENT CAPA redirect, a client can announce the capability to handle redirection
   • if a client can handle redirection, the data access commands (read and write) will be redirected
3. allow readonly and readwrite command in standalone mode, may be a breaking change
   • a client with redirect capability cannot process read commands on a replica by default
   • use READONLY command can allow read commands on a replica

[zuiderkwast]

High level LGTM.

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

[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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.99%. Comparing base (5a51bf5) to head (6aa197f).
Report is 17 commits behind head on unstable.

Additional details and impacted files

@@             Coverage Diff              @@
##           unstable     #325      +/-   ##
============================================
- Coverage     70.17%   69.99%   -0.19%     
============================================
  Files           110      110              
  Lines         60077    60108      +31     
============================================
- Hits          42160    42070      -90     
- Misses        17917    18038     +121     

Files	Coverage Δ
src/cluster.c	87.95% <ø> (+0.18%)	⬆️
src/commands.def	100.00% <ø> (ø)
src/networking.c	85.46% <100.00%> (+0.09%)	⬆️
src/server.c	88.55% <100.00%> (+0.01%)	⬆️
src/server.h	100.00% <ø> (ø)

... and 19 files with indirect coverage changes

[hwware]

@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]

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]

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]

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]

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

1. 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
2. Breaking changes should be not introduced via server-side knobs, such as configs/etc
3. Breaking changes should be opt'ed in by the client explicitly
4. 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]

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]

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.

[soloestoy]

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?

[soloestoy]

it's ready to be merged, @valkey-io/core-team please approve

[zuiderkwast]

Looks good in general. Some comments about CLIENT CAPA.

[enjoy-binbin]

i guess we also need to update the doc (READONLY) and add a new command doc

[zuiderkwast]

LGTM.

Other reviewers: Please take a look at the new subcommand CLIENT CAPA capa [capa...], returns OK, ignores unknown capabilities from the future. (They can be sent by future clients supporting future Valkey versions.)

We need docs for -REDIECT and for CLIENT CAPA.

[zuiderkwast]

READONLY not returning an error in standalone mode broke some test case for Valkey-py.

Reported by @mkmkme:

Yeah, the test started failing because the test was as simple as

async def test_readonly_invalid_cluster_state(self, r: valkey.Valkey):
    with pytest.raises(exceptions.ValkeyError):
        await r.readonly()

So it assumed readonly command always returns an error

I guess we should have added a check for CLIENT CAPA REDIRECT in the READONLY and READWRITE commands, to keep the old behaviour for old clients.