Bug 1073204 - btrfsmaintenance: Ineffective switch to systemd timers
btrfsmaintenance: Ineffective switch to systemd timers
Status: RESOLVED FIXED
Classification: openSUSE
Product: openSUSE Tumbleweed
Classification: openSUSE
Component: Other
Current
Other Other
: P5 - None : Normal (vote)
: ---
Assigned To: David Sterba
E-mail List
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-12-17 10:10 UTC by Antoine Belvire
Modified: 2019-03-20 16:35 UTC (History)
1 user (show)

See Also:
Found By: ---
Services Priority:
Business Priority:
Blocker: ---
Marketing QA Status: ---
IT Deployment: ---


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Antoine Belvire 2017-12-17 10:10:42 UTC
With snapshot 20171214, btrfsmaintenance has ineffectively switched to systemd timers.

Problems:

- Upon update from the old package to the new package, systemd timers are not present when doing a 'systemctl list-timers --all'. So no btrfs maintenance operation will be performed.

- Upon re-installation of the new package:
  * systemd timers are present but with n/a for all values
  * After reboot, timers are set, but with default periods which do not correspond to the configuration in /etc/sysconfig/btrfsmaintenance

- For both cases, one has to call '/usr/share/btrfsmaintenance/btrfsmaintenance-refresh-cron.sh systemd-timer' to properly set the timers according to the configuration.

- The btrfsmaintenance-refresh.service, which was already pretty useless as it cannot be started by the try-restart in %service_add_post, as it's a oneshot service, is worse now because it can only refresh cron symlinks. And as there is no protection in the refresh script to mutually exclude cron tasks and systemd timers, this may lead to have cron AND systemd timers activated for the same tasks.

- I don't see the point to keep the possibility to use cron tasks knowing that any update of the package will (try to) replace them by systemd timers. You'd better just remove the possibility to have cron tasks and remove the useless btrfsmaintenance-refresh.service, it would be simpler.
Comment 1 Thorsten Kukuk 2017-12-17 11:22:35 UTC
(In reply to Antoine Belvire from comment #0)
> With snapshot 20171214, btrfsmaintenance has ineffectively switched to
> systemd timers.
> 
> Problems:
> 
> - Upon update from the old package to the new package, systemd timers are
> not present when doing a 'systemctl list-timers --all'. So no btrfs
> maintenance operation will be performed.

Over 4 year old bug in systemd-rpm-macros nobody did care to fix and now, after it got fixed, the packages got released in the wrong order :(

> - The btrfsmaintenance-refresh.service, which was already pretty useless as
> it cannot be started by the try-restart %service_add_post, as it's a
> oneshot service, is worse now because it can only refresh cron symlinks. And
> as there is no protection in the refresh script to mutually exclude cron
> tasks and systemd timers, this may lead to have cron AND systemd timers
> activated for the same tasks.

The patch to switch btrfsmaintenance-refresh.service to systemd-timer did go lost when we debugged and fixed the systemd-rpm-macros bug ...
Re-submitted.

> - I don't see the point to keep the possibility to use cron tasks knowing
> that any update of the package will (try to) replace them by systemd timers.
> You'd better just remove the possibility to have cron tasks and remove the
> useless btrfsmaintenance-refresh.service, it would be simpler.

btrfsmaintenance-refresh.service is not useless, why do you think so? And in this case it's even quite handy to fix the broken systemd macros ...
Removing the cron parts would be nice, but would also make the migration much more complicated.
Comment 2 Antoine Belvire 2017-12-17 11:45:47 UTC
Thanks for your quick reply Thorsten.

(In reply to Thorsten Kukuk from comment #1)
> (In reply to Antoine Belvire from comment #0)
> > With snapshot 20171214, btrfsmaintenance has ineffectively switched to
> > systemd timers.
> > 
> > Problems:
> > 
> > - Upon update from the old package to the new package, systemd timers are
> > not present when doing a 'systemctl list-timers --all'. So no btrfs
> > maintenance operation will be performed.
> 
> Over 4 year old bug in systemd-rpm-macros nobody did care to fix and now,
> after it got fixed, the packages got released in the wrong order :(

Oh, OK, then bad luck…

> > - The btrfsmaintenance-refresh.service, which was already pretty useless as
> > it cannot be started by the try-restart %service_add_post, as it's a
> > oneshot service, is worse now because it can only refresh cron symlinks. And
> > as there is no protection in the refresh script to mutually exclude cron
> > tasks and systemd timers, this may lead to have cron AND systemd timers
> > activated for the same tasks.
> 
> The patch to switch btrfsmaintenance-refresh.service to systemd-timer did go
> lost when we debugged and fixed the systemd-rpm-macros bug ...
> Re-submitted.

OK, thanks! (For reference: it's sr#57815.)

> > - I don't see the point to keep the possibility to use cron tasks knowing
> > that any update of the package will (try to) replace them by systemd timers.
> > You'd better just remove the possibility to have cron tasks and remove the
> > useless btrfsmaintenance-refresh.service, it would be simpler.
> 
> btrfsmaintenance-refresh.service is not useless, why do you think so? And in
> this case it's even quite handy to fix the broken systemd macros ...
> Removing the cron parts would be nice, but would also make the migration
> much more complicated.

I find btrfsmaintenance-refresh.service useless because:
- It's never started automatically in spec file (because OneShot service, try-restart does nothing)
- It's only started at boot time, I think. So someone who changes the periods in the config file has to reboot to apply them? He probably wouldn't wait and would call the refresh script and then there is no need to start the service at boot.
- I mean maybe there is a usecase (make things consistent at boot time), but I'm a bit skeptical about this. Just my opinion though.

Thanks again.
Comment 3 Antoine Belvire 2017-12-17 11:51:02 UTC
(In reply to Antoine Belvire from comment #2)
> > The patch to switch btrfsmaintenance-refresh.service to systemd-timer did go
> > lost when we debugged and fixed the systemd-rpm-macros bug ...
> > Re-submitted.
> 
> OK, thanks! (For reference: it's sr#57815.)

Sorry, typo, it's sr#557815.
Comment 4 Thorsten Kukuk 2017-12-17 12:24:34 UTC
(In reply to Antoine Belvire from comment #2)

> I find btrfsmaintenance-refresh.service useless because:
> - It's never started automatically in spec file (because OneShot service,
> try-restart does nothing)

Typical mistake to only think about update in a running system. That's why we have so often packages, which only works after update in a running system but not after a fresh installation or ...
If you exclude this one case, in all other ways to install or update the
package you need it.

> - It's only started at boot time, I think. So someone who changes the
> periods in the config file has to reboot to apply them? He probably wouldn't
> wait and would call the refresh script and then there is no need to start
> the service at boot.

You can enhance the btrfsmaintenance-refresh.service to watch the 
sysconfig file and do that automatically.

> - I mean maybe there is a usecase (make things consistent at boot time), but
> I'm a bit skeptical about this. Just my opinion though.

There are more usecases where you need it at boot time then not.
Comment 5 Antoine Belvire 2017-12-17 12:33:03 UTC
(In reply to Thorsten Kukuk from comment #4)
> (In reply to Antoine Belvire from comment #2)
> 
> > I find btrfsmaintenance-refresh.service useless because:
> > - It's never started automatically in spec file (because OneShot service,
> > try-restart does nothing)
> 
> Typical mistake to only think about update in a running system. That's why
> we have so often packages, which only works after update in a running system
> but not after a fresh installation or ...
> If you exclude this one case, in all other ways to install or update the
> package you need it.

It's not called in any of the RPM scriptlet on fresh installation either (unless the not-yet-available fix for systemd-rpm-macros changes that?). It's only called on system startup.

> > - It's only started at boot time, I think. So someone who changes the
> > periods in the config file has to reboot to apply them? He probably wouldn't
> > wait and would call the refresh script and then there is no need to start
> > the service at boot.
> 
> You can enhance the btrfsmaintenance-refresh.service to watch the 
> sysconfig file and do that automatically.

Interesting.
Comment 6 Antoine Belvire 2017-12-17 14:56:56 UTC
(In reply to Thorsten Kukuk from comment #4)
> You can enhance the btrfsmaintenance-refresh.service to watch the 
> sysconfig file and do that automatically.

I've submitted a pull request to upstream for that: https://github.com/kdave/btrfsmaintenance/pull/41.
Comment 7 Antoine Belvire 2017-12-23 20:10:06 UTC
Snapshot 20171222 contains the fix --> resolved fixed

There's still a small problem of timers activated even if period is set to none in config (submit request 559643) but I guess it can be considered as a different issue.