Use ansible-runner instead of ansible-playbook #17688
Conversation
Ladas
force-pushed
the
use_ansible_runner
branch
from
761c3af
to
ab12c3d
Compare
lib/ansible/runner.rb
Outdated
| JSON.parse(result.output) | ||
| end | ||
|
|
||
| def prepare_tmp_structure | ||
| base_dir = "/tmp/ansible-runner/" | ||
| mkdir(base_dir) |
There was a problem hiding this comment.
Would it make sense to use Dir.mktmpdir here and just delete the whole directory after we're done?
There was a problem hiding this comment.
Maybe? Not sure, I was thinking to just put it to /tmp and the OS will take care of cleaning it.
|
@Fryguy @agrare @gtanzillo ok, so I did a workaround for the output. So now it's in a usable shape. So we just need |
lib/ansible/runner.rb
Outdated
| end | ||
|
|
||
| def mkdir(base_dir) | ||
| Dir.mkdir(base_dir) unless Dir.exist?(base_dir) |
There was a problem hiding this comment.
Can you just rescue EEXIST and save the conditional? Saves a stat() system call 😄
There was a problem hiding this comment.
I think I will just keep this, with hope that I can delete this once the mentioned bug is fixed in runner :-) ansible/ansible-runner#88
But you are right, before I was sharing the dir for many runs, so it would be faster, now I shouldn't even need it, since new empty temp dir is always created
lib/ansible/runner.rb
Outdated
| result = AwesomeSpawn.run!(ansible_command, :env => env_vars, :params => [{:extra_vars => JSON.dump(extra_vars)}, playbook_path]) | ||
| JSON.parse(result.output) | ||
| Dir.mktmpdir("ansible-runner") do |base_dir| | ||
| mkdir(base_dir + '/project') # without this, there is a silent fail of the ansible-runner command see https://github.com/ansible/ansible-runner/issues/88 |
There was a problem hiding this comment.
File.join would be better than + '/'
lib/ansible/runner.rb
Outdated
| parsed_stdout << JSON.parse(line) | ||
| rescue => e | ||
| _log.warn("Couldn't parse JSON from: #{e}") | ||
| end |
There was a problem hiding this comment.
I can see this being a good method by itself if we want to parse&return stdout and stderr but we can extract this when/if we go with that.
Ladas
force-pushed
the
use_ansible_runner
branch
from
80a2352
to
49e9d29
Compare
Use ansible-runner instead of ansible-playbook Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1599798
Use mktmpdir to clean up ansible-runner files after the run
Deal with output which is JSON per line and for now, deal with possible non json lines.
Add a result object so we can easily extend it in the future
Return integer return code, with possibility that the rc file is missing.
Ladas
force-pushed
the
use_ansible_runner
branch
from
7f06d07
to
b075243
Compare
lib/ansible/runner.rb
Outdated
|
|
||
| result = AwesomeSpawn.run!(ansible_command(base_dir), :env => env_vars, :params => [{:cmdline => "--extra-vars '#{JSON.dump(extra_vars)}'", :playbook => playbook_path}]) | ||
|
|
||
| return Ansible::Runner::Response.new(:return_code => return_code(base_dir), :stdout => result.output, :stderr => result.error) |
There was a problem hiding this comment.
Do you need the return here? I think it will actually throw an exception:
>> a = Dir.mktmpdir("test") { |base_dir| return base_dir + "hello" }
LocalJumpError: unexpected return
There was a problem hiding this comment.
I don't see the exception but I can remove it
| @return_code = return_code | ||
| @stdout = stdout | ||
| @parsed_stdout = parse_stdout(stdout) | ||
| @stderr = stderr |
There was a problem hiding this comment.
Is stderr not going to be json encoded?
There was a problem hiding this comment.
we might do that later, right now it always returns "", so I am not sure what the format will be. Might be the same as the stdout
Remove extra return and reformat
Ladas
force-pushed
the
use_ansible_runner
branch
from
8cc9dde
to
5e9738f
Compare
|
Checked commits Ladas/manageiq@e214bf2~...5e9738f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 lib/ansible/runner.rb
lib/ansible/runner/response.rb
|
Use ansible-runner instead of ansible-playbook
|
@Ladas I see this is merged and the BZ is in POST. We don't need ansible-runner RPM? |
|
@simaishi we do still need the RPM, it wasn't necessary for this PR but you are right that the BZ should have depended on having the RPM installed. |
|
@simaishi it does, but it is not in production, so people testing this are just installing it manually via pip (where the 1.0.4 is already released) |
Use ansible-runner instead of ansible-playbook
Depends on:
Implements:
https://bugzilla.redhat.com/show_bug.cgi?id=1599798