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.
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.
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.
I forgot about this
(In reply to Tomás Cohen Arazi from comment #3) > I forgot about this Yeah it had slipped my mind as well
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
Created attachment 114809 [details] [review] Bug 25674: Add placeholders for RabbitMQ configuration
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
Created attachment 114811 [details] [review] Bug 25674: Add documentation
I think this should be backported to 20.11.
(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.
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.
Created attachment 115005 [details] [review] Bug 25674: Remove default vhost
David, can you retry with the last patch?
This is blocking bug 26742.
(In reply to Jonathan Druart from comment #12) > Created attachment 115005 [details] [review] [review] > Bug 25674: Remove default vhost +1
Created attachment 117529 [details] [review] Bug 25674: Add placeholders for RabbitMQ configuration Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 117531 [details] [review] Bug 25674: Add documentation Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117532 [details] [review] Bug 25674: Remove default vhost Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
All works as expected, thanks.. Signing off
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.
(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.
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.
And I don't understand how this didn't get into 20.11.
(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.
(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!)
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?
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!
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>
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>
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>
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>
(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 ;)
Thanks David :)
Pushed to master for 21.05, thanks to everybody involved!
Do you think this may be backported to 20.11.x ?
(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!