https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Bug ID: 2283798 Summary: Location of unix socket is hardcoded in /etc/sysconfig/valkey Product: Fedora Version: 39 Hardware: x86_64 OS: Linux Status: NEW Component: valkey Severity: high Assignee: jonathan@almalinux.org Reporter: fedora@joshuanoeske.de CC: epel-packagers-sig@lists.fedoraproject.org, fedora@famillecollet.com, jonathan@almalinux.org, nathans@redhat.com, ngompa13@gmail.com Target Milestone: --- Classification: Fedora
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Joshua Noeske fedora@joshuanoeske.de changed:
What |Removed |Added ---------------------------------------------------------------------------- Comment|0 |updated
--- Comment #0 has been edited ---
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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Jonathan Wright jonathan@almalinux.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Doc Type|--- |If docs needed, set a value
--- Comment #1 from Jonathan Wright jonathan@almalinux.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
--- Comment #2 from Joshua Noeske fedora@joshuanoeske.de --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Jonathan Wright jonathan@almalinux.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags| |needinfo?(ngompa13@gmail.co | |m) | |needinfo?(nathans@redhat.co | |m) | |needinfo?(fedora@famillecol | |let.com)
--- Comment #3 from Jonathan Wright jonathan@almalinux.org --- 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@gmail.com @nathans@redhat.com @fedora@famillecollet.com
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
--- Comment #4 from Joshua Noeske fedora@joshuanoeske.de --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Remi Collet fedora@famillecollet.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(fedora@famillecol | |let.com) |
--- Comment #5 from Remi Collet fedora@famillecollet.com --- /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/
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Joe Orton jorton@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |jorton@redhat.com
--- Comment #6 from Joe Orton jorton@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
--- Comment #7 from Jonathan Wright jonathan@almalinux.org --- 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...
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Nathan Scott nathans@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(nathans@redhat.co | |m) |
--- Comment #8 from Nathan Scott nathans@redhat.com --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Neal Gompa ngompa13@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flags|needinfo?(ngompa13@gmail.co | |m) |
--- Comment #9 from Neal Gompa ngompa13@gmail.com --- (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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
--- Comment #10 from Jonathan Wright jonathan@almalinux.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |MODIFIED
--- Comment #11 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |MODIFIED
--- Comment #11 from Fedora Update System updates@fedoraproject.org --- 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 from Fedora Update System updates@fedoraproject.org --- 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
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|MODIFIED |ON_QA
--- Comment #13 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
--- Comment #14 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
--- Comment #15 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |ERRATA Status|ON_QA |CLOSED Fixed In Version| |valkey-7.2.5-8.fc40 Last Closed| |2024-07-12 04:17:49
--- Comment #16 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|valkey-7.2.5-8.fc40 |valkey-7.2.5-8.fc40 | |valkey-7.2.5-8.fc39
--- Comment #17 from Fedora Update System updates@fedoraproject.org --- 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.
https://bugzilla.redhat.com/show_bug.cgi?id=2283798
Fedora Update System updates@fedoraproject.org changed:
What |Removed |Added ---------------------------------------------------------------------------- Fixed In Version|valkey-7.2.5-8.fc40 |valkey-7.2.5-8.fc40 |valkey-7.2.5-8.fc39 |valkey-7.2.5-8.fc39 | |valkey-7.2.5-8.el8
--- Comment #18 from Fedora Update System updates@fedoraproject.org --- 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.
epel-packagers-sig@lists.fedoraproject.org
-
bugzilla＠redhat.com