-
-
Notifications
You must be signed in to change notification settings - Fork 74
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
EL6/7 and CentOS 6/7 support for st2 and st2repos #66
Conversation
…s' roles Signed-off-by: Matt Oswalt <[email protected]>
st2
and st2repos
rolesst2
and st2repos
roles
st2
and st2repos
rolesst2
and st2repos
roles
Signed-off-by: Matt Oswalt <[email protected]>
st2
and st2repos
rolesst2
and st2repos
roles
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.
👍
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.
Provided some comments below.
Can you check that it 100% works on a real RHEL
not just CentOS
? You can spin up AWS instance to test it.
Apart of that, since you're adding support for more OSes, can you sync up meta
file: https://github.com/Mierdin/ansible-st2/blob/8f1e76b94b974379ead556b67f8fec2ec170338b/roles/st2/meta/main.yml#L8 as well?
notify: | ||
- restart st2 | ||
- include: debian.yml | ||
when: ansible_os_family == 'Debian' |
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.
We can do 1 include without when
condition.
See @humblearner arppoach
include: mongodb_{{ ansible_pkg_mgr }}.yaml |
mongodb_apt.yml
and mongodb_yum.yaml
which looks nicer and cleaner.
It would be better if we follow one standard in all roles for consistency.
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.
Fixed in 6a4467f
yum: | ||
name: st2 | ||
state: latest | ||
disable_gpg_check: yes |
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.
Why we have disable_gpg_check: yes
for stable
and nothing for unstable
in next block? Any specific reason?
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.
No reason, just missed it. Added in 5a1f2e4
|
||
- name: Add keys to keyring | ||
become: yes | ||
apt_key: |
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 think it would be more solid if we check id
(identifier of key) here as well.
It wasn't there before, but it's good time to add it.
See example with Mongo:
id: 42F3E95A2C4F08279C4960ADD68FA50FEA312927 |
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.
Where would I go about finding that?
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.
Nevermind, added in f6ac495
name: stackstorm | ||
description: Stackstorm Repo | ||
file: st2 | ||
baseurl: https://packagecloud.io/StackStorm/stable/el/$releasever/$basearch |
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.
Missing {{ st2_pkg_repo }}
variable here which is responsible for repository we install (stable
/unstable
etc).
See: https://github.com/StackStorm/ansible-st2#variables and existing rule in deb
: https://github.com/StackStorm/ansible-st2/pull/66/files#diff-37045cfaa2d3524070e708be840837adR21
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 in c40a809
yum_repository: | ||
name: stackstorm | ||
description: Stackstorm Repo | ||
file: st2 |
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.
Here is yum repo file I got for CentOS
when adding st2 packagecloud repo:
[vagrant@centos7 ~]$ cat /etc/yum.repos.d/StackStorm_stable.repo
[StackStorm_stable]
name=StackStorm_stable
baseurl=https://packagecloud.io/StackStorm/stable/el/7/$basearch
repo_gpgcheck=1
gpgcheck=0
enabled=1
gpgkey=https://packagecloud.io/StackStorm/stable/gpgkey
sslverify=1
sslcacert=/etc/pki/tls/certs/ca-bundle.crt
metadata_expire=300
A lot of security checks here by PackageCloud. I think we should include more yum_repository
options where it's possible and looks easy.
Additionally, it would be nice to follow the same naming for the file and repo.
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.
So would you prefer I undo 8f1e76b? I was using the script to automatically generate this configuration previously
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'm for hardcoding safe values we can use easily via yum_repository
, without auto-generating them.
@Mierdin Do you have in plans adding Additionally, after reviewing both PRs I believe we should add these platforms in CI first for faster feedback loop, testing and better quality (see #69 and #68). |
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
Signed-off-by: Matt Oswalt <[email protected]>
@armab I don't see anything that would prevent CentOS6 from running, I just haven't tested it. I'll test and update the title accordingly, this should allow both. Also I am on board for modifying CI before merging this. |
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.
So yeah, as in any other PR the requests are similar:
- all platforms we support end-to-end testing
- Xenial (aleady covered by TravisCI)
- Trusty (already covered by TravisCI)
- CentOS6
- CentOS7
- RHEL6 (via AWS)
- RHEL7 (via AWS)
- Ansible Idempotence
Eg. Ansible-playbook re-run should end with the following results:changed=0.*failed=0
- name: Install epel-release repo (RedHat) | ||
become: yes | ||
yum: | ||
name: epel-release |
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.
Will this work on real RHEL6
?
You can test in AWS https://aws.amazon.com/marketplace/pp/B00CFQWLS6
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.
Yes, I will include both EL6 and EL7 in my 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.
So here we install epel-release
and still including epel
role?
Needs improvement to avoid repeating similar tasks.
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.
Fixed in e9ed597
when: st2_version != "stable" | ||
notify: | ||
- restart st2 | ||
- include: "{{ ansible_os_family }}.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.
Since it only about installing package, you can avoid splitting into {{ ansible_os_family }}.yml
here, just use package
module. http://docs.ansible.com/ansible/package_module.html
See solution in st2mistral
by @humblearner
ansible-st2/roles/st2mistral/tasks/main.yml
Lines 10 to 24 in 1426c6d
- name: Install latest st2mistral | |
become: yes | |
package: | |
name: st2mistral | |
state: latest | |
when: mistral_version == "stable" | |
tags: st2mistral | |
- name: Install latest st2mistral | |
become: yes | |
package: | |
name: st2mistral={{ mistral_version }} | |
state: present | |
when: mistral_version != "stable" | |
tags: st2mistral |
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.
Done in 3c1fd14
apt: | ||
name: "{{ item }}" | ||
state: present | ||
update_cache: yes |
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 will break Ansible Idempotence as @bigmstone discovered before, since it will apt-get upgrade
every time.
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.
Removed in 025ed66
yum_repository: | ||
name: stackstorm | ||
description: Stackstorm Repo | ||
file: st2 |
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'm for hardcoding safe values we can use easily via yum_repository
, without auto-generating them.
st2
and st2repos
rolesst2
and st2repos
roles
st2
and st2repos
rolesI am doing this in Ansible now, to work around the reference to '7Server', which would result in a 404 currently. This is documented here: StackStorm#89 I left TODOs in this patch, so will re-eval when StackStorm#89 is addressed
Signed-off-by: Matt Oswalt <[email protected]>
@armab @mstone Okay for reals this time, I have tested on EL6 and EL7, and I also re-ran this on CentOS 6/7 and Ubuntu to make sure those still work. Only outstanding issue is to make sure that I did the right thing here: f5bb4b5 I had to do this to get rid of the error: This was originally implemented in 11129e9 but everything seems to run just fine without it. |
become: yes | ||
package: | ||
name: ca-certificates | ||
state: latest |
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.
[ANSIBLE0010] Package installs should not use latest
The ansible-lint
warning is valid. Use state: installed
where possible.
Installing latest
package version on every playbook re-run is generally a bad practice (especially when ansible works in ansible-pull
mode).
For st2
we use latest
intentionally as option so user can specify st2_version=latest
to have latest StackStorm.
Probably we should change default behavior there someday later since it could be dangerous for default installs.
For everything else the safe way is to upgrade software on the server by running specific playbook which focuses on OS package upgrades (or patches) to have full control and react accordingly if something went wrong.
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.
Just to be clear, for the st2
role, you're saying I should provide a var that controls the parameter passed there, defaulting to latest
?
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.
Just change to state: installed
here, as ansible-lint
suggests.
[ANSIBLE0010] Package installs should not use latest
Sorry for my previous TL;DR comment ^^ 😄 Just wanted to give an explanation.
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.
So the problem I had with this specific line (not other places where I made this linting error) was that unless I updated ca-certificates, I'd get this error when st2mistral
runs:
Failure talking to yum: Cannot retrieve repository metadata (repomd.xml) for repository: StackStorm_stable. Please verify its path and try again
I just checked, and setting this to present
instead of latest
causes the error to come back up, as the package is already installed, so it doesn't do anything.
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.
Makes sense for an exception 👍
This looks like old EL6
version according to http://unix.stackexchange.com/questions/21310/yum-cannot-retrieve-repository-centos-6
If that happens on EL6
only, can you add conditional check to run this task only on EL6?
And we can add skip_ansible_lint
tag for this block.
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 may have a better fix for this, give me a few minutes.
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.
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.
@Mierdin It's not clear how 9c52c7b fixes the issue with outdated ca-certificates
.
According to http://stackoverflow.com/questions/26734777/yum-error-cannot-retrieve-metalink-for-repository-epel-please-verify-its-path and http://unix.stackexchange.com/questions/21310/yum-cannot-retrieve-repository-centos-6 2 possible fixes for outdated EL6
are:
- use
http
instead ofhttps
for package repo - update
ca-certificates
package
How yum -y repolist
fixes that? I'm probably don't have enough context here.
Additionally, can you reproduce this issue? As I understood it happens in some outdated EL 6.5
distro.
I'd say, - we don't support some outdated RHEL6.5 which was released in 2013
.
So after all, - I'm +1 to remove this block completely :) WDYT?
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.
Okay, per our slack conversation, I reverted back to the old approach and am now updating ca-certificates
using the yum
module, in d8714bb
apt: | ||
package: | ||
name: ftp://fr2.rpmfind.net/linux/centos/6/os/x86_64/Packages/libffi-devel-3.0.5-3.2.el6.x86_64.rpm | ||
state: latest |
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.
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.
Fixed in 7fa3a70
name: ftp://fr2.rpmfind.net/linux/centos/6/os/x86_64/Packages/libffi-devel-3.0.5-3.2.el6.x86_64.rpm | ||
state: latest | ||
when: ansible_os_family == "RedHat" and ansible_distribution_major_version == "6" | ||
ignore_errors: yes # Ignore error when already installed |
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.
That's a hack, - generally a sign of improvement. There should be a better way.
Additionally, I'm not sure if this block is idempotent.
Please double check that we don't download ftp://fr2.rpmfind.net/linux/centos/6/os/x86_64/Packages/libffi-devel-3.0.5-3.2.el6.x86_64.rpm
on every playbook re-run.
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.
Per https://docs.stackstorm.com/install/rhel6.html#system-requirements did you check if it's possible to enable server-optional
repository for RHEL6? That would be more elegant and preferable.
So this package dependency will be installed automatically by st2
.
See libffi-devel
is in st2
package requirements: https://packagecloud.io/StackStorm/stable/packages/el/6/st2-2.1.1-1.x86_64.rpm
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 tried that before, got this:
[root@ip-10-0-4-91 ~]# subscription-manager repos --enable rhel-6-server-optional-rpms
Error: rhel-6-server-optional-rpms is not a valid repository ID. Use --list option to see valid repositories.
[root@ip-10-0-4-91 ~]# subscription-manager repos --list
This system has no repositories available through subscriptions.
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.
Additionally, this task does run idempotently, now that I've changed to state: latest
:
TASK [st2 : Install libffi-devel on EL6] ***************************************
ok: [10.0.4.91]
This should also make the ignore_errors
statement pointless, so I have removed it in 100ca71. So as long as you don't prefer the server-optional
approach (and know what I'm doing wrong that is preventing me from installing libffi-devel
that way) then this should work better.
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.
+1
Thanks for explanation!
@@ -16,3 +16,6 @@ galaxy_info: | |||
- 7 | |||
categories: | |||
- system | |||
dependencies: | |||
- role: epel | |||
become: yes |
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.
Using become: yes
when including anything external is usually a bad sign.
The good practice is to become: yes
for every task in a role where it's 100% needed.
Eg. we do privilege escalation selectively per task, not per role.
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.
Ack, fixed in 9988842
@@ -16,3 +16,6 @@ galaxy_info: | |||
- 7 | |||
categories: | |||
- system | |||
dependencies: | |||
- role: epel |
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 have conditional check:
Include epel
only when ansible_os_family == "RedHat"
.
Even despite there is a OS check in epel
role, having condition here as well will look a bit better by logic.
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.
Fixed in 9988842
- st2mistral | ||
- st2 |
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.
Let's keep the original order.
We install st2
first (more significant) and then st2mistral
.
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.
Fixed in 56ed120
when: ansible_os_family == "RedHat" and ansible_distribution_major_version == "6" | ||
ignore_errors: yes # Ignore error when already installed | ||
|
||
- name: Install latest st2 package (stable) |
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.
(stable)
is not exactly what we install here.
Package repository (if you meant that) is controlled via st2_pkg_repo
, see: https://github.com/StackStorm/ansible-st2#variables
So (stable)
note could be removed since it's irrelevant for this particular block.
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.
Yeah, my intention with changing this was for easier debugging. Since both tasks had the same name, it was hard to tell from the output which one actually ran.
This should work better: 3c3f272
name: st2 | ||
state: latest | ||
when: st2_version == "latest" | ||
notify: | ||
- restart st2 | ||
tags: skip_ansible_lint | ||
|
||
- name: Install latest st2 package | ||
- name: Install latest st2 package (not stable) |
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.
Same as https://github.com/StackStorm/ansible-st2/pull/66/files#r97132662
(not stable)
note could be removed.
We can write name: Install pinned st2 package version
or similar explanation instead.
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.
Fixed in 3c3f272
# Fixes "Failure talking to yum: Cannot retrieve repository metadata (repomd.xml) for repository: StackStorm_stable. Please verify its path and try again" when installing st2 | ||
- name: Update ca-certificates package | ||
become: yes | ||
package: |
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.
Also, since this block is part of redhat.yml
, I think it's more expected to use native yum
module for installation, instead of cross-platform package
module.
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.
Fixed in d8714bb
sslverify: yes | ||
|
||
# Fixes "Failure talking to yum: Cannot retrieve repository metadata (repomd.xml) for repository: StackStorm_stable. Please verify its path and try again" when installing st2 | ||
- name: Update ca-certificates package |
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.
Since this fixes specific error for PackageCloud repository, maybe it's worth to move it before the
name: Add ST2 Repo
block?
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.
Reordered in d8714bb
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.
LGTM 👍
Really a lot of work and end-to-end testing here, Thank You @Mierdin for taking it so seriously!
Besides of minor changes remove name: epel-release
and reverting ca-certificates
package we're good to merge.
@armab ack your latest. Everything seems to work on EL6, and I addressed your two outstanding comments, so if those pass, this should be gtg. Will test on EL7 and CentOS6/7 and ubuntu, then merge latest |
The old FTP location was giving me some bad connection issues, this seems more stable
This patch adds support for the following operating systems:
The following roles have been created:
epel
The following roles have been modified (either to support
epel
installation, or to work on EL/CentOS in general):st2
st2repos
mongodb
st2mistral