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

Added support and tests on RHEL systems #18

Merged
merged 2 commits into from
Jan 24, 2020

Conversation

espenfl
Copy link
Collaborator

@espenfl espenfl commented Jan 14, 2020

fixes #14
fixes #16

Added support and tests on RHEL systems

@espenfl espenfl force-pushed the add_centos8_test branch 6 times, most recently from c5dc8bd to b6220e4 Compare January 15, 2020 09:09
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

just some questions but I guess this is anyhow work in progress
I'll look at the travis logs as well now

meta/main.yml Outdated Show resolved Hide resolved
molecule/default/molecule.yml Outdated Show resolved Hide resolved
molecule/default/requirements.yml Outdated Show resolved Hide resolved
@espenfl espenfl force-pushed the add_centos8_test branch 6 times, most recently from df7683d to 437ac3c Compare January 22, 2020 12:33
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

some comments looking through...

tasks/aiida-core.yml Outdated Show resolved Hide resolved
tasks/aiida-daemon.yml Outdated Show resolved Hide resolved
tasks/aiida-daemon.yml Outdated Show resolved Hide resolved
tasks/aiida-deps-debian.yml Show resolved Hide resolved
tasks/aiida-prepare.yml Outdated Show resolved Hide resolved
@espenfl espenfl force-pushed the add_centos8_test branch 2 times, most recently from f2284fc to c7b01d8 Compare January 22, 2020 13:41
@espenfl espenfl changed the title Added support for tests on CentOS 8 and moved locale override Added support and tests on RHEL systems Jan 24, 2020
@ltalirz ltalirz self-requested a review January 24, 2020 12:38
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

thanks a lot @espenfl - I think this generalization of the role is very useful and I hope it will help admins installing AiiDA going forward - we'll just need to get people to use it!

Most of my requests are minor.
There's of course the general issue that these pgrep checks for running processes are not resilient and should be replaced by alternatives where possible. Anyhow, we can open an issue for that and do it later.

tasks/aiida-daemon.yml Outdated Show resolved Hide resolved
tasks/aiida-daemon.yml Outdated Show resolved Hide resolved
tasks/aiida-deps-debian.yml Show resolved Hide resolved
tasks/aiida-deps-debian.yml Outdated Show resolved Hide resolved
tasks/aiida-deps-debian.yml Outdated Show resolved Hide resolved
tasks/aiida-deps-rhel.yml Show resolved Hide resolved
tasks/aiida-daemon.yml Show resolved Hide resolved
pip:
name:
- pip~=19.3
- numpy==1.16.4
# Flask is required for the AiiDA REST API
Copy link
Member

Choose a reason for hiding this comment

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

That should be taken care of by installing the rest extra of the AiIDA package

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Somehow it did not. Removing now and then we will see how it goes.

Copy link
Member

Choose a reason for hiding this comment

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

looking at where the git repo is installed, it seems to me we're simply not installing the rest extra

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha, tests are running fine now so that was probably added when one of the plugins was enabled. Maybe. By the way do we want to have the plugin installation enabled by default?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I see... I'm fine with disabling it by default.

This aiida role really started as a role to install the aiida environment for the Quantum Mobile. Eventually I might move out the plugins/codes/examples to a separate role.

Copy link
Member

Choose a reason for hiding this comment

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

As long as it is in this role, however, we should keep testing it (i.e. please keep it in the molecule playbook)

tasks/aiida-rest.yml Show resolved Hide resolved
tasks/aiida-rest.yml Outdated Show resolved Hide resolved
@espenfl espenfl force-pushed the add_centos8_test branch 2 times, most recently from 531ae46 to ca144d9 Compare January 24, 2020 14:45
@espenfl
Copy link
Collaborator Author

espenfl commented Jan 24, 2020

e.g. set cache_valid_time: 3600 for 1h

Not sure why that was marked as resolved. Should we not just update the cache on every trigger of ansible? I find that maybe the safest. Maybe not?

I'm surprised this does not trigger the idempotency check, since the default value is zero
Maybe it checks the result and would fail only if there actually were some updates?

Yes I suspect so, but have not actually checked.

@ltalirz
Copy link
Member

ltalirz commented Jan 24, 2020

Thanks for your work @espenfl - let me know once you think it's ready to be merged.
I see no barriers to merging this.

@espenfl
Copy link
Collaborator Author

espenfl commented Jan 24, 2020

Happy to help. Let us merge.

@ltalirz ltalirz self-requested a review January 24, 2020 16:50
Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

let's go!

@ltalirz ltalirz merged commit 9dfbe38 into marvel-nccr:master Jan 24, 2020
@ltalirz
Copy link
Member

ltalirz commented Jan 24, 2020

What did I say about the apt_cache ;-)
https://travis-ci.org/marvel-nccr/ansible-role-aiida/jobs/641438706#L758

Anyhow, I'll fix this as well as the rest extra

@espenfl
Copy link
Collaborator Author

espenfl commented Jan 24, 2020

Great. Did not consider that ;) In fact this is an example of tweaking to pass a test which yields a behavior you not necessarily
want in actual use (in case apt have some updates put in during the time slot that is). Of course the benefits as a whole of idempotence outweights this I guess. Thanks for fixing the remaining bits.

@ltalirz
Copy link
Member

ltalirz commented Jan 24, 2020

In fact this is an example of tweaking to pass a test which yields a behavior you not necessarily want in actual use (in case apt have some updates put in during the time slot that is).

I'm not sure that is true. Note that even if new versions of a package appear in between ansible runs, our role will actually not update them (default of apt: is state:present, not state:latest).

If I understand the ansible philosophy correctly, then in the "strict interpretation" apt-get update should actually be executed only once and never again.
The way to handle apt package updates via ansible is through setting up an automatic cron job (we do this on Materials Cloud servers for security updates) - it's not the job of the provisioner.
Of course, when you change the role and require a different minimal version of a package, then this should result in a changed state.

@espenfl
Copy link
Collaborator Author

espenfl commented Jan 24, 2020

Good points.

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.

fix role on CentOS and Fedora remove cloud_platform variable
2 participants