Improve systemd units #498
Conversation
btrbk supports hourly backups, so it makes sense for the timer to run hourly, too. Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
It does not make sense for the timer or the service to be started (the latter when done so manually) when neither of the two default config files exists. Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
systemd’s `AccuracySec` is mainly there in order to allow it to coalescing wake-ups for multiple timers. It’s default of `1min` minute should be enough for this. Using its `RandomizedDelaySec` in order to spread load from e.g. multiple sources that would all perform backups to one target, wouldn’t really make much sense either. Either, the value would need to be quite large, thereby making backups/snapshots too wobbly, or it wouldn’t be effective as at least the backups typically take quite some time. Signed-off-by: Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>
| @@ -1,6 +1,8 @@ | |||
| [Unit] | |||
| Description=btrbk backup | |||
| Documentation=man:btrbk(1) | |||
| ConditionPathExists=|@CONFDIR@/btrbk/btrbk.conf | |||
There was a problem hiding this comment.
I'm not using systemd myself, and my knowledge is a bit limited here (and no, not going to read through all the docs, all those options are one of the reasons for me not to use systemd).
This sounds like a good thing to have. does that mean that the service will not fail if the config file does not exist, even if you start it?
There was a problem hiding this comment.
Yes, AFAIU, it should not fail (that would require an AssertPathExists).
| @@ -1,9 +1,10 @@ | |||
| [Unit] | |||
| Description=btrbk daily backup | |||
| ConditionPathExists=|@CONFDIR@/btrbk/btrbk.conf | |||
There was a problem hiding this comment.
Is it a good idea to set this in the timer as well? a user might want to override the service with ExecStart=/bin/btrbk run -c some_other_config, which would somehow break the timer.
There was a problem hiding this comment.
Hmm... TBH, I'm not exactly sure how "bad" it is to run the time and let just the service not run because of the conditional.
Probably not at all, and at most it would be "conceptually" ugly, i.e. when the timer is considered to have been fired, but the service actually didn't.
But even this may not be considered that way by systemd experts.
On could argue, that if a user wants to override the .service he could as well just also override the .timer.
So.. yeah... I have no real strong opinion about this.
|
|
||
| [Timer] | ||
| OnCalendar=daily |
There was a problem hiding this comment.
I think daily is a good default. Some want it hourly, some daily, and I know (at least one) doing it minutely ;-)
There was a problem hiding this comment.
Forget about that commit... it was late in the night and I was pretty dumb ^^
I completely forgot that the times in the retention policies don't determine how often the tool runs, but how long the snapshots/etc. are kept... so hourly would be really bad.
It could however make sense, if somewhere we add the proper systemd way on how to override (described in systemd.unit(5)) the time, namely by adding a file:
/etc/systemd/system/btrbk.timer.d/foo.conf with just what shall be overridden:
[Timer]
OnCalendar=hourly
|
|
||
| [Timer] | ||
| OnCalendar=daily | ||
| AccuracySec=10min |
There was a problem hiding this comment.
Not sure myself why I set this, must have read something when initially playing around with btrbk and systemd. Guess the idea was to tell the timer it's ok to be delayed a bit, as there might be other concurrent btrbk timers running (e.g. with different config and execution times)
There was a problem hiding this comment.
Well then it depends:
Was your idea to spread the time when these fire off (to not cause all load at the same time)? Then RandomizedDelaySec= would be the appropriate setting, AFAIU.
Otherwise, I, personally would say, 1min (default) would be enough.
Actually if you give system "so much time" just to fold multiple runs... it would rather cause more load (because more btrbks may run at the same time).
I could update the commit with whatever you like in the end. (The OnCaleder thing goes away of course)
digint
force-pushed
the
master
branch
6 times, most recently
from
594fc6d
to
dac6350
Compare
Hey.
Some ideas…
Not so sure about the last commit, since I haven’t found why you had put
AccuracySecin at the first place. So if there was a good reason for it, just skip that commit.Thanks,
Chris.