-
-
Notifications
You must be signed in to change notification settings - Fork 115
Feat: adds the installation of an openrc service script #947
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Sorry, there were two bugs that have now being fixed. Actually, I'd rather have backrest running with the new openrc "User services" mechanism, but so far I didn't get much luck, so I'll just provide this one-2-one translation of your systemd unit. |
|
Thanks for the contribution, I think this change seems reasonable and I'm happy enough to support it in the installer. The biggest caveat I'd call out is that I don't have a system with openrc or any ability to test this on future upgrades or changes to the script. I'm inclined to err on the side of merging this since I don't forsee a reason it'd just stop working or that i'd want to overhaul the installer, but it's very possible that this installer capability could get broken (occasionally) in the future. |
|
Hello, I 100% understand your point. I'm willing to maintain this feature going forward. I'm using your program (of course!), so I'll update it as new versions come and the fact that's an active development project with many PRs, open issues etc. makes me believe that's worthed to support it. |
9fd14cf to
8ec1182
Compare
8ec1182 to
105097e
Compare
105097e to
ea61839
Compare
|
Sorry, I had forgotten to sign my commits and had my good time repairing that. |
|
This is my openrc for gentoo #!/sbin/openrc-run
name="backrest"
description="backrest Daemon"
command="/usr/bin/backrest"
command_args="-restic-cmd /usr/sbin/restic -data-dir /var/lib/backrest/ -config-file /var/lib/backrest/config.json"
command_background=true
pidfile="/run/backrest.pid"
supervisor=supervise-daemon
output_log="/var/log/backrest/backrest.log"
error_log="/var/log/backrest/backrest.log"
depend() {
need net
}
from: I think it would not be necessary to require a logger |
|
Hello, Thank you for chiming in. First of all, I'd like to know why you add all those args. I felt they were actually NOT needed, I only exported BACKREST_PORT as a variable (not elegant, I know.), but I'm totally in for including them if they are needed. And I'm particularly referring to the restic part. My understanding was that the current restic installation is automatically detected by backrest via a "restic" command call, but if I'm wrong it is clear that we must use your version of that line, instead. I don't "need" a logger, but I "use" a logger and the network ("if present, please start them before backrest"). On the logger side, I might agree with you, since backrest logs on his own. I see that your script misses the user line, are you running it as root. If so, why? Once I used it like this, all the backups were root 700 (if don't go wrong). Why would root own my backups? Finally, you put in the line for "supervise-daemon", I've been on the edge of putting it into my script, too, but then I simply didn't. Do you think that's a critical service that must/should be supervised? Maybe we should add it? Finally, on an Off Topic [sorry], but definitely interesting, note... Have you got an ebuild for this? EDIT: sorry, I just investigated your link better, THAT IS an overlay. I was more of the idea of creating an overlay with ONLY backrest, but I see you've got many things there... I'll check that out. Thank you for your contribution, |
I think skipping need net is fine, assuming this waits for LAN / WAN? The right way to handle this imo is to leave it up to the user to define backup start healthchecks that await the network.
right, I think whoami is the best way to do this. Users wanting to use root can run the installer with sudo.
thanks for taking care of commit signing, overall this lgtm. Will go ahead and merge as is for the next release. Thanks for contributing the support |
|
I use the root user because I use it as a backup for any data on the servers, email, web, SQL, etc., and I don't have restrictions when copying. Of course, this can have security implications, but in my case, I haven't seen a more convenient solution for backups. |
The user inode64 in the pullrequest garethgeorge#947 suggested to add the line: supervisor=supervise-daemon As I agree with him, the line is now added to the openrc service script.
This is my own implementation, responding to the Issue #946 (Add openrc support) which I myself opened.
I'd like to point out again that I did change the systemd (now systemd/openrc) detection method.