Bug 2283798 - Location of unix socket is hardcoded in /etc/sysconfig/valkey
Summary: Location of unix socket is hardcoded in /etc/sysconfig/valkey
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: valkey
Version: 39
Hardware: x86_64
OS: Linux
unspecified
high
Target Milestone: ---
Assignee: Jonathan Wright
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2024-05-29 12:30 UTC by Joshua Noeske
Modified: 2024-07-12 04:52 UTC (History)
7 users (show)

Fixed In Version: valkey-7.2.5-8.fc40 valkey-7.2.5-8.fc39 valkey-7.2.5-8.el8
Clone Of:
Environment:
Last Closed: 2024-07-12 04:17:49 UTC
Type: ---
Embargoed:


Attachments (Terms of Use)

Description Joshua Noeske 2024-05-29 12:30:01 UTC
Changing the location of the unixsocket in /etc/valkey/valkey.conf does not actually change its location as it is hardcoded in /etc/sysconfig/valkey. It took me quite some time to figure out why the location of my socket was not changing although I changed it. Either inform users about that or remove the hardcoded location of the socket, please.

Reproducible: Always

Steps to Reproduce:
1. Change location of unix socket in /etc/valkey/valkey.conf
2. Restart valkey
Actual Results:  
Unix socket still at /var/run/valkey/valkey.conf

Expected Results:  
Change the location of the unixsocket.

Comment 1 Jonathan Wright 2024-05-31 21:29:50 UTC
Using config files in /etc/sysconfig/<name> is pretty standard practice.  How would you suggest we make this more apparent to users?  I'm open to suggestions.

The main logic behind using this is to avoid having to `sed` the config file from upstream.

Comment 2 Joshua Noeske 2024-06-10 14:47:54 UTC
I think I do not have a perfect solution for that. But I guess that many users will expect that changing the corresponding configuration option in the config file does actually change something. Moreover, when migrating from redis, it might also be desirable to keep the socket and pid file at the same place as the old ones.

In my opinion, there should at least be a warning/info somewhere that mentions that the corresponding options in the config file are useless, pointing users to /etc/sysconfig/valkey. But in my opinion, the superior option is to actually make the option useful again.

What are the arguments against `sed`-ing the config? The only one I can imagine at the moment is that changes have to be monitored to change the sed config in case there are relevant changes upstream. But I would argue that changes to the location of socket and pid files are rather uncommon. But if there are more problems with it, please tell me.

Comment 3 Jonathan Wright 2024-06-10 14:58:30 UTC
Did you use the conversion script(s) (compat package) to move from redis to valkey?  I agree that keeping it the same might be ideal and currently the config gets ported - but /etc/sysconfig/valkey will override it for the settings set within it.  We could extract those settings from the config and update /etc/sysconfig/valkey with them.

Using sed against the default config is not the worst thing, but I like to avoid it where I can.  It's just my preference to avoid it, but if it leads to poor UX then we should just sed it and be done with it.

What about some commends inline in the config file mentioning that the settings are set in /etc/sysconfig/valkey?  Doing a `sed` to add comments is better in my book than modifying the actual options.

> But I would argue that changes to the location of socket and pid files are rather uncommon

My thoughts exactly, and why using /etc/sysconfig/valkey seemed like a good option here.

The only notice we could give users would be a message during %post when installing/upgrading the package, but most people don't really watch for messages there.

Would love some feedback from other maintainers here.  @ngompa13 @nathans @fedora

Comment 4 Joshua Noeske 2024-06-10 15:55:14 UTC
(In reply to Jonathan Wright from comment #3)
> Did you use the conversion script(s) (compat package) to move from redis to
> valkey?  

Yes, I did.

> I agree that keeping it the same might be ideal and currently the
> config gets ported - but /etc/sysconfig/valkey will override it for the
> settings set within it.  We could extract those settings from the config and
> update /etc/sysconfig/valkey with them.

Sounds like a sensible idea, in case it is decided in favour of keeping it at /etc/sysconfig/valkey.
 
> What about some commends inline in the config file mentioning that the
> settings are set in /etc/sysconfig/valkey?  Doing a `sed` to add comments is
> better in my book than modifying the actual options.

Again, sensible, if  /etc/sysconfig/valkey is kept.
 
> > But I would argue that changes to the location of socket and pid files are rather uncommon
> 
> My thoughts exactly, and why using /etc/sysconfig/valkey seemed like a good
> option here.

Actually, I rather meant that *upstream* changes of these locations are uncommon. 
I do think that there are some users, like me, who might want to change the location of these.
Furthermore, I personally prefer having all configs in one place instead of having to switch files for them, if it is not due to different `.conf` files in a `.d` directory.
 
> The only notice we could give users would be a message during %post when
> installing/upgrading the package, but most people don't really watch for
> messages there.

That could be an additional thing, but a comment in the config file is, at least in my opinion, much more likely to be read.

Comment 5 Remi Collet 2024-06-11 05:05:39 UTC
/etc/sysconfig is an old thing used by SysV init scripts

IMHO this is deprecated and should be avoided, especially for a new package

It was dropped from "redis" years ago

More: it will break the "config rewrite" feature which is only aware of the config file
https://valkey.io/commands/config-rewrite/

Comment 6 Joe Orton 2024-06-13 12:44:20 UTC
Strong agreement with Remi's comment 5 to not use /etc/sysconfig for any new service - these are  anachronistic and using override files is the right way to adjust systemd services. This has been a source of problems historically, people think they are shell scripts, but they are really a unique kind of config file with syntax specific to systemd.

Comment 7 Jonathan Wright 2024-06-13 14:17:12 UTC
Thanks for all the feedback.  I intend to remove /etc/sysconfig/valkey* from the package.  I'm working on a way to cleanly migrate away from this without causing breakages for users.

Side note...we need to update the packaging guidelines to advise against the usage of /etc/sysconfig/* config files in new packages.  If someone beats me to this PR I wouldn't complain...

Comment 8 Nathan Scott 2024-06-14 04:26:32 UTC
Apologies for the slow reply - looks like its already moved in a direction I agree with too FWIW - +1 to less /etc/sysconfig use, thanks.

Comment 9 Neal Gompa 2024-06-14 06:07:23 UTC
(In reply to Jonathan Wright from comment #7)
> Thanks for all the feedback.  I intend to remove /etc/sysconfig/valkey* from
> the package.  I'm working on a way to cleanly migrate away from this without
> causing breakages for users.
> 
> Side note...we need to update the packaging guidelines to advise against the
> usage of /etc/sysconfig/* config files in new packages.  If someone beats me
> to this PR I wouldn't complain...

No we don't. /etc/sysconfig config files are intended to be used as a preferred location for services that are not runtime configurable. A lot of services only can be configured at initialization via command line arguments or environment variables (particularly stuff written in Go, in my experience) and this is where you put environmentfiles for that.

Yes, it originates from SysV, but it's as good of a place as any for that.

That said, Valkey should not need such a file, so we should endeavor to get rid of it. I also want to port over some interesting stuff from my work on the openSUSE valkey package to the Fedora one to offer some more flexibility.

Comment 10 Jonathan Wright 2024-06-18 03:37:44 UTC
I'm working on this in https://src.fedoraproject.org/rpms/valkey/pull-request/4

I'd appreciate some eyes on the scriptlet logic.  Thanks to Carl George for bouncing ideas with me thus far on some of it.

Comment 11 Fedora Update System 2024-07-03 01:51:34 UTC
FEDORA-2024-2e3fb603d5 (valkey-7.2.5-8.fc39) has been submitted as an update to Fedora 39.
https://bodhi.fedoraproject.org/updates/FEDORA-2024-2e3fb603d5

Comment 12 Fedora Update System 2024-07-03 01:51:35 UTC
FEDORA-EPEL-2024-4e6abf760b (valkey-7.2.5-8.el8) has been submitted as an update to Fedora EPEL 8.
https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-4e6abf760b

Comment 13 Fedora Update System 2024-07-04 01:58:00 UTC
FEDORA-2024-0425165cce has been pushed to the Fedora 40 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-0425165cce`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-0425165cce

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 14 Fedora Update System 2024-07-04 02:01:34 UTC
FEDORA-2024-2e3fb603d5 has been pushed to the Fedora 39 testing repository.
Soon you'll be able to install the update with the following command:
`sudo dnf upgrade --enablerepo=updates-testing --refresh --advisory=FEDORA-2024-2e3fb603d5`
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2024-2e3fb603d5

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 15 Fedora Update System 2024-07-04 02:08:29 UTC
FEDORA-EPEL-2024-4e6abf760b has been pushed to the Fedora EPEL 8 testing repository.

You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2024-4e6abf760b

See also https://fedoraproject.org/wiki/QA:Updates_Testing for more information on how to test updates.

Comment 16 Fedora Update System 2024-07-12 04:17:49 UTC
FEDORA-2024-0425165cce (valkey-7.2.5-8.fc40) has been pushed to the Fedora 40 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 17 Fedora Update System 2024-07-12 04:17:52 UTC
FEDORA-2024-2e3fb603d5 (valkey-7.2.5-8.fc39) has been pushed to the Fedora 39 stable repository.
If problem still persists, please make note of it in this bug report.

Comment 18 Fedora Update System 2024-07-12 04:52:21 UTC
FEDORA-EPEL-2024-4e6abf760b (valkey-7.2.5-8.el8) has been pushed to the Fedora EPEL 8 stable repository.
If problem still persists, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.