Description
Hugo Agud
2019-09-09 06:25:49 UTC
Created attachment 96055 [details] [review] Bug 23571: [DO NOT PUSH] crash condition builder This patch adds t/db_dependent/bug_23571_crash_condition_builder.t file This file is not intended to be part of Koha, but it's used to emulate crash conditions. It will create 5000 users, 5000 biblios with 2 items each, and each item issued to a user. It really takes a lot of time, so please be zen about it ;) To use it execute prove t/db_dependent/bug_23571_crash_condition_builder.t Sponsored-by: Orex Digital Created attachment 96056 [details] [review] Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital I have installed the patch , and from sudo koha-shell kohadev I have launched two terminal session but there is no error message at second terminal, perhaps I have done something wrong SESSION 1 kohadev-koha@kohadevbox:/tmp$ perl /home/vagrant/kohaclone/misc/cronjobs/fines.pl -v -l -o Use of uninitialized value $lockfile in concatenation (.) or string at /home/vagrant/kohaclone/misc/cronjobs/fines.pl line 93. WARNING: Could not create lock file : No such file or directory Verify if directories /var/lock/koha/koha_kohadev, /var/lock/koha_fines_koha_kohadev or /tmp/koha_fines_koha_kohadev exist and check file permissions writing to /var/lib/koha/kohadev/tmp/koha_kohadev_2020-01-07.log Use of uninitialized value in subtraction (-) at /home/vagrant/kohaclone/C4/Overdues.pm line 250. Use of uninitialized value in numeric eq (==) at /home/vagrant/kohaclone/C4/Overdues.pm line 266. Use of uninitialized value in subtraction (-) at /home/vagrant/kohaclone/C4/Overdues.pm line 250. SESSION 2 kohadev-koha@kohadevbox:/home/vagrant$ perl /home/vagrant/kohaclone/misc/cronjobs/fines.pl -v -l -o Use of uninitialized value $lockfile in concatenation (.) or string at /home/vagrant/kohaclone/misc/cronjobs/fines.pl line 93. WARNING: Could not create lock file : No such file or directory Verify if directories /var/lock/koha/koha_kohadev, /var/lock/koha_fines_koha_kohadev or /tmp/koha_fines_koha_kohadev exist and check file permissions writing to /var/lib/koha/kohadev/tmp/koha_kohadev_2020-01-07.log Use of uninitialized value in subtraction (-) at /home/vagrant/kohaclone/C4/Overdues.pm line 250. Use of uninitialized value in numeric eq (==) at /home/vagrant/kohaclone/C4/Overdues.pm line 266 Created attachment 98898 [details] [review] Bug 23571: [DO NOT PUSH] crash condition builder This patch adds t/db_dependent/bug_23571_crash_condition_builder.t file This file is not intended to be part of Koha, but it's used to emulate crash conditions. It will create 5000 users, 5000 biblios with 2 items each, and each item issued to a user. It really takes a lot of time, so please be zen about it ;) To use it execute prove t/db_dependent/bug_23571_crash_condition_builder.t Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 98899 [details] [review] Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Created attachment 99263 [details] [review] Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 99264 [details] [review] Bug 23571: Add flock to fines.pl This patch adds a lock file for fines.pl. To test: 1. Apply this patch 2. In two separate consoles run misc/cronjobs/fines.pl SUCCESS => The first one runs normally, but the second one fails with a message flock failed for the lock file. 3. Sign off. Sponsored-by: Orex Digital Signed-off-by: Hugo Agud <hagud@orex.es> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 99265 [details] [review] Bug 23571: (follow-up) Only look for overdues after obtaining the lock No point in performing the overdues fetch before obtaining the lock and knowing we can do something with them. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Comment on attachment 99264 [details] [review] Bug 23571: Add flock to fines.pl Review of attachment 99264 [details] [review]: ----------------------------------------------------------------- So, this generally looks pretty good to me.. However... I'd love to see the locking mechanisms factored out into the Koha::Script base class so we could share the common code between command-line scripts and make adding flocking elsewhere a simple pattern to follow. Would you be willing to have a go at that Agustin? ::: misc/cronjobs/fines.pl @@ +82,5 @@ > cronlogaction(); > > +my ($lockfile, $LockFH); > +foreach ( > + '/var/lock/koha', How did you come up with these locations? I note in the equivalent in rebuild_zebra.pl we use a config variable, followed by a fallback order... I actually like the idea of using /var/lock/koha but I'm not sure I understand where if came from? A couple of things: 1. test plan says "the second one fails", but it actually prints. I would die instead. 2. That will make you avoid the indentation level you add with this patch. The algo would be: - try to get a lock - if lock already exists then exit/die with "cannot get lock" (or something more meaningful: "previous run is still alive, cannot start") - continue the normal process of the script (In reply to Jonathan Druart from comment #10) > A couple of things: > 1. test plan says "the second one fails", but it actually prints. I would > die instead. s/would/should! Also, do you know that you can lock a portion of the file? I have no idea why your code is that complicated, I think you should investigate this code: use Modern::Perl; use Fcntl qw(:flock); sub is_locked { unless ( flock(DATA, LOCK_EX|LOCK_NB) ) { return 1; } return 0; } die "is locked" if is_locked(); say "is not locked! sleeping 5 sec"; sleep(5); __DATA__ USED BY LOCK - DO NOT REMOVE Failing QA. Agustin please investigate comment 12 and tell us if there is a possibility to simplify your code. Created attachment 102772 [details] [review] [ALTERNATIVE] Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Martin Renvoize from comment #9) > Comment on attachment 99264 [details] [review] [review] > Bug 23571: Add flock to fines.pl > > Review of attachment 99264 [details] [review] [review]: > ----------------------------------------------------------------- > > So, this generally looks pretty good to me.. However... I'd love to see the > locking mechanisms factored out into the Koha::Script base class so we could > share the common code between command-line scripts and make adding flocking > elsewhere a simple pattern to follow. > > Would you be willing to have a go at that Agustin? > > ::: misc/cronjobs/fines.pl > @@ +82,5 @@ > > cronlogaction(); > > > > +my ($lockfile, $LockFH); > > +foreach ( > > + '/var/lock/koha', > > How did you come up with these locations? > > I note in the equivalent in rebuild_zebra.pl we use a config variable, > followed by a fallback order... I actually like the idea of using > /var/lock/koha but I'm not sure I understand where if came from? Take a look at 25109, in which I introduce a centralized place to calculate this path. (In reply to Jonathan Druart from comment #10) > A couple of things: > 1. test plan says "the second one fails", but it actually prints. I would > die instead. > 2. That will make you avoid the indentation level you add with this patch. > The algo would be: > - try to get a lock > - if lock already exists then exit/die with "cannot get lock" (or something > more meaningful: "previous run is still alive, cannot start") > - continue the normal process of the script Solved in my alternative patch. I chose to keep the output silent, unless --verbose is passed. This is the right behaviour for cronjobs, specially if we have the execution logged in the action_logs. (In reply to Jonathan Druart from comment #12) > Also, do you know that you can lock a portion of the file? I loved the trick when I read about it while implementing 25109. But as we support multiple instances running the same codebase, this won't work. We need a 'namespace' (per instance) and the only way I found to do it was in a lock file. (In reply to Jonathan Druart from comment #13) > Failing QA. Agustin please investigate comment 12 and tell us if there is a > possibility to simplify your code. Done :-D Awesome guys, this is really great to see. [U+1F600]. I'll put it high up my list to start playing with these bugs with a view to SO/QA after the Easter break. Can't locate object method "new" via package "Koha::Scrip" (perhaps you forgot to load "Koha::Scrip"?) at misc/cronjobs/fines.pl line 80. typo. Created attachment 102804 [details] [review] Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) (In reply to Jonathan Druart from comment #21) > Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the > catch block) "pach", that's quite ironical :) Created attachment 102821 [details] [review] Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) Created attachment 102844 [details] [review] Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Why no "\n" in the print statement, is that expected? (In reply to Jonathan Druart from comment #25) > Why no "\n" in the print statement, is that expected? I think I only missed it. Created attachment 102888 [details] [review] Bug 23571: (QA follow-up) Add missing newline in warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 102910 [details] [review] Bug 23571: Prevent concurrent execution of fines.pl This patch introduces locking in fines.pl. It does so by leveraging on bug 25109, which introduces a generic locking implementation on Koha::Script. The introduced changes: 1. Try to get the lock 2.a. If success, normal execution happens 2.b. If rejected, cronlogaction is called with a meaningful message and a normal exit happens, so we don't flood the logs in vain. --verbose will make the script print the same message on STDERR, as already is the case with this script To test: 1. Apply this patch 2. In two separate consoles run: $ kshell k$ perl misc/cronjobs/fines.pl --verbose SUCCESS => The first one runs normally, the second one exists really fast and prints a message about the lock. 3. Sign off :-D Sponsored-by: Orex Digital Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Amended-pach JD: Fix syntax issue (typo Koha::Scrip and ';' after the catch block) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102911 [details] [review] Bug 23571: (QA follow-up) Add missing newline in warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 102912 [details] [review] bug 23571: (QA follow-up) Remove perlcritic warning Bareword file handle opened at line 138, column 5. See pages 202,204 of PBP. (Severity: 5) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Nice work everyone! Pushed to master for 20.05 does not apply cleanly (Koha/Script.pm) to 19.11.x branch. please rebase if needed. > Solved in my alternative patch. I chose to keep the output silent, unless
> --verbose is passed. This is the right behaviour for cronjobs, specially if
> we have the execution logged in the action_logs.
I'd like to understand how it is "the right behavior" for a process to not output a message when a fatal error occurs ? I've seen the option -q added for quietness, but never to require -v to print fatal errors.
We ran the script manually and got nothing telling us it failed. The action logs is NOT a valid alternative for meaningful (and efficient) error messages.
(In reply to Blou from comment #33) > > Solved in my alternative patch. I chose to keep the output silent, unless > > --verbose is passed. This is the right behaviour for cronjobs, specially if > > we have the execution logged in the action_logs. > > I'd like to understand how it is "the right behavior" for a process to not > output a message when a fatal error occurs ? I've seen the option -q added > for quietness, but never to require -v to print fatal errors. > > We ran the script manually and got nothing telling us it failed. The action > logs is NOT a valid alternative for meaningful (and efficient) error > messages. As the script is (by default) only called on a daily basis, it may be relevant to send to STDERR the lock error (even without --verbose). Tomas, what do you think? |