Bug 25674 - Add RabbitMQ options to koha-create
Summary: Add RabbitMQ options to koha-create
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (command-line installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 26742
Blocks:
  Show dependency treegraph
 
Reported: 2020-06-04 13:01 UTC by Tomás Cohen Arazi
Modified: 2022-12-12 21:24 UTC (History)
5 users (show)

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


Attachments
Bug 25674: Add placeholders for RabbitMQ configuration (1.03 KB, patch)
2021-01-04 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25674: Adapt koha-create (5.32 KB, patch)
2021-01-04 16:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25674: Add documentation (2.28 KB, patch)
2021-01-04 16:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25674: Remove default vhost (722 bytes, patch)
2021-01-11 09:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25674: Add placeholders for RabbitMQ configuration (1.09 KB, patch)
2021-03-02 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25674: Adapt koha-create (5.38 KB, patch)
2021-03-02 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25674: Add documentation (2.34 KB, patch)
2021-03-02 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25674: Remove default vhost (787 bytes, patch)
2021-03-02 16:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25674: Add placeholders for RabbitMQ configuration (1.15 KB, patch)
2021-03-05 04:10 UTC, David Cook
Details | Diff | Splinter Review
Bug 25674: Adapt koha-create (5.44 KB, patch)
2021-03-05 04:10 UTC, David Cook
Details | Diff | Splinter Review
Bug 25674: Add documentation (2.43 KB, patch)
2021-03-05 04:10 UTC, David Cook
Details | Diff | Splinter Review
Bug 25674: Remove default vhost (841 bytes, patch)
2021-03-05 04:10 UTC, David Cook
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-06-04 13:01:58 UTC
Now that we plan to use RabbitMQ as message broken for a task queue manager, we need to be able to set the RabbitMQ options on instance creation.

It should fallback to localhost, etc.
Comment 1 David Cook 2020-06-04 23:45:28 UTC
I'm wondering if we should put connection information (ie hostname, port, username, password, vhost) in koha-conf.xml or have a dedicated file. I feel like koha-conf.xml is simpler for the developer, but a dedicated file would be easier for a sysadmin/operations.
Comment 2 David Cook 2020-06-04 23:45:35 UTC
I feel like this is also the right bug report for discussing virtual hosts VS instance-prefixed queue names. (Virtual hosts add sysadmin/operations overhead, but offers a better logical separation message data.) (Note that I haven't found a public method for deleting RabbitMQ queues. Deleting a virtual host might be easier from a RabbitMQ cleanup perspective.) I think requiring a sysadmin to set up RabbitMQ isn't necessarily a deal breaker, so long as we have appropriate error-handling and alert Koha admin users that they can't use certain functionality until RabbitMQ has been properly configured. This is my current inclination.
Comment 3 Tomás Cohen Arazi 2020-10-20 01:18:22 UTC
I forgot about this
Comment 4 David Cook 2020-10-20 01:21:21 UTC
(In reply to Tomás Cohen Arazi from comment #3)
> I forgot about this

Yeah it had slipped my mind as well
Comment 5 David Cook 2020-10-20 01:37:38 UTC
I haven't added koha-create options, but Bug 26742 does make the message broker connection configurable, so it's a good first step.

Would love a sign off on it :D
Comment 6 Tomás Cohen Arazi 2021-01-04 16:18:52 UTC
Created attachment 114809 [details] [review]
Bug 25674: Add placeholders for RabbitMQ configuration
Comment 7 Tomás Cohen Arazi 2021-01-04 16:18:57 UTC
Created attachment 114810 [details] [review]
Bug 25674: Adapt koha-create

This patch adds the following option switches to koha-create:

--mb-host
--mb-port
--mb-user
--mb-pass
--mb-vhost

They are used to override the default values on instance creation.

To test:
1. Play with the several option switches and verify that the resulting
   koha-conf.xml contains the expected data.
=> SUCCESS: It works as expected
2. Sign off :-D
Comment 8 Tomás Cohen Arazi 2021-01-04 16:19:02 UTC
Created attachment 114811 [details] [review]
Bug 25674: Add documentation
Comment 9 Tomás Cohen Arazi 2021-01-04 16:19:43 UTC
I think this should be backported to 20.11.
Comment 10 David Cook 2021-01-05 03:54:46 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> Created attachment 114810 [details] [review] [review]
> Bug 25674: Adapt koha-create
> 
> This patch adds the following option switches to koha-create:
> 
> --mb-host
> --mb-port
> --mb-user
> --mb-pass
> --mb-vhost
> 
> They are used to override the default values on instance creation.
> 
> To test:
> 1. Play with the several option switches and verify that the resulting
>    koha-conf.xml contains the expected data.
> => SUCCESS: It works as expected
> 2. Sign off :-D

My test plan:

1. cp debian/templates/koha-conf-site.xml.in /etc/koha/koha-conf-site.xml.in
2. cp debian/scripts/koha-create /usr/sbin/koha-create
3. koha-create --create-db --mb-host test1 --mb-port 8000 --mb-user test2 --mb-pass test3 --mb-vhost test4 test5
4. vi /etc/koha/sites/test5/koha-conf.xml
5. Note the following block near the bottom of the file:

<message_broker>
  <hostname>test1</hostname>
  <port>8000</port>
  <username>test2</username>
  <password>test3</password>
  <vhost>test4</vhost>
</message_broker>

6. koha-create --create-db test6
7. vi /etc/koha/sites/test6/koha-conf.xml
8. Note the following block near the bottom of the file:

<message_broker>
  <hostname>localhost</hostname>
  <port>61613</port>
  <username>guest</username>
  <password>guest</password>
  <vhost>koha_test6</vhost>
</message_broker>

I'm failing QA because it sets the feature defaults to a vhost of "koha_test6" when the vhost should be set to nothing by default. 

If someone tried to use koha-create --create-db test6 without any other intervention, the RabbitMQ functionality wouldn't work out of the box.
Comment 11 David Cook 2021-01-05 03:56:15 UTC
Likewise, I don't think we should create a vhost automatically, since there's no guarantee that the RabbitMQ server will be on the same host. While one could argue we could use the management HTTP API, that's not enabled out of the box either.
Comment 12 Jonathan Druart 2021-01-11 09:04:28 UTC
Created attachment 115005 [details] [review]
Bug 25674: Remove default vhost
Comment 13 Jonathan Druart 2021-01-29 06:53:34 UTC
David, can you retry with the last patch?
Comment 14 Jonathan Druart 2021-02-02 15:49:17 UTC
This is blocking bug 26742.
Comment 15 Tomás Cohen Arazi 2021-02-02 15:59:13 UTC
(In reply to Jonathan Druart from comment #12)
> Created attachment 115005 [details] [review] [review]
> Bug 25674: Remove default vhost

+1
Comment 16 Martin Renvoize 2021-03-02 16:50:11 UTC
Created attachment 117529 [details] [review]
Bug 25674: Add placeholders for RabbitMQ configuration

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize 2021-03-02 16:50:14 UTC
Created attachment 117530 [details] [review]
Bug 25674: Adapt koha-create

This patch adds the following option switches to koha-create:

--mb-host
--mb-port
--mb-user
--mb-pass
--mb-vhost

They are used to override the default values on instance creation.

To test:
1. Play with the several option switches and verify that the resulting
   koha-conf.xml contains the expected data.
=> SUCCESS: It works as expected
2. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2021-03-02 16:50:17 UTC
Created attachment 117531 [details] [review]
Bug 25674: Add documentation

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2021-03-02 16:50:20 UTC
Created attachment 117532 [details] [review]
Bug 25674: Remove default vhost

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2021-03-02 16:51:23 UTC
All works as expected, thanks..

Signing off
Comment 21 Martin Renvoize 2021-03-02 16:55:01 UTC
With bug 26363 adding a systemd unit file.. it raises the question of should this patch also do something for that.. Fridolin quite rightly raised questions about what user the unit worker should be running under.
Comment 22 David Cook 2021-03-02 22:26:46 UTC
(In reply to Martin Renvoize from comment #21)
> With bug 26363 adding a systemd unit file.. it raises the question of should
> this patch also do something for that.. Fridolin quite rightly raised
> questions about what user the unit worker should be running under.

I don't think so. I think that should be hard-coded to "%i-koha" like the rest of package Koha. 

Certainly a good catch by Frido though. I've opened bug 27844 for it. I'll do a patch for it quickly.
Comment 23 Tomás Cohen Arazi 2021-03-03 00:15:00 UTC
I clearly don't understand the need for the unit file,  but it looks like the process shouldn't be running as root

https://git.koha-community.org/Koha-community/Koha/src/branch/master/debian/scripts/koha-worker#L70

The shipped unit files are not linked to koha-create so I assume they are just not used. They are shipped just in case someone wants to use them.

The current scripts make it easy not to depend on systemd (KTD) and things like koha-docker rely on supervisord.

My feeling is we need a good rfc to discuss the ultimate goal, and each involved detail. Adding a hard dependency on systemd specifics can have some impact, that needs to be pondered.

I support this work, I just would like more clear goals.
Comment 24 Tomás Cohen Arazi 2021-03-03 00:16:09 UTC
And I don't understand how this didn't get into 20.11.
Comment 25 David Cook 2021-03-03 01:30:03 UTC
(In reply to Tomás Cohen Arazi from comment #23)
> I clearly don't understand the need for the unit file,  but it looks like
> the process shouldn't be running as root
> 
> https://git.koha-community.org/Koha-community/Koha/src/branch/master/debian/
> scripts/koha-worker#L70
> 

You're mixing up a couple things here. The "daemon" based koha-worker runs as <instance>-koha. It's all good. The issue is only with the systemd unit file from a different bug report. The systemd unit file would run the background worker as root (without the patch from 27844). I've already confirmed it in testing.

> The shipped unit files are not linked to koha-create so I assume they are
> just not used. They are shipped just in case someone wants to use them.
> 

I think you're right.

> The current scripts make it easy not to depend on systemd (KTD) and things
> like koha-docker rely on supervisord.
> 

supervisord would just be an alternative to systemd for process management. You shouldn't be using "daemon" with supervisord if I recall correctly. Can you elaborate on that point?

As for koha-testing-docker... I think it would benefit from different process management anyway. (For what it's worth, I use ubi-init for a legacy app on Red Hat 8, which uses systemd in containers, and it works quite well for a multi-service container.)

> My feeling is we need a good rfc to discuss the ultimate goal, and each
> involved detail. Adding a hard dependency on systemd specifics can have some
> impact, that needs to be pondered.
> 
> I support this work, I just would like more clear goals.

I agree. I've been saying for many years that we need goals and a long-term project roadmap instead of just stumbling along all separately. I think Jonathan has done great to make per-release road maps, but it would be good for there to be some discussion and consensus on where the project is going technically-speaking.

Personally, I would like the community to pursue containerization, but I don't really see it happening. There's too much technical debt, moving parts, container inexperience, inertia, etc.

I find systemd to be much better than "daemon" when working on a full VM. 

When it comes to containers, we should be using a different approach to process management anyway. If we want to do multiple services in a container, we could use systemd (see bug 27844). Or we could use supervisord. 

I know that supervisord is considered a "moderately heavy-weight" approach according to https://docs.docker.com/config/containers/multi-service_container/, but koha_koha_1 is a heavy-weight container.

Anyway, I think we're off topic at this point anyway.
Comment 26 David Cook 2021-03-03 05:02:21 UTC
(In reply to Martin Renvoize from comment #21)
> With bug 26363 adding a systemd unit file.. it raises the question of should
> this patch also do something for that.. 

Ohh... I think I understand you now. 

Yeah good question. I think at this point probably not, but only because of koha-testing-docker (and I guess koha-docker). 

The systemd stuff certainly needs more thinking about overall... probably not ready to switch to it.

(Apologies Tomas for misunderstanding you too!)
Comment 27 Martin Renvoize 2021-03-03 09:21:55 UTC
Yes, lets move the overarching systemd/containerisation/whatever conversation elsewhere.

Any chance you could progress this bug with a QA stamp David... seems like we've discussed the only point I wasn't sure about and come to a reasonable conclusion so I reckon it's probably ready to go?
Comment 28 David Cook 2021-03-05 04:09:35 UTC
My test plan:

1. cp debian/templates/koha-conf-site.xml.in /etc/koha/koha-conf-site.xml.in
2. cp debian/scripts/koha-create /usr/sbin/koha-create
3. koha-create --create-db --mb-host test1 --mb-port 8000 --mb-user test2 --mb-pass test3 --mb-vhost test4 test5
4. vi /etc/koha/sites/test5/koha-conf.xml
5. Note the following block near the bottom of the file:

<message_broker>
  <hostname>test1</hostname>
  <port>8000</port>
  <username>test2</username>
  <password>test3</password>
  <vhost>test4</vhost>
</message_broker>

6. koha-create --create-db test6
7. vi /etc/koha/sites/test6/koha-conf.xml
8. Note the following block near the bottom of the file:

<message_broker>
  <hostname>localhost</hostname>
  <port>61613</port>
  <username>guest</username>
  <password>guest</password>
  <vhost></vhost>
</message_broker>

testing 4 commit(s) (applied to 5038a57 '39 Bug 26742: Remove die from connect')

Processing files before patches
|========================>| 1 / 1 (100.00%)
Processing files after patches
|========================>| 1 / 1 (100.00%)

 OK     debian/docs/koha-create.xml

Processing additional checks OK!
Comment 29 David Cook 2021-03-05 04:10:43 UTC
Created attachment 117776 [details] [review]
Bug 25674: Add placeholders for RabbitMQ configuration

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

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 30 David Cook 2021-03-05 04:10:47 UTC
Created attachment 117777 [details] [review]
Bug 25674: Adapt koha-create

This patch adds the following option switches to koha-create:

--mb-host
--mb-port
--mb-user
--mb-pass
--mb-vhost

They are used to override the default values on instance creation.

To test:
1. Play with the several option switches and verify that the resulting
   koha-conf.xml contains the expected data.
=> SUCCESS: It works as expected
2. Sign off :-D

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

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 31 David Cook 2021-03-05 04:10:51 UTC
Created attachment 117778 [details] [review]
Bug 25674: Add documentation

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

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 32 David Cook 2021-03-05 04:10:55 UTC
Created attachment 117779 [details] [review]
Bug 25674: Remove default vhost

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

Signed-off-by: David Cook <dcook@prosentient.com.au>
Comment 33 David Cook 2021-03-05 04:16:44 UTC
(In reply to Martin Renvoize from comment #27)
> Any chance you could progress this bug with a QA stamp David... 

My inbox is swamped but I couldn't say no ;)
Comment 34 Martin Renvoize 2021-03-05 06:52:41 UTC
Thanks David :)
Comment 35 Jonathan Druart 2021-03-05 12:51:37 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 36 Fridolin Somers 2021-03-08 15:49:15 UTC
Do you think this may be backported to 20.11.x ?
Comment 37 Tomás Cohen Arazi 2021-03-08 16:11:23 UTC
(In reply to Tomás Cohen Arazi from comment #24)
> And I don't understand how this didn't get into 20.11.

My answer is 'yes'. But let's hear others!