Bug 12169 - Improve reliability of sip_shutdown script
Summary: Improve reliability of sip_shutdown script
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-01 14:16 UTC by Colin Campbell
Modified: 2022-06-06 20:25 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.11.00
Circulation function:


Attachments
Proposed patch (2.85 KB, patch)
2014-05-01 14:33 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 12169 Use pid file in sipserver shutdown (2.77 KB, patch)
2020-02-24 16:49 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 12169 Use pid file in sipserver shutdown (2.84 KB, patch)
2021-03-30 12:51 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 12169: Use pid file in sipserver shutdown (2.91 KB, patch)
2021-05-07 15:20 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 12169: Fix SIPconfig.xml, restore missing end tag (672 bytes, patch)
2021-05-07 15:21 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2014-05-01 14:16:09 UTC
the sip_shutdown script rightly announces the brittleness of it approach to identifying the process to kill.
A more reliable method is available by getting the server to use the standard approach of writing a pidfile which is part of Net::Server's functionality. Change script to use this and alter config scripts accordingly
Comment 1 Colin Campbell 2014-05-01 14:33:47 UTC
Created attachment 27878 [details] [review]
Proposed patch

Adds options to sample config and simplifies sample shutdown script
Comment 2 Larry Baerveldt 2015-03-06 11:07:59 UTC
This patch hardcodes the user and group as "koha" and "koha". You should not hardcode the user and group in the SIPConfig.xml file; those need to be gathered at the Koha creation phase, either through koha-create and koha-enable-sip for package, or with makefile.PL for git sites.
Comment 3 Martin Renvoize (ashimema) 2020-02-24 16:49:55 UTC
Created attachment 99533 [details] [review]
Bug 12169 Use pid file in sipserver shutdown

Change sample SIPConfig to request server writes a pid file
Use this in shutdown rather than the current open to error
method
Have also added to the Config parameters to ensure that the sipserver
runs as the correct user and sets its own session id. These are always
useful but makes it easier for users to run up the sipserver as root at
boot time similarly to apache, mysql etc
Added to the sample config where to locate other server parameter
documentation
Removed from the sample config the unedifying, unwanted and
purely historical http example
Comment 4 Martin Renvoize (ashimema) 2020-02-24 16:50:35 UTC
Rebased, followups to follow to appease the QA failure.
Comment 5 Martin Renvoize (ashimema) 2020-02-24 19:48:06 UTC
Rebased, followups to follow to appease the QA failure.
Comment 6 Martin Renvoize (ashimema) 2020-02-24 19:49:50 UTC
I agree with Larry here, but at the moment I'm struggling to work out where in the code I should look for using the user variables in the SIPConfig.xml template.
Comment 7 Jonathan Druart 2020-02-25 08:31:35 UTC
Are those scripts (sip_shutdown.sh and sip_run.sh)still used by anybody?
It seems that they are quite obsolete. The debian package script must be used now, koha-sip has the start/stop that deals with the pid file.
Comment 8 Katrin Fischer 2020-02-25 09:21:53 UTC
(In reply to Jonathan Druart from comment #7)
> Are those scripts (sip_shutdown.sh and sip_run.sh)still used by anybody?
> It seems that they are quite obsolete. The debian package script must be
> used now, koha-sip has the start/stop that deals with the pid file.

What if someone is using Koha from a tarball installation?
Comment 9 Martin Renvoize (ashimema) 2020-02-25 10:43:28 UTC
Both fair points... not sure where to go with this one then.. the scripts in fact state at the very top of the file that they're 'example scripts' anyway and may need modification.

I do wonder if we can deprecate them...
Comment 10 Martin Renvoize (ashimema) 2021-03-26 10:45:04 UTC
Well.. we still have a fair few dev/standard installs running from git/tarballs.

I don't think we're alone either.. I do see chatter on the lists and irc occasionally with people asking about that install type on various OS's
Comment 11 Martin Renvoize (ashimema) 2021-03-30 12:44:57 UTC
(In reply to Larry Baerveldt from comment #2)
> This patch hardcodes the user and group as "koha" and "koha". You should not
> hardcode the user and group in the SIPConfig.xml file; those need to be
> gathered at the Koha creation phase, either through koha-create and
> koha-enable-sip for package, or with Makefile.PL for git sites.

As far as I can tell, the file modified here is A) only an example file and B) only used for dev/standard installs and isn't referenced in the Makefile at all.

So, I don't think it affects packages at all, it just makes sip work more reliably for Dev/Standard installs.
Comment 12 Martin Renvoize (ashimema) 2021-03-30 12:51:58 UTC
Created attachment 119001 [details] [review]
Bug 12169 Use pid file in sipserver shutdown

Change sample SIPConfig to request server writes a pid file

Use this pid in shutdown rather than the current open to error
method.

Have also added to the Config parameters to ensure that the sipserver
runs as the correct user and sets its own session id. These are always
useful but makes it easier for users to run up the sipserver as root at
boot time similarly to apache, mysql etc

Added to the sample config where to locate other server parameter
documentation.

Removed from the sample config the unedifying, unwanted and
purely historical http example

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Kyle M Hall (khall) 2021-05-07 15:20:20 UTC
Created attachment 120702 [details] [review]
Bug 12169: Use pid file in sipserver shutdown

Change sample SIPConfig to request server writes a pid file

Use this pid in shutdown rather than the current open to error
method.

Have also added to the Config parameters to ensure that the sipserver
runs as the correct user and sets its own session id. These are always
useful but makes it easier for users to run up the sipserver as root at
boot time similarly to apache, mysql etc

Added to the sample config where to locate other server parameter
documentation.

Removed from the sample config the unedifying, unwanted and
purely historical http example

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall (khall) 2021-05-07 15:21:15 UTC
Created attachment 120703 [details] [review]
Bug 12169: Fix SIPconfig.xml, restore missing end tag

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Kyle M Hall (khall) 2021-05-07 15:22:11 UTC
Package install already have this type of functionality. Good to see it available for non-package installs! Only open question is that of an upgrade path. I think upgrade notes will be needed.
Comment 16 Jonathan Druart 2021-06-21 12:03:24 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 17 Kyle M Hall (khall) 2021-07-02 13:38:31 UTC
Enhancement not backported to 21.05.x