[BUG] --dir on command line cannot override .conf about valkey HOT 11 OPEN

I think this is worth fixing and I would like to propose a generic fix that allows any command line argument to override its counterpart in the config file. @daniel-house I believe you had some concerns/questions about this use case?

@jonathanspw it would be helpful if you could describe the alternatives for your use cases as well.

from valkey.

I think this is worth fixing and I would like to propose a generic fix that allows any command line argument to override its counterpart in the config file. @daniel-house I believe you had some concerns/questions about this use case?

@jonathanspw it would be helpful if you could describe the alternatives for your use cases as well.

The only alternative is to use sed in our migration scripts to modify the redis config when converting to valkey. I'd rather not do that, but it's not out of the question either.

I figure this might impact others as well and there's no real downside to fixing it, so this is the more ideal approach IMO.

from valkey.

It doesn't seem to matter if it's a file or not. The thing is that for the dir option, we just do chdir immediately. The dir value isn't even store anywhere.

./valkey-server --dir "banana" --dir ".."

*** FATAL CONFIG FILE ERROR (Version 255.255.255) ***
Reading the configuration file, at line 2
>>> 'dir "banana"'
No such file or directory

The error message is from strerror(errno) after a failed chdir().

Likewise, when getting the value (e.g. ./valkey-cli CONFIG GET DIR) just does getcwd().

from valkey.

Does the issue exist in redis before or Not? Or it only happens in some special OS since Fedora/RH ecosystem is mentioned in the top comment?

If the issue happens in redis OSS , I think it belongs to a new feature instead of a bug

from valkey.

I don't see a need for this change. The default value of dir is ./ at

cd yourdir ; valkey valkey.conf ...

Maybe my problem is an insufficient understanding of how this fits into systemd.

from valkey.

@jonathanspw The problem isn't tied to file vs command line. The problem is the invalid value in the file.

What value of dir do you have in the file? The default value is "./" which I believe always exists. How come there's a different value and how did that get into the file?

from valkey.

@jonathanspw The problem isn't tied to file vs command line. The problem is the invalid value in the file.

What value of dir do you have in the file? The default value is "./" which I believe always exists. How come there's a different value and how did that get into the file?

Conversion from redis. The path can be defined as the old redis dir, for example /var/lib/redis. The conversion script move this to .bak, copies the data over to the expected valkey dir, in the case of Fedora/EPEL /var/lib/valkey, so it no longer exists, relying on the command line override to point to the valkey dir, but the config value persists and causes the issue.

from valkey.

OK, I get it. So maybe sed is the best you can do.

Each 'dir' config does a chdir so we use the current dir in the file system to as the state. Therefore I can't see how we can fix this without breaking something else. Can you?

from valkey.

OK, I get it. So maybe sed is the best you can do.

Each 'dir' config does a chdir so we use the current dir in the file system to as the state. Therefore I can't see how we can fix this without breaking something else. Can you?

I think we can fix it. The way we evaluate the options today essentially concatenates the conf file and the command line configs. Imagine that we have a config file config_1 value_1 \n config_2 value_2 and a list of command line configs config_1 value_2 \n config_3 value_3, we get a combined string of config_1 value_1 \n config_2 value_2 \n config_1 value_2 \n config_3 value_3. We then start parsing this sting and if config_1 here is dir, we immediately chdir, which would fail and then abort the server. What I propose would be adding step after the concatenation which handles the overrides properly such that the later config wins and we will get config_1 value_2 \n config_2 value_2 \n config_3 value_3.

This is a genetic enough solution that can be documented easily: "command lines configs always take precedence".

from valkey.

How about just not immediately executing chdir? After setting all the config variables, then apply chdir to whatever is in the dir-field.

Are there other directives that would cause a problem? For example, do we have something weird where

chdir a (--dir a)
--property1=x
chdir b (--dir b)
--property2=y

would break if it were replaced with

--property1=x
--property2=y
chdir b

from valkey.

@PingXie @daniel-house In general, we can't just skip all but the last chdir. In general chdir a; chdir b means chdir a/b. Consider this:

valkey-server --dir foo --dir ..

Or any other relative walk in the file system.

We can't assume foo/.. = . That doesn't hold if foo is a symlink to somewhere else.

We can ignore previous dirs if the last dir is an absolut path though. Otherwise we can concat them with "/". I mean for --dir d:

• If dir is empty, store dir = d
• else if b starts with "/", store dir = b
• else store dir = dir + "/" + d.

I'm not sure it covers all of chdir's behaviour but I hope so. Wdut?

from valkey.