add better daemon support #37
Conversation
the main intention with this is to have different defaults for when the amdfan is run as a service vs when a user wants to simply run it on the terminal additionally, the daemon now supports forking itself off in the background, which is the expected behaviour on some process managers, such as openrc. on that note, this is a good read: https://jdebp.uk/FGA/unix-daemon-design-mistakes-to-avoid.html Signed-off-by: Mazunki Hoksaas <rolferen@gmail.com>
with this, we are closing down the application safely, and additionally adding support for some nice interfacing Signed-off-by: Mazunki Hoksaas <rolferen@gmail.com>
| @@ -39,10 +39,12 @@ These are all addressed in Amdfan, and as long as I’ve still got some AMD card | |||
| Usage: amdfan [OPTIONS] COMMAND [ARGS]... | |||
|
|
|||
| Options: | |||
| --help Show this message and exit. | |||
| -v, --version Show the version and exit. | |||
There was a problem hiding this comment.
liking this, was thinking of adding it as well, but got sidetracked with work. I pulled down your branch and and just messing with it locally.
There was a problem hiding this comment.
yep, i feel you. i've had some time to tinker lately, which is why i've overhauled a bunch of things. i hope it doesn't feel like i'm gutting the project :P
also the order of the parameters for the --stdout / --no-logfile were causing the daemon to not be able to start up with any commands as the stdout kwarg was being passed to the function when it was expecting no_logfile.
|
Looks good, I pushed up a couple minor changes to deal with the flake8 issues being reported: https://github.com/mcgillij/amdfan/actions/runs/10153195863/job/28076076158#step:5:12 There was a pretty significant bug though with the order of the parameters preventing any of the daemon commands from working.
So I just changed the order so it would start up. |
amdfan/commands.py
Outdated
| @@ -95,9 +95,10 @@ class FileDescriptorOpt(click.ParamType): | |||
| show_default=True, | |||
| ) | |||
| @click.option( | |||
| "--stdout", | |||
There was a problem hiding this comment.
This line breaks the daemon from the cli, moving it down a line fixes it. I pushed up a commit to get this working.
There was a problem hiding this comment.
ah, i should've tested that before i blindly added it after reading the pr notes, lol. i thought "hm, the flag name may suggest there's no logging at all, instead of stdout-ing it". i kinda wish --no-${opt} was supported by click natively tbh. good catch
|
nice :) anything else we should change before updating the version? maybe the min_speed thingie, or the QA issue we briefly talked about? once the version is released i'll bump the ebuild on gentoo, as the current version there is broken |
|
I have a local branch started to cut a new release, but if you want to get anything else prior lemme know. Otherwise I'll push it up tonight if I have time, after work. |
|
I think that's fine. I kind of want to modify how we load the logger and add to the monitor output, so to make it more configurable (also including an option to test the config file before attempting to reload the service), but I think we can do that at another time |
|
yeah being able to validate the config would be nice |
As I mentioned in issue #36 , I wanted to add support for running the service in the background. This PR does just that, and also adds some cleaner shutdown to the controller. I've added some better logging statements and better --help documentation.
Keep in mind the logger is using the same logger as it used to on the terminal, meaning the rich colours we see may be interpreted weirdly by some editors when viewing them. That said, I feel like if that's an issue for some people, that's the user's editor's issue, and not really our fault. If this is a problem, we may choose to add some configuration to it (as we might want to do with the verbosity, fwiw).
The main changes is that
amdfan daemonby default now redirects the output to/var/log/amdfan.log. This can be reverted with--no-logfileor--stdout, or a manual logfile can be set with--logfile=.After some discussion with some init-smart people, I decided that it's better to explicitly request the daemon to run in the background with
--background. By default it will stay running in the foreground, as expected by most init systems.Some commits include more details
Closes: #36