Bug 25109 - Add execution locking to Koha::Script
Summary: Add execution locking to Koha::Script
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 26645 28485 23571
  Show dependency treegraph
 
Reported: 2020-04-10 19:00 UTC by Tomás Cohen Arazi
Modified: 2022-06-06 20:24 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 25109: Add lockdir configuration entry (5.07 KB, patch)
2020-04-10 22:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add entry in about.pl about the configuration lockdir entry (1.82 KB, patch)
2020-04-10 22:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add ->new and ->lock_exec to Koha::Script (3.64 KB, patch)
2020-04-10 22:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add lockdir configuration entry (5.06 KB, patch)
2020-04-10 22:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add entry in about.pl about the configuration lockdir entry (1.82 KB, patch)
2020-04-10 22:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add ->new and ->lock_exec to Koha::Script (3.64 KB, patch)
2020-04-10 22:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add lockdir configuration entry (5.06 KB, patch)
2020-04-13 11:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add entry in about.pl about the configuration lockdir entry (1.82 KB, patch)
2020-04-13 11:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add ->new and ->lock_exec to Koha::Script (4.88 KB, patch)
2020-04-13 11:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add 'wait' tests (1.96 KB, patch)
2020-04-13 11:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add lockdir configuration entry (5.13 KB, patch)
2020-04-13 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25109: Add entry in about.pl about the configuration lockdir entry (1.89 KB, patch)
2020-04-13 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25109: Add ->new and ->lock_exec to Koha::Script (4.95 KB, patch)
2020-04-13 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25109: Add 'wait' tests (2.03 KB, patch)
2020-04-13 13:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25109: (QA follow-up) Fix typo in configuration entry name (857 bytes, patch)
2020-04-13 15:03 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: (QA follow-up) Add a test for lockdir writable in about.pl (1.95 KB, patch)
2020-04-13 18:48 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25109: Add lockdir configuration entry (5.19 KB, patch)
2020-04-14 08:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25109: Add entry in about.pl about the configuration lockdir entry (1.95 KB, patch)
2020-04-14 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25109: Add ->new and ->lock_exec to Koha::Script (5.00 KB, patch)
2020-04-14 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25109: Add 'wait' tests (2.09 KB, patch)
2020-04-14 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25109: (QA follow-up) Fix typo in configuration entry name (931 bytes, patch)
2020-04-14 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25109: (QA follow-up) Add a test for lockdir writable in about.pl (2.02 KB, patch)
2020-04-14 08:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-04-10 19:00:54 UTC

    
Comment 1 Tomás Cohen Arazi 2020-04-10 22:40:42 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2020-04-10 22:40:49 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2020-04-10 22:40:59 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2020-04-10 22:46:50 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2020-04-10 22:46:57 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2020-04-10 22:47:12 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2020-04-13 09:49:37 UTC
1.
  <zebra_lockdir>/var/lock/koha/__KOHASITE__</zebra_lockdir>
+ <lockdir>/var/lock/koha/__KOHASITE__</lockdir>

So should we remove zebra_lockdir (and replace it with $lockdir/zebra)?

2.
+        "The 'script' parameter is mandatory. You should usually pass \$0"

$0? Should be "script", right?
Comment 8 Tomás Cohen Arazi 2020-04-13 10:29:45 UTC
(In reply to Jonathan Druart from comment #7)
> 1.
>   <zebra_lockdir>/var/lock/koha/__KOHASITE__</zebra_lockdir>
> + <lockdir>/var/lock/koha/__KOHASITE__</lockdir>
> 
> So should we remove zebra_lockdir (and replace it with $lockdir/zebra)?

Yes, but on a separate bug. If we agree on this, I can work on getting rid of zebra_lockdir of course.

> 2.
> +        "The 'script' parameter is mandatory. You should usually pass \$0"
> 
> $0? Should be "script", right?

I tried to say that the 'script' parameter should generally be passed $0. This comes from my try to write the tests for this, in which Script.t was always picked if I used $0.
Comment 9 Tomás Cohen Arazi 2020-04-13 11:48:06 UTC
Taking care of some QA concerns.
Comment 10 Tomás Cohen Arazi 2020-04-13 11:49:14 UTC
Created attachment 102814 [details] [review]
Bug 25109: Add lockdir configuration entry
Comment 11 Tomás Cohen Arazi 2020-04-13 11:49:23 UTC
Created attachment 102815 [details] [review]
Bug 25109: Add entry in about.pl about the configuration lockdir entry
Comment 12 Tomás Cohen Arazi 2020-04-13 11:49:29 UTC
Created attachment 102816 [details] [review]
Bug 25109: Add ->new and ->lock_exec to Koha::Script
Comment 13 Tomás Cohen Arazi 2020-04-13 11:49:34 UTC
Created attachment 102817 [details] [review]
Bug 25109: Add 'wait' tests

This simple patch introduces a test for the lock waiting scenario. It
replicates the previous tests, but calls a script that passes the wait
=> 1 parameter to ->lock_exec.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2020-04-13 11:50:59 UTC
Thanks for the feedback, Jonathan!
Comment 15 Kyle M Hall 2020-04-13 13:46:11 UTC
Created attachment 102840 [details] [review]
Bug 25109: Add lockdir configuration entry

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Kyle M Hall 2020-04-13 13:46:20 UTC
Created attachment 102841 [details] [review]
Bug 25109: Add entry in about.pl about the configuration lockdir entry

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Kyle M Hall 2020-04-13 13:46:23 UTC
Created attachment 102842 [details] [review]
Bug 25109: Add ->new and ->lock_exec to Koha::Script

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall 2020-04-13 13:46:25 UTC
Created attachment 102843 [details] [review]
Bug 25109: Add 'wait' tests

This simple patch introduces a test for the lock waiting scenario. It
replicates the previous tests, but calls a script that passes the wait
=> 1 parameter to ->lock_exec.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Jonathan Druart 2020-04-13 14:15:28 UTC
lockdir vs lock_dir, please fix.

Once fixed, I guess we will have a problem if the dir is not writable, but we need a -w test for the about warning?
Comment 20 Tomás Cohen Arazi 2020-04-13 14:58:21 UTC
(In reply to Jonathan Druart from comment #19)
> lockdir vs lock_dir, please fix.
> 
> Once fixed, I guess we will have a problem if the dir is not writable, but
> we need a -w test for the about warning?

Oops. If it is not writeable it will raise a relevant exception and report back.
Comment 21 Tomás Cohen Arazi 2020-04-13 15:03:45 UTC
Created attachment 102856 [details] [review]
Bug 25109: (QA follow-up) Fix typo in configuration entry name

This patch fixes an error in the configuration retrieving line.
Comment 22 Tomás Cohen Arazi 2020-04-13 18:48:01 UTC
Created attachment 102887 [details] [review]
Bug 25109: (QA follow-up) Add a test for lockdir writable in about.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Jonathan Druart 2020-04-14 08:30:49 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> (In reply to Jonathan Druart from comment #7)
> > 1.
> >   <zebra_lockdir>/var/lock/koha/__KOHASITE__</zebra_lockdir>
> > + <lockdir>/var/lock/koha/__KOHASITE__</lockdir>
> > 
> > So should we remove zebra_lockdir (and replace it with $lockdir/zebra)?
> 
> Yes, but on a separate bug. If we agree on this, I can work on getting rid
> of zebra_lockdir of course.

I opened bug 25141.
Comment 24 Jonathan Druart 2020-04-14 08:35:58 UTC
Created attachment 102904 [details] [review]
Bug 25109: Add lockdir configuration entry

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2020-04-14 08:36:01 UTC
Created attachment 102905 [details] [review]
Bug 25109: Add entry in about.pl about the configuration lockdir entry

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 26 Jonathan Druart 2020-04-14 08:36:04 UTC
Created attachment 102906 [details] [review]
Bug 25109: Add ->new and ->lock_exec to Koha::Script

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 27 Jonathan Druart 2020-04-14 08:36:07 UTC
Created attachment 102907 [details] [review]
Bug 25109: Add 'wait' tests

This simple patch introduces a test for the lock waiting scenario. It
replicates the previous tests, but calls a script that passes the wait
=> 1 parameter to ->lock_exec.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2020-04-14 08:36:10 UTC
Created attachment 102908 [details] [review]
Bug 25109: (QA follow-up) Fix typo in configuration entry name

This patch fixes an error in the configuration retrieving line.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 29 Jonathan Druart 2020-04-14 08:36:14 UTC
Created attachment 102909 [details] [review]
Bug 25109: (QA follow-up) Add a test for lockdir writable in about.pl

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 30 Martin Renvoize 2020-04-14 16:23:52 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 31 Joy Nelson 2020-05-05 21:28:46 UTC
enhancement not backported to 19.11.x
Comment 32 Jonathan Druart 2021-02-05 14:52:25 UTC
(In reply to Jonathan Druart from comment #23)
> (In reply to Tomás Cohen Arazi from comment #8)
> > Yes, but on a separate bug. If we agree on this, I can work on getting rid
> > of zebra_lockdir of course.
> 
> I opened bug 25141.

Still waiting for a patch there.
Comment 33 Jonathan Druart 2021-02-05 14:54:16 UTC
Once again we have the proof that using sleep in tests is VERY bad.

+    sleep 1; # Make sure we start after the fork

Tests are failing randomly on Jenkins, quite often (at least twice a week).

Example for Koha_Master_D10 (#467)

15:01:46 koha_1       |     #   Failed test 'Exception found'
15:01:46 koha_1       |     #   at t/Koha/Script.t line 69.
15:01:46 koha_1       |     #                   ''
15:01:46 koha_1       |     #     doesn't match '(?^u:Unable to acquire the lock.*)'
15:01:46 koha_1       |     # Looks like you failed 1 test of 3.
15:01:46 koha_1       | 
15:01:46 koha_1       | #   Failed test 'lock_exec() tests'
15:01:46 koha_1       | #   at t/Koha/Script.t line 87.
15:01:46 koha_1       | # Looks like you failed 1 test of 4.