Bug 28320 - SIP SC Status message should check the DB connection
Summary: SIP SC Status message should check the DB connection
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: SIP2 (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-05-11 13:47 UTC by Nick Clemens
Modified: 2021-05-25 14:50 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.06,20.05.12


Attachments
Bug 28320: Add DB connection check to the SIP SC status message (5.32 KB, patch)
2021-05-11 13:49 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28320: Add DB connection check to the SIP SC status message (5.38 KB, patch)
2021-05-12 10:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 28320: Add DB connection check to the SIP SC status message (5.43 KB, patch)
2021-05-14 08:48 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-05-11 13:47:30 UTC
Some SIP machines, automated materials handlers for example, connect to the SIP client and maintain a connection, sending SC status messages to keep the connection alive

This works, but can lead to issues if the SIP connection outlasts the DB connection.

To mitigate this, we should build a DB connection attempt into the SC status call, and terminate the SIP connection if we cannot reach the DB
Comment 1 Nick Clemens 2021-05-11 13:49:51 UTC
Created attachment 120843 [details] [review]
Bug 28320: Add DB connection check to the SIP SC status message

This patch adds a lookup of the sip user during an SC status, confirming that
our DB ocnnection is working, and that our user is still valid

Additionally, it adds support for SC status to the sip_cli_emulator and adds basic
test coverage for the SC status message

To test:
1 - Apply patch
2 - Restart SP server
3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -m sc_status_request -su term1 -sp term1
4 - prove -v t/db_dependent/SIP/Message.t
Comment 2 Kyle M Hall 2021-05-12 10:24:15 UTC
Created attachment 120860 [details] [review]
Bug 28320: Add DB connection check to the SIP SC status message

This patch adds a lookup of the sip user during an SC status, confirming that
our DB ocnnection is working, and that our user is still valid

Additionally, it adds support for SC status to the sip_cli_emulator and adds basic
test coverage for the SC status message

To test:
1 - Apply patch
2 - Restart SP server
3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -m sc_status_request -su term1 -sp term1
4 - prove -v t/db_dependent/SIP/Message.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 3 David Cook 2021-05-13 01:49:19 UTC
What happens with that die()? Does that crash the server or just severs the connection?
Comment 4 David Cook 2021-05-13 01:50:29 UTC
Also, DBIx::Class should reconnect to the database, so I'm a bit confused by what's going on here...
Comment 5 Martin Renvoize 2021-05-13 07:16:05 UTC
I should have commented on the bug instead of asking Nick/Kyle directly.. I agree, the die feels weird and I'm surprised the db connection isn't retried.
Comment 6 Nick Clemens 2021-05-13 11:09:04 UTC
(In reply to Martin Renvoize from comment #5)
> I should have commented on the bug instead of asking Nick/Kyle directly.. I
> agree, the die feels weird and I'm surprised the db connection isn't retried.

(In reply to David Cook from comment #3)
> What happens with that die()? Does that crash the server or just severs the
> connection?

The die simply kills the current connection, the server keeps running and can be reconnected to, to test:
 1 - telnet localhost 6001
 2 - input: 9300CNterm1|COterm1|CPCPL|
 3 - Should get a  94 response
 4 - Stop the DB in another terminal - docket stop koha_db_1
 5 - input to telnet: 9900302.00
 6 - "Connection closed by foreign host."
 7 - Start the DB
 8 - repeat 1-3
 9 - input to telnet: 9900302.00
10 - You get a 98 response

The DB connection should be retried, and generally is:
 1 - telnet localhost 6001
 2 - input: 9300CNterm1|COterm1|CPCPL|
 3 - Should get a  94 response
 4 - In another terminal: sudo koha-mysql kohadev
 5 - SHOW FULL PROCESSLIST;
 6 - You should see a connection that represents your SIP session above
 7 - You can send a 99 SC status to reset the sleep/time so you can identify which one is your sip connection
    input to telnet: 9900302.00
 8 - Once you have identified, in MySQL, kill the correct prcoess number:
    KILL 3
 9 - Send the 99 again
10 - The DB reconnects and continues without interrupting the SIP session


What we have seen is that for long standing SIP sessions, the DB won't reconnect gracefully. In these cases the SC statuses keep reporting connection, but the next checkin fails. After that failure the DB may reconnect and continue, but numbers of items can pass through the AMH/Self check without checking in. Pinging the DB connection eliminates the issue, as does a restart of the SIP server by forcing new connections. In the case where the SC status would fail/die, the machines reconnect to the SIP server, and we fail a status message rather than a checkin
Comment 7 David Cook 2021-05-14 02:23:02 UTC
Sounds like a difficult one to troubleshoot :(
Comment 8 Martin Renvoize 2021-05-14 08:48:42 UTC
Created attachment 120955 [details] [review]
Bug 28320: Add DB connection check to the SIP SC status message

This patch adds a lookup of the sip user during an SC status, confirming that
our DB ocnnection is working, and that our user is still valid

Additionally, it adds support for SC status to the sip_cli_emulator and adds basic
test coverage for the SC status message

To test:
1 - Apply patch
2 - Restart SP server
3 - perl misc/sip_cli_emulator.pl -a localhost -p 6001 -l CPL -m sc_status_request -su term1 -sp term1
4 - prove -v t/db_dependent/SIP/Message.t

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2021-05-14 08:49:39 UTC
Tested and works as expected. I read into the die and agree it's how it should be done in this case..  nice to learn.

Passing QA
Comment 10 Jonathan Druart 2021-05-17 14:20:17 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 11 Fridolin Somers 2021-05-18 10:20:57 UTC
Pushed to 20.11.x for 20.11.06
Comment 12 Andrew Fuerste-Henry 2021-05-25 13:49:54 UTC
Pushed to 20.05.x for 20.05.12
Comment 13 Victor Grousset/tuxayo 2021-05-25 14:50:39 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.