-
Notifications
You must be signed in to change notification settings - Fork 22
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
Reduce artimed extras #172
Conversation
We should rename the artimed_extras role to galaxykickstart |
I think we should remove it completely and move the data managers part to a new data managers role (If we decide we want to keep this functionality) |
I am ok with complete removing and a data_managers role. |
1e144ff
to
8b9e6b6
Compare
Can you outline this in an issue? I am looking at this as well. |
bc3280f
to
0ca05ab
Compare
skip unnecessary restarts.
…ncy dir to galaxy role
therefore move the action up from roles/galaxy.movedata/tasks/import.yml to roles/set_supervisor_env_vars/tasks/main.yml
This startup script takes as argument the inventory to be used and passes the process on to supervisor as PID1 (which allows graceful stopping of processes).
to work without admin api key for galaxy newer than 16.01.
so that an empty list simply causes skipping of the task.
and rename artimed_extras to data_managers.
update galaxy role and switch back to galaxyproject galaxy-extras role
galaxy_tools_admin_user_password to group_vars
b2ede40
to
2cf6689
Compare
@drosofff this is ready for review! |
@@ -1,9 +1,9 @@ | |||
[submodule "roles/galaxyprojectdotorg.galaxy-tools"] | |||
path = roles/galaxyprojectdotorg.galaxy-tools | |||
url = https://github.com/galaxyproject/ansible-galaxy-tools.git | |||
url = https://github.com/mvdbeek/ansible-galaxy-tools.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ansible-galaxy-tools is forked in ARTbio repo. Why not relying on your fork (if rolling back the changeset revision to the one you are providing is required, it is not a problem to me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we're automatically synchronizing this from upstream in a crontab, I'm afraid we may break that synchronization. The changes are already in galaxyproject/ansible-galaxy-tools#31, so as soon as that get's merged in we will have it in the artbio fork as well. (Which I consider to be a backup as discussed in #62)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. when you say in #62
I put this script in my crontab:
https://gist.github.com/mvdbeek/37b77326e6921f963993with this we are updating our forks every 2 hours.
where is running your crontab ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My imac in the lab.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I am sure we can do better for the sustainability of the synchonization
OK, look nice 👍 |
I have this error with the ansible-playbook run on the branch
Apparently an API key issue. However I had to sync / update the submodules that have been changed in the branch and do not guarantee that this is not the problem: from my git session:
|
Yep, this should be 188e7cd136052f1e00efa3d19ffbcd9fe8f29dd5. In the ansible-artimed repo try a git submodule sync && git submodule update. |
Works for me, but I ran this on a new machine. I suspect the problem comes from updating docker.
This is a lockup, can you do a supervisorctl stop docker and see if it passes through? |
|
@drosofff |
It is not:
Please test your commits yourself cause I have no time to do it anymore this week |
I have tested this in vagrant, where it works. If you don't have time to test it this week then don't test it, I will move forward. |
Coming back to this PR after numerous testing and restesting in vagrant, IFB cloud, AWS cloud... first issueHow this branch currently diverge from the gcc2016 branch ? are the modifications in Vagrantfile the only changes ? If yes, the question is shall we merge this branch, or the gcc2016 ? second issueI think that the role
otherwise, the new playbook implies that you have to restart manually Galaxy which is a regression from the current master. I understand that this comes from a notify statement from another role, whose log should be also removed if we restart Galaxy in our playbook. Third (most important) issueThere is a complex issue (at least for me) with the /tmp directory. Probably with the rights of /tmp but could be also its deletion... or its non-deletion. Here is an exemple:
But it can also happen that the restarting of supervisorctl (galaxy:uwsgi) fails due to the absence of this /tmp (probably deleted in a previous playbook round). You can restart just by And last, but not least, I finally figured out why the installation of deseq2 package systematically fails with the new simplified playbook: In summary, the behavior of the /tmp file along the playbook run is not clear to me because I understand that it can be manipulated by several submodules, including our galaxy-tools submodule. But I feel this important /tmp feature is still a bit floppy (not well automated yet). 4th issuethe data_managers role is not crystal clear too me. Is it really an important feature or just a rest from the previous playbook ? Finally, I would really like much to merge this PR (or a PR from gcc2016 if equivalent) with the master, to move forward. But avoiding regression in the automation. As Bjorn said, we are working for usability not for geeks. |
file: dest={{ galaxy_tools_base_dir }}/install_tool_shed_tools.py state=absent | ||
|
||
- name: Remove data manager task file | ||
file: src={{ item }} dest={{ galaxy_tools_base_dir }}/ state=absent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be
file: dest={{ galaxy_tools_base_dir }}/{{ item|basename }} state=absent
otherwise, /tmp is deleted and replaying the playbook fails
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this said, the data_managers role seems obsolete.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, I agree.
Yes, those are the only changes ... I wanted to demo ansible without the automatic provisioning that
I am intentionally removing these things, as they should be done once and once only when the play has finished, from inside the play, not the role (The role should only notify of a necessary restart, while the play implements the restart. I'll add this before merging the PR). All this restarting unnecessarily slows down the playbook, which in return limits the amount of testing we can do in travis. For the third issue, it comes down to #172 (comment) , which should solve most of these problems. The undelrying problem is that the tool installation script is copied and removed, which doesn't really make sense. It should become part of ephemeris, and then we just install ephemeris. |
baa61c9
to
8c2aa1c
Compare
@@ -1,6 +1,9 @@ | |||
- name: start galaxy | |||
supervisorctl: name='galaxy:' state=started | |||
|
|||
- name: restart galaxy | |||
supervisorctl: name='galaxy:' state=restarted | |||
|
|||
- name: restart galaxy handler |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this one is never just "start" ?
38e21e7
to
d3cd54b
Compare
@@ -1 +1 @@ | |||
Subproject commit fda11a2e1fe72c5a425079fcf50369f318287217 | |||
Subproject commit 48750d41f75de22dbd4059ae72fee2e7f6f4673e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should not this updated to 34ec0ce959c8b4a2f95c1c7f19104cffbd73696e ?
Summary of changes: