Bug 25245 - Add a plugin hook to allow running code on a nightly basis
Summary: Add a plugin hook to allow running code on a nightly basis
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Plugin architecture (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
: 20897 (view as bug list)
Depends on:
Blocks:
 
Reported: 2020-04-22 10:57 UTC by Martin Renvoize
Modified: 2020-07-02 12:42 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25245: Add plugins_nightly.pl cronjob script (2.00 KB, patch)
2020-04-22 11:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl cronjob script (2.00 KB, patch)
2020-04-22 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl to the default cron (1.00 KB, patch)
2020-04-22 11:12 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl cronjob script (2.07 KB, patch)
2020-04-22 17:02 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl to the default cron (1.07 KB, patch)
2020-04-22 17:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl cronjob script (1.95 KB, patch)
2020-04-27 12:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: (follow-up) Use Koha::Logger (1.03 KB, patch)
2020-04-27 12:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl to the default cron (1.00 KB, patch)
2020-04-27 12:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl cronjob script (1.95 KB, patch)
2020-04-27 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: (follow-up) Use Koha::Logger (995 bytes, patch)
2020-04-27 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl to the default cron (1.00 KB, patch)
2020-04-27 12:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl to the default cron (1.46 KB, patch)
2020-04-27 12:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25188: Make circulation notes more prominent on the patron details tab (2.01 KB, patch)
2020-05-05 06:53 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl cronjob script (2.00 KB, patch)
2020-05-27 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25245: (follow-up) Use Koha::Logger (1.03 KB, patch)
2020-05-27 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl to the default cron (1.51 KB, patch)
2020-05-27 14:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl cronjob script (2.05 KB, patch)
2020-06-19 11:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: (follow-up) Use Koha::Logger (1.08 KB, patch)
2020-06-19 11:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25245: Add plugins_nightly.pl to the default cron (1.56 KB, patch)
2020-06-19 11:22 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2020-04-22 10:57:45 UTC
Some plugins, Recalls for example, require the addition of a cronjob that runs on a nightly basis.

We could greatly ease the installation of such plugins by introducing a new default cron script to that runs nightly and simply iterated through plugins implementing a cronjob_nightly method.
Comment 1 Martin Renvoize 2020-04-22 11:05:46 UTC
Created attachment 103454 [details] [review]
Bug 25245: Add plugins_nightly.pl cronjob script

This script simply iterates through installed plugins that impliment a
cronjob_nightly method and runs said method.
Comment 2 Martin Renvoize 2020-04-22 11:12:34 UTC
Created attachment 103460 [details] [review]
Bug 25245: Add plugins_nightly.pl cronjob script

This script simply iterates through installed plugins that impliment a
cronjob_nightly method and runs said method.
Comment 3 Martin Renvoize 2020-04-22 11:12:37 UTC
Created attachment 103461 [details] [review]
Bug 25245: Add plugins_nightly.pl to the default cron
Comment 4 Martin Renvoize 2020-04-22 11:19:28 UTC
Test plan

1/ Apply patch
2/ Ensure your cron setup calls the new plugins_nightly.pl
3/ Install the recalls plugin and skip the 'setup cron_nightly.pl' step
4/ The recalls plugin should function correctly even though you've not done the manual cron intervention.
5/ Signoff
Comment 5 Kyle M Hall 2020-04-22 17:02:19 UTC
Created attachment 103497 [details] [review]
Bug 25245: Add plugins_nightly.pl cronjob script

This script simply iterates through installed plugins that impliment a
cronjob_nightly method and runs said method.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2020-04-22 17:03:03 UTC
Created attachment 103498 [details] [review]
Bug 25245: Add plugins_nightly.pl to the default cron

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Marcel de Rooy 2020-04-24 06:22:25 UTC
Some QA comments:
use Modern::Perl
Where do you need FindBin for here?
I understand the test on enable_plugins but I feel that it should be part of GetPlugins somehow. If you did not enable them, GetPlugins should take care of it and return nothing.
The if(@plugins) is not needed. You are looping thru @plugins inside.
Why do you warn $_ ?

Second patch
If you are adjusting the debian koha-conf, you should also touch the other one.
Comment 8 Martin Renvoize 2020-04-27 12:32:45 UTC
Created attachment 103767 [details] [review]
Bug 25245: Add plugins_nightly.pl cronjob script

This script simply iterates through installed plugins that impliment a
cronjob_nightly method and runs said method.
Comment 9 Martin Renvoize 2020-04-27 12:32:48 UTC
Created attachment 103768 [details] [review]
Bug 25245: (follow-up) Use Koha::Logger

Update script to use Koha::Logger to capture method failures.
Comment 10 Martin Renvoize 2020-04-27 12:32:51 UTC
Created attachment 103769 [details] [review]
Bug 25245: Add plugins_nightly.pl to the default cron
Comment 11 Martin Renvoize 2020-04-27 12:45:06 UTC
Created attachment 103774 [details] [review]
Bug 25245: Add plugins_nightly.pl cronjob script

This script simply iterates through installed plugins that impliment a
cronjob_nightly method and runs said method.
Comment 12 Martin Renvoize 2020-04-27 12:45:09 UTC
Created attachment 103775 [details] [review]
Bug 25245: (follow-up) Use Koha::Logger

Update script to use Koha::Logger to capture method failures.
Comment 13 Martin Renvoize 2020-04-27 12:45:13 UTC
Created attachment 103776 [details] [review]
Bug 25245: Add plugins_nightly.pl to the default cron
Comment 14 Martin Renvoize 2020-04-27 12:49:05 UTC
Created attachment 103778 [details] [review]
Bug 25245: Add plugins_nightly.pl to the default cron
Comment 15 Martin Renvoize 2020-04-27 12:52:09 UTC
(In reply to Marcel de Rooy from comment #7)
> Some QA comments:
> use Modern::Perl

Corrected

> Where do you need FindBin for here?

We don't, I've removed it.. I coded it in a hurry whilst I thought of the idea ;)

> I understand the test on enable_plugins but I feel that it should be part of
> GetPlugins somehow. If you did not enable them, GetPlugins should take care
> of it and return nothing.

Agreed, but I'm not sure of the other implications.. as such I'd prefer to do that in a distinct bug.

> The if(@plugins) is not needed. You are looping thru @plugins inside.

Agreed, corrected.

> Why do you warn $_ ?

Following prior art. It makes sense to warn like this for such errors.  I have however added a Koha::Logger based log line too now so we don't lose warnings for cases where cron sends them to dev/null.

> 
> Second patch
> If you are adjusting the debian koha-conf, you should also touch the other
> one.

Amended patch.
Comment 16 David Cook 2020-04-28 00:53:10 UTC
Oh... I like the sound of this. Could be very handy.
Comment 17 Marcel de Rooy 2020-04-28 13:07:25 UTC
(In reply to Martin Renvoize from comment #15)
> > Why do you warn $_ ?
> 
> Following prior art. It makes sense to warn like this for such errors.  I
> have however added a Koha::Logger based log line too now so we don't lose
> warnings for cases where cron sends them to dev/null.

You probably mean something else? Like $@ ? The wonderous names of Perl special variables. I am really not sure what $_ would be referring to in this catch block?
Comment 18 Tomás Cohen Arazi 2020-04-28 13:10:24 UTC
(In reply to Marcel de Rooy from comment #17)
> (In reply to Martin Renvoize from comment #15)
> > > Why do you warn $_ ?
> > 
> > Following prior art. It makes sense to warn like this for such errors.  I
> > have however added a Koha::Logger based log line too now so we don't lose
> > warnings for cases where cron sends them to dev/null.
> 
> You probably mean something else? Like $@ ? The wonderous names of Perl
> special variables. I am really not sure what $_ would be referring to in
> this catch block?

eval sets $@, Try::Tiny preserves it just in case you need it for other purposes, and sets $_. So, yes: it is the correct way with Try::Tiny. Look at all the API controllers for examples.
Comment 19 Marcel de Rooy 2020-04-28 13:13:04 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> (In reply to Marcel de Rooy from comment #17)
> > (In reply to Martin Renvoize from comment #15)
> > > > Why do you warn $_ ?
> > > 
> > > Following prior art. It makes sense to warn like this for such errors.  I
> > > have however added a Koha::Logger based log line too now so we don't lose
> > > warnings for cases where cron sends them to dev/null.
> > 
> > You probably mean something else? Like $@ ? The wonderous names of Perl
> > special variables. I am really not sure what $_ would be referring to in
> > this catch block?
> 
> eval sets $@, Try::Tiny preserves it just in case you need it for other
> purposes, and sets $_. So, yes: it is the correct way with Try::Tiny. Look
> at all the API controllers for examples.

Ah great Thanks
Comment 20 Martin Renvoize 2020-04-28 16:05:01 UTC
Ah.. sorry.. I hadn't spotted you were worried about the$_ as apposed to $@.. I thought it was the introduction of a 'warn' at all you were commenting on.

Yes, as Tomas explains, Try::Tiny catches $@ and throws it into $_.. it's a peculiarity of that module.. I've wondered for a while about switching it out with Syntax::Keyword::Try which I believe is now recommended by the Perl echelons.
Comment 21 Jonathan Druart 2020-05-04 10:37:47 UTC
I have the feeling that this should have been implemented on top of bug 22417 (with bug 1993 in mind).
Comment 22 Kyle M Hall 2020-05-04 13:13:47 UTC
(In reply to Jonathan Druart from comment #21)
> I have the feeling that this should have been implemented on top of bug
> 22417 (with bug 1993 in mind).

Is there any reason this cannot be updated to use the functionality of those bugs once they've been taken care of? It doesn't look like there is a definite end in site for them.
Comment 23 Tomás Cohen Arazi 2020-05-04 13:23:01 UTC
(In reply to Kyle M Hall from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > I have the feeling that this should have been implemented on top of bug
> > 22417 (with bug 1993 in mind).
> 
> Is there any reason this cannot be updated to use the functionality of those
> bugs once they've been taken care of? It doesn't look like there is a
> definite end in site for them.

I agree here. Bug 22417 is a longer shot IMHO.
Comment 24 Jonathan Druart 2020-05-04 13:46:45 UTC
The main inconvenient of this implementation is that the time the job will start is not configurable.
Comment 25 David Cook 2020-05-04 23:53:43 UTC
(In reply to Jonathan Druart from comment #24)
> The main inconvenient of this implementation is that the time the job will
> start is not configurable.

That's true although that's a criticism I have of many of the daily cronjobs. (We're switching over more to using the Koha Debian packages, and noticed one of the gaps between it and our custom installs is the inflexibility of the cronjobs out of the box.)
Comment 26 David Cook 2020-05-05 00:06:55 UTC
(In reply to Tomás Cohen Arazi from comment #23)
> (In reply to Kyle M Hall from comment #22)
> > (In reply to Jonathan Druart from comment #21)
> > > I have the feeling that this should have been implemented on top of bug
> > > 22417 (with bug 1993 in mind).
> > 
> > Is there any reason this cannot be updated to use the functionality of those
> > bugs once they've been taken care of? It doesn't look like there is a
> > definite end in site for them.
> 
> I agree here. Bug 22417 is a longer shot IMHO.

I agree with everyone in a way.

I've spent years talking about and working on task scheduler functionality for Koha, and I don't think it's happening any time soon. Cron is a well-established familiar task scheduler that can do a good enough job. 

I'd argue Bug 22417 tries to be too comprehensive, and that we'd be better off just adding RabbitMQ and Net::Stomp as dependencies, and then doing a light experimental implementation here. However, even if we had only 1 worker consuming from RabbitMQ, that's another process to manage. It adds new administrative overhead and fragility that we don't have by continuing to use cron.

Frido also made a good point back in Portland about queue based task schedulers (like the one I wrote). From a vendor point of view, you might not want all your Koha instances running their background jobs at the same time. If those background jobs use APIs, you might end up hitting rate limits for instance. Or if you're sending out lots of emails, you might get flagged as a spammer. So it becomes prudent for vendors to be able to stagger background jobs on a server.

Of course, that criticism applies to using cron.daily for background jobs too.
Comment 27 David Cook 2020-05-05 01:00:21 UTC
(In reply to David Cook from comment #26)
> I'd argue Bug 22417 tries to be too comprehensive, and that we'd be better
> off just adding RabbitMQ and Net::Stomp as dependencies, and then doing a
> light experimental implementation here.

Actually, that wouldn't work. We would need a proper task scheduler.

It's a nice idea in theory, but I don't think it's happening any time soon, so I'd say go with cron and try to solve this one in the future :/.
Comment 28 David Cook 2020-05-05 03:10:39 UTC
Actually, in Jonathan's defence again, on Ubuntu 18.04 it wouldn't be a "nightly" method so much as a daily/every morning method when run in cron.daily, as it's set up to run at 6:25am out of the box.

Maybe it should go in /etc/cron.d/koha-common instead with a better time defined. And then local admins can customize that.
Comment 29 Katrin Fischer 2020-05-05 06:46:27 UTC
I feel like this could already help a lot of plugin authors and libraries as it will allow to streamline installing plugins that need to process data overnight. An even more flexible solution would be great (agreed!), but maybe this is a good enough first step even with its limitations?
Comment 30 Katrin Fischer 2020-05-05 06:53:49 UTC
Created attachment 104345 [details] [review]
Bug 25188: Make circulation notes more prominent on the patron details tab

This patch adds a more prominent circulation note to the moremember.pl
details screen.

To test:

1) Add a circulation note to a patron record.
2) Note that it displays prominently on the checkout tab, but only under
the Library Use block on the details tab.
3) Apply the patch.
4) Check that the note is now displayed prominently at the top of the
details (moremember.pl) screen.

https://bugs.koha-community.org/show_bug.cgi?id=25118

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Katrin Fischer 2020-05-05 06:54:38 UTC
Comment on attachment 104345 [details] [review]
Bug 25188: Make circulation notes more prominent on the patron details tab

Wrong bug number...
Comment 32 Martin Renvoize 2020-05-05 07:14:22 UTC
I didn't want to get stuck in the queue waiting for any of the schedulers rewrite bugs so it was a deliberate choice not to base this code upon them.  Cron is a simple system and this is really designed for simple use cases... the idea being we just want a task to run each 'night', but we're not too fussy as to when.

We already have a hook added to trigger plugin actions prior to the fines job and I like the idea of slowly adding such triggers to the start/end of other regular tasks so one can dovetail workloads as required.. but for general 'process stuff daily' task I personally feel this is enough.

You can happily gain a little more control over the scheduling within your plugin code by doing a date comparison as the first part of the job in your plugin so one could for instance set the tasks to take place ever X days rather than nightly, or every Friday for example.

It would also allow simple installation of the existing recalls plugin which currently requires lots of background manipulation to install.
Comment 33 David Cook 2020-05-05 23:11:10 UTC
(In reply to Martin Renvoize from comment #32)
> You can happily gain a little more control over the scheduling within your
> plugin code by doing a date comparison as the first part of the job in your
> plugin so one could for instance set the tasks to take place ever X days
> rather than nightly, or every Friday for example.
> 

What do you think about putting the cronjob in /etc/cron.d/koha-common instead of /etc/cron.daily/koha-common?

The former could run at very configurable time whereas the latter runs at 6:25am out of the box. 

I could see Koha sysadmins customizing /etc/cron.d/koha-common much more readily than /etc/crontab (as you couldn't want to changethe time for all daily cronjobs).

I think that would be a very reasonable change to make to the current patch set?
Comment 34 Katrin Fischer 2020-05-18 10:18:58 UTC
> What do you think about putting the cronjob in /etc/cron.d/koha-common
> instead of /etc/cron.daily/koha-common?
> 
> The former could run at very configurable time whereas the latter runs at
> 6:25am out of the box. 
> 
> I could see Koha sysadmins customizing /etc/cron.d/koha-common much more
> readily than /etc/crontab (as you couldn't want to changethe time for all
> daily cronjobs).
> 
> I think that would be a very reasonable change to make to the current patch
> set?

A nightly job in the daily file makes sense to me - that's where I would be looking for it. And I'd rather schedule it at a standard time tha a new arbitrary one. And if you want another time, you could just move it?
Comment 35 Martin Renvoize 2020-05-18 11:02:02 UTC
I agree with Katrin here I think.. I would say it's clearer to have the default in the .daily file as it's intended to be run daily and for an admin to move it to .d if they are keen to specify an exact time.

Also, as an aside, it is my understanding that these two schemes work in different ways.. cron.d runs more like the original cron and just 'does a thing at time X' where as cron.daily (and friends) uses anacron in the background and as such 'does thing some time after X, with random delay Y and retries if the server is down'.
Comment 36 David Cook 2020-05-19 02:20:37 UTC
(In reply to Katrin Fischer from comment #34)
> A nightly job in the daily file makes sense to me - that's where I would be
> looking for it. And I'd rather schedule it at a standard time tha a new
> arbitrary one. And if you want another time, you could just move it?

That puts the onus on the system administrator for every out-of-the-box install to change it though from 6:30am to a different time.
Comment 37 David Cook 2020-05-19 02:26:29 UTC
(In reply to Martin Renvoize from comment #35)
> I agree with Katrin here I think.. I would say it's clearer to have the
> default in the .daily file as it's intended to be run daily and for an admin
> to move it to .d if they are keen to specify an exact time.
> 

I suppose so. I'm just thinking about out-of-the-box minimal installations. It's no drama for me to change a cronjob time, but I wonder a bit about many small scale sysadmins out there using Koha. 

> Also, as an aside, it is my understanding that these two schemes work in
> different ways.. cron.d runs more like the original cron and just 'does a
> thing at time X' where as cron.daily (and friends) uses anacron in the
> background and as such 'does thing some time after X, with random delay Y
> and retries if the server is down'.

anacron should never be installed on a server. It's really more suited to desktops and laptops. 

That said, /etc/crontab will use it if it's present and executable.

I'm looking at a customized Debian 9 Jessie and I see anacron installed and executable, and I'm looking at a Ubuntu 18.04 Bionic and I see that anacron is not installed. 

Sure enough, the Debian Jessie appears to be a Desktop image, while the Ubuntu is a headless server image. 

I think that actually helps make my case a bit for not using /etc/cron.daily heh.

But that's just my 2 cents. It's not a hill I want to die on by any means. Happy for it to go in /etc/cron.daily if everyone else does.
Comment 38 Tomás Cohen Arazi 2020-05-27 14:37:19 UTC
Created attachment 105400 [details] [review]
Bug 25245: Add plugins_nightly.pl cronjob script

This script simply iterates through installed plugins that impliment a
cronjob_nightly method and runs said method.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Tomás Cohen Arazi 2020-05-27 14:37:25 UTC
Created attachment 105401 [details] [review]
Bug 25245: (follow-up) Use Koha::Logger

Update script to use Koha::Logger to capture method failures.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 40 Tomás Cohen Arazi 2020-05-27 14:37:30 UTC
Created attachment 105402 [details] [review]
Bug 25245: Add plugins_nightly.pl to the default cron

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 41 Tomás Cohen Arazi 2020-05-27 14:42:59 UTC
I think this implementation is simple and covers the use case it is intended to. If we happen to have a task scheduler in a future, we could replace this and many other things we do with cron with it.

I don't think the Koha::Logger follow-up should be pushed along with this, as we don't have a 'cron' entry in log4perl.
Comment 42 David Cook 2020-05-27 23:23:53 UTC
(In reply to Tomás Cohen Arazi from comment #41)
> I think this implementation is simple and covers the use case it is intended
> to. If we happen to have a task scheduler in a future, we could replace this
> and many other things we do with cron with it.
> 

Agreed.

With one of my OAI-PMH plugins, I'm writing a custom task scheduler, but it's a complex piece of work, so I'd be happy to use the plugin nightly hook in the meantime (especially if it were backported to 19.11).
Comment 43 Jonathan Druart 2020-06-15 08:52:10 UTC
I am seeing only 1 signed-off-by lines per patches.
Comment 44 Martin Renvoize 2020-06-19 11:21:56 UTC
Created attachment 106067 [details] [review]
Bug 25245: Add plugins_nightly.pl cronjob script

This script simply iterates through installed plugins that impliment a
cronjob_nightly method and runs said method.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Martin Renvoize 2020-06-19 11:22:00 UTC
Created attachment 106068 [details] [review]
Bug 25245: (follow-up) Use Koha::Logger

Update script to use Koha::Logger to capture method failures.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Martin Renvoize 2020-06-19 11:22:04 UTC
Created attachment 106069 [details] [review]
Bug 25245: Add plugins_nightly.pl to the default cron

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 47 Martin Renvoize 2020-06-19 11:22:32 UTC
Restored SO lines.
Comment 48 Martin Renvoize 2020-07-02 12:42:15 UTC
*** Bug 20897 has been marked as a duplicate of this bug. ***