http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #2 from Doug Kingston <dpk@randomnotes.org> --- Created attachment 22045 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22045&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22044|0 |1 is obsolete| | CC| |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |kyle@bywatersolutions.com --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #7 from Doug Kingston <dpk@randomnotes.org> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Robin Sheat <robin@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz --- Comment #9 from Robin Sheat <robin@catalyst.net.nz> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #11 from Doug Kingston <dpk@randomnotes.org> --- Created attachment 22506 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22506&action=edit Bug 11078 Add locking to rebuild_zebra (QA patch 10/28) Updates the debian configuration template as mentioned in the comments. -- You are receiving this mail because: You are watching all bug changes.

(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

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #13 from Doug Kingston <dpk@randomnotes.org> --- (In reply to Robin Sheat from comment #12) 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 :)

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #15 from Robin Sheat <robin@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #17 from Robin Sheat <robin@catalyst.net.nz> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Stefano Bargioni <bargioni@pusc.it> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bargioni@pusc.it -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #20 from Doug Kingston <dpk@randomnotes.org> --- As noted in chat, these patches are built on those for bug 6435 on which this bug depends. Can you test these 2 together? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=6435 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |mtompset@hotmail.com --- Comment #25 from M. Tompsett <mtompset@hotmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #27 from Doug Kingston <dpk@randomnotes.org> --- Created attachment 22985 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22985&action=edit Address coding style issues from koha-qa.pl -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #29 from M. Tompsett <mtompset@hotmail.com> --- 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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #31 from Doug Kingston <dpk@randomnotes.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Doug Kingston <dpk@randomnotes.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #22458|0 |1 is obsolete| | Attachment #22506|0 |1 is obsolete| | Attachment #22508|0 |1 is obsolete| | Attachment #22532|0 |1 is obsolete| | Attachment #22985|0 |1 is obsolete| | Attachment #22992|0 |1 is obsolete| | --- Comment #33 from Doug Kingston <dpk@randomnotes.org> --- Created attachment 23220 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23220&action=edit Bug 11078 Add locking to rebuild_zebra (rollup 11/28) This patch is a rollup of earlier patches and moves the lockfile to a parallel directory to the other zebra lock files. This should address all the issues as to where this file should be indifferent cases. It now behaves just like the other zebra lock files. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #35 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Doug, is this ready for sign off/QA with your follow ups? You can switch the status :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23221|0 |1 is obsolete| | --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 23574 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23574&action=edit Bug 11078 Add locking to rebuild_zebra.pl (path fix) Add missing reference to subdirectory for rebuild_zebra locks under zebra lock diretory. This is already handled correctly in the docs, template and makefiles. The code in fact works either way, but docs and reality should match. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #40 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Above comment was by me.. I seem to have been logged in as the wrong user after doing some work on our sandboxes.. Oops. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #42 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I've now also tested this with packages.. everything goes to where one would expect now. Perfect.. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #44 from Doug Kingston <dpk@randomnotes.org> --- (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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #46 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Marcel, I would be grateful if you could take this on - feeling a bit out of my depth here. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Component|Tools |Command-line Utilities -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Version|3.14 |master -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|6435 | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #49 from Doug Kingston <dpk@randomnotes.org> --- (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.

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #51 from M. Tompsett <mtompset@hotmail.com> --- (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? :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #53 from M. Tompsett <mtompset@hotmail.com> --- (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. :) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #55 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 25172 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25172&action=edit Bug 11078: Follow-up for flock eval This patch adds a subroutine where the flock is once tested in an eval. Just to be sure that flock has been implemented and prevent a fatal error. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #57 from M. de Rooy <m.de.rooy@rijksmuseum.nl> ---

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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #59 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Doug Kingston <dpk@randomnotes.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #25172|0 |1 is obsolete| | --- Comment #61 from Doug Kingston <dpk@randomnotes.org> --- Created attachment 25330 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25330&action=edit Bug 11078 Address February QA concerns This patch should address the concerns raised in the most recent QA. I believe I have tested all the new code paths here. 1. Add code to check if flock is available and ignore locking if its missing (from M. de Rooy, modified to pass perlcritic) 2. Change 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 Testing advice: You can force the various paths of the lockdir logic by a) removing lockdir location from config file or removing (temporarily) the zebra lock directory (forcing use of /var/lock) b) changing mode of /var/lock to be unwritable by others (forcing /tmp) I use the flock perl command to "take" the lock and then run rebuild_zebra.pl to make sure it wait or aborts as appropriate. Typically used something like flock /var/lock/koha/zebradb/rebuild/rebuild..LCK sleep 60 while you invoke rebuild_zebra.pl in a different window. I am deleting the patch from Marcel since it failed perlcritic. I've fixed the use of eval and included it in my patch. Thanks Marcel! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Doug Kingston <dpk@randomnotes.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #25114|0 |1 is obsolete| | Attachment #25330|0 |1 is obsolete| | --- Comment #63 from Doug Kingston <dpk@randomnotes.org> --- Created attachment 25412 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25412&action=edit Add locking to rebuild_zebra (revised patch 2014-02-18) 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #66 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 25447 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=25447&action=edit 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.) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 --- Comment #68 from Doug Kingston <dpk@randomnotes.org> --- 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! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 M. de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #70 from M. de Rooy <m.de.rooy@rijksmuseum.nl> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |gmcharlt@gmail.com --- Comment #72 from Galen Charlton <gmcharlt@gmail.com> --- Pushed to master, along with a follow-up that does some trivial tidying of the code. Thanks, Doug! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11078 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolyn.somers@biblibre.co | |m --- Comment #74 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.