Bug 30684 - koha-* scripts --restart should start even when not running
Summary: koha-* scripts --restart should start even when not running
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Alex Buckley
QA Contact: Testopia
URL:
Keywords:
: 24991 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-05-05 03:01 UTC by Alex Buckley
Modified: 2023-06-08 22:26 UTC (History)
7 users (show)

See Also:
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


Attachments
Bug 30684: When Plack is not running koha-plack --restart should warn and then start Plack (1.39 KB, patch)
2022-05-05 03:22 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 30684: When Plack is not running koha-plack --restart should warn and then start Plack (1.43 KB, patch)
2022-05-05 21:20 UTC, David Nind
Details | Diff | Splinter Review
Bug 30684: When a service isn't running koha-* scripts --restart should start it (4.82 KB, patch)
2022-06-07 08:58 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 30684: When a service isn't running koha-* scripts --restart should start it (4.87 KB, patch)
2022-07-14 20:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 30684: (follow up) Split restart output across multiple lines (4.13 KB, patch)
2022-08-17 10:23 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 30684: (follow-up) Use log_warning_msg for the "not running" warning (3.52 KB, patch)
2022-08-17 22:54 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 30684: When a service isn't running koha-* scripts --restart should start it (4.95 KB, patch)
2022-08-18 08:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 30684: (follow-up) Use log_warning_msg for the "not running" warning (3.59 KB, patch)
2022-08-18 08:19 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Buckley 2022-05-05 03:01:44 UTC
Currently, when Plack is not running, if you run sudo koha-plack --restart {$instancename} you get: "Error: Plack not running for ${instancename}"

It would be nice if Koha would warn that Plack is not currently running and then start it, rather than just erroring.
Comment 1 Alex Buckley 2022-05-05 03:22:56 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
Comment 2 David Cook 2022-05-05 05:03:39 UTC
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...
Comment 3 David Nind 2022-05-05 21:20:53 UTC
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>
Comment 4 Alex Buckley 2022-05-05 21:35:28 UTC
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
Comment 5 David Cook 2022-05-05 23:38:43 UTC
(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!
Comment 6 Alex Buckley 2022-05-05 23:39:19 UTC
(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!
Comment 7 Marcel de Rooy 2022-05-13 09:37:58 UTC
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
Comment 8 David Cook 2022-05-15 23:26:51 UTC
(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...
Comment 9 David Cook 2022-05-15 23:27:07 UTC
(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
Comment 10 Marcel de Rooy 2022-05-16 06:26:13 UTC
(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.
Comment 11 Alex Buckley 2022-05-18 05:09:52 UTC
(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
Comment 12 Marcel de Rooy 2022-05-18 06:10:17 UTC
(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.
Comment 13 Alex Buckley 2022-05-18 21:40:35 UTC
(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!
Comment 14 Alex Buckley 2022-05-20 04:13:32 UTC
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.
Comment 15 Alex Buckley 2022-06-07 08:58:43 UTC
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
Comment 16 Alex Buckley 2022-06-07 09:03:23 UTC
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
Comment 17 David Cook 2022-06-09 03:12:22 UTC
(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.
Comment 18 Alex Buckley 2022-06-09 07:20:44 UTC
(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!
Comment 19 David Nind 2022-07-14 20:22:19 UTC
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>
Comment 20 David Nind 2022-07-14 20:29:01 UTC
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):.
Comment 21 Jonathan Druart 2022-08-09 08:39:31 UTC
I think you should have a look at the different things raised on bug 24991.
Comment 22 Alex Buckley 2022-08-10 08:16:44 UTC
(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.
Comment 23 Alex Buckley 2022-08-17 04:44:21 UTC
(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
Comment 24 Jonathan Druart 2022-08-17 08:38:49 UTC
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.
Comment 25 Alex Buckley 2022-08-17 08:45:21 UTC
(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!
Comment 26 Alex Buckley 2022-08-17 10:23:16 UTC
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
Comment 27 Alex Buckley 2022-08-17 10:24:52 UTC
(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?
Comment 28 Jonathan Druart 2022-08-17 10:40:57 UTC
Why did you replace log_daemon_msg with echo?
Comment 29 Alex Buckley 2022-08-17 12:00:21 UTC
(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?
Comment 30 Jonathan Druart 2022-08-17 14:12:44 UTC
Alex, try 'log_warning_msg' for the "not running" warning.
Comment 31 Alex Buckley 2022-08-17 22:54:52 UTC
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
Comment 32 Alex Buckley 2022-08-17 22:58:00 UTC
(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?
Comment 33 Jonathan Druart 2022-08-18 08:19:10 UTC
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>
Comment 34 Jonathan Druart 2022-08-18 08:19:15 UTC
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>
Comment 35 Jonathan Druart 2022-08-18 08:20:04 UTC
*** Bug 24991 has been marked as a duplicate of this bug. ***
Comment 36 Tomás Cohen Arazi 2022-08-18 12:05:32 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 37 Alex Buckley 2022-08-19 01:33:29 UTC
Thank you for your assistance Jonathan!
Comment 38 Lucas Gass 2022-10-03 22:47:03 UTC
Enhancement will not be backported to 22.05.x for 22.05.06