Make message broker server settings configurable

[sphuber]

Fixes #1074
Fixes #2685
Replaces #4118

This PR makes the configuration of the message broker (currently RabbitMQ) configurable.
So far I have implemented:

• Create Communicator URL from broker configuration of current profile instead of hardcoded
• Extend the Profile class to contain message broker information
• Add options to verdi quick/setup to configure the message broker
• Add configuration migration to update existing configurations with current broker defaults

The implementation of #4118 was tested by me locally and by LLNL and works for those two use cases.
Things that remain to be done:

• Update the documentation
• This PR already adds the concept of additional URI parameters, but isn't yet fully plumbed through to the profile setup. The use case at LLNL does not require this advanced functionality and it is not clear what use case would in the future and what the requirements would be. But it should be either completely removed, or completely functional. I am tempted towards adding the remaining required functionality, rather than removing what I have already implemented. Decision we are leaving the concept of additional query parameters but don't plumb it into the CLI. Expert users can then manually edit the config for the time being. If it becomes a common use case, this can be integrated into the CLI.
• Do docker images need to be updated? Decision seems that everything should still work fine.
• Should we do additional testing, beyond our unit tests, that using multiple users works? I tried myself locally to create another user with password, but not yet over SSH with SSL.

[sphuber]

@ltalirz and @chrisjsewell after further testing and fine tuning with LLNL, I have updated and cleaned my implementation of making RabbitMQ configuration fully customizable. As you can see, I have three remaining checkboxes with questions that need to be answered and I would like your input.

1. Finalizing implementation of advanced parameters: these are additional parameters that can be added as query parameters in the URI to connect to the RabbitMQ server:

BROKER_VALID_PARAMETERS = [
    'heartbeat',  # heartbeat timeout in seconds
    'cafile',  # string containing path to ca certificate file
    'capath',  # string containing path to ca certificates
    'cadata',  # base64 encoded ca certificate data
    'keyfile',  # string containing path to key file
    'certfile',  # string containing path to certificate file
    'no_verify_ssl',  # boolean disables certificates validation
]

If I understood correctly, LLNL did not even need them, and what is currently exposed through verdi setup was more than enough for their use case. I would add this anyway, for completion sake, the only question is "how"? I am not sure that we should expose a single option for each, and currently they are also stored as a dictionary in the config, unlike the other parameters. However, that would mean that users would have to either update the config file manually (which I don't really like) or we need to have an option that "takes a dictionary". Also this is messy, I would say. Would be possible I guess by allowing a literal string representation of it, but seems sub optimal. But I fear that fully expanding each in its own option would bloat the interface enormously for options that are most likely never necessary to be specified. Thoughts?

2. Docker images: which ones need to be updated and should this be done in this PR. This PR needs to go into v1.4.0 such that LLNL can start to deploy it and don't want to tie it down unnecessarily. If not crucial, I would deconnect it, but not sure if that breaks things. @yakutovicha could you please comment?

3. As you can see, there is very little added testing. I just added explicit tests for the URL generator, but everything else is tested implicitly through the existing verdi quicksetup/setup tests and the fact that we can still successfully install on the CI. In terms of live testing, LLNL have obviously tested this and I have also tested a setup, where I connect from AiiDA on my laptop to RabbitMQ running on my workstation. I would say this is sufficient, especially since I don't really know what other (unit) tests to add.

[ltalirz]

Re 1.
To me, this question seems similar to the one concerning how to represent the caching configuration inside the setup.json (once we merge it).
While I find editing JSON manually a bit tedious, I do find yaml quite comfortable to edit in a text editor, so I wouldn't be too worried about the rabbitmq config not being editable via verdi config at the moment (although I do think we should support this eventually).

As for verdi setup/quicksetup, I agree to follow the current pattern of adding a new option for every parameter.

Re 2.
I'm not quite sure what you mean here - the aiida-core docker image on dockerhub is built automatically both for every released tag as well as for the head of the develop branch (i.e. they can start using aiidateam/aiida-core:latest shortly after you merge this PR).
As long as profiles are set up with the --non-interactive option, nothing should need changing in the Dockerfile, correct?

Re 3.
I guess we can check once we go through the code. @chrisjsewell if you have time to review this, that would be great; otherwise let me know.

[codecov]

Codecov Report

Merging #4341 into develop will increase coverage by 0.03%.
The diff coverage is 88.04%.

@@             Coverage Diff             @@
##           develop    #4341      +/-   ##
===========================================
+ Coverage    79.29%   79.32%   +0.03%     
===========================================
  Files          468      468              
  Lines        34620    34713      +93     
===========================================
+ Hits         27449    27533      +84     
- Misses        7171     7180       +9     

Flag	Coverage Δ
#django	72.94% <82.91%> (+0.04%)	⬆️
#sqlalchemy	72.15% <88.04%> (+0.06%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
aiida/cmdline/commands/cmd_status.py	73.92% <29.42%> (-4.91%)	⬇️
aiida/manage/external/rmq.py	69.90% <95.24%> (+2.43%)	⬆️
aiida/manage/configuration/profile.py	94.68% <97.37%> (+0.79%)	⬆️
aiida/cmdline/commands/cmd_computer.py	81.20% <100.00%> (ø)
aiida/cmdline/commands/cmd_setup.py	95.29% <100.00%> (+0.97%)	⬆️
aiida/cmdline/params/options/commands/setup.py	94.69% <100.00%> (+0.78%)	⬆️
...iida/manage/configuration/migrations/migrations.py	97.37% <100.00%> (+0.71%)	⬆️
aiida/manage/manager.py	95.34% <100.00%> (ø)
aiida/engine/daemon/client.py	72.42% <0.00%> (-1.14%)	⬇️
aiida/transports/plugins/local.py	81.54% <0.00%> (+0.26%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea033b4...be2f20c. Read the comment docs.

[yakutovicha]

1. Docker images: which ones need to be updated and should this be done in this PR.
    This PR needs to go into v1.4.0 such that LLNL can start to deploy it and don't want to tie it down unnecessarily.
    If not crucial, I would deconnect it, but not sure if that breaks things. @yakutovicha could you please comment?

If I understand correctly, the launch of the RabbitMQ server won't be affected by this PR, right? If yes, then aiida-prerequisites image should remain as is. Otherwise, the init script that launches RabbitMQ might be modified.

What concerns the AiiDA profile setup (I think it is the part affected the most), it is rather a question for you. If the default values will be used for the RabbitMQ connection setup when --non-interactive argument is provided, everything should be alright:

aiida-core/.docker/opt/configure-aiida.sh

Lines 28 to 35 in 0f0dda4

	verdi quicksetup \
	--non-interactive \
	--profile "${PROFILE_NAME}" \
	--email "${USER_EMAIL}" \
	--first-name "${USER_FIRST_NAME}" \
	--last-name "${USER_LAST_NAME}" \
	--institution "${USER_INSTITUTION}" \
	--db-backend "${AIIDADB_BACKEND}"

The fact that tests are passing suggests that profile and localhost computer setup went without problems.

[sphuber]

Thanks for looking into this @yakutovicha

If I understand correctly, the launch of the RabbitMQ server won't be affected by this PR, right? If yes, then aiida-prerequisites image should remain as is. Otherwise, the init script that launches RabbitMQ might be modified.

I think it should continue to work. However, what might be needed in the future is a way to configure a RabbitMQ server with non-default settings. But then again, thinking about it, one shouldn't use the aiida-prerequisites because that comes with the RabbitMQ server inside, correct?

What concerns the AiiDA profile setup (I think it is the part affected the most), it is rather a question for you. If the default values will be used for the RabbitMQ connection setup when --non-interactive argument is provided, everything should be alright:

Yes, I intentionally provided defaults, such that with --non-interactive nothing changes and user is not even prompted for these broker settings.

[sphuber]

Re 1.
To me, this question seems similar to the one concerning how to represent the caching configuration inside the setup.json (once we merge it).
While I find editing JSON manually a bit tedious, I do find yaml quite comfortable to edit in a text editor, so I wouldn't be too worried about the rabbitmq config not being editable via verdi config at the moment (although I do think we should support this eventually).

As for verdi setup/quicksetup, I agree to follow the current pattern of adding a new option for every parameter.

Thanks for the comment. In that case, I will leave the implementation as is, such that the code does understand these additional parameters, but for now they will have to be added manually in the config and the profile setup CLI just doesn't include the option to set it.

[yakutovicha]

I think it should continue to work. However, what might be needed in the future is a way to configure a RabbitMQ server with non-default settings. But then again, thinking about it, one shouldn't use the aiida-prerequisites because that comes with the RabbitMQ server inside, correct?

Yes, the RabbitMQ server remains inside the container.

In case non-default settings should be added, we can implement that with some system variables or with configuration files. I think it shouldn't be a problem.

[chrisjsewell]

Hey yeh sorry, went down a bit of a rabbit-hole setting this up (pun intended)

But tested with a docker-compose network (the proper way to use docker 😉), and all works well:

Dockerfile-local

FROM phusion/baseimage:0.11
CMD ["/sbin/my_init"]
RUN /sbin/install_clean \
    python3 \
    python3-dev \
    python3-pip \
    python3-distutils \
    python3-setuptools \
    python3-venv \
    git \
    build-essential \
    gcc
RUN pip3 install setuptools wheel
COPY requirements/requirements-py-3.6.txt .
RUN pip3 install -r requirements-py-3.6.txt 
COPY . aiida-core
RUN cd aiida-core; pip3 install --no-deps .
ENV TZ Europe/Zurich
# RUN verdi quicksetup -n --config aiida-core/docker-quicksetup.yml
# RUN verdi daemon start
# RUN verdi status

docker-quicksetup.yml

profile: tester
email: myemail@host.com
first_name: chris
last_name: sewell
institution: EPFL
db_engine: postgresql_psycopg2
db_backend: django
db_host: database
db_port: 5432
su_db_username: pguser
su_db_password: password
su_db_name: template1
db_name: aiida_db
db_username: aiida
db_password: password
broker_protocol: amqp
broker_username: guest
broker_password: guest
broker_host: messaging
broker_port: 5672

docker-compose.yml

version: '3.4'

services:

  database:
    image: postgres:12.3
    container_name: aiida-database
    environment:
      POSTGRES_USER: pguser
      POSTGRES_PASSWORD: password
    volumes:
      - "aiida-postgres-db:/var/lib/postgresql/data"

  messaging:
    image: rabbitmq:3.8.3-management
    container_name: aiida-rmq
    environment:
      RABBITMQ_DEFAULT_USER: guest
      RABBITMQ_DEFAULT_PASS: guest
    volumes:
      - "aiida-rmq-data:/var/lib/rabbitmq/"

  core:
    image: aiidalocal/test1
    build:
      context: .
      dockerfile: ./Dockerfile-local
    container_name: aiida-core
    depends_on:
      - database
      - messaging

volumes:
  aiida-postgres-db:
    name: aiida-postgres-db
  aiida-rmq-data:
    name: aiida-rmq-data

docker-compose up --build -d 
docker exec -it aiida-core  /bin/bash
docker-compose down -v 

[chrisjsewell]

Should we do additional testing, beyond our unit tests... but not yet over SSH with SSL
Finalizing implementation of advanced parameters

I guess, without adding something like pytest-docker to the test suite, it's difficult to include "proper" testing of this.
But with the above files, its at least a bit easier to test this works manually

[sphuber]

Thanks a lot for giving this a test @chrisjsewell . In that case, I think it is fine to merge this. Could you please approve?

[chrisjsewell]

Yep had a read down and all looks good to me 👍

[sphuber]

Yep had a read down and all looks good to me +1

Uhm, I might be losing it, but it seems you requested changes instead of approving it 😅

[chrisjsewell]

nope definitely you 🙄