Backport methods from Automate to conversion_host - part2 #18064

ghost · 2018-10-05T14:16:41Z

This PR is the continuation of #18033 to backport Conversion Host related code to the ConversionHost model. In this PR, we handle the methods that execute through SSH.

Depends on:

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

ghost · 2018-10-05T14:17:16Z

@miq-bot add-label transformation, technical debt, wip

miq-bot · 2018-10-05T14:26:53Z

Checked commit fabiendupont@7de90d6 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 18 offenses detected

app/models/conversion_host.rb

ghost · 2018-10-08T16:20:22Z

@miq-bot add-reviewer agrare

ghost · 2018-10-09T14:19:32Z

@miq-bot remove-label wip

ghost · 2018-10-09T14:20:05Z

@miq-bot add-label hammer/yes

agrare · 2018-10-10T13:12:32Z

app/models/conversion_host.rb

  #  - The number of concurrent tasks has not reached the limit
  def eligible?
-    source_transport_method.present? && check_resource_credentials && check_concurrent_tasks
+    source_transport_method.present? && check_ssh_connection && check_concurrent_tasks


Where is this method going to be called from and how often? Without storing or caching the result of the ssh_connection I'm worried this will be called a lot.

Could we use the existing authentication_check from the linked resource which stores the status in the authentication record?

This will be called by the throttler to identify if hosts are eligible for a specific task. We could limit it to check that credential is present for the resource, and not that the connection is valid. This would move this check to the time we actually interact with the conversion_host: run conversion, role check/enable/disable...

authentication_check exists for Host but not for Vm. And in the case of Vm, the resource relies on credentials associated to the provider.

authentication_check exists for Host but not for Vm

That is just one include AuthenticationMixin away :)

And in the case of Vm, the resource relies on credentials associated to the provider.

So the ssh keys aren't actually associated to the Vm? I thought that is what we were doing for openstack conversion vms.

Nope. I guess it was also a UI challenge as today we don't have authentication for Vm and that would mean integrating a new tab or config dialog in the UI. And if we add authentication to Vm, how does it interact with Embedded Ansible ? IMO, it should be shared with Embedded Ansible, as user would be frustrated to create authentication tokens in 2 different places.

So, it was decided that the private key used for migration would be attached to the OpenStack provider and that somehow the associated public key would be injected in the VM.

For that one, do you want to limit the check to credentials existence ?

agrare · 2018-10-10T13:27:18Z

app/models/conversion_host.rb

  end

  def source_transport_method
    return 'vddk' if vddk_transport_supported
    return 'ssh' if ssh_transport_supported
  end

+  def ipaddress(family = nil)
+    ips = ipaddresses
+    return ips.first unless %w(ipv4 ipv6).include?(family)


How about defaulting family = "ipv4" so we don't have two ways of doing this?

The idea was to filter out invalid family.

You could always raise unless %w(ipv4 ipv6).include?(family) if you're worried about it, but if someone passes in an invalid family then ipaddresses.detect { |ip| IPAddr.new(ip).send("#{family}?") } will do that for you if someone from the future tries ip.ipv9?

But I don't want to raise, rather gracefully fallback to first IP address whatever its family.
What's the common approach in ManageIQ code ? raising or falling back ?

Falling back to the first IP address when you asked for an invalid family is confusing, makes it seem like it succeeded.
We usually prefer to blow up on invalid input rather than hide potential bugs that could creep up later on.

agrare · 2018-10-10T13:27:36Z

app/models/conversion_host.rb

+  def ipaddress(family = nil)
+    ips = ipaddresses
+    return ips.first unless %w(ipv4 ipv6).include?(family)
+    ips.select { |ip| IPAddr.new(ip).send("#{family}?") }.first


.detect is a little bit cleaner than .select {}.first

agrare · 2018-10-10T13:44:07Z

app/models/conversion_host.rb

  private

+  def ipaddresses
+    resource.ipaddresses.unshift(address).unshift(resource.try(:ipaddress)).reject(&:blank?)


This might get a bit cleaner if you did something like

def ipaddress return address if address

above, then this is just resource.ipaddresses...might not even need a method for it

This doesn't check the family of address. But I agree it would be more readable on more lines, like:

ips = resource.ipaddresses ips.unshift(address) if address ips.unshift(resource.try(:ipaddress)) unless resource.try(:ipaddress).nil?

...and it doesn't really need a method.

This wasn't checking the address family either. I mean instead of calling ips = ipaddresses up here just do something like

def ipaddress(family = "ipv4") return address if address && IPAddr.new(address).send("#{family}?") resource.ipaddresses.detect { |ip| IPAddr.new(ip).send("#{family}?") } end

Well, how do you account for resource.ipaddress if the resource is a host ?
That would look like this, no ?

def ipaddress(family = "ipv4") return address if address && IPAddr.new(address).send("#{family}?") begin return resource.ipaddress if resource.ipaddress && IPAddr.new(resource.ipaddress).send("#{family}?") end resource.ipaddresses.detect { |ip| IPAddr.new(ip).send("#{family}?") } end

Why does it have to be different? They both expose resource.ipaddresses

Correct. I had not read the ipaddress method, so I thought the logic was different.

👍 cool cool

…wrapper

agrare

Overall looks good to me, we should see about delegating the credential check to the providers authentication status to prevent the constant ssh checks

…t_from_automate_pt2 Backport methods from Automate to conversion_host - part2 (cherry picked from commit 7d140f5) https://bugzilla.redhat.com/show_bug.cgi?id=1634029

simaishi · 2018-10-12T15:48:02Z

Hammer backport details:

$ git log -1
commit 9a1c355ae8a39dcabf75d06c00e06e8dca861cfc
Author: Adam Grare <agrare@redhat.com>
Date:   Thu Oct 11 14:42:17 2018 -0400

    Merge pull request #18064 from fdupont-redhat/conversion_host_backport_from_automate_pt2
    
    Backport methods from Automate to conversion_host - part2
    
    (cherry picked from commit 7d140f55d6856d35151f94c8f42ead287a227c22)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1634029

miq-bot changed the title ~~Backport methods from Automate to conversion_host - part2~~ [WIP] Backport methods from Automate to conversion_host - part2 Oct 5, 2018

miq-bot added technical debt v2v wip labels Oct 5, 2018

This was referenced Oct 8, 2018

Add support to send data on standard input when executing a command ManageIQ/manageiq-gems-pending#379

Merged

[Cancelled] Add support for VMware to OpenStack transformation ManageIQ/manageiq-content#430

Closed

miq-bot requested a review from agrare October 8, 2018 16:21

ghost mentioned this pull request Oct 9, 2018

Expose ConversionHost and ServiceTemplateTransformationPlanTask methods ManageIQ/manageiq-automation_engine#246

Merged

miq-bot changed the title ~~[WIP] Backport methods from Automate to conversion_host - part2~~ Backport methods from Automate to conversion_host - part2 Oct 9, 2018

miq-bot added hammer/yes and removed wip labels Oct 9, 2018

ghost mentioned this pull request Oct 10, 2018

Add stdin option to shell_exec ManageIQ/manageiq-gems-pending#382

Merged

agrare self-assigned this Oct 10, 2018

agrare reviewed Oct 10, 2018

View reviewed changes

fabiendupont added 7 commits October 11, 2018 07:21

Backport methods from Automate to conversion_host - part2

067f2e1

Add new methods and tests

5550716

Add cluster methods and preflight_check for task

0942012

Fix authentication params for OpenStack. Force sudo to exec virt-v2v-…

e477a60

…wrapper

Use MiqSshUtil gem for SSH related operations

f241aa9

Fix call to shell_exec with stdin

edc88b3

Refactor ipaddress method

fc8cd75

ghost force-pushed the conversion_host_backport_from_automate_pt2 branch from e0bf938 to fc8cd75 Compare October 11, 2018 11:22

agrare approved these changes Oct 11, 2018

View reviewed changes

agrare merged commit 7d140f5 into ManageIQ:master Oct 11, 2018

agrare added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 11, 2018

ghost deleted the conversion_host_backport_from_automate_pt2 branch October 12, 2018 06:44

simaishi added hammer/backported and removed hammer/yes labels Oct 12, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backport methods from Automate to conversion_host - part2 #18064

Backport methods from Automate to conversion_host - part2 #18064

ghost commented Oct 5, 2018 •

edited by ghost

Loading

ghost commented Oct 5, 2018

miq-bot commented Oct 5, 2018

ghost commented Oct 8, 2018

ghost commented Oct 9, 2018

ghost commented Oct 9, 2018

agrare Oct 10, 2018

ghost Oct 10, 2018

agrare Oct 10, 2018

ghost Oct 10, 2018

ghost Oct 10, 2018

agrare Oct 10, 2018

ghost Oct 10, 2018

agrare Oct 10, 2018 •

edited

Loading

ghost Oct 10, 2018

agrare Oct 10, 2018

agrare Oct 10, 2018

agrare Oct 10, 2018

ghost Oct 10, 2018 •

edited by ghost

Loading

agrare Oct 10, 2018

ghost Oct 10, 2018

agrare Oct 10, 2018

ghost Oct 10, 2018

agrare Oct 10, 2018

agrare left a comment

simaishi commented Oct 12, 2018

Backport methods from Automate to conversion_host - part2 #18064

Backport methods from Automate to conversion_host - part2 #18064

Conversation

ghost commented Oct 5, 2018 • edited by ghost Loading

ghost commented Oct 5, 2018

miq-bot commented Oct 5, 2018

ghost commented Oct 8, 2018

ghost commented Oct 9, 2018

ghost commented Oct 9, 2018

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 Oct 10, 2018 • 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

ghost Oct 10, 2018 • edited by ghost 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

agrare left a comment

Choose a reason for hiding this comment

simaishi commented Oct 12, 2018

ghost commented Oct 5, 2018 •

edited by ghost

Loading

agrare Oct 10, 2018 •

edited

Loading

ghost Oct 10, 2018 •

edited by ghost

Loading