Use `ansible-runner` in EmbeddedAnsible #18687

NickLaMuro · 2019-04-24T23:02:26Z

This is home of the integration branch of the changes necessary to fully switch over to using ansible-runner for EmbeddedAnsible.

Currently this is just a clone of the work done over in #18657 as I will be inheriting that that work from @Fryguy going forward, and that effort is far from being fully functioning code and mergeable into master. This branch is created just to pick up where that work stopped, and avoid

Do to the nature of this change, however, most of the changes will need to be applied all at once, so this branch will be a merge point for smaller line items, and in charge of being kept up to date with master.

The above has changed. After some discussion, we are going to ship this PR mostly as is from the original [POC] effort, but make sure specs are passing and in place as much as possible from what originally existed for the "AWX version" of EmbeddedAnsible. While this will mean master might be a bit unstable for a bit, it means we can get a build for others to validate against quicker without any extra leg work to do a separate build.

TODO

Fix rubocop failures
Fix specs
Rebase as needed

Links

Original [POC]: [WIP] [PoC] Replace Embedded Ansible with ansible-runner based solution #18657
Architecture Design Doc: [RFC] EmbeddedAnsible with ansible-runner-based implementation manageiq-design#45

Closes ManageIQ/manageiq-design#45

NickLaMuro

Left some notes about the changes I have done so far, along with some questions I had while adding some of the specs back in. A decent amount of the notes are also in the commit messages.

Let me know your thoughts on these @Fryguy and @carbonin .

NickLaMuro · 2019-06-04T16:20:34Z

app/models/manageiq/providers/embedded_ansible/crud_common.rb

+ :args => args,
+ :class_name => name,
+ :method_name => method_name,
+ :role => "ems_operations", # TODO: This should go to a git_owner


Should this be changed to the "embedded_ansible" role for now, since we plan on reusing that currently before we move to the "federated git" stuff?

What is this actually doing? It probably doesn't really need a role if it's just doing the object CRUD as it's not actually talking to a provider anymore, right?

@carbonin well, wouldn't this be determining which worker is able to pick up a job, and since this is a shared lib, that could include both running the playbooks and cloning them.

For right now, I think ideally we only want to be doing that on the appliances that have the embedded ansible role, correct? (I could be thinking about this wrong as well)

I think the only thing that will go through this method boils down to the raw_create_in_provider invocation which is just creating AR objects.

@Fryguy can you weigh in here?

I think for now, this should be on the embedded ansible role. Part of the issue is that previously all actions went to Tower, so they needed a Task + Notification. So, for now, we still need that even though it's as simple as a DB create. On later refactoring and cleanup though, I agree with @carbonin that we can be more selective about what goes where, perhaps not even using the queue at all.

I think the only thing that will go through this method boils down to the raw_create_in_provider invocation which is just creating AR objects.

I'm not sure, because I didn't get into the other objects like credentials, but you may be right.

Yeah, the "for now" was the big question being asked here. But yeah, I will make the change then.

I think the only thing that will go through this method boils down to the raw_create_in_provider invocation which is just creating AR objects.

Just for some context from EmbeddedAnsible::ConfigurationScriptSource:

def self.raw_create_in_provider(manager, params) params.delete(:scm_type) if params[:scm_type].blank? params.delete(:scm_branch) if params[:scm_branch].blank? transaction { create!(params.merge(:manager => manager)).tap(&:sync) } end

(sync being what does the git operations)

Which, I think based on the previous implementation where tower was involved, this is still done through the queue (since we changed nothing in the UI) because we would have to wait to hear back from tower that everything was cloned properly.

config/settings.yml

NickLaMuro · 2019-06-04T22:52:39Z

...models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb


- include ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::ConfigurationScriptSource
- include ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::TowerApi
+ validates :name, :presence => true # TODO: unique within region?


For now, is this TODO valid if we are still having a single "EmbeddedAnsible" appliance (no federated git)?

I don't think one has anything to do with the other.

This model represents an external git repo so even if we store the repo on multiple appliances in the future (the implication, I imagine, of federated git) there will still only be one record.

I was more concerned about Embedded Ansible being enabled in multiple regions. I would expect users might use the same repo in each, and if they are replicated then at the global you would have conflicts.

Yeah, I understand @Fryguy's concern about replication ... I don't think we can ensure unique repo names any more than we can ensure unique service names, right?

...models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb

app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb

NickLaMuro · 2019-06-07T16:22:37Z

...s/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb

 end
+
+ # TODO: Create a local repo instead... this will probably fail sporatically
+ # using a live repo


My git foo late last night was not up to the task of figuring out how to do this, so I didn't bother...

But my guess is that this would be a better approach going forward.

Lol I didn't see this before the meeting ... carry on.... 🤦‍♂️

Thinking I am going to fix this in a follow, for what it is worth.

NickLaMuro · 2019-06-07T16:23:23Z

...s/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb

+ let(:params) do
+ {
+ :name => "hello_world",
+ :scm_url => "https://github.com/NickLaMuro/ansible-tower-samples"


Note: I used my fork of this instead since I was able to add an "other_branch" for some other specs down below.

For future self...original repo is here: https://github.com/ansible/ansible-tower-samples

NickLaMuro · 2019-06-07T16:25:13Z

...s/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb

+ result = record.update_in_provider update_params
+
+ expect(result).to be_an(described_class)
+ expect(result.scm_branch).to eq("other_branch")


Just noticed that I probably should have done a check on the current git branch... maybe something better to be tested when we implement lib/git_worktree.rb and tested then.

Note: A lot of specs in this file were intentionally written to be high level, since we should tests the guts of the .sync and helper methods more thoroughly when we do switch to rugged.

NickLaMuro · 2019-06-07T16:28:36Z

spec/models/manageiq/providers/embedded_ansible/automation_manager/credential_spec.rb

 end

 before do
 EvmSpecHelper.assign_embedded_ansible_role
 end

- it_behaves_like 'ansible credential'
+ # it_behaves_like 'ansible credential'


So this, as described in the commit, was a punt for now. I did this simply because most of the logic that was being tested in the spec was around the crud_common.rb, which was getting exercised elsewhere, and not the specific implementation details of each credential type.

I can try and put those specs together, but I want to finish fixing up the job_spec.rb first, and since we plan on doing much more work around credentials, I figured this is one place that we can skip tests for now knowing it will need to be revisited before a release.

app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb

carbonin

Mostly just replying to @NickLaMuro's self-review.

...models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb

carbonin · 2019-06-07T18:07:01Z

...models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb


- include ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::ConfigurationScriptSource
- include ManageIQ::Providers::AnsibleTower::Shared::AutomationManager::TowerApi
+ validates :name, :presence => true # TODO: unique within region?


I don't think one has anything to do with the other.

This model represents an external git repo so even if we store the repo on multiple appliances in the future (the implication, I imagine, of federated git) there will still only be one record.

app/models/manageiq/providers/embedded_ansible/automation_manager/credential.rb

carbonin · 2019-06-07T18:51:23Z

app/models/manageiq/providers/embedded_ansible/crud_common.rb

+ :args => args,
+ :class_name => name,
+ :method_name => method_name,
+ :role => "ems_operations", # TODO: This should go to a git_owner


What is this actually doing? It probably doesn't really need a role if it's just doing the object CRUD as it's not actually talking to a provider anymore, right?

app/models/manageiq/providers/embedded_ansible/provider.rb

carbonin · 2019-06-07T18:55:10Z

...s/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source_spec.rb

 end
+
+ # TODO: Create a local repo instead... this will probably fail sporatically
+ # using a live repo


Lol I didn't see this before the meeting ... carry on.... 🤦‍♂️

carbonin · 2019-06-07T18:57:32Z

spec/models/manageiq/providers/embedded_ansible/automation_manager/job/status_spec.rb

+ expect(status.normalized_status).to eq(['failed', 'Stack creation failed'])
+ end
+
+ # TODO: remove or implement? Is canceling something we can handle?


I think you can execute runner in the background and can also stop it https://ansible-runner.readthedocs.io/en/latest/standalone.html#executing-runner-in-the-background

But this can wait for a more targeted fix after this PR I think.

NickLaMuro · 2019-06-07T22:06:49Z

@Fryguy @carbonin Okay, I think I have got the specs and changes requested implemented, so hopefully this goes green. Going to step away, but will probably start on the rebasing effort assuming tests are passing.

I think as part of that, I am going to copy and push this branch as is to another one just so some of the references in the commit messages stick around for the future. Will link it in the PR description.

Update: I lied. Got a few things to fix now.

NickLaMuro · 2019-06-10T18:26:39Z

Okay... NOW tests are working for "realsies" this time...

I will work on rebasing after lunch.

Fryguy · 2019-06-10T18:29:08Z

💚 💚 💚

NickLaMuro · 2019-06-11T00:12:02Z

spec/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_spec.rb

+ end
+
+ # TODO: Determine if we want to have a uniqueness validation to
+ # replicate this functionality, otherwise delete this case.


Also, forgot to bring this up (or maybe I did and it is buried in the review comments here), but curious your thoughts on this comment. Might make sense, might not.

I don't think we really have an analogous case because we're not creating an external reference for the job run. We can probably delete this bit.

NickLaMuro · 2019-06-11T01:56:55Z

@Fryguy @carbonin Okay, I think this is finally good to go now (pending tests passing).

I kinda went a little overboard with making the rebase as small as possible, but I think everything thing should be in place properly. All of my [FIXUP] commits have been merged in where appropriate (and a lot of them had to be split up to be merged into Jason's commits), and I dropped one of Jason's as well:

Fix issue where using the same local_var/method name collides

Since it was basically a [FIXUP] commit as well.

I have also pushed the old version of this branch prior to a rebase here:

master...NickLaMuro:ansible_runner_integration_old

So you can cross reference anything that was here previously if you feel that is necessary.

NickL Note: The specs mostly represent the previous spec definitions pulled in from `it_behaves_like 'ansible configuration_script'`, but updates those to properly handle the code that doesn't inherit from the ansible_tower provider code any more. That said, some behaviour has been dropped since it no longer makes sense. One change specifically that couldn't be supported as part of this change is the validations, since we would need to add those on the model side in ManageIQ (previously handled via tower), and am unsure if that is functionality we want to keep or not. The test is left there as a note (commented out).

NickL Note: Pulls in specs from the manageiq-providers-ansible_tower repo for the job/status.rb, but makes them relevant for the new ansible_runner code. The job/status.rb model also is a bit odd in it's implementation. Mostly trying to keep things as close to how they worked previously, though as mentioned in the comments, `OrchestrationStack::Status` and `MiqTask` don't have a 1-to-1 status comparison, so some "fudging" was necessary to keep things as DRY as possible and be able to use some of the inherited methods from `OrchestrationStack::Status`. Also worth noting is the change of #merge_extra_vars compared to what is done in manageiq-providers-ansible_tower. Here, we don't return a hash of `{:extra_vars => my_merged_vars}` since we would end up just accessing the `:extra_vars` key directly anyway and only that in the only caller `#run`. Also also: The specs added to this commit are from yours truely (NickL), and there is a decent amount here that we still need to implement, but some that my stupid head can't figure out how to test properly currently so I think this is "good enough for government work" for the time being. Left a lot of notes in this one since there are clearly some spots that need to be implemented if we are going to try and keep a parity with the existing functionality.

Using the "Tower" nomenclature is no longer valid when referring to EmbeddedAnsible. ... and yes, "nomenclature" is in my vernacular thanks to "The Big Lebowski" deal with it

This was pedantic and just was bugging me... move along...

We need the parent playbook id in order to find the filesystem path for the playbook later on.

miq-bot · 2019-06-11T15:16:05Z

Checked commits NickLaMuro/manageiq@e574cf6~...93eb27a with ruby 2.3.3, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
42 files checked, 6 offenses detected

Gemfile

❗ - Line 78, Col 16 - Layout/ExtraSpacing - Unnecessary spacing detected.

app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script.rb

⚠️ - Line 24, Col 5 - Rails/ActiveRecordAliases - Use update! instead of update_attributes!.

app/models/manageiq/providers/embedded_ansible/automation_manager/configuration_script_source.rb

⚠️ - Line 31, Col 7 - Rails/ActiveRecordAliases - Use update! instead of update_attributes!.
⚠️ - Line 98, Col 15 - Rails/ActiveRecordAliases - Use update! instead of update_attributes!.

app/models/manageiq/providers/embedded_ansible/automation_manager/job.rb

⚠️ - Line 103, Col 5 - Rails/ActiveRecordAliases - Use update! instead of update_attributes!.

app/models/service_ansible_playbook.rb

⚠️ - Line 170, Col 9 - Rails/ActiveRecordAliases - Use update! instead of update_attributes!.

carbonin

❤️

himdel · 2019-06-12T10:13:49Z

This is breaking ansible credential detail screen

[----] I, [2019-06-12T10:06:17.932383 #1592:2b09d3cc82a8]  INFO -- :   Rendered /home/himdel/manageiq-ui-classic/app/views/ansible_credential/show.html.haml within layouts/application (18.2ms)
[----] F, [2019-06-12T10:06:17.932576 #1592:2b09d3cc82a8] FATAL -- : Error caught: [ActionView::Template::Error] uninitialized constant ManageIQ::Providers::EmbeddedAnsible::AutomationManager::MachineCredential::API_ATTRIBUTES
/home/himdel/manageiq-ui-classic/app/helpers/ansible_credential_helper/textual_summary.rb:15:in `textual_group_options'
/home/himdel/manageiq-ui-classic/app/helpers/textual_summary_helper.rb:67:in `block (2 levels) in process_textual_info'
/home/himdel/manageiq-ui-classic/app/helpers/textual_summary_helper.rb:66:in `collect'
/home/himdel/manageiq-ui-classic/app/helpers/textual_summary_helper.rb:66:in `block in process_textual_info'
/home/himdel/manageiq-ui-classic/app/helpers/textual_summary_helper.rb:65:in `collect'
/home/himdel/manageiq-ui-classic/app/helpers/textual_summary_helper.rb:65:in `process_textual_info'
/home/himdel/manageiq-ui-classic/app/views/layouts/_textual_groups_generic.html.haml:4:in `__home_himdel_manageiq_ui_classic_app_views_layouts__textual_groups_generic_html_haml___2540066593120628741_69884863908380'

And, coincidentally, UI travis - https://travis-ci.org/ManageIQ/manageiq-ui-classic/jobs/544619532#L1936 .

himdel · 2019-06-12T10:28:43Z

@NickLaMuro please let us know how to fix this.

I think re-adding API_ATTRIBUTES to wherever they're needed (and adding tests) sounds like the best solution, but if you're in the process of some more changes, maybe we can just mark the test pending UI side, or add a Error flash message or something...

(There is no ansible credentials screen until then)

(That said, given the changes here, I have no idea where to put those API_ATTRIBUTES, sooo.. your fight now I guess :).)

carbonin · 2019-06-12T13:23:49Z

So you knew about the UI using this but removed it anyway? :) Braino I guess ;)

@himdel yes, we knew the screen was broken, but we also knew that merging this meant all of embedded ansible was fairly broken so it didn't seem too big a deal. We can't reimplement an entire feature in a single PR (even a 1000+ line one). The specs were harder to know about which is obviously the reasoning for having ManageIQ/manageiq-ui-classic#4921 around.

All that said, will #18854 fix the specs? It's just adding constants so I'm comfortable merging it if it will unblock you.

himdel · 2019-06-12T13:40:58Z

All that said, will #18854 fix the specs? It's just adding constants so I'm comfortable merging it if it will unblock you.

It does, thanks! :)

See: https://github.com/ManageIQ/manageiq-gems-pending/blob/hammer/lib/gems/pending/util/vmdb-logger.rb#L106 This will still print as an ERROR level but the backtrace will not be printed to the log as this is a known error condition where the backtrace is not needed. Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1729166 Note, embedded ansible was converted to use runner starting in ivanchuk, therefore this is a hammer only change. See: ManageIQ#18687

miq-bot added wip dependencies labels Apr 24, 2019

miq-bot added the unmergeable label May 17, 2019

NickLaMuro force-pushed the ansible_runner_integration branch from 24c5e13 to 4e70a79 Compare May 21, 2019 13:15

miq-bot added unmergeable and removed unmergeable labels May 21, 2019

NickLaMuro force-pushed the ansible_runner_integration branch from 4e70a79 to 68170a6 Compare May 22, 2019 18:12

miq-bot added unmergeable and removed unmergeable labels May 23, 2019

NickLaMuro force-pushed the ansible_runner_integration branch from 68170a6 to 409ba2b Compare May 30, 2019 20:59

miq-bot removed the unmergeable label May 30, 2019

NickLaMuro force-pushed the ansible_runner_integration branch from 53fdb37 to e051fb0 Compare June 7, 2019 05:49

carbonin mentioned this pull request Jun 7, 2019

Embedded Ansible setup guide ManageIQ/guides#276

Closed

NickLaMuro commented Jun 7, 2019

View reviewed changes

carbonin reviewed Jun 7, 2019

View reviewed changes

carbonin mentioned this pull request Jun 7, 2019

Seed plugin ansible content #18844

Merged

Fryguy added core/embedded ansible enhancement refactoring technical debt labels Jun 7, 2019

NickLaMuro force-pushed the ansible_runner_integration branch from 6a7f1fa to c7445fb Compare June 7, 2019 22:04

NickLaMuro force-pushed the ansible_runner_integration branch from c7445fb to ab05314 Compare June 10, 2019 17:54

Fryguy mentioned this pull request Jun 10, 2019

[WIP] [PoC] Replace Embedded Ansible with ansible-runner based solution #18657

Closed

NickLaMuro commented Jun 11, 2019

View reviewed changes

NickLaMuro force-pushed the ansible_runner_integration branch from f221a8b to 4574a26 Compare June 11, 2019 01:50

NickLaMuro changed the title ~~[WIP] Use ansible-runner in EmbeddedAnsible~~ Use ansible-runner in EmbeddedAnsible Jun 11, 2019

Fryguy and others added 7 commits June 11, 2019 10:14

Add synchronization of playbooks

edadbed

Refactor EmbeddedAnsible CRUD helpers into a module

5856659

s/tower/runner/ in service_ansible_playbook_spec

9feb3a5

Using the "Tower" nomenclature is no longer valid when referring to EmbeddedAnsible. ... and yes, "nomenclature" is in my vernacular thanks to "The Big Lebowski" deal with it

Use proper class method notation

9dac412

This was pedantic and just was bugging me... move along...

Allow playbooks to run using ansible-runner from automate

93eb27a

We need the parent playbook id in order to find the filesystem path for the playbook later on.

NickLaMuro force-pushed the ansible_runner_integration branch from 4574a26 to 93eb27a Compare June 11, 2019 15:14

carbonin self-assigned this Jun 11, 2019

carbonin approved these changes Jun 11, 2019

View reviewed changes

carbonin merged commit eee93ff into ManageIQ:master Jun 11, 2019

carbonin added the changelog/yes label Jun 11, 2019

carbonin modified the milestones: Sprint 113 Ending Jun 10, 2019, Sprint 114 Ending Jun 24, 2019 Jun 11, 2019

himdel mentioned this pull request Jun 12, 2019

Broken travis from other repos ManageIQ/manageiq-ui-classic#4921

Closed

himdel mentioned this pull request Jun 12, 2019

Ansible Credential textual summary - show nicer error when credential class is missing API_ATTRIBUTES ManageIQ/manageiq-ui-classic#5694

Closed

Fryguy mentioned this pull request Jun 13, 2019

[V2V] Modify active_tasks so that it always reloads #18860

Merged

This was referenced Jun 18, 2019

Re-add EmbeddedAnsible::ConfiguredSystem#ext_management_system #18883

Merged

[EmbeddedAnsible::ConfigurationScriptSource] stub REPO_DIR (fix attempt #3) #18894

Merged

[EmbeddedAnsible::Credential] use id for manager_ref #18897

Merged

jrafanie mentioned this pull request Aug 29, 2019

[HAMMER] log_backtrace won't print backtrace for MiqException errors #19229

Merged

lfu mentioned this pull request Oct 25, 2019

Cleanup after Ansible runner. #19383

Merged

Fryguy mentioned this pull request Nov 14, 2019

[RFC] EmbeddedAnsible with ansible-runner-based implementation ManageIQ/manageiq-design#45

Closed

Use ansible-runner in EmbeddedAnsible #18687

Use ansible-runner in EmbeddedAnsible #18687

Conversation

NickLaMuro commented Apr 24, 2019 • edited by carbonin Loading

TODO

Links

NickLaMuro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro Jun 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carbonin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Jun 7, 2019 • edited Loading

NickLaMuro commented Jun 10, 2019

Fryguy commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickLaMuro commented Jun 11, 2019

miq-bot commented Jun 11, 2019

carbonin left a comment

Choose a reason for hiding this comment

himdel commented Jun 12, 2019 • edited Loading

himdel commented Jun 12, 2019 • edited Loading

carbonin commented Jun 12, 2019

himdel commented Jun 12, 2019

Use `ansible-runner` in EmbeddedAnsible #18687

Use `ansible-runner` in EmbeddedAnsible #18687

NickLaMuro commented Apr 24, 2019 •

edited by carbonin

Loading

NickLaMuro Jun 7, 2019 •

edited

Loading

NickLaMuro commented Jun 7, 2019 •

edited

Loading

himdel commented Jun 12, 2019 •

edited

Loading

himdel commented Jun 12, 2019 •

edited

Loading