-
Notifications
You must be signed in to change notification settings - Fork 14
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
idr050: modifications for OMERO 5.4 #103
Conversation
ansible/idr-centos-init.yml
Outdated
state: present | ||
|
||
- name: centos-init | Update locate db | ||
command: updatedb |
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 isn't idempotent
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.
I hadn't added previously. @mtbc : was the behavior on idr-experimental ok for you? If so, I could drop. Otherwise, the only thing that occurs to me is to setup in cron.
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.
I did an updatedb
manually there. Waiting for crond
to run /etc/cron.daily/mlocate
would be fine.
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.
I don't think we've set up cron anywhere else for the IDR. @manics : do you foresee any issues?
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.
Does updating the mlocate db have any performance impact?
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.
For the short window where it's running, a slight one. In general, no, I don't think locate is going to be a significant burden on any of our instances.
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.
mlocate's significantly better than predecessors in my limited experience.
- name: centos-init | Install locate | ||
become: yes | ||
yum: | ||
name: mlocate |
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.
What's the background for this? The PR ome/openmicroscopy#5684 in the description doesn't mention it.
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.
It's not an OMERO-specific request but an "IDR devops" request. i.e. for the likes of Mark to better find things on these servers.
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.
From a "devops" perspective we shouldn't really need this since it means we haven't defined/documented where everything is. Does it need to be on every server, or just the OMERO ones?
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.
OMERO and Database was the initial requirement.
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.
Can you make it specific to those then? idr-00-preinstall.yml
is run by everything- publicidr, ftp, and the k8s vae.
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.
Added idr_debug
after discussion in #idr
.
Note: also dropped the cron section since @mtbc pointed out that a daily
job is installed by default.
ansible/idr-omero-readonly.yml
Outdated
config set -- omero.sessions.session_manager ome.services.sessions.InMemorySessionManagerImpl | ||
config set -- omero.security.event_provider ome.security.basic.InMemoryEventProvider | ||
config set -- omero.cluster.read_only true | ||
config set -- omero.pixeldata.memoizer.dir.local /tmp/BioFormatsCache |
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.
Does this mean the NFS share https://github.com/IDR/deployment/blob/IDR-0.4.7-1/ansible/idr-omero-readonly.yml#L24 can either be removed or made read-only?
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.
It could be made read-only, but this is actually one of the places where I think we've gone too far. Specifically, I want to be able to write to the global BioFormatsCache from the read-only servers. I don't need to yet since we're not bumping our Bio-Formats version, but we will need to soon.
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.
@mtbc suggests in https://trello.com/c/q5L39XPZ/5-bioformatscache-is-expecting-read-write that if I omit this line, then the old behavior will be used. Likely worth testing.
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.
Pushed a removal of omero.pixeldata.memoizer.dir.local
Relaunched travis with gh-109 |
This reverts commit 1dcfc48.
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.
One optional suggestion
yum: | ||
name: mlocate | ||
state: present | ||
when: idr_debug | default(false) | bool |
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.
Optional: If it's going to make things easier in future set a default in https://github.com/IDR/deployment/blob/IDR-0.4.7-1/ansible/group_vars/all.yml
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.
I thought about that, but since I haven't rolled it out elsewhere, I think I'll leave this as "internal API" for the moment. But agreed, having a section in all.yml
with "expert" variables will be useful.
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.
When we move to Ansible 2.5 we can use the new never
tag instead http://docs.ansible.com/ansible/latest/user_guide/playbooks_tags.html
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.
Nice.
openstack_volume_device: /dev/vdb | ||
openstack_volume_source: "{{ idr_volume_dockermanager_data_src }}" | ||
|
||
|
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 is fine for now, but we'll need to clean up all references to these servers in idr-proxy at some point.
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.
Agreed. I couldn't tell where I'd conflict with your work, so tried to go for the simplest thing possible.
From my point-of-view as soon as all 0.4.8 is merged, this can be merged. |
👍 What are the remaining steps to getting this merged and deployed on |
Happy to run on prod50 today from a merged stated if there are no objections. |
see: ome/openmicroscopy#5684