Add methods to conversion_host to build virt-v2v wrapper options #18033

ghost · 2018-09-28T13:37:41Z

When developing initial code for V2V, the conversion host object didn't exist, so an implementation was made in Automate to interact with oVirt hosts. Now that the ConversionHost class has been implemented, it would be better to move that code into the ConversionHost class, hence this PR.

Due to coupling between conversion host and task, it appeared that some methods from /Transformation/Common/AssessTransformation would also be useful in the ServiceTemplateTransformationPlanTask class, so I added them.

The methods for ServiceTemplateTransformationPlanTask will probably have to be exposed in Automate Model. This is still to be defined.

Depends on:

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

ghost · 2018-09-28T13:46:39Z

@miq-bot add-label transformation, technical debt

agrare · 2018-09-28T14:54:51Z

app/models/conversion_host.rb

+
+  def active_tasks
+    ServiceTemplateTransformationPlanTask.where(:state => 'active').select do |task|
+      task.conversion_host == self


We should move this to a relation so that we don't need to loop over tasks and it looks like .conversion_host is itself a different db query with an options hash.

How do we do that ? Simply add has_many :service_template_transformation_plan_task ?

That would mean a schema change on miq_request_tasks, wouldn't it ? So too late for hammer.

The relation between conversion_host and tasks is probably not that trivial. Currently, the ServiceTemplateTransformationPlanTask class is a sub-sub-class of MiqRequestTask, so the relationship would imply add a conversion_host_id in miq_request_tasks.

From a semantic point of view, this wouldn't make sense, as the conversion host is specific to ServiceTemplateTransformationPlanTask. But technically, it's pretty easy and can be safely ignore by other sub-classes of MiqRequestTask.

What do you think @agrare ?

@fdupont-redhat for starters this ServiceTemplateTransformationPlanTask should be belongs_to :conversion_host and yes we'd need to add a schema reference to the base class to do that.

Then this just becomes ConversionHost has_many :service_template_transformation_plan_tasks (just haha that's a mouthful)

From a semantic point of view, this wouldn't make sense, as the conversion host is specific to ServiceTemplateTransformationPlanTask

yeah that's how STI works, all columns for all sub-classes go in the same table. I wish there was a better way

This way is going to perform really badly compared to a pure db query approach, though I guess no worse than the existing automate approach.

If we need to keep this for backport purposes I'm okay with that but lets put in the schema changes to get this right going forward.

@agrare Here is the PR for schema change: ManageIQ/manageiq-schema#281.
Anyway, we're too late for Hammer, aren't we ?

Yes we had a window after they branched but before the first build but afaik @simaishi has cut a beta-1 tag

Okay @fdupont-redhat now that the schema change is in we can do this with a has_many and if you add a scope on :state => "active" to MiqRequestTask we can do the active tasks with -> { active }

blomquisg · 2018-10-01T15:17:39Z

app/models/conversion_host.rb

+    return 'ssh' if ssh_transport_supported
+  end
+
+  def conversion_options(task)


If you do change the task to a true relationship, then this method could probably be replaced with a number of delegate methods.

See comment to create relationship.

Definitely. What release are we targeting there ? Hammer or I-release ? Given that we're past code freeze for Hammer, I'd say, we're aiming I-release, unless we've got a derogation 😜

Well, the relationship is made so task belongs to conversion_host. Then, I'm not sure how I can delegate from conversion_host to task, as it has many tasks. That would require that the task is passed to the conversion. Is there a way to do it ? I'm a delegation newbie.

agrare · 2018-10-03T15:20:37Z

app/models/conversion_host.rb

+    options.merge(source_provider_options).merge(destination_provider_options)
+  end
+
+  def conversion_options_source_provider_vmwarews_vddk(vm, _storage)


@fdupont-redhat should these below methods be on the conversion_task model? What from the conversion_host do they use?

Indeed, they should be in the task... And that also answers #18033 (comment)

👍 awesome, if there is anything that the task needs from the conversion_host we can delegate those to the conversion_host as @blomquisg said above

…d fingerprint for host

ghost · 2018-10-03T18:08:50Z

@agrare I have added a method to Host class to retrieve the host fingerprint. However, I'm not sure how to stub that, so I wouldn't say no to some help.

agrare · 2018-10-03T18:36:49Z

app/models/host.rb

@@ -500,6 +500,24 @@ def mac_addresses
    hardware.nil? ? [] : hardware.mac_addresses
  end

+  def fingerprint


@fdupont-redhat is this specific to an esx host? If so I think we have a method in VMwareWebService to get this let me look into it.

Yeah we do this here which you can access through the vdlVcConnection.

This returns the actual vddk connection but we could split it up to add an API method to return the thumb print in its own method.

It's probably not necessary to have a dedicated method, even though authentication shouldn't be required to get the thumbprint.

ghost · 2018-10-04T12:20:35Z

@gmcculloug @agrare @JPrause
As ManageIQ/manageiq-schema#281 has been backported to Hammer, I'd expect this PR to be backported too. If you're ok with that, could you add the hammer/yes label, please ?

agrare · 2018-10-04T12:45:05Z

app/models/host.rb

@@ -500,6 +501,10 @@ def mac_addresses
    hardware.nil? ? [] : hardware.mac_addresses
  end

+  def thumbprint_sha1
+    ESXThumbPrint.new(ipaddress, authentication_userid, authentication_password).to_sha1
+  end


👍 much better, this should go to the vmware specific host model though: https://github.com/ManageIQ/manageiq-providers-vmware/blob/master/app/models/manageiq/providers/vmware/infra_manager/host_esx.rb since this doesn't make sense for all hosts.

Done: ManageIQ/manageiq-providers-vmware#328

app/models/service_template_transformation_plan_task.rb

agrare · 2018-10-04T12:53:02Z

app/models/conversion_host.rb

+  has_many :service_template_transformation_plan_tasks, :dependent => :nullify
+
+  def active_tasks
+    service_template_transformation_plan_tasks { active }


You have to define the scope in miq_request_task:

scope :active, -> { where(:state => "active") }

Then here it will look like:

has_many :active_tasks, -> { active }, :class_name => ServiceTemplateTransformationPlanTask

Tried it but it fails during tests.

app/models/service_template_transformation_plan_task.rb

agrare · 2018-10-04T13:05:09Z

app/models/service_template_transformation_plan_task.rb

+      },
+      send("conversion_options_source_provider_#{source_ems.emstype}_#{source_transport_method}", source_storage),
+      send("conversion_options_destination_provider_#{destination_ems.emstype}", destination_cluster, destination_storage)
+    ].inject(&:merge)


The array and inject(&:merge) is more complicated than it needs to be

Can you keep it simple?

conversion_options_source_provider_meth = "conversion_options_source_provider_#{source_ems.emstype}_#{source_transport_method}" conversion_options_dest_provider_meth = "conversion_options_destination_provider_#{destination_ems.emstype}" options = { :source _disks => source_disks.map { |disk| disk[:path] }, :network_mappings => network_mappings } options.merge(send(conversion_options_source_provider_meth, source_storage)) options.merge(send(conversion_options_dest_provider_meth, destination_cluster, destination_storage)) options

It might not be as slick but it takes less brainpower to figure out what is going on if you're just scanning.

I tried to look smart ;). Reverted.

agrare · 2018-10-04T13:10:15Z

app/models/service_template_transformation_plan_task.rb

+          :scheme => destination_ems.security_protocol == 'non-ssl' ? 'http' : 'https',
+          :host   => destination_ems.hostname,
+          :port   => destination_ems.port,
+          :path   => destination_ems.api_version


Not needed for this PR but seeing this twice (once for vmware and once for openstack) I wonder if we should put this as a generic method on the endpoint so you could do something like dest_ems.default_endpoint.uri

Would be nice. Should it be an abstract method in InfraManager overridden in each provider ?

…mented failing scope

agrare · 2018-10-04T16:43:22Z

app/models/conversion_host.rb

+
+  def eligible?
+    return true if concurrent_transformation_limit.nil?
+    puts "Active tasks: #{active_tasks}"


Probably left in accidentally

…asks.

agrare

LGTM

miq-bot · 2018-10-05T12:16:50Z

Checked commits fabiendupont/manageiq@289c53f~...5f683be with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

ghost · 2018-10-05T12:35:18Z

@agrare I've added extra checks for eligibility (and more to come in another PR). However, I'm not really happy with the spec:

I have to stub active_tasks while I'd expect the scope to do the job
I have to stub the authentications relationship for OpenStack provider, while I'd expect that FactoryGirl.create(:authentication_ssh_keypair, :resource => ems) would be enough.

An expert eye would be welcome.

…t_from_automate Add methods to conversion_host to build virt-v2v wrapper options (cherry picked from commit fcd7161) https://bugzilla.redhat.com/show_bug.cgi?id=1634029

simaishi · 2018-10-05T15:57:32Z

Hammer backport details:

$ git log -1
commit 7853eb4e5c305948b29e504e978a26660eebec1e
Author: Adam Grare <agrare@redhat.com>
Date:   Fri Oct 5 08:54:57 2018 -0400

    Merge pull request #18033 from fdupont-redhat/conversion_host_backport_from_automate
    
    Add methods to conversion_host to build virt-v2v wrapper options
    
    (cherry picked from commit fcd7161de1680821293abba4e3c3418f95d37b5b)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1634029

miq-bot added technical debt v2v labels Sep 28, 2018

ghost force-pushed the conversion_host_backport_from_automate branch from e6f0a70 to 8a918cb Compare September 28, 2018 14:23

agrare reviewed Sep 28, 2018

View reviewed changes

blomquisg reviewed Oct 1, 2018

View reviewed changes

agrare self-assigned this Oct 2, 2018

Add methods to conversion_host to build virt-v2v wrapper options

289c53f

ghost force-pushed the conversion_host_backport_from_automate branch from 8a918cb to 289c53f Compare October 3, 2018 07:28

Leverage task / conversion host relationship

02b51cc

ghost force-pushed the conversion_host_backport_from_automate branch from 9eafeab to 02b51cc Compare October 3, 2018 14:41

ghost mentioned this pull request Oct 3, 2018

expose auth_key to automate for AuthTokens ManageIQ/manageiq-automation_engine#239

Closed

agrare reviewed Oct 3, 2018

View reviewed changes

Move conversion_options logic to task rather than conversion_host. Ad…

4e00a40

…d fingerprint for host

agrare reviewed Oct 3, 2018

View reviewed changes

fabiendupont added 2 commits October 4, 2018 07:54

Use vmware_web_service library to get ESX thumbprint

9adf783

Fix rubocop

b2e4f75