valkey
valkey copied to clipboard
valkey-io
[BUG] --dir on command line cannot override .conf
Describe the bug
Passing --dir option on command line cannot override what is already set by the config file.
To reproduce
Set a non-existent path for dir in the config file. Start valkey, passing --dir <path/that/exists> on command line. Valkey will fail to start.
Expected behavior
CLI arguments should override the config as they are more specific.
Additional information
This is impacting the planning for the migration scripts from redis to valkey within the Fedora/RH ecosystem. I'd hoped to use /etc/sysconfig/valkey to set CLI options that are called in by the systemd service. This works for all options except for dir.
The following slack threads have more context and the start of this discussion:
https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713202807288929 https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713217457828379 https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713202054590909 https://valkey-oss-developer.slack.com/archives/C06RQ5P6QPL/p1713202054590909
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.
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.
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().
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
I don't see a need for this change. The default value of dir is ./ at https://github.com/valkey-io/valkey/blob/8abeb79f52cbf02d16d546f48e16592173593132/valkey.conf#L517 and NULL at https://github.com/valkey-io/valkey/blob/8abeb79f52cbf02d16d546f48e16592173593132/src/config.c#L3280 so it can easily be handled via
cd yourdir ; valkey valkey.conf ...
Maybe my problem is an insufficient understanding of how this fits into systemd.
@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?
@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.
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?
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".
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
@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?
@jonathanspw I assume you figured out a workaround here? Do we want to close this, I'm not sure we'll ever really implement this.
Yeah we can close this. I've actually removed the sysconfig file from the RPM and am only using the config file by default now and just modifying it as necessary. It's cleaner all around for users.
Context: https://bugzilla.redhat.com/show_bug.cgi?id=2283798