Bug 11078 - rebuild_zebra.pl can lose updates due to race condition during full rebuilds
Summary: rebuild_zebra.pl can lose updates due to race condition during full rebuilds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P4 enhancement (vote)
Assignee: Doug Kingston
QA Contact: Marcel de Rooy
URL:
Keywords:
: 7151 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-10-18 05:10 UTC by Doug Kingston
Modified: 2015-06-04 23:30 UTC (History)
12 users (show)

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


Attachments
Add locking to rebuild_zebra to prevent races (3.99 KB, patch)
2013-10-18 05:13 UTC, Doug Kingston
Details | Diff | Splinter Review
flock testing program (519 bytes, text/x-perl-script)
2013-10-18 05:14 UTC, Doug Kingston
Details
Bug 6345 Add locking to rebuild_zebra (4.13 KB, patch)
2013-10-21 22:17 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra to prevent race conditions (revised 10/27) (5.18 KB, patch)
2013-10-28 02:27 UTC, Doug Kingston
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra (QA patch 10/28) (1005 bytes, patch)
2013-10-29 02:25 UTC, Doug Kingston
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra (QA Patch 2 10/28) (1.30 KB, patch)
2013-10-29 06:35 UTC, Doug Kingston
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra (QA Patch 3 10/29) (1.18 KB, patch)
2013-10-30 04:23 UTC, Doug Kingston
Details | Diff | Splinter Review
Address coding style issues from koha-qa.pl (1.77 KB, patch)
2013-11-17 20:09 UTC, Doug Kingston
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra (QA patch 5) (2.00 KB, patch)
2013-11-18 05:40 UTC, Doug Kingston
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra (rollup 11/28) (6.66 KB, patch)
2013-11-29 07:24 UTC, Doug Kingston
Details | Diff | Splinter Review
Minor fix to path used by rebuild_zebra for locking to match docs. (1.06 KB, patch)
2013-11-29 08:09 UTC, Doug Kingston
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra.pl (6.73 KB, patch)
2013-12-16 12:05 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra.pl (path fix) (1.12 KB, patch)
2013-12-16 12:06 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 11078 Add locking to rebuild_zebra.pl (6.98 KB, patch)
2014-02-07 09:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11078: Follow-up for flock eval (2.50 KB, patch)
2014-02-10 14:08 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11078 Address February QA concerns (5.96 KB, patch)
2014-02-17 02:44 UTC, Doug Kingston
Details | Diff | Splinter Review
Add locking to rebuild_zebra (revised patch 2014-02-18) (11.15 KB, patch)
2014-02-19 07:10 UTC, Doug Kingston
Details | Diff | Splinter Review
Bug 11078: Add locking to rebuild_zebra (revised patch 2014-02-18) (11.45 KB, patch)
2014-02-19 14:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 11078: QA Follow-up for missing file permissions on lockfile (5.11 KB, patch)
2014-02-19 14:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Doug Kingston 2013-10-18 05:10:23 UTC
The race condition exists whether you are doing incremental updates with
a periodic cronjob or with the new daemon mode.  Suppose you start a full rebuild
at time T0 which will take until T20 to extract the records.  Suppose also at T10,
a biblio or auth is updated and processed through the zebraqueue by T15.
In this situation the updated record in zebra will be overwritten when
the full rebuild records are uploaded to zebra after T20.  We prevent this
by only allowing one rebuild_zebra per koha instance to be running at one time.

This patch adds flock based locking for rebuild_zebra.pl on a per-instance basis. This prevents races between full rebuilds and background incremental updates from the zebraqueue table in the database.

When running in daemon mode (see bug 6435), incremental updates will be skipped while a full rebuild is running, and resume afterwards.  A full rebuild or other
adhoc request will wait for any previous lock to clear.

Tested by flocking the lock file while invoking rebuild_zebra.pl in
various modes (daemon, adhoc zebraqueue task, and full rebuild) using
flock program I will attach to bug.
Comment 1 Doug Kingston 2013-10-18 05:13:27 UTC Comment hidden (obsolete)
Comment 2 Doug Kingston 2013-10-18 05:14:51 UTC
Created attachment 22045 [details]
flock testing program

Small perl program to flock files and run commands

This program can be used to test the flock functionality introduced in patch 0003 also attached to this bug.

Test I performed that QA can replicate (modify for your test instance name):

flock the file, and make sure a full rebuild waits for the lock to clear before proceeding
  window 1: flock /var/lock/koha_rebuild_zebra_koha_test/lock sleep 15
  window 2: ./rebuild_zebra.pl -b -a -v
window 2 should wait for window 1's sleep to time out before proceeding.

test that an flock from another process with cause the daemon mode to pause updates and resume when the flock is removed
  window 1: ./rebuild_zebra.pl -daemon -sleep 2 -v -z -b -a
  window 2: flock /var/lock/koha_rebuild_zebra_koha_test/lock sleep 15
start window 2 after window 1 is printing a message every 2 seconds.  when you launch the sleep, it will stop the updates which will resume when the sleep times out.

remove /var/log/koha_rebuild_zebra_koha_test and make sure its recreated on the next invocation of rebuild_zebra.pl.
Comment 3 Martin Renvoize 2013-10-21 22:17:05 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize 2013-10-21 22:21:11 UTC
Works as expected to me..

Set rebuild_zebra running, and then tried to run a fruther rebuild only moments later..  The lock was found to be present and so the rebuild did not run.

It's a simple patch, but a useful one.
Comment 5 Kyle M Hall 2013-10-21 22:59:43 UTC
I believe this patch needs to respect the configuration directory setting for installations. For example, if I'm installing from git, I would want the lockfile to be in /home/koha/koha-dev/var/lock/koha_rebuild_zebra rather than /var/lock/koha_rebuild_zebra.
Comment 6 Doug Kingston 2013-10-21 23:50:13 UTC
How can I get at this config parameter at runtime, or is there a install time rewrite template I can use to ensure the directory specified is respecting the "configuration directory setting"?
Comment 7 Doug Kingston 2013-10-22 00:41:24 UTC
So there appears to be no support for this configuration right now.
Implementing probably means:
Update Makefile.export to support __RUN_DIR__
Update rewrite-config.PL to support __RUN_DIR__
Update rebuild-zebra.pl to use __RUN_DIR__ instead of /var/run

I don't think we need this step since we won't chown this directory:
Update Makefile to support KOHA_DEST_RUN_DIR
Comment 8 Doug Kingston 2013-10-28 02:27:27 UTC Comment hidden (obsolete)
Comment 9 Robin Sheat 2013-10-28 22:16:09 UTC
Oh good, I've been meaning to do this for a while.

A few comments:
* it creates /var/lock/koha_rebuild_zebra_databasename/lock, why not /var/lock/koha/rebuild_zebra_databasename? That way if we add more locking stuff, it will all be under the 'koha' directory.
* it doesn't look like it handles lockdir not being set in the config very nicely, it should probably default to /var/lock if the config setting isn't available, otherwise it seems it would try to create a directory under '/'.
* it doesn't set up the lockdir config for package installations, this would be a very simple matter of adding <lockdir>/var/lock</lockdir> in debian/templates/koha-conf-site.xml.in. It's OK to hard-code the value in there because it'll always be the same.
Comment 10 Doug Kingston 2013-10-29 02:19:23 UTC
(In reply to Robin Sheat from comment #9)
> Oh good, I've been meaning to do this for a while.
> 
> A few comments:
> * it creates /var/lock/koha_rebuild_zebra_databasename/lock, why not
> /var/lock/koha/rebuild_zebra_databasename? That way if we add more locking
> stuff, it will all be under the 'koha' directory.
The existing koha scripts that create koha instances call
  install -d -o $kohauser -g $kohauser /var/lock/koha/$kohasite
When invoked this way, only the last directory in this path is owned
by the user and the intervening directories are owned by root.  rebuild_zebra.pl
is run as $kohauser so it cannot create another directory under /var/lock/koha.

> * it doesn't look like it handles lockdir not being set in the config very
> nicely, it should probably default to /var/lock if the config setting isn't
> available, otherwise it seems it would try to create a directory under '/'.
My changes make sure this is set for proper installs (see the change to
rewrite-config.pl, but I am happy to put in a fallback if QA team wants it.
This level of protection is not present for other config variables.
Lots of other things will go pear-shaped if their definition is missing.

> * it doesn't set up the lockdir config for package installations, this would
> be a very simple matter of adding <lockdir>/var/lock</lockdir> in
> debian/templates/koha-conf-site.xml.in. It's OK to hard-code the value in
> there because it'll always be the same.
Thanks for the pointer, I did not know about that template.  This is just what code reviews are for.
Comment 11 Doug Kingston 2013-10-29 02:25:26 UTC Comment hidden (obsolete)
Comment 12 Robin Sheat 2013-10-29 02:37:07 UTC
(In reply to Doug Kingston from comment #10)
> The existing koha scripts that create koha instances call
>   install -d -o $kohauser -g $kohauser /var/lock/koha/$kohasite
> When invoked this way, only the last directory in this path is owned
> by the user and the intervening directories are owned by root. 
> rebuild_zebra.pl
> is run as $kohauser so it cannot create another directory under
> /var/lock/koha.

Yeah, I just checked and found that. In that case, it might be best to use that pattern? If it's only applicable to a package installation, it is something we could put in just for that case without too much difficulty.

> My changes make sure this is set for proper installs (see the change to
> rewrite-config.pl, but I am happy to put in a fallback if QA team wants it.
> This level of protection is not present for other config variables.
> Lots of other things will go pear-shaped if their definition is missing.

Your changes only apply to _new_ installs. Anyone with a pre-existing installation who doesn't update their koha-conf.xml will have problems. This will be most people.

> Thanks for the pointer, I did not know about that template.  This is just
> what code reviews are for.

No problem :)
Comment 13 Doug Kingston 2013-10-29 06:16:03 UTC
(In reply to Robin Sheat from comment #12)
> (In reply to Doug Kingston from comment #10)
> > The existing koha scripts that create koha instances call
> >   install -d -o $kohauser -g $kohauser /var/lock/koha/$kohasite
> > When invoked this way, only the last directory in this path is owned
> > by the user and the intervening directories are owned by root. 
> > rebuild_zebra.pl
> > is run as $kohauser so it cannot create another directory under
> > /var/lock/koha.
> 
> Yeah, I just checked and found that. In that case, it might be best to use
> that pattern? If it's only applicable to a package installation, it is
> something we could put in just for that case without too much difficulty.
I don't think we can use that pattern because it relies on running as root.  I think the best solution here is to have each instance create its own directory under /var/lock which is guaranteed to be in mode 1777 (with the t bit set).
Since /var/lock is often on tmpfs its gone after a reboot and we can't rely in
a subdirectory maintained.  I think this is now in the safest, most maintable state.

> 
> > My changes make sure this is set for proper installs (see the change to
> > rewrite-config.pl, but I am happy to put in a fallback if QA team wants it.
> > This level of protection is not present for other config variables.
> > Lots of other things will go pear-shaped if their definition is missing.
> 
> Your changes only apply to _new_ installs. Anyone with a pre-existing
> installation who doesn't update their koha-conf.xml will have problems. This
> will be most people.
Fair enough, I'll update it to use /var/lock if the lockdir variable is not defined.

> 
> > Thanks for the pointer, I did not know about that template.  This is just
> > what code reviews are for.
> 
> No problem :)
Comment 14 Doug Kingston 2013-10-29 06:35:15 UTC Comment hidden (obsolete)
Comment 15 Robin Sheat 2013-10-30 01:13:44 UTC
(In reply to Doug Kingston from comment #13)
> I don't think we can use that pattern because it relies on running as root. 

Well, I'm trying to figure a way that'll work with the scheme that the packages currently use. For them we can expect /var/lock/koha/instancename to exist and have the correct user permissions, as it's already used for locking zebrasrv. So we could have a rebuild-zebra.lck (or whatever) lock file under there.

> Since /var/lock is often on tmpfs its gone after a reboot and we can't rely
> in
> a subdirectory maintained.  I think this is now in the safest, most
> maintable state.

We can rely on it for packages as it's created at boot, if needed (and is already being used.)

> Fair enough, I'll update it to use /var/lock if the lockdir variable is not
> defined.

Cool.
Comment 16 Doug Kingston 2013-10-30 03:27:04 UTC
(In reply to Robin Sheat from comment #15)
> (In reply to Doug Kingston from comment #13)
> > I don't think we can use that pattern because it relies on running as root. 
> 
> Well, I'm trying to figure a way that'll work with the scheme that the
> packages currently use. For them we can expect /var/lock/koha/instancename
> to exist and have the correct user permissions, as it's already used for
> locking zebrasrv. So we could have a rebuild-zebra.lck (or whatever) lock
> file under there.
My code works with the system as implemented (packages or no packages).
There is not a easy way to get the instance name (unless I am mistaken) right
now without introducing yet more variables and possible problems.  Is there a pressing reason for the lock file to live under /var/lock/koha/instancename
that warrants the additional complexity?  And whatever we come up with needs
to work for new and legacy installs.  Ideas that keep get us closer without
undue complexity are welcome.  Otherwise, lets get this in and iterate.

> 
> > Since /var/lock is often on tmpfs its gone after a reboot and we can't rely
> > in
> > a subdirectory maintained.  I think this is now in the safest, most
> > maintable state.
> 
> We can rely on it for packages as it's created at boot, if needed (and is
> already being used.)
> 
> > Fair enough, I'll update it to use /var/lock if the lockdir variable is not
> > defined.
> 
> Cool.
Comment 17 Robin Sheat 2013-10-30 03:39:00 UTC
(In reply to Doug Kingston from comment #16)
> My code works with the system as implemented (packages or no packages).

I know, I'm trying to get it to a point where it also works cleanly with the packages.

> There is not a easy way to get the instance name (unless I am mistaken) right
> now without introducing yet more variables and possible problems.  

There is for packages, we can put __KOHASITE__ in the koha-conf.xml template. It's not super tidy, as we'll then be creating an unnecessary directory under that, but that's not the end of the world by any means.

> Is there
> a pressing reason for the lock file to live under /var/lock/koha/instancename
> that warrants the additional complexity?  

I'm not sure if there are any specific debian policies on what goes in the lock dir, but as we progress towards making things debian-clean, we ought to avoid doing things in an obviously inconsistent fashion.

> And whatever we come up with needs
> to work for new and legacy installs.  Ideas that keep get us closer without
> undue complexity are welcome.  Otherwise, lets get this in and iterate.

I'd say for now to modify the template patch to be:
/var/lock/koha/__KOHASITE__
and it'll be clean enough. Not quite as much as I'd like, but it'll do, and it's hidden away. And we know this is safe to do as they're configured in the init.d script.

It'll also work on tar, git, etc. systems as they'll use plain /var/lock.
Comment 18 Doug Kingston 2013-10-30 04:23:44 UTC Comment hidden (obsolete)
Comment 19 Robin Sheat 2013-10-31 22:51:26 UTC
I attempted to have a go testing this, however the patches don't apply on current master:

Applying: Bug 11078 Add locking to rebuild_zebra to prevent race conditions.
/home/robin/catalyst/koha/.git/rebase-apply/patch:67: tab in indent.
	if (flock(LockFH, LOCK_EX|LOCK_NB)) {
/home/robin/catalyst/koha/.git/rebase-apply/patch:68: tab in indent.
	    do_one_pass() if ( zebraqueue_not_empty() );
/home/robin/catalyst/koha/.git/rebase-apply/patch:69: tab in indent.
	    flock(LockFH, LOCK_UN);
/home/robin/catalyst/koha/.git/rebase-apply/patch:70: tab in indent.
	}
fatal: sha1 information is lacking or useless (misc/migration_tools/rebuild_zebra.pl).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 11078 Add locking to rebuild_zebra to prevent race conditions.
The copy of the patch that failed is found in:
   /home/robin/catalyst/koha/.git/rebase-apply/patch

This usually happens if they're on top of a local patch or something that isn't in master. It probably just needs a quick rebasing.
Comment 20 Doug Kingston 2013-11-10 21:35:36 UTC
As noted in chat, these patches are built on those for bug 6435 on which this bug depends.  Can you test these 2 together?
Comment 21 Martin Renvoize 2013-11-12 09:17:06 UTC
Patch applies as specified over bug 6435.

This all seems to still work as expected to me; however I'm not best placed for judging whether all Robins packages cases have yet been met.. Could you take another look now it applies again Robin.. remember it needs bug 6435 applying first?
Comment 22 Robin Sheat 2013-11-12 11:08:33 UTC
I don't know when I'll get the chance to test it, but from reading the code I'm pretty happy with how it looks and I'd rather get it into master sooner rather than later because I've been wanting file locking for approximately ever. So I'd say don't wait for my approval. I can always quickly push through a patch for the package-related stuff if we discover a problem.
Comment 23 Martin Renvoize 2013-11-12 11:15:36 UTC
Awesome, thanks for looking over the code Robin.. I'll do one last test (with packages as I'm currently learning how to build them and this'll be a good exercise).  

I'll check the various files end up where you've expressed they should and then sign off.  Hopefully someone will QA it promptly after that.. I've also been wanting locking in mainstream forever.. Nice work Doug!

Martin
Comment 24 Katrin Fischer 2013-11-16 20:16:19 UTC
Waiting for a sign off here ... :)
Comment 25 M. Tompsett 2013-11-17 15:41:56 UTC
mtompset@ubuntu:~/kohaclone$ git reset --hard origin/master
HEAD is now at 73d4ac9 Merge branch 'new/bug11163'
mtompset@ubuntu:~/kohaclone$ git bz apply 6435
Bug 6435 - [ENH] Added daemon mode parameters to rebuild_zebra.pl

22859 - Bug 6435 Add daemon mode to rebuild_zebra.pl

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 6435 Add daemon mode to rebuild_zebra.pl
mtompset@ubuntu:~/kohaclone$ git bz apply 11078
Bug 11078 - rebuild_zebra.pl can lose updates due to race condition during full rebuilds

22458 - Bug 11078 Add locking to rebuild_zebra to prevent race  conditions (revised 10/27)
22506 - Bug 11078 Add locking to rebuild_zebra (QA patch 10/28)
22508 - Bug 11078 Add locking to rebuild_zebra (QA Patch 2 10/28)
22532 - Bug 11078 Add locking to rebuild_zebra (QA Patch 3 10/29)

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 11078 Add locking to rebuild_zebra to prevent race conditions.
Applying: Bug 11078 rebuild-zebra can lose updates, QA patch 1
Applying: Bug 11078 Add locking to rebuild_zebra (QA Patch 2 10/28)
Applying: Bug 11078 Add locking to rebuild_zebra (QA Patch 3 10/29)
mtompset@ubuntu:~/kohaclone$ ~/qa-test-tools/koha-qa.pl -v 2 -c 4
testing 4 commit(s) (applied to d8a974a 'Bug 6435 Add daemon mode to rebuild_z')

 FAIL   misc/migration_tools/rebuild_zebra.pl
   OK     pod
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 182)
                forbidden pattern: tab char (line 181)
                forbidden pattern: tab char (line 183)
                forbidden pattern: tab char (line 180)
   OK     valid
   FAIL   critic
                # InputOutput::ProhibitBarewordFileHandles: Got 1 violation(s).
                # InputOutput::ProhibitTwoArgOpen: Got 1 violation(s).
                # ValuesAndExpressions::ProhibitLeadingZeros: Got 1 violation(s).

 OK     rewrite-config.PL
   OK     pod
   OK     forbidden patterns
   OK     valid
   OK     critic

 OK     etc/koha-conf.xml
   OK     xml_valid

Doug, could you clean up the problems?
Perl::Critic::Policy::InputOutput::ProhibitTwoArgOpen - Write open $fh, q{<}, $filename; instead of open $fh, "<$filename";.
Perl::Critic::Policy::InputOutput::ProhibitBarewordFileHandles - Write open my $fh, q{<}, $filename; instead of open FH, q{<}, $filename;
Perl::Critic::Policy::ValuesAndExpressions::ProhibitLeadingZeros - Write oct(755) instead of 0755.

I'm wondering if this a tightening up of the perlcritic stuff?
$ perlcritic --version
1.117
Comment 26 M. Tompsett 2013-11-17 17:21:12 UTC
mtompset@ubuntu:/var/lock$ ls -la
total 0
drwxrwxrwt  4 root     root      80 Nov 17 12:04 .
drwxr-xr-x 18 root     root     680 Nov 17 10:30 ..
drwxr-xr-x  2 www-data root      40 Nov 17 10:20 apache2
drwxr-xr-x  2 whoopsie whoopsie  60 Nov 17 10:20 whoopsie

My /var/lock directory has a nice sticky bit set: drwxrwxrwT (the t on the end)
This means when I ran this, and the directory created was owned by mtompset. I don't see why we can't get more consistency in terms of the lock file.
Comment 27 Doug Kingston 2013-11-17 20:09:27 UTC Comment hidden (obsolete)
Comment 28 Doug Kingston 2013-11-18 05:40:54 UTC Comment hidden (obsolete)
Comment 29 M. Tompsett 2013-11-20 05:48:07 UTC
I just applied 6435, 11078, and then moved my ~/koha-dev to ~/koha-dev_pretest.
I then I did the standard:
    perl Makefile.PL
    make
    make test
    make install
And then proceeded to look at ~/koha-dev/etc/koha-conf.xml for the lockdir value: /var/lock?!

I did a git rebase -i and squashed them all together to see which files were modified in a git commit --amend. You didn't touch Makefile.PL?
Comment 30 Doug Kingston 2013-11-20 07:35:23 UTC
(In reply to M. Tompsett from comment #29)
> I just applied 6435, 11078, and then moved my ~/koha-dev to
> ~/koha-dev_pretest.
> I then I did the standard:
>     perl Makefile.PL
>     make
>     make test
>     make install
> And then proceeded to look at ~/koha-dev/etc/koha-conf.xml for the lockdir
> value: /var/lock?!
> 
> I did a git rebase -i and squashed them all together to see which files were
> modified in a git commit --amend. You didn't touch Makefile.PL?

What did you expect it to be?

The more I look at the use of INSTALL_BASE, __INSTALL_BASE__, the $prefix in rewrite_config.PL, the more scared I get.  The fact that $prefix defaults to /usr looks wrong since that creates /usr/var/{lock,log} which is not conforming with POSIX.  Is this line not getting substituted:
  "__LOCK_DIR__" => "$ENV{'INSTALL_BASE'}/var/lock",
Comment 31 Doug Kingston 2013-11-26 01:08:05 UTC
In discussions with Galen over the weekend, we agreed that we should treat the zebra_rebuild locking exactly how we are currently handling ZEBRA_LOCK_DIR, so there will be one more pass at this to do so.  Probably sort this on Tuesday evening.
Comment 32 Martin Renvoize 2013-11-26 09:17:24 UTC
It's great to see this bug making such progress.. just wanted to add my thanks to Doug, your doing a Stirling job keeping on top of it.

Martin
Comment 33 Doug Kingston 2013-11-29 07:24:59 UTC Comment hidden (obsolete)
Comment 34 Doug Kingston 2013-11-29 08:09:55 UTC Comment hidden (obsolete)
Comment 35 Katrin Fischer 2013-12-03 11:35:52 UTC
Doug, is this ready for sign off/QA with your follow ups? You can switch the status :)
Comment 36 Doug Kingston 2013-12-05 07:02:56 UTC
These two patches should do the trick.  Ready for re-review and QA.
Comment 37 Martin Renvoize 2013-12-16 12:05:10 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize 2013-12-16 12:06:48 UTC Comment hidden (obsolete)
Comment 39 PTFS Europe Sandboxes 2013-12-16 12:11:27 UTC
Setting Singed Off:

* Reviewed code and it looks good, also ran QA scripts and it passed all tests.
* Ran through full install procedure for dev install - files files placed as epxected.
* Ran through full install procedure for standard install - again files were placed as expected.

Tested locking with deamon mode + manual rebuild - Worked as expected.
Tested locking with two manual rebuilds - Wored as expected.
Tested locking with croned rebuild + manual rebuild - Worked as expected.

I've not build a package using these files as yet, but the code looks as though it should work as expected too.

Signing off.. good luck Mr/Mrs QA ;)
Comment 40 Martin Renvoize 2013-12-16 12:13:00 UTC
Above comment was by me.. I seem to have been logged in as the wrong user after doing some work on our sandboxes.. Oops.
Comment 41 Martin Renvoize 2013-12-31 09:55:20 UTC
*** Bug 7151 has been marked as a duplicate of this bug. ***
Comment 42 Martin Renvoize 2014-01-03 11:52:40 UTC
I've now also tested this with packages.. everything goes to where one would expect now.  Perfect..
Comment 43 Katrin Fischer 2014-01-31 19:50:57 UTC
I'd love to see this moving forward, but I am not sure how to fully test.
Comment 44 Doug Kingston 2014-02-02 20:24:24 UTC
(In reply to Katrin Fischer from comment #43)
> I'd love to see this moving forward, but I am not sure how to fully test.

So, this feature adds locking so that basically there can ever only be one instance of rebuild_zebra.pl running at any given time.

Incremental updates in deamon try to obtain the lock and skip the current cycle if they cannot get the the lock.  Other updates try to get the lock and block waiting for it to get free if its locked.

Testing should check these behaviors.  The flock testing program lets you apply the lock without having real conflicting updates.  You can test using that to create conflicts or just use to instances of rebuild_zebra.pl as would happen for real.  Using verbose mode you will get messages indicating if lock contention is detected.

Cases to test:
1) 2 non-daemon invocations of a full rebuild; the second invocation should see the first has taken the lock and it should block until the first finishes.
2) daemon running and user starts a large full rebuild.  Run the daemon mode incremental updater.  Add or modify a record and make sure it gets updated.
Then request a full rebuild while the daemon version is also running.  Once the full rebuild has started, modify a record which will add an incremental update to the zebraqueue.  You should see log messages from the daemon indicating that it is skipping the current pass as long as the full rebuild is still running.

Does this help?
Comment 45 Marcel de Rooy 2014-02-04 14:40:46 UTC
If someone does not beat me to it, I would like to put some QA time in this report. But just for the record, I would not recommend updating records during a full rebuild. Catalogers should sleep some time :)
Comment 46 Katrin Fischer 2014-02-04 14:49:35 UTC
Hi Marcel, I would be grateful if you could take this on - feeling a bit out of my depth here.
Comment 47 Marcel de Rooy 2014-02-07 09:44:32 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2014-02-07 10:27:38 UTC
QA Comment:
First, I like this patch and would like to see it reach master soon. Compliments for the solid testing of Martin by the way. I have taken the liberty of trusting Martin and Robin here for the package related stuff.
I tested daemon and non-daemon simultaneously. Added some debug sleeps and prints to have some grip on the tests.
Although I am inclined to pass QA on this patch (no complaints from qa tools), I see some room for minor improvements in a follow-up:

1) There is the probably very rare circumstance of: The flock call produces a fatal error if used on a machine that doesn't implement flock(2), fcntl(2) locking, or lockf(3).
Could you wrap the flock call inside an eval in a function? The daemon should only eval the flock at least once..

2) You are waiting on the lock with the one-time invocation. If I am not running daemon mode, but I run the one-time invocation via cron very regularly, I would (personally) prefer to skip the one-pass instead of waiting for the lock.
I agree that it is somewhat arbitrary what is the best thing here. Could we resolve that by adding a simple command-line parameter that tells rebuild-zebra to wait or skip? (Skip by default, I guess :-)

3) What about t/db_dependent/zebra_config.pl, called from Search.t?

4) And finally :) what about the defaulting to /var/lock if the zebra lock has not been defined or so? mkdir /var/lock/rebuild: Permission denied at misc/migration_tools/rebuild_zebra.pl line 161. I guess that this permission problem could popup in many cases more..

With four points after all, I am sorry that I did no longer dare to move it to Passed QA rightaway. So first setting it to Failed QA in order to get some feedback. Thanks.
Comment 49 Doug Kingston 2014-02-10 03:53:03 UTC
(In reply to M. de Rooy from comment #48)
> QA Comment:
> First, I like this patch and would like to see it reach master soon.
> Compliments for the solid testing of Martin by the way. I have taken the
> liberty of trusting Martin and Robin here for the package related stuff.
> I tested daemon and non-daemon simultaneously. Added some debug sleeps and
> prints to have some grip on the tests.
> Although I am inclined to pass QA on this patch (no complaints from qa
> tools), I see some room for minor improvements in a follow-up:
> 
> 1) There is the probably very rare circumstance of: The flock call produces
> a fatal error if used on a machine that doesn't implement flock(2), fcntl(2)
> locking, or lockf(3).
> Could you wrap the flock call inside an eval in a function? The daemon
> should only eval the flock at least once..

Please point me at a similar usage elsewhere so I can make sure to do what you intend here.

> 
> 2) You are waiting on the lock with the one-time invocation. If I am not
> running daemon mode, but I run the one-time invocation via cron very
> regularly, I would (personally) prefer to skip the one-pass instead of
> waiting for the lock.
> I agree that it is somewhat arbitrary what is the best thing here. Could we
> resolve that by adding a simple command-line parameter that tells
> rebuild-zebra to wait or skip? (Skip by default, I guess :-)

I do not have a preference on the default behavior here.  I can also see people being surprised that their command exits without waiting by default as the most
likely interference is from an incremental update at about the same time which will exit quickly.  Adding a flag to control waiting or exiting is fine.  It should complement the default behavior.

I would like some sort of consensus from the community before I start changing code.  If a couple of more people concur, I can make the change.  Lets try to close on this issue quickly - can you reach out to a couple of senior folks for an opinion?  Otherwise, lets leave as is.

> 
> 3) What about t/db_dependent/zebra_config.pl, called from Search.t?

Well spotted, I will add a patch for this file too.  Its a one liner.

> 
> 4) And finally :) what about the defaulting to /var/lock if the zebra lock
> has not been defined or so? mkdir /var/lock/rebuild: Permission denied at
> misc/migration_tools/rebuild_zebra.pl line 161. I guess that this permission
> problem could popup in many cases more..

Something like this?
my $lockdir = C4::Context->config("zebra_lockdir") // "/var/lock";
$lockdir .= "/rebuild";
unless (-d $lockdir) {
    make_path($lockdir, {verbose=>0, mode=>oct(755), error=>\$err})
    $lockdir = "/var/lock" if (@$err);
}
my $lockfile = $lockdir . "/rebuild..LCK";


> 
> With four points after all, I am sorry that I did no longer dare to move it
> to Passed QA rightaway. So first setting it to Failed QA in order to get
> some feedback. Thanks.
Comment 50 Robin Sheat 2014-02-10 04:02:59 UTC
(In reply to Doug Kingston from comment #49)
> Please point me at a similar usage elsewhere so I can make sure to do what
> you intend here.

Near the bottom of xt/yaml_valid.pl has an example of using eval. You probably don't need an error check here though, just the eval. If the platform can't handle flock, the best thing we can do is move on. Maybe output a message if -v has been specified.

> I would like some sort of consensus from the community before I start
> changing code.  If a couple of more people concur, I can make the change. 
> Lets try to close on this issue quickly - can you reach out to a couple of
> senior folks for an opinion?  Otherwise, lets leave as is.

Hmm, an option on what to do is fine, though I think generally I'd want it to just die, so I don't run the risk of having 50 processes all sitting waiting. It should die with an output on STDERR though, as this is likely to be an uncommon event and might be indicative of an issue.
Comment 51 M. Tompsett 2014-02-10 04:10:23 UTC
(In reply to Robin Sheat from comment #50)
[SNIP]
> I think generally I'd want it
> to just die, so I don't run the risk of having 50 processes all sitting
> waiting.

Wouldn't a semaphore, as opposed to a lock be more useful for this kind of case? :)
Comment 52 Robin Sheat 2014-02-10 04:18:32 UTC
(In reply to M. Tompsett from comment #51)
> Wouldn't a semaphore, as opposed to a lock be more useful for this kind of
> case? :)

For our purposes, we want a semaphore of size 1, which is what a file lock implements.
Comment 53 M. Tompsett 2014-02-10 04:29:02 UTC
(In reply to Robin Sheat from comment #52)
> For our purposes, we want a semaphore of size 1, which is what a file lock
> implements.

Not if you wanted to be able to have a full-index waiting for partial-reindex to finish (or vice versa). There is some fanciness that could be done, if it was desired. Then you could decide to block or die depending on a set of rules. I know. I know. Just dreaming crazy wild. A lock is sufficient. :)
Comment 54 Martin Renvoize 2014-02-10 09:12:53 UTC
Agreed, I think it's important to have the option of die vs wait for running the script via cron, and I'm erring on the side of having die/skip as the default too.  It would certainly help keep the number of invocations minimal..
Comment 55 Marcel de Rooy 2014-02-10 14:08:20 UTC Comment hidden (obsolete)
Comment 56 Marcel de Rooy 2014-02-10 14:09:58 UTC
(In reply to Doug Kingston from comment #49)
> (In reply to M. de Rooy from comment #48)
> Please point me at a similar usage elsewhere so I can make sure to do what
> you intend here.

Instead of looking for or describing it, I wrote a few lines. Please test.
Comment 57 Marcel de Rooy 2014-02-10 14:15:41 UTC
> I do not have a preference on the default behavior here.  I can also see
> people being surprised that their command exits without waiting by default
> as the most
> likely interference is from an incremental update at about the same time
> which will exit quickly.  Adding a flag to control waiting or exiting is
> fine.  It should complement the default behavior.
> 
> I would like some sort of consensus from the community before I start
> changing code.  If a couple of more people concur, I can make the change. 
> Lets try to close on this issue quickly - can you reach out to a couple of
> senior folks for an opinion?  Otherwise, lets leave as is.

We have some replies now (moving the balance to skipping). The argument of a cronjob that starts a new incremental update every 2 minutes while the rebuild is running 60 minutes gives you 30 jobs just waiting, is imo sound enough to default to skip instead of wait.
An additional parameter could be added to include the wait behavior as your patch implemented.
Comment 58 Marcel de Rooy 2014-02-10 14:21:50 UTC
> > 4) And finally :) what about the defaulting to /var/lock if the zebra lock
> > has not been defined or so? mkdir /var/lock/rebuild: Permission denied at
> > misc/migration_tools/rebuild_zebra.pl line 161. I guess that this permission
> > problem could popup in many cases more..
> 
> Something like this?
> my $lockdir = C4::Context->config("zebra_lockdir") // "/var/lock";
> $lockdir .= "/rebuild";
> unless (-d $lockdir) {
>     make_path($lockdir, {verbose=>0, mode=>oct(755), error=>\$err})
>     $lockdir = "/var/lock" if (@$err);
> }
> my $lockfile = $lockdir . "/rebuild..LCK";

Sorry, that my comment here was not that clear (defaulting was not the right word, it should better be fall back). I actually meant that we should better choose another folder than /var/lock. If you run the zebra job without root permissions, you will probably have no permissions in /var/lock.
Since this should be an exception, why not just fall back to /tmp (OR just skip the locking stuff: if the install is dubious, who complains..) BTW there are other places in Koha where write permission in /tmp is just assumed. In every case, it is much more likely to be so. Your choice.
Comment 59 Marcel de Rooy 2014-02-10 18:52:24 UTC
(In reply to M. de Rooy from comment #58)
> If you run the zebra job without root
> permissions, you will probably have no permissions in /var/lock.

$ ls -ld /run/lock
drwxr-xr-x. 4 root root 80 Feb 10 03:09 /run/lock

Redhat/Fedora distro's may point /var/lock via /var/run/lock to /run/lock on tmpfs. As you see, the perms do not help.
Comment 60 Doug Kingston 2014-02-16 23:03:29 UTC
(In reply to M. de Rooy from comment #58)
> > > 4) And finally :) what about the defaulting to /var/lock if the zebra lock
> > > has not been defined or so? mkdir /var/lock/rebuild: Permission denied at
> > > misc/migration_tools/rebuild_zebra.pl line 161. I guess that this permission
> > > problem could popup in many cases more..
> > 
> > Something like this?
> > my $lockdir = C4::Context->config("zebra_lockdir") // "/var/lock";
> > $lockdir .= "/rebuild";
> > unless (-d $lockdir) {
> >     make_path($lockdir, {verbose=>0, mode=>oct(755), error=>\$err})
> >     $lockdir = "/var/lock" if (@$err);
> > }
> > my $lockfile = $lockdir . "/rebuild..LCK";
> 
> Sorry, that my comment here was not that clear (defaulting was not the right
> word, it should better be fall back). I actually meant that we should better
> choose another folder than /var/lock. If you run the zebra job without root
> permissions, you will probably have no permissions in /var/lock.
> Since this should be an exception, why not just fall back to /tmp (OR just
> skip the locking stuff: if the install is dubious, who complains..) BTW
> there are other places in Koha where write permission in /tmp is just
> assumed. In every case, it is much more likely to be so. Your choice.

OK.  I'll change the fallback location to /tmp - guaranteed to always.  Looks like a default behavior for adhoc invocation should should be to exit when it cannot get the lock, with an option to wait_for_lock.
Comment 61 Doug Kingston 2014-02-17 02:44:40 UTC Comment hidden (obsolete)
Comment 62 Marcel de Rooy 2014-02-17 07:48:59 UTC
Second patch does not apply.
Comment 63 Doug Kingston 2014-02-19 07:10:10 UTC Comment hidden (obsolete)
Comment 64 Marcel de Rooy 2014-02-19 12:28:10 UTC
(In reply to Doug Kingston from comment #63)
> This is a new combined patch (original+QA changes).  Its been build against
> HEAD so it should apply to a new branch at origin/master.  It replaces the
> two patches from earlier.

Yes, it applies. Will have a look now..
Comment 65 Marcel de Rooy 2014-02-19 14:49:25 UTC
Created attachment 25446 [details] [review]
Bug 11078: Add locking to rebuild_zebra (revised patch 2014-02-18)

This patch adds locking to rebuild_zebra.pl to ensure that simultaneous
changes are prevented (as one is likely to overwrite the other).
Incremental updates in daemon mode will skipped if the lock is busy
and they will be picked up on the next pass.  Non-daemon mode
invocations will also exit immediately if they cannot get the lock
unless the new flag -wait-for-lock is specified, in which case they
will wait until the get the lock and then proceed.

Supporting changes made to Makefile.PL and templates for the new
locking directory (paralleling the other zebra lock directories).
We stash the zebra_lockdir in koha-conf.xml so rebuild_zebra.pl
can find it.

To address earlier QA concerns we:
1. added code to check if flock is available and ignore locking if
its missing (from M. de Rooy)

2. changed default for adhoc invocations to abort if they cannot
obtain the lock.  Added option -wait-for-lock if the user prefers
to wait until the lock is free, and then continue processing.

3. added missing entry to t/db_dependent/zebra_config.pl

4. added a fallback locking directory of /tmp

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Doug merged the original patch with the QA changes.
Just for the record, noting here that the original patch was tested
extensively too by Martin Renvoize.
I have added a followup for some exceptional cases.
Comment 66 Marcel de Rooy 2014-02-19 14:50:55 UTC
Created attachment 25447 [details] [review]
Bug 11078: QA Follow-up for missing file permissions on lockfile

The original patch creates a lockfile in the ZEBRA_LOCKDIR.
It can fall back to /var/lock or even /tmp.
If the create fails, it dies. This can be considered as very exceptional.

This followup adjusts the fallback location in /var/lock or /tmp slightly.
It appends the database name to the folder in order to prevent interfering
between multiple Koha instances. Creation of the lockfile has been moved
to a subroutine extending directory and file creation testing.

In the very unlikely case that we cannot create the lockfile (after three
separate tries), this follow-up allows you to continue instead of die.
This is just as we did before we had file locking here. Every time skipping
a reindex could cause more harm than continuing and having the race condition
once in a while.

Test plan:
Test adding and removing lockdir from your koha-conf.xml. Check fallback.
Note that fallback in /var/lock or /tmp must contain database name.
Remove the lockdir config line and remove permissions from fallback. In
this case the reindex should continue but with a warning.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested with daemon and one-off invocation simultaneously.
Tested new wait parameter.
Tried all variations of lock directory (changing permissions etc.)
Comment 67 Marcel de Rooy 2014-02-19 15:03:12 UTC
QA Comment:
Thanks Doug for all adjustments. As you can see, I still saw a reason to add a followup to cover some (very) exceptional cases.
Just for the record: I would prefer (next time) to keep the original patch with signoff and just add QA follow-ups. But no reason to do that now anymore!

I would like to suggest the following route now:
Please apply both patches and test again (hopefully last time).
You could add your signoff on the second patch.
Since we had Martin for the signoff and considering the second patch as a QA follow-up, I will pass QA on this report after receiving your confirmation.
Note that we have no warnings too from the QA tools.
Comment 68 Doug Kingston 2014-02-24 01:09:57 UTC
I have retested with the new QA patch on top of my patch.  I believe I have exercised all new code paths and directory fallbacks my manipulating various file/directory permissions to force the use and failure of fallback options.  Adding and removing zebra_lockdir from koha-conf.xml also tested.  Works with and without this entry.  Adhoc wait-for-lock also tested again and works as intended.

Looks good to me!
Comment 69 Marcel de Rooy 2014-02-24 07:31:32 UTC
(In reply to Doug Kingston from comment #68)
> I have retested with the new QA patch on top of my patch.  I believe I have
> exercised all new code paths and directory fallbacks my manipulating various
> file/directory permissions to force the use and failure of fallback options.
> Adding and removing zebra_lockdir from koha-conf.xml also tested.  Works
> with and without this entry.  Adhoc wait-for-lock also tested again and
> works as intended.
> 
> Looks good to me!

Thanks..
Comment 70 Marcel de Rooy 2014-02-24 07:53:11 UTC
With reference to comment48, comment67 and what happened in the meantime, I am passing QA on this report now.
Code looks good to me. No complaints from koha-qa.

Still want to pass this note to Galen however:
If nicely installed with a lockdir in the zebra dirs and consequently run under same user context, the locking will work just fine.
And note that if the locking would not work out, there are no serious problems either. The locking just prevent an incidental race condition.
But the last few days I had some second thoughts about the two fallbacks, especially when people have no lockdir (although they should) and they might run zebra under different user contexts (absolutely no recommendation!). In that (very) rare case we could have two concurrent processes locking files at two different locations, providing some sense of false security. But I mention again: this would be an exception built on two faults.
So, for me pushing these two patches is fine. But removing the fallbacks for the above reason (this could be done in one-liner patch now), would be fine too, although you could still run rebuild-zebra with different permissions in the remaining lockdir too..

Passed QA
Comment 71 Martin Renvoize 2014-02-24 08:36:04 UTC
Nice work guys, excellent testing all round and excellent follow through Doug!  This is such a good example of seeing things through from beginning to end.. shame not all patches get this far ;)
Comment 72 Galen Charlton 2014-02-28 22:29:20 UTC
Pushed to master, along with a follow-up that does some trivial tidying of the code.

Thanks, Doug!
Comment 73 Marcel de Rooy 2014-03-01 10:39:59 UTC
(In reply to Galen Charlton from comment #72)
> Pushed to master, along with a follow-up that does some trivial tidying of
> the code.

Thanks, Galen. Let us leave the cuddled else-discussions outside Koha, but moving to the other camp is quite hard :)
Comment 74 Fridolin SOMERS 2014-05-27 11:00:24 UTC
Pushed to 3.14.x, will be in 3.14.07

Really nice job, congratulations.

We may revamp koha-index-daemon-ctl.sh with rebuild_zebra.pl now.