Lua set question when using min-slaves-to-write configure.

[evedark1]

I use Redis 4.0.10 and find something wrong when using Lua script.
using the following steps:

1. set the configure with "min-slaves-to-write=1".
2. start redis as master with no slave.
3. I use redis-cli to execute "SET test testval", return "(error) NOREPLICAS Not enough good slaves to write." as I hoped.
4. I use lua to call "./redis-cli --eval write.lua test , testval", but write success without error. The script:
   redis.cal("SET", KEYS[1], ARGV[1])
5. I can get the key write in lua script.

I think I should get an error when using lua script. Is it a bug in Redis?

[itamarhaber]

Hello @evedark1

Haven't tried reproducing, but if that's the case this is definitely an issue /cc @antirez @soloestoy @oranagra @yossigo

[soloestoy]

That's because the check about min-slaves-to-write is in processCommand:

int processCommand(client *c) {
...
    /* Don't accept write commands if there are not enough good slaves and
     * user configured the min-slaves-to-write option. */
    if (server.masterhost == NULL &&
        server.repl_min_slaves_to_write &&
        server.repl_min_slaves_max_lag &&
        c->cmd->flags & CMD_WRITE &&
        server.repl_good_slaves_count < server.repl_min_slaves_to_write)
    {
        flagTransaction(c);
        addReply(c, shared.noreplicaserr);
        return C_OK;
    }

But eval is not a write command, and in Lua script, redis.call()->luaRedisGenericCommand, we directly use function call() and round the check...

[soloestoy]

At first I think we can fix it just copy the check and put the codes in luaRedisGenericCommand, but there are many other checks in processCommand , and EXEC also has the problem.

@antirez maybe we need a new way to check the write commands?

[oranagra]

Introducing more checks before executing commands inside lua scripts we may be facing more problems like #5250 and #1875.
i.e. we can't fail a redis command in a script due to a local condition inside that server (which may be different on the master and slave), we can only do that based on the contents of the dataset.

[soloestoy]

Introducing more checks before executing commands inside lua scripts we may be facing more problems like #5250 and #1875.

@oranagra I agree with you, so just let it go.

[antirez]

Hello @evedark1, yes, that's a bug. Thanks for reporting, I'll work on it ASAP to resolve it and backport the fix to Redis 4. Moreover the bug is quite critical, because Lua scripting completely bypasses a very important write condition that you can setup in your master. However I need to check with more care about what could be a good way to fix it indeed... Not sure if it's a good idea to just add more checks inside Lua itself or abstract away some code as @soloestoy hints, or what could be the side effects as @oranagra said. I don't think that actually we are going to have problems, since the place where the script is aborted, is exactly where scripts calling random commands will abort once a write is triggered, so we can already fail there which is a good thing.

[oranagra]

@antirez we fail a write after a random command in the same way, but in the case of random commands the same failure will happen on the master and on the slave (same dataset, and same command sequence), but checking for slaves / sub-slaves status (or anything about the server's current state) will yield different result in the master and the slave, so the same script will execute (or fail) differently on the master and slave.

[antirez]

@oranagra yes, but what should happen is that:

1. Failing scripts should not be replicated at all by the master.
2. Slaves should not process such directive.

[antirez]

A problem that we have btw is that AFAIK the failing script not replicating is not enforced by the code. It is just a side effect of write commands being the only one, in theory, to increment the dirty counter... This should kinda work most of the times but it's a design fragility. For instance I don't remember without checking the code what happens if a Lua script calls a command that forces the command propagation without incrementing server.dirty or other possible combinations of things. But that is the old problem again of just enabling effects replication and avoid all this complexity anyway. So my POV is that we need the check, and we should also try to make sure that normally everything happens as expected (see 1 and 2 points in my previous comment), but then make it more robust once we disable scripts body replication.

[felixplajer]

@antirez I just came across this bug as well - any updates on a fix?

[StefanKarlsson321]

@antirez Any updates on this?