Add a validation for conversion hosts #18135

ghost · 2018-10-26T14:09:44Z

When a no conversion host is configured in the destination EMS, the migration tasks will stall because the throttler won't be able to assign a conversion host. To avoid hanging requests, we should check that at least one conversion host is available in the destination EMS. This PR implements a validation method in ServiceTemplateTransformationPlanRequest, that will be exposed in Automate.

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

ghost · 2018-10-26T14:44:42Z

@miq-bot add-label transformation, enhancement, hammer/yes
@miq-bot add-reviewer agrare

JPrause · 2018-10-26T18:29:10Z

@miq-bot add_label blocker

agrare · 2018-10-26T18:56:31Z

app/models/service_template_transformation_plan_request.rb

@@ -15,6 +15,16 @@ def source_vms
    vm_resources.where(:status => [ServiceResource::STATUS_QUEUED, ServiceResource::STATUS_FAILED]).pluck(:resource_id)
  end

+  def validate_conversion_hosts
+    transformation_mapping.transformation_mapping_items.each do |item|
+      next unless %w(EmsCluster CloudTenant).include?(item.source_type)


You can do transformation_mapping.transformation_mapping_items.reject { |item| %w(EmsCluster CloudTenant).include?(item.source_type).each ...

agrare · 2018-10-26T18:57:45Z

app/models/service_template_transformation_plan_request.rb

+  def validate_conversion_hosts
+    transformation_mapping.transformation_mapping_items.each do |item|
+      next unless %w(EmsCluster CloudTenant).include?(item.source_type)
+      ems = item.destination_type.constantize.find(item.destination_id).ext_management_system


This can just be item.destination.ext_management_system, :destination is polymorphic so rails basically does _type.find(_id) for you

agrare · 2018-10-26T19:04:56Z

app/models/service_template_transformation_plan_request.rb

+    transformation_mapping.transformation_mapping_items.each do |item|
+      next unless %w(EmsCluster CloudTenant).include?(item.source_type)
+      ems = item.destination_type.constantize.find(item.destination_id).ext_management_system
+      conversion_hosts = ConversionHost.all.select { |ch| ch.ext_management_system == ems }


Hm this is unfortunate, something like ConversionHost.select { |ch| ch.ext_management_system == ems } would be nicer but we should really add an ems_id to ConversionHost if we need to do this sort of thing so we don't need to load every conversion host.

I think it'd be better to add associations to ext_management_system like:

has_many :host_conversion_hosts, :through => :hosts, :source => :conversion_host has_many :vm_conversion_hosts, :through => :vms, :source => :conversion_host def conversion_hosts host_conversion_hosts + vm_conversion_hosts end

miq-bot · 2018-10-30T14:15:51Z

Checked commits fabiendupont/manageiq@0868e07~...8342dbb 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 · 2018-10-30T14:26:43Z

@jameswnl can you review?

ghost · 2018-10-30T15:41:07Z

@miq-bot add-reviewer jameswnl

jameswnl · 2018-10-30T18:30:07Z

app/models/service_template_transformation_plan_request.rb

@@ -15,6 +15,13 @@ def source_vms
    vm_resources.where(:status => [ServiceResource::STATUS_QUEUED, ServiceResource::STATUS_FAILED]).pluck(:resource_id)
  end

+  def validate_conversion_hosts
+    transformation_mapping.transformation_mapping_items.select { |item| %w(EmsCluster CloudTenant).include?(item.source_type) }.each do |item|
+      return false if item.destination.ext_management_system.conversion_hosts.empty?


how about

transformation_mapping.transformation_mapping_items.select do |item| %w(EmsCluster CloudTenant).include?(item.source_type) end.all? do |item| item.destination.ext_management_system.conversion_hosts.present? end

Nice syntax. Will try to remember it.

jameswnl · 2018-10-30T19:42:07Z

spec/models/service_template_transformation_plan_request_spec.rb

+      let(:host) { FactoryGirl.create(:host, :ext_management_system => FactoryGirl.create(:ext_management_system, :zone => FactoryGirl.create(:zone))) }
+      let(:conversion_host) { FactoryGirl.create(:conversion_host, :resource => host) }
+
+      it { expect(request.validate_conversion_hosts).to eq(false) }


use be false instead of eq(false)

jameswnl · 2018-10-30T19:42:52Z

spec/models/service_template_transformation_plan_request_spec.rb

@@ -5,6 +5,32 @@
      ServiceResource.new(:resource => vm, :status => status)
    end
  end
+
+  let(:dst_ems) { FactoryGirl.create(:ext_management_system) }


Any reason not to move these lets to be inside Line57?

jameswnl · 2018-10-30T19:49:35Z

app/models/service_template_transformation_plan_request.rb

@@ -15,6 +15,13 @@ def source_vms
    vm_resources.where(:status => [ServiceResource::STATUS_QUEUED, ServiceResource::STATUS_FAILED]).pluck(:resource_id)
  end

+  def validate_conversion_hosts


Will valid_conversion_hosts? be a more obvious method name?

Name already validated in ManageIQ/manageiq-automation_engine#263. It would mean changing it again.

jameswnl · 2018-10-30T19:50:06Z

spec/models/service_template_transformation_plan_request_spec.rb

+      let(:host) { FactoryGirl.create(:host, :ext_management_system => dst_ems) }
+      let(:conversion_host) { FactoryGirl.create(:conversion_host, :resource => host) }
+
+      it { expect(request.validate_conversion_hosts).to eq(false) }


jameswnl · 2018-10-30T19:50:51Z

spec/models/service_template_transformation_plan_request_spec.rb

+
+      it { expect(request.validate_conversion_hosts).to eq(false) }
+    end
+  end


Can you add a test for using vm as conversion host?

agrare · 2018-10-31T12:51:24Z

spec/models/service_template_transformation_plan_request_spec.rb

@@ -1,10 +1,13 @@
+require 'byebug'


How'd that sneak in ;)

Don't know. Where have you seen it ? 😆

agrare · 2018-10-31T12:52:26Z

@fdupont-redhat just remove the byebug then I'm good with this, @jameswnl what do you think?

jameswnl · 2018-10-31T13:05:46Z

spec/models/service_template_transformation_plan_request_spec.rb

+    end
+
+    context 'conversion host exists in EMS and resource is a Host' do
+      let(:dst_ems) { FactoryGirl.create(:ems_redhat) }


Can these lets be shared with the previous context? You can still keep the :dst_ems local

jameswnl · 2018-10-31T13:22:56Z

spec/models/service_template_transformation_plan_request_spec.rb

+      let(:plan) { ServiceTemplateTransformationPlan.create_catalog_item(catalog_item_options) }
+      let(:request) { FactoryGirl.create(:service_template_transformation_plan_request, :source => plan) }
+
+      it 'returns true' do


nitpick: how about it 'passes' do ?

jameswnl · 2018-10-31T13:23:18Z

spec/models/service_template_transformation_plan_request_spec.rb

+      let(:plan) { ServiceTemplateTransformationPlan.create_catalog_item(catalog_item_options) }
+      let(:request) { FactoryGirl.create(:service_template_transformation_plan_request, :source => plan) }
+
+      it 'returns false' do


nitpick: how about it 'fails' do ?

jameswnl · 2018-10-31T13:24:08Z

spec/models/service_template_transformation_plan_request_spec.rb

+    end
+
+    context 'conversion host exists in EMS and resource is a Vm' do
+      let(:dst_ems) { FactoryGirl.create(:ems_openstack) }


Similarly with these lets, some of them can be shared with previous contexts

jameswnl · 2018-10-31T18:16:46Z

Just realized I didn't hit the submit after adding these comments 😢

Add a validation for conversion hosts (cherry picked from commit 8340f7c) https://bugzilla.redhat.com/show_bug.cgi?id=1640816

simaishi · 2018-11-01T14:35:23Z

Hammer backport details:

$ git log -1
commit 898d25b81c271b2b35ac0969d577e93f06b33d3b
Author: Adam Grare <[email protected]>
Date:   Wed Oct 31 13:58:06 2018 -0400

    Merge pull request #18135 from fdupont-redhat/bz1640816
    
    Add a validation for conversion hosts
    
    (cherry picked from commit 8340f7c01af08acbcf76623ed562019b8bdb52d3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1640816

fabiendupont added 2 commits October 26, 2018 10:05

Add a validation for conversion hosts

0868e07

Fix rubocop

ac22e42

ghost closed this Oct 26, 2018

ghost reopened this Oct 26, 2018

miq-bot added enhancement hammer/yes v2v labels Oct 26, 2018

miq-bot requested a review from agrare October 26, 2018 14:45

ghost mentioned this pull request Oct 26, 2018

Expose validate_conversion_hosts for TransformationPlanRequest ManageIQ/manageiq-automation_engine#263

Merged

agrare self-assigned this Oct 26, 2018

ghost mentioned this pull request Oct 26, 2018

Deny request if no conversion host is configured ManageIQ/manageiq-content#455

Merged

miq-bot added the blocker label Oct 26, 2018

agrare reviewed Oct 26, 2018

View reviewed changes

Add associations to ext_management_system

8342dbb

miq-bot requested a review from jameswnl October 30, 2018 15:46

jameswnl reviewed Oct 30, 2018

View reviewed changes

agrare reviewed Oct 31, 2018

View reviewed changes

Simplify method. Add test for VM conversion host.

d7273e1

ghost force-pushed the bz1640816 branch from 597c2fc to d7273e1 Compare October 31, 2018 13:02

agrare approved these changes Oct 31, 2018

View reviewed changes

agrare merged commit 8340f7c into ManageIQ:master Oct 31, 2018

agrare added this to the Sprint 98 Ending Nov 5, 2018 milestone Oct 31, 2018

jameswnl reviewed Oct 31, 2018

View reviewed changes

simaishi pushed a commit that referenced this pull request Nov 1, 2018

Merge pull request #18135 from fdupont-redhat/bz1640816

898d25b

Add a validation for conversion hosts (cherry picked from commit 8340f7c) https://bugzilla.redhat.com/show_bug.cgi?id=1640816

simaishi added hammer/backported and removed hammer/yes labels Nov 1, 2018

ghost deleted the bz1640816 branch November 7, 2018 15:40

ghost mentioned this pull request Nov 13, 2018

[In Progress Cards] Migration should fail when denied due to lack of conversion host ManageIQ/manageiq-v2v#786

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a validation for conversion hosts #18135

Add a validation for conversion hosts #18135

ghost commented Oct 26, 2018

ghost commented Oct 26, 2018

JPrause commented Oct 26, 2018

agrare Oct 26, 2018

agrare Oct 26, 2018

agrare Oct 26, 2018

agrare Oct 26, 2018

miq-bot commented Oct 30, 2018

agrare commented Oct 30, 2018

ghost commented Oct 30, 2018

jameswnl Oct 30, 2018

ghost Oct 30, 2018

jameswnl Oct 30, 2018

ghost Oct 30, 2018

jameswnl Oct 30, 2018

ghost Oct 31, 2018

jameswnl Oct 30, 2018

ghost Oct 30, 2018

jameswnl Oct 30, 2018

ghost Oct 30, 2018

jameswnl Oct 30, 2018

ghost Oct 31, 2018

agrare Oct 31, 2018

ghost Oct 31, 2018

agrare commented Oct 31, 2018

jameswnl Oct 31, 2018

jameswnl Oct 31, 2018

jameswnl Oct 31, 2018

jameswnl Oct 31, 2018

jameswnl commented Oct 31, 2018

simaishi commented Nov 1, 2018

Add a validation for conversion hosts #18135

Add a validation for conversion hosts #18135

Conversation

ghost commented Oct 26, 2018

ghost commented Oct 26, 2018

JPrause commented Oct 26, 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

miq-bot commented Oct 30, 2018

agrare commented Oct 30, 2018

ghost commented Oct 30, 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

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 commented Oct 31, 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

jameswnl commented Oct 31, 2018

simaishi commented Nov 1, 2018