Put our ansible roles in standard location #159

kbrock · 2021-06-26T03:30:43Z

Put our ansible roles in standard location manageiq-appliance#324

Roles are found in /etc/defalult/roles and /usr/share/ansible/roles.
This information can be found in ansible.cfg and reference/config

This PR puts our roles into the share directory to follow standard practices and not require that we have a custom configuration file updating the path.

The goal here is to remove the root centric configuration file. aka ManageIQ/manageiq#20394

ASIDE: The reuse docs does list fewer directories, but I put in a PR to fix those docs. ansible/ansible#75135

/cc @NickLaMuro @Fryguy

NickLaMuro · 2021-06-28T15:07:52Z

@kbrock Can you add a link to the ansible documentation in the description where it states this is the default path?

Fryguy · 2021-06-28T15:54:47Z

My concern here is that custom roles installed by the user directly or via galaxy would get merged with ours, which might be problematic, particularly during future upgrades. I think one reason we had them separated (and also later in the path) was to avoid that conflict.

kbrock · 2021-06-28T16:07:59Z

@NickLaMuro sorry to do the whole "all things ansible are nick's domain"
But just putting our code into a standard location seems to make the most sense

getting the custom path out of our config is my primary driver here

NickLaMuro · 2021-06-28T16:51:36Z

This is part of what I mentioned in a related PR:

ManageIQ/manageiq#21287 (comment)

But here is my findings (and they aren't great). The rationale for a custom path really wasn't brought up much here when they were introduced in these two PRs:

However, it seems like Fabien did run into an issue when it finally was enabled via:

Add default roles paths to .ansible.cfg manageiq-appliance#252

Which was missing the /usr/share/ansible/roles now because we overwrote it.

My guess any discussion of why this decision was made happened in a meeting between @Fryguy, Nick (Carboni), and myself, and wasn't jotted down anywhere 😩

Based on ManageIQ/manageiq-appliance#252 it seems like ovirt throws its own playbooks there as well, so it is probably fine for us to move our playbooks into that location as well.

That all said, you are going to have to make changes to the .ansible.cfg and seeding of the roles all at the same time so you don't break a build, so all that needs to be fleshed out before merging this piece.

kbrock · 2021-06-28T17:40:16Z

~~I had thought that /share/ was in the path but it looks like user path and /etc/ansible/roles are the only 2 paths in there.~~

I just pushed an update where we are using etc instead of share

kbrock · 2021-06-28T19:06:38Z

ooh, shoot. yea, /usr/share/ansible/roles is in there ==> https://docs.ansible.com/ansible/latest/reference_appendices/config.html#default-roles-path
This is also where oVirt installs them (because it is the default)

~~I'm thinking I should~~ changed it back to share but am open for comment.

kbrock · 2021-06-28T21:28:18Z

That all said, you are going to have to make changes to the .ansible.cfg and seeding of the roles all at the same time so you don't break a build, so all that needs to be fleshed out before merging this piece.

I don't see this as lockstep at all.

This PR will work standalone. Both directories are in our current path. That also goes for if we decide to skip links and just put the directories into the share

If we decide to modify our default ansible role path, then yes. We need to make sure this change goes in first.

NickLaMuro · 2021-06-28T21:51:05Z

@kbrock no, that is just incorrect. We have hard coded content for EmbeddedAnsible that is pointing ansible_consolidated:

kbrock · 2021-06-28T23:32:42Z

@NickLaMuro This is how I saw it working in my mind

Before

rpm build

Seeding moves files from various locations in our repos to manageiq/content/ansible_consolidated.
rpm installs all of manageiq to /var/www/miq/vmdb (including ansible_consolidated/roles)

runtime

Ansible role path has /var/www/miq/vmdb/content/ansible_consolidated/ and /usr/share/ansible/roles.
role is picked up from /var/www/miq/vmdb/content/ansible_consolidated/

After

rpm build

one addition:

all roles in /var/www/miq/vmdb/content/ansible_consolidated/ are also linked from /usr/share/ansible/roles

These files are going to the application root, which ends up in the tarball. But everything happens on the build machine, before the target machine even ecists. So I suppose we can be creative where we put those directores into the tarball, but I for now, I was just thinking we would leave these in their current location. The key difference is adding those (duplicate) symbolic links.

runtime

role can be picked up from either location, since they are basically duplicates

Further changes

Changes not in this PR but the intent is to make this change.

If we remove /var/www/miq/vmdb/content/ansible_consolidated/ from our role path, we will pick up the roles from /usr/share/ansible/roles instead.

If we moved consolidated over to share instead of symbolic linking (probably will never happen) then they will get picked up
from /usr/share/ansible/roles.

NickLaMuro · 2021-06-29T00:18:17Z

I am actually fine with the concept of moving everything into /usr/share, and figured that is what you were suggesting we do.

That said, to do that, we would have to either do a very long an complex process of PRs to do it one by one, or a few in lock step.

The wrinkle in the "one by one" option is it seems you are using it as a justification to hold up:

ManageIQ/manageiq#21287

(an assumption I am currently making)

Which is solving a bug where playbook just do not run on podified... at all.

So with that said, I don't think we really have the luxury to take our time and find a slow solution here. If you want to be pedantic and solve this the #RightWay™ out of the gate, we should do that, otherwise, we probably should just go with the solution in ManageIQ/manageiq#21287 and iterate later. It makes things sloppy for now, but otherwise we are holding others for something that should probably go out now in a patch release.

</rant>

kbrock · 2021-08-13T23:57:18Z

Installing the ansible configuration file into a user's directory is sketchy. It also means we would have to install this to every user that needs to run the library.

Since we only rely upon the ansible configuration file to set the path, it will be simpler to just install our content into the default path and drop our user specific configuration. This is where ovirt also installs the same type of shared code.

My first draft put symbolic from /usr/share to /var/www/manageiq but most (all?) rpms do not rely upon symbolic links and just put the files where they belong/are expected.

@NickLaMuro do we override the format to JSON, or are we relying upon the configuration to set that or are we using a command line configuration to override it?

NickLaMuro · 2021-08-14T16:37:42Z

@NickLaMuro do we override the format to JSON, or are we relying upon the configuration to set that or are we using a command line configuration to override it?

Currently, we use the configuration to override that. However, as discussed, I think it does make sense to change the format in the Ansible::Runner library at runtime, and not make it a global config default.

I can put a PR together to change that configuration so we configure that through command line flags or env vars (I forget which is easier or makes more sense).

kbrock · 2021-08-19T01:55:27Z

update: fixed merge conflict.

@Fryguy we currently do not install other playbooks (or even other rpm packages) on our appliances/pods, so handling those special cases seems very edge case.

Since we run as manageiq user, we need to make sure that these playbooks are in a standard location that all users can access.

Please share alternatives if you don't feel comfortable and I can implement them.

the share directory is the standard location for installing ansible playbooks this puts our playbooks into that location so they can be picked up without the need to modify the load path Move ansible playbooks to standard location This lets us drop custom path definition and remove the need to introduce any configuration changes.

NickLaMuro · 2021-08-19T20:43:57Z

@kbrock I am good with this change conceptually, but is there a way we can’t test this before I blindly approve?

not sure how this one slipped by manageiq user was not able to access the data directory, so none of the lower directories were accessible either. This would blow up while creating the ansible repo (via an error in git_repo not accessing lock directory) this fixes that.

miq-bot · 2021-08-19T23:36:04Z

Checked commits kbrock/manageiq-rpm_build@9ad151c~...45614cb with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. ⭐

Fryguy · 2021-08-20T16:06:22Z

rpm_spec/manageiq.spec.in

@@ -102,6 +102,12 @@ cd %{_builddir}
 #sample configuration files
 %{__mv} %{buildroot}%{app_root}/config/cable.yml.sample %{buildroot}%{app_root}/config/cable.yml

+# Move roles to playbook share


Suggested change

# Move roles to playbook share

# Move roles to ansible share

Fryguy

Discussed with @kbrock and I am good with this approach.

Minor, but I think I'd prefer if the special seeding method did the consolidation directly into /usr/share/ansible/roles, thereby avoiding the extra mv at rpm build time, but that path doesn't exist on Macs so that might introduce complications. For now, I think what's in this PR is the best course of action.

kbrock · 2021-08-20T17:06:22Z

UN-WIP: fixed typo s/playbook/role

Tested rpm and all files are ending up in the right spot.
Tested that ansible is able to find the roles (thanks Nick)

kbrock · 2021-08-20T18:01:37Z

ugh. I fixed one of the playbook typos but not the other.
I'll sneak this typo into another pr.

We are making plenty of changes to build

kbrock added the refactoring label Jun 26, 2021

kbrock mentioned this pull request Jun 26, 2021

[WIP] ANSIBLE_LOCAL_TEMP/ANSIBLE_REMOTE_TEMP vars ManageIQ/manageiq#21287

Closed

kbrock assigned NickLaMuro Jun 28, 2021

kbrock force-pushed the ansible-path branch from dd98389 to 7850d15 Compare June 28, 2021 17:38

kbrock force-pushed the ansible-path branch from 7850d15 to 1ae9d7f Compare June 28, 2021 21:17

kbrock changed the title ~~put our ansible roles in standard location~~ [WIP] put our ansible roles in standard location Jun 30, 2021

kbrock added the wip label Jun 30, 2021

kbrock force-pushed the ansible-path branch from 1ae9d7f to 6e71552 Compare August 13, 2021 23:45

NickLaMuro mentioned this pull request Aug 16, 2021

[Ansible::Runner] Set ANSIBLE_STDOUT_CALLBACK ManageIQ/manageiq#21382

Merged

miq-bot added the unmergeable label Aug 18, 2021

kbrock force-pushed the ansible-path branch from 6e71552 to 5112f9d Compare August 18, 2021 21:28

kbrock force-pushed the ansible-path branch from 5112f9d to 9ad151c Compare August 19, 2021 20:24

kbrock changed the title ~~[WIP] put our ansible roles in standard location~~ Put our ansible roles in standard location Aug 19, 2021

kbrock mentioned this pull request Aug 19, 2021

Put our ansible roles in standard location ManageIQ/manageiq-appliance#324

Merged

kbrock added enhancement New feature or request and removed refactoring labels Aug 19, 2021

kbrock removed unmergeable wip labels Aug 19, 2021

Fryguy reviewed Aug 20, 2021

View reviewed changes

Fryguy approved these changes Aug 20, 2021

View reviewed changes

Fryguy merged commit 0d61495 into ManageIQ:master Aug 20, 2021

Fryguy assigned Fryguy and unassigned NickLaMuro Aug 20, 2021

kbrock deleted the ansible-path branch August 20, 2021 17:10

kbrock mentioned this pull request Aug 25, 2021

fix ansible playbook comment typo #178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put our ansible roles in standard location #159

Put our ansible roles in standard location #159

kbrock commented Jun 26, 2021 •

edited

Loading

NickLaMuro commented Jun 28, 2021

Fryguy commented Jun 28, 2021

kbrock commented Jun 28, 2021

NickLaMuro commented Jun 28, 2021 •

edited by chessbyte

Loading

kbrock commented Jun 28, 2021 •

edited

Loading

kbrock commented Jun 28, 2021 •

edited

Loading

kbrock commented Jun 28, 2021

NickLaMuro commented Jun 28, 2021

kbrock commented Jun 28, 2021 •

edited

Loading

NickLaMuro commented Jun 29, 2021 •

edited

Loading

kbrock commented Aug 13, 2021

NickLaMuro commented Aug 14, 2021

kbrock commented Aug 19, 2021 •

edited

Loading

NickLaMuro commented Aug 19, 2021

miq-bot commented Aug 19, 2021

Fryguy Aug 20, 2021

Fryguy left a comment

kbrock commented Aug 20, 2021

kbrock commented Aug 20, 2021

Put our ansible roles in standard location #159

Put our ansible roles in standard location #159

Conversation

kbrock commented Jun 26, 2021 • edited Loading

NickLaMuro commented Jun 28, 2021

Fryguy commented Jun 28, 2021

kbrock commented Jun 28, 2021

NickLaMuro commented Jun 28, 2021 • edited by chessbyte Loading

kbrock commented Jun 28, 2021 • edited Loading

kbrock commented Jun 28, 2021 • edited Loading

kbrock commented Jun 28, 2021

NickLaMuro commented Jun 28, 2021

kbrock commented Jun 28, 2021 • edited Loading

Before

rpm build

runtime

After

rpm build

runtime

Further changes

NickLaMuro commented Jun 29, 2021 • edited Loading

kbrock commented Aug 13, 2021

NickLaMuro commented Aug 14, 2021

kbrock commented Aug 19, 2021 • edited Loading

NickLaMuro commented Aug 19, 2021

miq-bot commented Aug 19, 2021

Fryguy Aug 20, 2021

Choose a reason for hiding this comment

Fryguy left a comment

Choose a reason for hiding this comment

kbrock commented Aug 20, 2021

kbrock commented Aug 20, 2021

kbrock commented Jun 26, 2021 •

edited

Loading

NickLaMuro commented Jun 28, 2021 •

edited by chessbyte

Loading

kbrock commented Jun 28, 2021 •

edited

Loading

kbrock commented Jun 28, 2021 •

edited

Loading

kbrock commented Jun 28, 2021 •

edited

Loading

NickLaMuro commented Jun 29, 2021 •

edited

Loading

kbrock commented Aug 19, 2021 •

edited

Loading