Support function flags in script EVAL via shebang header #10126

yoav-steinberg · 2022-01-17T14:16:36Z

In #10025 we added a mechanism for flagging certain properties for Redis Functions. This lead us to think we'd like to "port" this mechanism to Redis Scripts (EVAL) as well.

One good reason for this, other than the added functionality is because it addresses the poor behavior we currently have in EVAL in case the script performs a (non DENY_OOM) write operation during OOM state. See #8478 (And a previous attempt to handle it via #10093) for details. Note that in Redis Functions all write operations (including DEL) will return an error during OOM state unless the function is flagged as allow-oom in which case no OOM checking is performed at all.

This PR:

Enables setting EVAL (and SCRIPT LOAD) script flags as defined in Redis Functions Flags #10025.
Provides a syntactical framework via shebang for additional script annotations and even engine selection (instead of just lua) for scripts.
Provides backwards compatibility so scripts without the new annotations will behave as they did before.
Appropriate tests.
Changes EVAL[SHA]/_RO to be flagged as STALE commands. This makes it possible to flag individual scripts as allow-stale or not flag them as such. In backwards compatibility mode these commands will return the MASTERDOWN error as before.
Changes SCRIPT LOAD to be flagged as a STALE command. This is mainly to make it logically compatible with the change to EVAL in the previous point. It enables loading a script on a stale server which is technically okay it doesn't relate directly to the server's dataset. Running the script does, but that won't work unless the script is explicitly marked as allow-stale.

Even though not strictly required this should replace #10093. We also suggest closing #8478: using the shebang will cause the script to change its default behavior so it'll deny any writes during OOM state.

Usage example:

#!lua flags=no-writes,allow-stale
local x = redis.call('get','x')
return x

Note that even though the LUA syntax doesn't support hash tag comments .lua files do support a shebang tag on the top so they can be executed on Unix systems like any shell script. LUA's luaL_loadfile handles this as part of the LUA library. In the case of luaL_loadbuffer, which is what Redis uses, I needed to fix the input script in case of a shebang manually. I did this the same way luaL_loadfile does, by replacing the first line with a single line feed character.

…ptPrepareForRun` can know it's in EVAL mode too.

src/eval.c

Co-authored-by: sundb <sundbcn@gmail.com>

zuiderkwast · 2022-01-20T12:17:03Z

It's a pretty cool trick to use the shebang, but one would expect that it's also usable in a Unix system. A shebang starting with #!lua would invoke the lua executable but the flags=... argument makes no sense for the lua interpreter. How about a syntax where it actually makes sense, (or could make sense provided a local redis library is made available)? Maybe something like this?:

#!lua -l redis -e 'redis.flags("no-writes","allow-stale")'

oranagra · 2022-01-20T14:39:36Z

@zuiderkwast the thing is that we want to be able to parse the flags and store them per script without executing it.
i.e. unlike the previous attempt, we can reject a write script on a read-only replica without even executing the Lua vm.

with your suggestion, we either execute that line to get the flags, or alternatively just attempt to parse it, but it'll be odd if it looks like Lua code where in fact we just parse it.

i'd rather use simple string splitting to get the flags (and maybe other args in the future.
the fact that it's not compatible with Unix CLI doesn't bother me.

zuiderkwast · 2022-01-20T16:34:50Z

i'd rather use simple string splitting to get the flags (and maybe other args in the future.
the fact that it's not compatible with Unix CLI doesn't bother me.

The fact that it's masquerading as something which it's isn't is potentially confusing.

How about allowing them within any kind of comment, with an additional "redis flags" tag on e.g. one of the two first lines of the script, so depending on the language, either of the below (idea, not exact syntax):

-- Redis flags=no-writes,allow-stale
/* Redis flags=no-writes,allow-stale */
# Redis flags=no-writes,allow-stale

oranagra

looks good. obviously missing tests 8-)

src/script.h

src/eval.c

src/script.c

src/eval.c

src/script.c

src/eval.c

- functions_flags -> script_flags - use new behavior on shebang (even without flags)

Co-authored-by: Oran Agra <oran@redislabs.com>

…s into script-shebang-flags

Including backwards compatibility mode for no shebang and tests.

tests/unit/scripting.tcl

src/script.h

tests/unit/scripting.tcl

Co-authored-by: Oran Agra <oran@redislabs.com>

oranagra · 2022-01-23T14:52:29Z

@redis/core-team please approve.

src/eval.c

src/script.c

oranagra · 2022-01-24T14:50:23Z

For the record, this was conceptually approved in a core-team meeting.

oranagra · 2022-02-07T20:01:14Z

@yoav-steinberg i just realized that this broke the defragger.
it won't crash, but will fail defragging the actual script allocation.
while we're fixing this, maybe extend it to defrag the functions data structures?
mind fixing this?

In redis#10126 the global `lua_script` dict became a dict to a custom `luaScript` struct with an internal `robj` in it instead of a generic `sds` -> `robj` dict. This means we need custom code to defrag it and since scripts should never really cause much fragmentation it makes more sense to simply remove the defrag code for scripts.

This reverts commit e05c979.

Remove scripts defragger since it was broken since #10126 (released in 7.0 RC1). would crash the server if defragger starts in a server that contains eval scripts. In #10126 the global `lua_script` dict became a dict to a custom `luaScript` struct with an internal `robj` in it instead of a generic `sds` -> `robj` dict. This means we need custom code to defrag it and since scripts should never really cause much fragmentation it makes more sense to simply remove the defrag code for scripts.

yoav-steinberg added 2 commits January 17, 2022 16:13

WIP: Support function flags in script EVAL via shebang header

076ccf4

wip: make sure we know we're in EVAL mode, needs to be fixed so `scri…

0176281

…ptPrepareForRun` can know it's in EVAL mode too.

oranagra added the 7.0-RC1-must-have label Jan 17, 2022

yoav-steinberg added 2 commits January 17, 2022 17:30

Don't set curr_run_ctx if script isn't going to run.

c105613

flags backwards compatibility mode

19d8f2d

oranagra added release-notes indication that this issue needs to be mentioned in the release notes state:major-decision Requires core team consensus labels Jan 18, 2022

oranagra linked an issue Jan 18, 2022 that may be closed by this pull request

[BUG] Lua script doesn't return OOM when making arbitrary delete command first #8478

Closed

oranagra mentioned this pull request Jan 18, 2022

By default deny all scripting writes during OOM. #10093

Closed

sundb reviewed Jan 18, 2022

View reviewed changes

src/eval.c Outdated Show resolved Hide resolved

src/eval.c Show resolved Hide resolved

oranagra mentioned this pull request Jan 18, 2022

Redis Programmability Documentation redis/redis-doc#1725

Merged

yoav-steinberg and others added 3 commits January 19, 2022 08:47

Update src/eval.c

f2d2451

Co-authored-by: sundb <sundbcn@gmail.com>

CR cleanup

678d430

More robust handling of shebang arguments (ex. ignore trailing spaces)

51a31c4

oranagra reviewed Jan 20, 2022

View reviewed changes

MeirShpilraien reviewed Jan 20, 2022

View reviewed changes

src/eval.c Show resolved Hide resolved

MeirShpilraien reviewed Jan 20, 2022

View reviewed changes

src/script.c Outdated Show resolved Hide resolved

sundb reviewed Jan 21, 2022

View reviewed changes

src/eval.c Outdated Show resolved Hide resolved

yoav-steinberg and others added 6 commits January 21, 2022 14:04

CR fix: shebang with no line break.

186fd6a

CR fixes

94ea288

- functions_flags -> script_flags - use new behavior on shebang (even without flags)

CR comment

150ef8b

Update src/script.h

ff1610c

Co-authored-by: Oran Agra <oran@redislabs.com>

Merge branch 'script-shebang-flags' of github.com:yoav-steinberg/redi…

f000cbc

…s into script-shebang-flags

WIP: Tests

5ca1f24

yoav-steinberg marked this pull request as ready for review January 21, 2022 12:55

yoav-steinberg added 2 commits January 22, 2022 14:07

CR: generalize functions/eval error messages.

2bda07e

Support scripting when stale (based on flags)

a4828b1

Including backwards compatibility mode for no shebang and tests.

oranagra reviewed Jan 22, 2022

View reviewed changes

yoav-steinberg added 4 commits January 22, 2022 20:42

CR cleanups.

5b7a6a7

CR cleanup.

0ab7113

CR fix: make script load a STALE command.

658b8c7

CR: added test for write flag on read only replica.

0b2d617

oranagra reviewed Jan 23, 2022

View reviewed changes

tests/unit/scripting.tcl Outdated Show resolved Hide resolved

Update tests/unit/scripting.tcl

1996ec5

Co-authored-by: Oran Agra <oran@redislabs.com>

oranagra added the approval-needed Waiting for core team approval to be merged label Jan 23, 2022

Merge branch 'unstable' into script-shebang-flags

090a4f9

yossigo reviewed Jan 23, 2022

View reviewed changes

src/eval.c Show resolved Hide resolved

src/script.c Show resolved Hide resolved

yoav-steinberg added 5 commits January 24, 2022 09:52

CR cleanup.

15241ee

skip replconf tests on external

5fa65bd

CR

e478e64

Cleanups in MISCONF error messages.

02318d2

cleanup

8a45313

oranagra approved these changes Jan 24, 2022

View reviewed changes

oranagra merged commit 7eadc5e into redis:unstable Jan 24, 2022

yoav-steinberg mentioned this pull request Feb 9, 2022

Fix Eval scripts defrag (broken 7.0 in RC1) #10271

Merged

yoav-steinberg added a commit to yoav-steinberg/redis that referenced this pull request Feb 10, 2022

Revert "Remove scripts defragger since it was broken since redis#10126."

ffa19b8

This reverts commit e05c979.

eduardobr mentioned this pull request Apr 2, 2022

[CRASH] Replication crashes on EVAL when replica-serve-stale-data=no (introduced in Redis 7) #10508

Closed

oranagra mentioned this pull request Aug 11, 2022

Support for newer Lua versions? #7382

Closed

Support function flags in script EVAL via shebang header #10126

Support function flags in script EVAL via shebang header #10126

Uh oh!

Conversation

yoav-steinberg commented Jan 17, 2022 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zuiderkwast commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra commented Jan 20, 2022

Uh oh!

zuiderkwast commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oranagra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jan 23, 2022

Uh oh!

Uh oh!

Uh oh!

oranagra commented Jan 24, 2022

Uh oh!

oranagra commented Feb 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

yoav-steinberg commented Jan 17, 2022 •

edited by oranagra

Loading

zuiderkwast commented Jan 20, 2022 •

edited

Loading

zuiderkwast commented Jan 20, 2022 •

edited

Loading