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

Use ansible os_tasks to create OOI service and endpoint. #6

Merged
merged 33 commits into from
Apr 3, 2018

Conversation

gwarf
Copy link
Member

@gwarf gwarf commented Mar 31, 2018

Use ansible os_tasks to create OOI service and endpoint.
Reorganize files.
Update ansible, travis and yamllint configuration.
Address complaints from yamllint and ansible-lint.

Copy link
Member

@brucellino brucellino left a comment

Choose a reason for hiding this comment

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

I suggest a few changes, overall.

  1. Stick to the var: value style.
  2. Avoid use of shell module in favour of mysql modules.

There are a few other cosmetic and style suggestions which can wait for future releases.

@@ -6,7 +6,7 @@
# XXX This should be configurable
- name: Add default VOs LSC files
copy:
src: ./lsc/
src: ./templates/lsc/
Copy link
Member

Choose a reason for hiding this comment

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

not to be a pedant, but "copy" takes static files. These should be taken from files/ instead of templates/

I can't find where this is written in the documentation, so perhaps I am remembering incorrectly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@@ -1,5 +1,5 @@
---
- hosts: all
- hosts: packstack
become: yes
become_method: sudo
tasks:
Copy link
Member

Choose a reason for hiding this comment

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

In a later version, I would suggest re-using a role for this - AAROC.certificates or something from egi-qc

Copy link
Member Author

@gwarf gwarf Apr 3, 2018

Choose a reason for hiding this comment

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

Yes we need to have a dedicated role for this, for sure.
@brucellino can you please open an issue for this?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, done in #8

- name: Add VOMS filter
blockinfile:
path: /etc/keystone/keystone-paste.ini
block: |
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't there be a regexp to say where this block should go ? It seems like this may become non-idempotent.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an inifile and this block is indepedant, so this shouldn't be that much a problem, but I will replace it by some inifile tasks, it will definitely be cleaner/safer.

gpgcheck: no
gpgkey: http://repository.egi.eu/community/keys/APPDBCOMM-DEB-PGP-KEY.asc

- name: Install OOI
Copy link
Member

Choose a reason for hiding this comment

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

maintain a common syntax for tasks. instead of one-liner (var=value) you should have one value per line : (var: value)
So instead of yum: name=python-ooi state=installed,

yum:
  name: python-ooi
  state: instaled

This format also makes it easier to catch what has changed when you do a diff (changes are only one per line)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

section: composite:occi_api_11
option: keystone
# yamllint disable-line rule:line-length
value: cors http_proxy_to_wsgi compute_req_id faultwrap sizelimit osprofiler authtoken keystonecontext legacy_v2_compatible occi osapi_compute_app_v21
Copy link
Member

Choose a reason for hiding this comment

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

This is quite a long line ! I would keep this in a var file, so that you can manage it easier, and use {{ keystone_vars }} or something like that. Maybe keep this for when this becomes a role.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I tried to also use the syntax with a > to allow to split the line, don't know exactly why but the task was changing/reloading all the time.
The fact is also that this line could change on different deployment, it's based on another value of the inifile. Need to find later a better way to manage this.

Copy link
Member

Choose a reason for hiding this comment

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

can't you just do a with_items: keystone_vars ? then you would see which one specifically changed

Copy link
Member Author

Choose a reason for hiding this comment

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

I think the problem was more a bug of ini_file with values split using > so not much we can address. As I said this shouldn't really be a static line, let's plan to update this in the roles and keep it like for the time being.

@@ -26,11 +29,7 @@

- name: Install OpenStack {{ os_release }} repository
yum: name=centos-release-openstack-{{ os_release }} state=installed
Copy link
Member

Choose a reason for hiding this comment

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

Use the var: value style
Make sure to keep the values using variables ( {{ }} ) protected by interveted commas :
name=centos-release-openstack-{{ os_release }} -> name: "centos-release-openstack-{{ os_release }}"

@@ -287,32 +286,59 @@
- name: Fix endpoints in keystone database
block:
- name: Look for HTTPS 5000/v2.0 url
shell: mysql keystone -s -N -e 'select url from endpoint where url LIKE "http://%5000/v2.0"' | wc -l
shell: >
Copy link
Member

Choose a reason for hiding this comment

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

AVOID SHELL ! IT IS A WEAK MAN's CRUTCH !
Use something from http://docs.ansible.com/ansible/latest/modules/list_of_database_modules.html#mysql

Copy link
Member Author

@gwarf gwarf Apr 3, 2018

Choose a reason for hiding this comment

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

I do not see a way with this modules to easily interact with the database apart using mysl_db with state: import and a .sql file.
I think that in fact it would be better to use the native os_ modules to edit the endpoints, could you please add a bug for this, as we will have to clarify what all the endpoints could be?

mysql keystone
-e "update endpoint set
url='https://{{ server_fqdn }}:35357/v3' where
url='http://{{hostvars[inventory_hostname]['ansible_default_ipv4']['address']}}:35357/v3';"
when: https_35357_v3.stdout != '0'
Copy link
Member

Choose a reason for hiding this comment

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

@@ -388,3 +414,8 @@

- name: restart neutron
service: name=neutron-server state=restarted

Copy link
Member

Choose a reason for hiding this comment

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

use the var: value style

@@ -15,12 +15,15 @@ python:

install:
- pip install ansible
# - pip install shade shade-ansible
Copy link
Member

Choose a reason for hiding this comment

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

since there's a requirements now, why not simplify install by using it.

Copy link
Member Author

Choose a reason for hiding this comment

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

For the time being not everything is needed for the tests and it will speed things to not install shade that has a lot of dependencies.
To be changed once more through tests will be in place.

Copy link
Member

@brucellino brucellino left a comment

Choose a reason for hiding this comment

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

What a beautiful review.

@brucellino brucellino merged commit 541be5c into EGI-Federation:master Apr 3, 2018
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