Summary: | koha-* scripts --restart should start even when not running | ||
---|---|---|---|
Product: | Koha | Reporter: | Alex Buckley <alexbuckley> |
Component: | Command-line Utilities | Assignee: | Alex Buckley <alexbuckley> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | david, dcook, jonathan.druart, lucas, m.de.rooy, robin, wizzyrea |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21366 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24991 |
||
Change sponsored?: | Sponsored | Patch complexity: | Small patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This enhancement restarts services if they are not running when using the --restart option for koha-plack, koha-indexer, koha-sip, koha-worker, koha-z3950-responder, and koha-zebra .
An example of a message if plack is not running and the service is restarted:
koha-plack --restart kohadev
[warn] Plack not running for kohadev. ... (warning).
[ ok ] Starting Plack daemon for kohadev:.
Previously if a service was not running an error message was generated (Error: {servicename} not running for ${instancename}") and a start command for the service was required.
|
Version(s) released in: |
22.11.00
|
Circulation function: | |||
Attachments: |
Bug 30684: When Plack is not running koha-plack --restart should warn and then start Plack
Bug 30684: When Plack is not running koha-plack --restart should warn and then start Plack Bug 30684: When a service isn't running koha-* scripts --restart should start it Bug 30684: When a service isn't running koha-* scripts --restart should start it Bug 30684: (follow up) Split restart output across multiple lines Bug 30684: (follow-up) Use log_warning_msg for the "not running" warning Bug 30684: When a service isn't running koha-* scripts --restart should start it Bug 30684: (follow-up) Use log_warning_msg for the "not running" warning |
Description
Alex Buckley
2022-05-05 03:01:44 UTC
Created attachment 134622 [details] [review] Bug 30684: When Plack is not running koha-plack --restart should warn and then start Plack Test plan: 1. Stop plack 2. Run: sudo koha-plack --restart {instance} 3. Observe you get the error: "Error: Plack not running for ${instancename}" and that plack is not started 4. Apply patch. 5. If you are using kohadevbox you must copy koha-plack to /usr/sbin/: sudo cp debian/scripts/koha-plack /usr/sbin/koha-plack 6. Run: sudo koha-plack --restart {instance} 7. Confirm you get a warning: "Plack not running for ${instancename}. Starting plack now" and that plack is started Sponsored-by: Catalyst IT I'm not 100% sure of the "warn" there but overall I like this idea. The current behaviour for "koha-plack --restart" is pretty annoying... Created attachment 134665 [details] [review] Bug 30684: When Plack is not running koha-plack --restart should warn and then start Plack Test plan: 1. Stop plack 2. Run: sudo koha-plack --restart {instance} 3. Observe you get the error: "Error: Plack not running for ${instancename}" and that plack is not started 4. Apply patch. 5. If you are using kohadevbox you must copy koha-plack to /usr/sbin/: sudo cp debian/scripts/koha-plack /usr/sbin/koha-plack 6. Run: sudo koha-plack --restart {instance} 7. Confirm you get a warning: "Plack not running for ${instancename}. Starting plack now" and that plack is started Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Thanks, David C and David N for your interaction with this bug report! I can certainly remove the 'warn' if that's the general consensus (In reply to Alex Buckley from comment #4) > I can certainly remove the 'warn' if that's the general consensus I just took another look at debian/scripts/koha-plack and it looks like we use "warn" elsewhere, so actually just ignore my earlier comment! (In reply to David Cook from comment #5) > (In reply to Alex Buckley from comment #4) > > I can certainly remove the 'warn' if that's the general consensus > > I just took another look at debian/scripts/koha-plack and it looks like we > use "warn" elsewhere, so actually just ignore my earlier comment! Perfect, thanks David! Not sure if this behavior change has a broader community consensus. Please ask on the mailing list. Note that the error was by design: If you did not start it, you cannot restart it. So restart should stop and start. Other scripts like koha-zebra have the same logic. And this is already for many years. Changing plack only here is inconsistent. Changing it in more places after a long time should have a very good reason. FQA for now (In reply to Marcel de Rooy from comment #7) > Note that the error was by design: If you did not start it, you cannot > restart it. So restart should stop and start. That design is inconsistent with how services work in general on Linux systems though, which makes them counter-intuitive for sysadmins. I know I've frequently been frustrated by Koha's koha-* scripts due to how their start/stop/restart is different. > Other scripts like koha-zebra have the same logic. And this is already for > many years. Changing plack only here is inconsistent. Changing it in more > places after a long time should have a very good reason. Fair point about it being inconsistent across the scripts though... I think making the koha-* scripts more in line with Linux services is a very good reason though. I'm sure it would spare many sysadmins many headaches... (In reply to David Cook from comment #8) > I think making the koha-* scripts more in line with Linux services is a very > good reason though. I'm sure it would spare many sysadmins many headaches... Especially when used in automation (In reply to David Cook from comment #8) > I think making the koha-* scripts more in line with Linux services is a very > good reason though. I'm sure it would spare many sysadmins many headaches... Sounds like a good reason. Will change title and hope for some additional patches. (In reply to Marcel de Rooy from comment #10) > (In reply to David Cook from comment #8) > > I think making the koha-* scripts more in line with Linux services is a very > > good reason though. I'm sure it would spare many sysadmins many headaches... > > Sounds like a good reason. Will change title and hope for some additional > patches. Thanks for your thoughts Marcel and David. Please correct me if I'm misunderstanding, but I should amend the files below, making the restart function in each stop and then start the service? - koha-indexer - koha-sip - koha-worker - koha-z3950-responder - koha-zebra Also, do you think I should still write to the mailing list to see if there is a broader community consensus, or just proceed with the above? Thanks, Alex (In reply to Alex Buckley from comment #11) > Please correct me if I'm misunderstanding, but I should amend the files > below, making the restart function in each stop and then start the service? > > - koha-indexer > - koha-sip > - koha-worker > - koha-z3950-responder > - koha-zebra Yes, please. > Also, do you think I should still write to the mailing list to see if there > is a broader community consensus, or just proceed with the above? I would say that it does not hurt to do so. It might even trigger a quick signoff. (In reply to Marcel de Rooy from comment #12) > (In reply to Alex Buckley from comment #11) > > Please correct me if I'm misunderstanding, but I should amend the files > > below, making the restart function in each stop and then start the service? > > > > - koha-indexer > > - koha-sip > > - koha-worker > > - koha-z3950-responder > > - koha-zebra > > Yes, please. > > > Also, do you think I should still write to the mailing list to see if there > > is a broader community consensus, or just proceed with the above? > > I would say that it does not hurt to do so. It might even trigger a quick > signoff. Sure thing, thanks Marcel! I've written to the Koha community dev mailing list, to check if there is broader community consensus for this change. I'll also work on making those changes to the koha-* scripts with the aim of attaching the patches next week. Created attachment 135747 [details] [review] Bug 30684: When a service isn't running koha-* scripts --restart should start it Test plan: 1. Stop plack, indexer, sip, worker, z3950 and zebra. Note: To test Z39.50 restarting you will need to setup the server: https://wiki.koha-community.org/wiki/Setting_up_the_Z39.50_and_SRU_Server#Setting_up_the_Z39.50_and_SRU_Server 2. Run restart command for each service: - [Plack] sudo koha-plack --restart {instance} - [Indexer] sudo koha-indexer --restart {instance} - [SIP] sudo koha-sip --restart {instance} - [Worker] sudo koha-worker --restart {instance} - [z3950] sudo koha-z3950-responder --restart {instance} - [Zebra] sudo koha-zebra --restart {instance} 3. Observe the services do not get restarted. Instead you get an error saying the service is not running 4. Apply patch 5. If you are using kohadevbox you must copy koha-* scripts to /usr/sbin/: sudo cp debian/scripts/koha-plack /usr/sbin/koha-plack sudo cp debian/scripts/koha-indexer /usr/sbin/koha-indexer sudo cp debian/scripts/koha-sip /usr/sbin/koha-sip sudo cp debian/scripts/koha-worker /usr/sbin/koha-worker sudo cp debian/scripts/koha-z3950-responder /usr/sbin/koha-z3950-responder sudo cp debian/scripts/koha-zebra /usr/sbin/koha-zebra 6. Repeat step 2 7. This time confirm that each service is restarted Sponsored-by: Catalyst IT Hi Marcel and David, Apologies it seems I was overly optimistic about when I could get back to this bug report. Thank you David for replying to my request for comment email to the community mailing list! I have been working on this patchset this evening and I tried to add the following line to the else block of restart_plack() in debian/scripts/koha-plack: stop_plack $instancename && start_plack $instancename I found that when plack is not running if I tried koha-plack --restart I would get the following error from that above line: [FAIL] Error: Plack not running for kohadev: failed! I had a discussion with Chris C about this and we concluded that you cannot stop a service that is not running. You will only get an error that the service is not running. Looking at how Apache does it we thought it would be best for the restart functions in the koha-* scripts to check if the service is running and if not then only start the service, not try to stop then start. Our proposed functionality is contained in the attached patchset for all the koha-* scripts with restart functions. Can you please let me know your thoughts when you have a chance? Thanks, Alex (In reply to Alex Buckley from comment #16) > Looking at how Apache does it we thought it would be best for the restart > functions in the koha-* scripts to check if the service is running and if > not then only start the service, not try to stop then start. > > Our proposed functionality is contained in the attached patchset for all the > koha-* scripts with restart functions. > > Can you please let me know your thoughts when you have a chance? > > Thanks, > Alex Sounds good to me. Easy to get the Starman PID to see if it's running. (In reply to David Cook from comment #17) > (In reply to Alex Buckley from comment #16) > > Looking at how Apache does it we thought it would be best for the restart > > functions in the koha-* scripts to check if the service is running and if > > not then only start the service, not try to stop then start. > > > > Our proposed functionality is contained in the attached patchset for all the > > koha-* scripts with restart functions. > > > > Can you please let me know your thoughts when you have a chance? > > > > Thanks, > > Alex > > Sounds good to me. Easy to get the Starman PID to see if it's running. Thanks David! Created attachment 137738 [details] [review] Bug 30684: When a service isn't running koha-* scripts --restart should start it Test plan: 1. Stop plack, indexer, sip, worker, z3950 and zebra. Note: To test Z39.50 restarting you will need to setup the server: https://wiki.koha-community.org/wiki/Setting_up_the_Z39.50_and_SRU_Server#Setting_up_the_Z39.50_and_SRU_Server 2. Run restart command for each service: - [Plack] sudo koha-plack --restart {instance} - [Indexer] sudo koha-indexer --restart {instance} - [SIP] sudo koha-sip --restart {instance} - [Worker] sudo koha-worker --restart {instance} - [z3950] sudo koha-z3950-responder --restart {instance} - [Zebra] sudo koha-zebra --restart {instance} 3. Observe the services do not get restarted. Instead you get an error saying the service is not running 4. Apply patch 5. If you are using kohadevbox you must copy koha-* scripts to /usr/sbin/: sudo cp debian/scripts/koha-plack /usr/sbin/koha-plack sudo cp debian/scripts/koha-indexer /usr/sbin/koha-indexer sudo cp debian/scripts/koha-sip /usr/sbin/koha-sip sudo cp debian/scripts/koha-worker /usr/sbin/koha-worker sudo cp debian/scripts/koha-z3950-responder /usr/sbin/koha-z3950-responder sudo cp debian/scripts/koha-zebra /usr/sbin/koha-zebra 6. Repeat step 2 7. This time confirm that each service is restarted Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Testing notes (using koha-testing-docker): 1. Not related to this bug, but for koha-sip and koha-zebra there is no error message before and after the patch is applied. (All the other scripts say "Error: X not running for {instancename}: failed!".) 2. For koha-testing-docker, need to copy scripts as per step 5. 3. Format of the message after the patch is applied, examples: - Plack not running for kohadev.:Starting Plack daemon for kohadev:. - Worker not running for kohadev (default).:Starting Koha worker daemon for kohadev (default):. I think you should have a look at the different things raised on bug 24991. (In reply to Jonathan Druart from comment #21) > I think you should have a look at the different things raised on bug 24991. Thanks Jonathan, I'll take a look. (In reply to Jonathan Druart from comment #21) > I think you should have a look at the different things raised on bug 24991. Hi Jonathan, I've taken a look at bug 24991. My patch is a bit different to Liz's as I'm attempting to implement consistent restart logic to all the koha-* scripts. Also, I'm not checking after the else statement whether SIP is running - so am avoiding the problem Marcel noted in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24991#c9 Would you mind please clarifying for me if you have any particular points in bug 24991 that you think I should implement in my patchset? Kind regards, Alex Ok go with your patch first then. Two things however: 1. koha-sip (at least, but maybe others) is missing \n in the output for restart if stopped (see second patch from bug 24991) $ koha-sip --verbose --stop kohadev Stopping SIP server for kohadev:. $ koha-sip --verbose --restart kohadev Warning: SIP server not running for kohadev.:Starting SIP server for kohadev:. 2. in koha-zebra if [ "$verbose" != "no" ]; then - log_daemon_msg "Error: Zebra not running for ${name}" - log_end_msg 1 + log_daemon_msg "Zebra not running for ${name}." + start_zebra ${name} else + start_zebra ${name} return 1 fi returning 1 here is wrong. (In reply to Jonathan Druart from comment #24) > Ok go with your patch first then. > > Two things however: > 1. koha-sip (at least, but maybe others) is missing \n in the output for > restart if stopped (see second patch from bug 24991) > > $ koha-sip --verbose --stop kohadev > Stopping SIP server for kohadev:. > $ koha-sip --verbose --restart kohadev > Warning: SIP server not running for kohadev.:Starting SIP server for > kohadev:. > > 2. in koha-zebra > if [ "$verbose" != "no" ]; then > - log_daemon_msg "Error: Zebra not running for ${name}" > - log_end_msg 1 > + log_daemon_msg "Zebra not running for ${name}." > + start_zebra ${name} > else > + start_zebra ${name} > return 1 > fi > > returning 1 here is wrong. Thanks, Jonathan. Working on fixing those two issues up! Created attachment 139247 [details] [review] Bug 30684: (follow up) Split restart output across multiple lines Also, return 0 rather than return 1 in else clause of koha-zebra Sponsored-by: Catalyst IT, New Zealand (In reply to Alex Buckley from comment #25) > (In reply to Jonathan Druart from comment #24) > > Ok go with your patch first then. > > > > Two things however: > > 1. koha-sip (at least, but maybe others) is missing \n in the output for > > restart if stopped (see second patch from bug 24991) > > > > $ koha-sip --verbose --stop kohadev > > Stopping SIP server for kohadev:. > > $ koha-sip --verbose --restart kohadev > > Warning: SIP server not running for kohadev.:Starting SIP server for > > kohadev:. > > > > 2. in koha-zebra > > if [ "$verbose" != "no" ]; then > > - log_daemon_msg "Error: Zebra not running for ${name}" > > - log_end_msg 1 > > + log_daemon_msg "Zebra not running for ${name}." > > + start_zebra ${name} > > else > > + start_zebra ${name} > > return 1 > > fi > > > > returning 1 here is wrong. > > Thanks, Jonathan. Working on fixing those two issues up! I've attached a patch hopefully fixing up those two issues. Please let me know if any other changes should be made? Why did you replace log_daemon_msg with echo? (In reply to Jonathan Druart from comment #28) > Why did you replace log_daemon_msg with echo? My apologies Jonathan. I replaced log_daemon_msg with echo because I went through all 6 scripts and found it wasn't just koha-sip where the output was not split over multiple lines. For example, with only the first patch applied (i.e. log_daemon_msg being used) koha-plack gives this output: vagrant@kohadevbox:kohaclone((0e08cf28f57...))$ sudo koha-plack --restart kohadev [ ok ] Plack not running for kohadev.:[....] Starting Plack daemon for kohadev:. --- With the second patch applied (i.e. echo being used) the koha-plack gives this output correctly split over two lines: vagrant@kohadevbox:scripts((69bb9e5e337...))$ sudo koha-plack --restart kohadev Plack not running for kohadev. [ ok ] Starting Plack daemon for kohadev:. --- I attempted keeping log_daemon_msg, and adding a newline character \n to the end (log_daemon_msg "Plack not running for ${instancename}.\n"), however, the newline character was not interpreted correctly, see below: vagrant@kohadevbox:kohaclone((69bb9e5e337...))$ sudo koha-plack --restart kohadev [ ok ] Plack not running for kohadev.\n:[....] Starting Plack daemon for kohadev:. Likely, there is some way for the \n character to be correctly interpreted by log_daemon_msg, I could not find how though - do you know how to do that? Alex, try 'log_warning_msg' for the "not running" warning. Created attachment 139305 [details] [review] Bug 30684: (follow-up) Use log_warning_msg for the "not running" warning Also, return 0 rather than return 1 in else clause of koha-zebra Sponsored-by: Catalyst IT, New Zealand (In reply to Jonathan Druart from comment #30) > Alex, try 'log_warning_msg' for the "not running" warning. Thanks Jonathan. Is that follow-up any better now? Created attachment 139345 [details] [review] Bug 30684: When a service isn't running koha-* scripts --restart should start it Test plan: 1. Stop plack, indexer, sip, worker, z3950 and zebra. Note: To test Z39.50 restarting you will need to setup the server: https://wiki.koha-community.org/wiki/Setting_up_the_Z39.50_and_SRU_Server#Setting_up_the_Z39.50_and_SRU_Server 2. Run restart command for each service: - [Plack] sudo koha-plack --restart {instance} - [Indexer] sudo koha-indexer --restart {instance} - [SIP] sudo koha-sip --restart {instance} - [Worker] sudo koha-worker --restart {instance} - [z3950] sudo koha-z3950-responder --restart {instance} - [Zebra] sudo koha-zebra --restart {instance} 3. Observe the services do not get restarted. Instead you get an error saying the service is not running 4. Apply patch 5. If you are using kohadevbox you must copy koha-* scripts to /usr/sbin/: sudo cp debian/scripts/koha-plack /usr/sbin/koha-plack sudo cp debian/scripts/koha-indexer /usr/sbin/koha-indexer sudo cp debian/scripts/koha-sip /usr/sbin/koha-sip sudo cp debian/scripts/koha-worker /usr/sbin/koha-worker sudo cp debian/scripts/koha-z3950-responder /usr/sbin/koha-z3950-responder sudo cp debian/scripts/koha-zebra /usr/sbin/koha-zebra 6. Repeat step 2 7. This time confirm that each service is restarted Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 139346 [details] [review] Bug 30684: (follow-up) Use log_warning_msg for the "not running" warning Also, return 0 rather than return 1 in else clause of koha-zebra Sponsored-by: Catalyst IT, New Zealand Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> *** Bug 24991 has been marked as a duplicate of this bug. *** Pushed to master for 22.11. Nice work everyone, thanks! Thank you for your assistance Jonathan! Enhancement will not be backported to 22.05.x for 22.05.06 |