Bug 26742 - Add configuration for message broker
Summary: Add configuration for message broker
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: David Cook
QA Contact: Victor Grousset/tuxayo
URL:
Keywords:
Depends on: 22417
Blocks: 25674
  Show dependency treegraph
 
Reported: 2020-10-20 00:42 UTC by David Cook
Modified: 2021-03-14 19:25 UTC (History)
7 users (show)

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


Attachments
Bug 26742: Add configuration to koha-conf.xml for message broker (2.71 KB, patch)
2020-10-20 00:56 UTC, David Cook
Details | Diff | Splinter Review
Bug 26742: Add configuration to koha-conf.xml for message broker (2.91 KB, patch)
2020-10-20 01:35 UTC, David Cook
Details | Diff | Splinter Review
Bug 26742: Add configuration to koha-conf.xml for message broker (2.95 KB, patch)
2020-11-21 12:50 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 26742: Add configuration to koha-conf.xml for message broker (3.01 KB, patch)
2020-12-09 16:46 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 26742: Remove die from connect (1.03 KB, patch)
2020-12-14 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26742: Add configuration to koha-conf.xml for message broker (3.05 KB, patch)
2021-03-02 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26742: Remove die from connect (1.10 KB, patch)
2021-03-02 16:51 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 David Cook 2020-10-20 00:42:08 UTC
It should be possible to configure the message broker.

While an out of the box install may run the message broker on the same host as Koha, this is not a recommended scenario. It's better to have the message broker on a separate host.
Comment 1 David Cook 2020-10-20 00:56:04 UTC
Created attachment 112013 [details] [review]
Bug 26742: Add configuration to koha-conf.xml for message broker

The connection details for the message broker should be configurable.
This patch adds configuration options to koha-conf.xml. If they
are not specified, then default connection details will be used.
Comment 2 David Cook 2020-10-20 01:35:59 UTC
Created attachment 112015 [details] [review]
Bug 26742: Add configuration to koha-conf.xml for message broker

The connection details for the message broker should be configurable.
This patch adds configuration options to koha-conf.xml. If they
are not specified, then default connection details will be used.
Comment 3 David Cook 2020-10-20 01:36:58 UTC
Test plan:

1. Apply the patch
2. koha-plack --restart kohadev
3. Go to http://localhost:8081/cgi-bin/koha/about.pl and check "Message broker" status says "running"
4. Edit /etc/koha/sites/kohadev/koha-conf.xml
5. Add the following:
<message_broker>
    <hostname>localhost</hostname>
    <port>61613</port>
    <username>guest</username>
    <password>guest</password>
    <vhost></vhost>
</message_broker>
6. echo 'flush_all' | nc -q 1 memcached 11211
7. koha-plack --restart kohadev
8. Go to http://localhost:8081/cgi-bin/koha/about.pl and check "Message broker" status says "running"
9. Edit /etc/koha/sites/kohadev/koha-conf.xml
10. Change message broker config to the following:
<message_broker>
    <hostname>badname</hostname>
    <port>40000</port>
    <username>guest</username>
    <password>guest</password>
    <vhost></vhost>
</message_broker>
11. echo 'flush_all' | nc -q 1 memcached 11211
12. koha-plack --restart kohadev
13. Go to http://localhost:8081/cgi-bin/koha/about.pl and check "Message broker" status says "connection failed"
Comment 4 Michal Denar 2020-11-21 12:50:34 UTC
Created attachment 113906 [details] [review]
Bug 26742: Add configuration to koha-conf.xml for message broker

The connection details for the message broker should be configurable.
This patch adds configuration options to koha-conf.xml. If they
are not specified, then default connection details will be used.

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 5 David Cook 2020-11-22 22:12:59 UTC
Thanks, Michal!
Comment 6 Victor Grousset/tuxayo 2020-12-09 16:46:07 UTC
Created attachment 114288 [details] [review]
Bug 26742: Add configuration to koha-conf.xml for message broker

The connection details for the message broker should be configurable.
This patch adds configuration options to koha-conf.xml. If they
are not specified, then default connection details will be used.

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 7 Jonathan Druart 2020-12-10 14:28:25 UTC
1. Wouldn't it make sense to keep "login" instead of "username" in the koha-conf.xml?

2. You are adding a die in the ->connect method, is it really useful?
If $stomp->connect fails we already get an exception.
Comment 8 David Cook 2020-12-11 00:35:42 UTC
(In reply to Jonathan Druart from comment #7)
> 1. Wouldn't it make sense to keep "login" instead of "username" in the
> koha-conf.xml?
> 

I don't think so, since "username" is more standard than "login". It correlates better with the RabbitMQ documentation for instance (https://www.rabbitmq.com/access-control.html), so I think sysadmins and such would find username more user-friendly.

> 2. You are adding a die in the ->connect method, is it really useful?
> If $stomp->connect fails we already get an exception.

I am very sure that $stomp->connect does not raise an exception when it fails without my patch. 

Without my patch, when would you get that exception? When it tries to subscribe? if that's the case, I think that's raising an exception too late.
Comment 9 Jonathan Druart 2020-12-11 08:13:55 UTC
There is "Failed to connect: Error connecting to localhost:61613: Cannot assign requested address at /usr/share/perl5/Net/Stomp.pm line 27.; giving up at /usr/share/perl5/Net/Stomp.pm line 27" that is raised by the constructor.
So you are right, not by $stomp->connect but already by our Koha::BackgroundJob->connect.

It hasn't been advertised by the commit message, I think it need discussion and tests are missing. Can we move this part to its own bug report?
Comment 10 David Cook 2020-12-14 00:54:25 UTC
(In reply to Jonathan Druart from comment #9)
> There is "Failed to connect: Error connecting to localhost:61613: Cannot
> assign requested address at /usr/share/perl5/Net/Stomp.pm line 27.; giving
> up at /usr/share/perl5/Net/Stomp.pm line 27" that is raised by the
> constructor.
> So you are right, not by $stomp->connect but already by our
> Koha::BackgroundJob->connect.
> 

Hmm, did you get that when RabbitMQ was down? What if RabbitMQ is up but the auth details are wrong? I feel like I didn't get an exception in that latter case.

> It hasn't been advertised by the commit message, I think it need discussion
> and tests are missing. Can we move this part to its own bug report?

I suppose so.

I don't really have the stamina at the moment to work on anything, so happy to just have it moved to Failed QA as well.
Comment 11 Jonathan Druart 2020-12-14 09:07:33 UTC
Created attachment 114372 [details] [review]
Bug 26742: Remove die from connect

The change was not advertised and it needs to be done on a separate bug
report (+ test coverage)
Comment 12 Tomás Cohen Arazi 2020-12-14 11:54:29 UTC
I think koha-create should handle this. Also, shouldn't we set a default vhost?
Comment 13 Jonathan Druart 2020-12-14 13:16:08 UTC
Yes, changes to koha-create, makefile.pl and rewrite-config.PL are missing.
Comment 14 David Cook 2021-01-04 22:20:23 UTC
(In reply to Jonathan Druart from comment #13)
> Yes, changes to koha-create, makefile.pl and rewrite-config.PL are missing.

I think that those can be handled by follow-ups like Bug 25674. Bug 26742 is just moving hard-coded values out of code and into configuration files.
Comment 15 David Cook 2021-01-04 22:21:20 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> I think koha-create should handle this. Also, shouldn't we set a default
> vhost?

Not specifying a vhost makes it so that the default vhost is used. You could probably achieve the same thing using a value of "/" to explicitly indicate the default vhost, but I haven't tried that specifically.
Comment 16 Tomás Cohen Arazi 2021-01-04 22:37:54 UTC
(In reply to David Cook from comment #14)
> (In reply to Jonathan Druart from comment #13)
> > Yes, changes to koha-create, makefile.pl and rewrite-config.PL are missing.
> 
> I think that those can be handled by follow-ups like Bug 25674. Bug 26742 is
> just moving hard-coded values out of code and into configuration files.

Go test it :-)
Comment 17 David Cook 2021-01-05 01:14:14 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Go test it :-)

It's on my list! I was tackling more urgent bugs this morning, but I'm getting there ;).
Comment 18 David Cook 2021-01-05 03:56:50 UTC
(In reply to Tomás Cohen Arazi from comment #16)
> Go test it :-)

Looks good overall but failed it because I don't think it should be setting a vhost by default.
Comment 19 Martin Renvoize 2021-03-02 16:50:59 UTC
Created attachment 117533 [details] [review]
Bug 26742: Add configuration to koha-conf.xml for message broker

The connection details for the message broker should be configurable.
This patch adds configuration options to koha-conf.xml. If they
are not specified, then default connection details will be used.

Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-03-02 16:51:03 UTC
Created attachment 117534 [details] [review]
Bug 26742: Remove die from connect

The change was not advertised and it needs to be done on a separate bug
report (+ test coverage)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2021-03-02 16:52:14 UTC
Tested this as part of testing bug 25674.. so adding my additional signoff line :)
Comment 22 Jonathan Druart 2021-03-05 12:51:41 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 23 Fridolin Somers 2021-03-08 15:47:44 UTC
Pushed to 20.11.x for 20.11.04
Comment 24 Andrew Fuerste-Henry 2021-03-14 19:25:46 UTC
Missing dependency, not backported to 20.05