[V2V] Run the playbook on the appliance with the conversion host in inventory #18613

ghost · 2019-04-02T12:34:09Z

In existing implementation, CloudForms connect to the conversion host via SSH to execute the ansible-runner command. The Ansible playbooks are now shipped in the appliance to keep the playbooks in sync with the backend capabilities. This PR updates the extra_vars hash to match v2v-conversion-host-ansible-1.12 requirements and calls ansible-runner on the CloudForms appliance.

To do so, it generates the runtime directory for the playbook:

the inventory contains the conversion host hostname or IP address
the credentials are passed in temporary files, which are deleted as soon as ansible-runner ends
the extra vars are passed in a file

The ansible-runner command is then run via AwesomeSpawn.run.

Note: this PR is dedicated to Hammer. Another one will be created to use Ansible::Runner. This will require some changes to Ansible::Runner class to allow specifying the inventory and the credentials.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1622728

Depends on: ManageIQ/manageiq-api#535, #18541

Fryguy · 2019-04-02T13:34:52Z

app/models/conversion_host.rb

+  def ansible_playbook(playbook, extra_vars = {}, auth_type = nil)
+    host = hostname || ipaddress
+
+    command = "ansible-playbook #{playbook} --inventory #{host}, --become -vvv"


Should the comma be there after the host?

Yes, that's how you define the inventory from a list of hostnames / IP addresses. Otherwise, it looks for a file.

Fryguy · 2019-04-02T13:35:33Z

app/models/conversion_host.rb

+    command = "ansible-playbook #{playbook} --inventory #{host}, --become -vvv"
+
+    auth = authentication_type(auth_type) || authentications.first
+    command += " --user #{auth.userid}"


Minor, but prefer << over += for String building to avoid intermediate strings.

Fryguy · 2019-04-02T13:36:06Z

app/models/conversion_host.rb


-    connect_ssh { |ssu| ssu.shell_exec(command) }
+    _log.info("FDUPONT - Calling Ansible playbook: #{command}")


Don't forget to remove these FDUPONT debug lines

Yes, and also because they might contain some sensitive data.

ghost · 2019-04-05T13:58:34Z

@miq-bot add-label transformation, bug, hammer/yes, wip

ghost · 2019-04-15T21:24:41Z

@miq-bot remove-label wip

ghost · 2019-04-15T21:36:46Z

@miq-bot add-reviewer @djberg96
@miq-bot add-reviewer @agrare

agrare · 2019-04-16T17:30:30Z

@fdupont-redhat now that this is intended for backport to hammer, I thought we were going to continue to use ansible-playbook on hammer and refactor to use ansible::runner with added inventory support on master.

agrare · 2019-04-16T17:31:22Z

Spoke offline to @fdupont-redhat and there seems to be an issue with becoming root over ssh, it should be working and isn't. I think we should be fixing that instead of replacing with ansible-runner for a backport to fix an issue that isn't understood.

djberg96 · 2019-04-16T17:45:52Z

@agrare @fdupont-redhat What's the issue with root over ssh? Does it happen for both userid/password and private key?

dmetzger57 · 2019-04-16T18:11:53Z

@agrare @fdupont-redhat refactoring to use runner is substantial, I'd prefer not to see such a large refactor (and change for QE) in a Z-Stream and instead understand the failure of something that is fully expected to work (root over ssh) and fix the underlying issue.

ghost · 2019-04-16T19:12:54Z

@agrare What do you think of the initial commit: 2b1a60c ? It was only a start as it didn't update the async task.

agrare · 2019-04-16T19:29:48Z

@fdupont-redhat yeah I'm good with that (minus the logs obviously)

ghost · 2019-04-17T21:13:21Z

@agrare thank you for pointing out my stupidity. I reverted to using ansible-playbook.
@agrare @djberg96 Could you please review again, please ?

agrare

@fdupont-redhat I'm really worried about that MiqTask lookup, and I think updating the task is making the ansible_playbook method more complex than it needs to be. It would be better IMO to just return the awesome_spawn CommandResult to the caller and let it handle updating the task.

Is there any way to get the miq_task_id to these methods calling ansible_playbook so that we don't need to do a loop over every task looking for ones on this conversion host?

app/models/conversion_host.rb

agrare · 2019-04-18T12:32:17Z

app/models/conversion_host.rb

-  def ansible_playbook(playbook, extra_vars = {})
-    command = "ansible-playbook #{playbook} -i #{ipaddress}"
+  def ansible_playbook(playbook, extra_vars = {}, auth_type = nil)
+    task = MiqTask.all.select { |t| t.context_data.present? && t.context_data[:conversion_host_id] == id }.sort_by(&:created_on).last


This is where having proper associations would help, this is bringing back every miq_task and loading up the context_data hash in ruby 😱

We could use a .where clause here instead couldn't we? Totally untested, but this is the general idea:

MiqTask.where.not(:context_data => nil) .where(:context_data[:conversion_host_id]) => id)

I'd rather pass the task_id to the calling method and update the task there so we don't have to do any searching at all

Indeed, we have the task id, so I changed the code to pass it to the *_conversion_host_role methods, that in turn pass it to ansible_playbook.

app/models/conversion_host.rb

agrare · 2019-04-18T12:41:42Z

app/models/conversion_host.rb

  rescue => e
-    _log.error("Ansible playbook '#{playbook}' failed for '#{resource.name}' with [#{e.class}: #{e}]")
+    errormsg = "Ansible playbook '#{playbook}' failed for '#{resource.name}' with [#{e.class}: #{e}]"


Hm if you just raise I don't think there's going to be much meaningful information in the exception, it is just going to be a RuntimeError and you won't see the result stdout.

Good point. And we capture the result.output in the ensure part.

agrare · 2019-04-18T12:42:57Z

app/models/conversion_host.rb

+      ansible_output_name = playbook.split('/').last.split('.').first
+      task&.update_context(task.context_data.merge!(ansible_output_name => result.output))
+    end
+    if !ssh_private_key_file.nil? && File.exist?(ssh_private_key_file.path)


Just a style thing but we usually prefer ssh_private_key_file.present? instead of !.nil?

agrare · 2019-04-18T12:44:05Z

app/models/conversion_host.rb

+      task&.update_context(task.context_data.merge!(ansible_output_name => result.output))
+    end
+    if !ssh_private_key_file.nil? && File.exist?(ssh_private_key_file.path)
+      ssh_private_key_file.close unless ssh_private_key_file.closed?


What case is this handling? If there was an exception writing the file before it was closed? Probably better to handle this with a begin/rescue block right around writing the file then.

In case the SSH private key file creation fails on line 301.

Yeah that's what I thought so in that case I think it'd be better to have a more localized rescue like:

ssh_private_key_file = Tempfile.new('ansible_key') begin ssh_private_key_file.write(auth.auth_key) ensure ssh_private_key_file.close end

app/models/conversion_host.rb

agrare · 2019-04-18T16:27:00Z

app/models/conversion_host.rb

+      ansible_output_name = playbook.split('/').last.split('.').first
+      task&.update_context(task.context_data.merge!(ansible_output_name => result.output))
+    end
+    File.delete(ssh_private_key_file) if ssh_private_key_file.present? && File.exist?(ssh_private_key_file.path)


This can be simplified to ssh_private_key_file&.unlink

agrare · 2019-04-18T16:28:31Z

app/models/conversion_host.rb

+    raise unless result.exit_status.zero?
+  ensure
+    unless result.nil?
+      ansible_output_name = playbook.split('/').last.split('.').first


Pretty sure you can just do File.basename(playbook, ".yml")

>> File.basename("/usr/share/v2v-conversion-host-ansible/playbooks/conversion_host_check.yml", ".yml") => "conversion_host_check"

agrare · 2019-04-18T16:35:42Z

app/models/conversion_host.rb

+    raise unless result.exit_status.zero?
+  ensure
+    task&.update_context(task.context_data.merge!(File.basename(playbook, '.yml') => result.output)) unless result.nil?
+    ssh_private_key_file&.unlink


Nice! much better

miq-bot · 2019-04-18T16:35:45Z

Checked commits fabiendupont/manageiq@2b1a60c~...4d18a99 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

agrare

👍 LGTM

…ybook [V2V] Run the playbook on the appliance with the conversion host in inventory (cherry picked from commit 1a2f07f) https://bugzilla.redhat.com/show_bug.cgi?id=1694229

simaishi · 2019-04-25T13:26:39Z

Hammer backport details:

$ git log -1
commit 14d5c668d3ad382b71ea02f14e833ac3cdc772a6
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Apr 18 12:58:56 2019 -0400

    Merge pull request #18613 from fdupont-redhat/v2v_conversion_host_playbook
    
    [V2V] Run the playbook on the appliance with the conversion host in inventory
    
    (cherry picked from commit 1a2f07f84a71c88dfdcd2cec936b7aba4e9f768b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1694229

jerryk55 · 2019-05-02T17:39:52Z

@fdupont-redhat does this PR obviate the requirement to change to running ansible-runner on the appliance as I was previously investigating? It was my impression that we were calling ansible-playbook on the remote host via ssh (not ansible-runner as mentioned in the top-level description here. Thanks in advance.

agrare · 2019-05-03T12:27:09Z

@jerryk55 we still want to use ansible-runner, this was just intended for backport so we wanted to minimize the changes made

jerryk55 · 2019-05-03T14:31:26Z

@agrare maybe I'm just lost at sea but intending this for backport seems orthogonal to the question. Maybe we can talk face-to-face about this Monday?

agrare · 2019-05-03T14:38:55Z

does this PR obviate the requirement to change to running ansible-runner on the appliance as I was previously investigating?

No it doesn't

It was my impression that we were calling ansible-playbook on the remote host via ssh (not ansible-runner as mentioned in the top-level description here. Thanks in advance.

This change calls ansible-playbook on the appliance passing the target host as inventory instead of calling ansible-playbook on the target host over ssh

jerryk55 · 2019-05-03T14:46:00Z

@agrare I understand that. What had been explained to me by @jameswnl was that the reason I was supposed to look at migrating to ansible-runner, was so that we could run it on the appliance instead of calling ansible-playbook on the target host over ssh. Now that we're calling ansible-playbook on the appliance, what is the justification for migrating to ansible-runner? Thanks.

Fryguy · 2019-05-03T14:56:00Z

@jerryk55 Because writing our own ansible-playbook wrapper goes against using ansible-runner which is itself an ansible-playbook wrapper that does all the right stuff and is owned by the ansible team.

Fryguy reviewed Apr 2, 2019

View reviewed changes

miq-bot changed the title ~~Run the playbook on the appliance with the conversion host in inventory~~ [WIP] Run the playbook on the appliance with the conversion host in inventory Apr 5, 2019

miq-bot added bug hammer/yes v2v wip labels Apr 5, 2019

ghost force-pushed the v2v_conversion_host_playbook branch from 17134e5 to 30cb4db Compare April 5, 2019 14:42

mturley mentioned this pull request Apr 5, 2019

Enable (un-hide) the Conversion Hosts tab of the Settings page ManageIQ/manageiq-v2v#922

Merged

fabiendupont added 8 commits April 15, 2019 10:34

Run the playbook on the appliance with the conversion host in inventory

2b1a60c

Prefer << over +=

ed39704

Use ansible-runner instead of ansible-playbook

4a0adca

Fix rubocop

1c8d12b

Update task context_data only for enable/disable

ec54ccc

Fix rubocop

3d6e5ff

Use source_transport_method

707d7f5

Add specs

2565bc5

ghost force-pushed the v2v_conversion_host_playbook branch from 55e6b92 to 2565bc5 Compare April 15, 2019 21:02

miq-bot changed the title ~~[WIP] Run the playbook on the appliance with the conversion host in inventory~~ Run the playbook on the appliance with the conversion host in inventory Apr 15, 2019

miq-bot removed the wip label Apr 15, 2019

miq-bot requested review from djberg96 and agrare April 15, 2019 21:37

fabiendupont added 3 commits April 17, 2019 16:42

Reverting to using ansible-playbook

b41be35

Use << instead of +=

1137218

Fix rubocop

ea531cd

agrare self-assigned this Apr 18, 2019

ghost changed the title ~~Run the playbook on the appliance with the conversion host in inventory~~ [V2V] Run the playbook on the appliance with the conversion host in inventory Apr 18, 2019

agrare reviewed Apr 18, 2019

View reviewed changes

djberg96 reviewed Apr 18, 2019

View reviewed changes

app/models/conversion_host.rb Show resolved Hide resolved

Pass the task id to ansible_playbook.

0ed6982

djberg96 approved these changes Apr 18, 2019

View reviewed changes

agrare reviewed Apr 18, 2019

View reviewed changes

Simplify code

4d18a99

agrare reviewed Apr 18, 2019

View reviewed changes

agrare approved these changes Apr 18, 2019

View reviewed changes

agrare merged commit 1a2f07f into ManageIQ:master Apr 18, 2019

agrare added this to the Sprint 110 Ending Apr 29, 2019 milestone Apr 18, 2019

simaishi added hammer/backported and removed hammer/yes labels Apr 25, 2019


		connect_ssh { \|ssu\| ssu.shell_exec(command) }
		_log.info("FDUPONT - Calling Ansible playbook: #{command}")

[V2V] Run the playbook on the appliance with the conversion host in inventory #18613

[V2V] Run the playbook on the appliance with the conversion host in inventory #18613

Conversation

ghost commented Apr 2, 2019 • edited by simaishi 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

ghost commented Apr 5, 2019

ghost commented Apr 15, 2019

ghost commented Apr 15, 2019

agrare commented Apr 16, 2019

agrare commented Apr 16, 2019

djberg96 commented Apr 16, 2019

dmetzger57 commented Apr 16, 2019

ghost commented Apr 16, 2019

agrare commented Apr 16, 2019

ghost commented Apr 17, 2019

agrare 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

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

agrare Apr 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Apr 18, 2019

agrare left a comment

Choose a reason for hiding this comment

simaishi commented Apr 25, 2019

jerryk55 commented May 2, 2019

agrare commented May 3, 2019

jerryk55 commented May 3, 2019 • edited Loading

agrare commented May 3, 2019

jerryk55 commented May 3, 2019

Fryguy commented May 3, 2019

ghost commented Apr 2, 2019 •

edited by simaishi

Loading

agrare Apr 18, 2019 •

edited

Loading

jerryk55 commented May 3, 2019 •

edited

Loading