Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker: Start/Stop daemon only when profile exist #6257

Merged

Conversation

unkcpz
Copy link
Member

@unkcpz unkcpz commented Jan 20, 2024

The container will try to start the daemon. But in #6252, we allowed user to skip the default profile setup, this will fail the daemon start and pop an traceback message to container shell.
In this commit, I run verdi profile show to check if the profile exist, and start daemon only if it can start.

@unkcpz unkcpz force-pushed the fix/6252/start-daemon-when-profile-set branch 6 times, most recently from f8ea609 to 355da79 Compare January 21, 2024 11:23
@unkcpz unkcpz changed the title Docker: Start daemon only when profile exist Docker: Start/Stop daemon only when profile exist Jan 23, 2024
@unkcpz unkcpz force-pushed the fix/6252/start-daemon-when-profile-set branch 3 times, most recently from 7baf82a to 1efa938 Compare January 24, 2024 10:19
@unkcpz unkcpz requested a review from sphuber January 24, 2024 10:21
Comment on lines 6 to 10
# Supress rabbitmq version warning
# If it is built using RMQ version > 3.8.15 (as we did for the `aiida-core-with-services` image) which has the issue as described in
# https://github.com/aiidateam/aiida-core/wiki/RabbitMQ-version-to-use
# We explicitly set consumer_timeout to disabled in /etc/rabbitmq/rabbitmq.conf
verdi config set warnings.rabbitmq_version False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this be run every time the container is started? Guess this would have to be setup just once, shouldn't it? When the profile is set up the first time, just like you set verdi config set warnings.development_version False

Copy link
Member Author

@unkcpz unkcpz Feb 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think you are right. It is not proper to put it here (at least it is not proper to put it in two places) which will execute every time the daemon start.
But if the config is put in the aiida-prepare.sh as it did before. Then if the SETUP_DEFAULT_AIIDA_PROFILE is set to false, the config will also be skipped and users will always see two warnings if the profile is not set by default.

I think more properly is that the verdi config can run independent of profile, but I think it is not the case at the moment correct? I run verdi config set warnings.rabbitmq_version False without profile, I got:

Traceback (most recent call last):
  File "/opt/conda/lib/python3.10/site-packages/aiida/common/extendeddicts.py", line 49, in __getattr__
    return self[attr]
KeyError: 'profile'

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you try with the --global flag? By default verdi config set will apply it to the current default profile, which if that doesn't exist of course fails. But maybe setting globally will work.

Copy link
Member Author

@unkcpz unkcpz Feb 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found an issue #6273 of setting config without profile.
After fixing it (see #6274, hope I did it correct.), I can move the config set out from profile setting and --global will passed automatically if the profile not exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the verdi config set --global bug solved, I guess this can now move forward, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --global is not needed, if the profile not exist the config set will fallback to set as global. I update the PR, ready for the review.

@unkcpz unkcpz force-pushed the fix/6252/start-daemon-when-profile-set branch from 1efa938 to f572fab Compare February 8, 2024 16:38
Copy link
Contributor

@sphuber sphuber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One final request @unkcpz (just a rename), then we can merge this

@unkcpz unkcpz force-pushed the fix/6252/start-daemon-when-profile-set branch from 2ecb24b to c73bcfa Compare February 8, 2024 17:22
unkcpz and others added 4 commits February 8, 2024 18:23
Daemon stop as well

[pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

f-m

iron docker CI format
@unkcpz unkcpz force-pushed the fix/6252/start-daemon-when-profile-set branch from c73bcfa to da08a5b Compare February 8, 2024 17:23
@sphuber sphuber merged commit 0a5b200 into aiidateam:main Feb 8, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants