Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] "CLIENT CAPA redirect" has no effect in scripts #868

Open
gmbnomis opened this issue Aug 4, 2024 · 2 comments · May be fixed by #1781
Open

[BUG] "CLIENT CAPA redirect" has no effect in scripts #868

gmbnomis opened this issue Aug 4, 2024 · 2 comments · May be fixed by #1781

Comments

@gmbnomis
Copy link
Contributor

gmbnomis commented Aug 4, 2024 •
edited
Loading

Describe the bug

A read or write command should be redirected when issued by a client that understands "REDIRECT". This does not work when the command is part of a script.

To reproduce

Connect the CLI client to a replica and do:

localhost:6380> client capa redirect
OK
localhost:6380> set foo bar
(error) REDIRECT redis:6379
localhost:6380> EVAL "return redis.call('SET', KEYS[1], ARGV[1])" 1 foo bar
(error) READONLY You can't write against a read only replica. script: d8f2fad9f8e86a53d2a6ebd960b33c4972cacc37, on @user_script:1.
localhost:6380> get foo
(error) REDIRECT redis:6379
localhost:6380> EVAL "return redis.call('GET', KEYS[1])" 1 foo
"bar"

(Note: I did a set foo bar on the primary to test replication, that's why the last command returns "bar".)

Expected behavior

REDIRECT should be issued regardless of the context if the client understands redirect. (for reference, Valkey cluster issues the same "MOVED" in both cases:

127.0.0.1:30004> readwrite
OK
127.0.0.1:30004> set b b
(error) MOVED 3300 127.0.0.1:30001
127.0.0.1:30004> EVAL "return redis.call('SET', KEYS[1], ARGV[1])" 1 b b
(error) MOVED 3300 127.0.0.1:30001

)
@madolson madolson moved this to Optional for rc1 in Valkey 8.0 Aug 5, 2024
@madolson
Copy link
Member

madolson commented Aug 5, 2024 •
edited
Loading

It's a little odd EVAL_RO get's redirected to the primary, but not EVAL. We should evaluate if we want the REDIRECT flag to act more like the cluster mode version.

gmbnomis added a commit to gmbnomis/valkey that referenced this issue Feb 25, 2025
@gmbnomis
standalone REDIRECT: Fix scripting and further MULTI/EXEC scenarios
16377c6 
In cluster mode, the necessity of a "MOVED" response is mainly determined based
on the keys of a command whereas in standalone redirect mode, the necessity of a
"REDIRECT" response is determined based on the command's flags.

It turns out that looking at keys is necessary. Moreover, the currently
used command flags do not encompass all situations in which a REDIRECT is
necessary:

- WATCH command (redirect in READWRITE mode is necessary)
- transactions with no keys must not be redirected
- scripting scenarios: scripts (with keys) must be redirected in some situations
  (issue valkey-io#868).

The new logic is a heavily "distilled" version of `getNodeByQuery` used in
cluster mode. As we don't need to look at slots and their consistency, the
information necessary is the flags of the command and whether it accesses at
least one key.

Signed-off-by: Simon Baatz <[email protected]>
@gmbnomis gmbnomis linked a pull request Feb 25, 2025 that will close this issue
Open
gmbnomis added a commit to gmbnomis/valkey that referenced this issue Feb 25, 2025
@gmbnomis
standalone REDIRECT: Fix scripting and further MULTI/EXEC scenarios
727f400 
In cluster mode, the necessity of a "MOVED" response is mainly determined based
on the keys of a command whereas in standalone redirect mode, the necessity of a
"REDIRECT" response is determined based on the command's flags.

It turns out that looking at keys is necessary. Moreover, the currently
used command flags do not encompass all situations in which a REDIRECT is
necessary:

- WATCH command (redirect in READWRITE mode is necessary)
- transactions with no keys must not be redirected
- scripting scenarios: scripts (with keys) must be redirected in some situations
  (issue valkey-io#868).

The new logic is a heavily "distilled" version of `getNodeByQuery` used in
cluster mode. As we don't need to look at slots and their consistency, the
information necessary is the flags of the command and whether it accesses at
least one key.

Signed-off-by: Simon Baatz <[email protected]>
gmbnomis added a commit to gmbnomis/valkey that referenced this issue Feb 25, 2025
@gmbnomis
standalone REDIRECT: Fix scripting and further MULTI/EXEC scenarios
5fdd75e 
In cluster mode, the necessity of a "MOVED" response is mainly determined based
on the keys of a command whereas in standalone redirect mode, the necessity of a
"REDIRECT" response is determined based on the command's flags.

It turns out that looking at keys is necessary. Moreover, the currently
used command flags do not encompass all situations in which a REDIRECT is
necessary:

- WATCH command (redirect in READWRITE mode is necessary)
- transactions with no keys must not be redirected
- scripting scenarios: scripts (with keys) must be redirected in some situations
  (issue valkey-io#868).

The new logic is a heavily "distilled" version of `getNodeByQuery` used in
cluster mode. As we don't need to look at slots and their consistency, the
information necessary is the flags of the command and whether it accesses at
least one key.

Signed-off-by: Simon Baatz <[email protected]>
@gmbnomis
Copy link
Contributor Author

... We should evaluate if we want the REDIRECT flag to act more like the cluster mode version.

@madolson Yes, it turned out that there are indeed other cases we need to take care of. I addressed the ones I am aware of in PR #1781, but there is a fundamental difference between cluster and standalone modes with respect to publish/subscribe that I don't know how to solve best. I created issue #1780 for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
Valkey 8.0
Status: Todo
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

standalone REDIRECT: Fix scripting and further MULTI/EXEC scenarios gmbnomis/valkey
2 participants
@gmbnomis @madolson