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

Add $evm.ansible_runner method to miq_ae_service #198

Conversation

agrare
Copy link
Member

@agrare agrare commented Jul 12, 2018

Allow ansible playbooks to be invoked from automate methods via
ansible-runner.

The Ansible::Runner class is defined here https://github.com/ManageIQ/manageiq/blob/master/lib/ansible/runner.rb

Depends on: ManageIQ/manageiq#17705

Allow ansible playbooks to be invoked from automate methods via
ansible-runner.
@gmcculloug gmcculloug self-assigned this Jul 12, 2018
@gmcculloug gmcculloug requested a review from mkanoor July 12, 2018 14:52
@gmcculloug
Copy link
Member

@mkanoor I briefly discussed this with @agrare.

My thought: This is a synchronous operations so will be subject to the automate timeout. Should we consider running this through MiqTask and if so what does that mean for the output. I can also see either having a separate method or parameter to control if it runs sync or through a task.

@agrare
Copy link
Member Author

agrare commented Jul 12, 2018

I can also see either having a separate method or parameter to control if it runs sync or through a task.

Yeah we didn't do this originally because it was intended to be used by ruby methods implementing operations which were expected to have already been queued. Given that this may be called directly we should add way to run from the queue.

The caller will be responsible for setting the zone and queue name, is that reasonable for an automate author to provide?

@agrare agrare force-pushed the allow_ansible_runner_to_be_called_from_automate branch 3 times, most recently from 95fad7f to 26e9ff5 Compare July 13, 2018 16:20
@agrare
Copy link
Member Author

agrare commented Jul 13, 2018

Okay @gmcculloug @mkanoor I updated this to create a task and run the playbook over the queue

@agrare agrare force-pushed the allow_ansible_runner_to_be_called_from_automate branch from 26e9ff5 to 788e972 Compare July 13, 2018 17:01
@miq-bot
Copy link
Member

miq-bot commented Jul 13, 2018

Checked commits agrare/manageiq-automation_engine@7000ad5~...788e972 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@mkanoor
Copy link
Contributor

mkanoor commented Jul 13, 2018

@agrare
We have to account for the fact that
- When run as part of a state machine step it would have to be run as a async task
- When run as part of a regular method it would be a synchronous task

We make this distinction when we run the playbook method

Wouldn't it be better to swap out this line and call the runner here

We have all the logic for retries, task management, state machine, automate workspaces that can all be reused, we might have to override the execute method.

@agrare
Copy link
Member Author

agrare commented Jul 13, 2018

@mkanoor I'm good with that, I didn't know that class existed :)

Would you prefer adding a execute_with_runner or an argument to the existing execute method? We'll have to tweak the build_options_hash method if we use the same method

@gmcculloug
Copy link
Member

I have been thinking that it would be better to use a method type instead of calling from an inline ruby method as well. One of the key elements is writing back to the workspace which would not be available when calling from within an inline method.

But the class that @mkanoor referenced is specifically for embedded playbooks and that may be getting ahead of integrating AnsibleRunner into Automate.

I would suggest we get together an have a design session as we need to discuss, environment variables, credentials, where the playbook is located as well as where the output is stored. Along with the state-machine and retry comments above.

Overall, since I am realizing that writing to the workspace will be a big requirement I would prefer we drop this approach and discuss enhancing or creating a new method type to support direct AnsibleRunner calls.

@gmcculloug
Copy link
Member

Closing as the solution needs to be closer to what was described in #198 (comment).

I recommend opening a RFE BZ to track this request with details about the requirements of what parameters need to be supported.

@gmcculloug gmcculloug closed this Jul 17, 2018
@agrare agrare deleted the allow_ansible_runner_to_be_called_from_automate branch July 17, 2018 14:09
@Loicavenel
Copy link

@gmcculloug we do have RFEs on this topic for providers, the RFE is not starting "using ansible runner in Automate" but this PR is the best way to achieve it. Can you guys get a quick "design session" and to identify the work?

@gmcculloug
Copy link
Member

@Loicavenel Sure, we can have a general design discussion around Ansible Runner integration. I'm just trying to make sure I understand the basic requirements of priority of this request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants