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.

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.

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.

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"?

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

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.

(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.

(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 :)

(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 :)

(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.

(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.

(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.

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.

As noted in chat, these patches are built on those for bug 6435 on which this bug depends.  Can you test these 2 together?

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?

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.

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

Waiting for a sign off here ... :)

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

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.

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?

(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",

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.

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

Doug, is this ready for sign off/QA with your follow ups? You can switch the status :)

These two patches should do the trick.  Ready for re-review and QA.

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 ;)

Above comment was by me.. I seem to have been logged in as the wrong user after doing some work on our sandboxes.. Oops.

*** Bug 7151 has been marked as a duplicate of this bug. ***

I've now also tested this with packages.. everything goes to where one would expect now.  Perfect..

I'd love to see this moving forward, but I am not sure how to fully test.

(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?

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 :)

Hi Marcel, I would be grateful if you could take this on - feeling a bit out of my depth here.

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.

(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.

(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.

(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? :)

(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.

(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. :)

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..

(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.

> 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.

> > 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.

(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.

(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.

Second patch does not apply.

(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..

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.

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.)

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.

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!

(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..

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

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 ;)

Pushed to master, along with a follow-up that does some trivial tidying of the code.

Thanks, Doug!

(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 :)

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.