Fix transformation host selection #379

ghost · 2018-07-28T22:02:28Z

The comparison fails because nil.to_i equals to 0. This leads to endless loop on the AcquireTransformationHost and VMTransform states.

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

ghost · 2018-07-28T22:08:26Z

@miq-bot add-label transformation
@miq-bot add-label bug
@miq-bot add-label gaprindashvili/yes
@miq-bot add-label blocker

gmcculloug · 2018-07-29T12:36:54Z

content/automate/ManageIQ/Transformation/TransformationHosts/Common.class/__methods__/utils.rb

@@ -43,9 +43,9 @@ def self.get_runners_count_by_ems(ems, factory_config)

            def self.get_transformation_host(task, factory_config, handle = $evm)
              ems = handle.vmdb(:ext_management_system).find_by(:id => task.get_option(:destination_ems_id))
-              ems_max_runners = ems.custom_get('Max Transformation Runners').to_i || factory_config['ems_max_runners'] || 1
+              ems_max_runners = ems.custom_get('Max Transformation Runners') || factory_config['ems_max_runners'] || 10


I would expect that ems_max_runners is an integer and we should not have to call to_i on line 48 when you are going to evaluate it. Also 10 should be a class constant DEFAULT_EMS_MAX_RUNNERS.

module Common class Utils DEFAULT_EMS_MAX_RUNNERS = 10 def initialize(handle = $evm)

You could do something like this:
ems_max_runners = (ems.custom_get('Max Transformation Runners') || factory_config['ems_max_runners'] || DEFAULT_EMS_MAX_RUNNERS).to_i

A better refactoring would be to move this logic into a method where you could potentially log where the value was coming from if you felt that was important.

def ems_max_runners if ems.custom_get('Max Transformation Runners') <log message> ems.custom_get('Max Transformation Runners').to_i elsif factory_config['ems_max_runners'] <log message> factory_config['ems_max_runners'].to_i else <log message> DEFAULT_EMS_MAX_RUNNERS end

ems_max_runners is not an integer when it is a custom attributes, because custom attributes are stored as strings.
Anyway, it's a good idea to create a method, as the message will help debugging.

gmcculloug · 2018-07-29T12:37:35Z

...automate/ManageIQ/Transformation/TransformationHosts/Common.class/__methods__/vmtransform.rb

@@ -32,8 +32,8 @@ def main
              @handle.log(:info, "Transformation - Started On: #{start_timestamp}")

              destination_ems = @handle.vmdb(:ext_management_system).find_by(:id => task.get_option(:destination_ems_id))
-              max_runners = destination_ems.custom_get('Max Transformation Runners').to_i || factory_config['max_transformation_runners_by_ems'] || 10
-              if Transformation::TransformationHosts::Common::Utils.get_runners_count_by_ems(destination_ems, factory_config) >= max_runners
+              max_runners = destination_ems.custom_get('Max Transformation Runners') || factory_config['max_transformation_runners_by_ems'] || 10


Same concepts apply here as well.

I'm using the embedded method to not duplicate code. A separate method is even a better idea because the default value lives in a single method.

gmcculloug · 2018-07-30T12:50:32Z

content/automate/ManageIQ/Transformation/TransformationHosts/Common.class/__methods__/utils.rb

+                factory_config['ems_max_runners'].to_i
+              else
+                handle.log(:info, "Using default max transformation runners: #{DEFAULT_EMS_MAX_RUNNERS}")
+                DEFAULT_EMS_MAX_RUNNERS


The default value was 1 in the get_transformation_host previously here https://github.com/ManageIQ/manageiq-content/pull/379/files#diff-44115b1bf3b0e7b502024d527948ef6dL46

now DEFAULT_EMS_MAX_RUNNERS = 10 which is what VMTransform expects.

Does the value need to be flexible?

If so I would suggest adding another option to the end of the ems_max_runners method.

Example: def self.ems_max_runners(ems, factory_config, handle = $evm, max_runners = DEFAULT_EMS_MAX_RUNNERS)

and override the value from get_transformation_host.

Good idea. Indeed, it may change over time. And we may see a more complex heuristic to determine max_runners. I changed it also for host_max_runners.

mkanoor · 2018-07-30T14:08:53Z

content/automate/ManageIQ/Transformation/TransformationHosts/Common.class/__methods__/utils.rb

@@ -16,6 +19,19 @@ def self.get_runners_count_by_host(host, handle = $evm)
              handle.vmdb(:service_template_transformation_plan_task).where(:state => 'active').select { |task| task.get_option(:transformation_host_id) == host.id }.size
            end

+            def self.host_max_runners(host, factory_config, max_runners = DEFAULT_HOST_MAX_RUNNERS, handle = $evm)


@fdupont-redhat Is this just a Utility class which only has class methods that you want to embed into other classes. We don't need to have an initialize or the call at the bottom if you are going to just call it from other methods.

I noticed this as well as there is an empty main method defined. I'm good with including those changes here if appropriate but that refactoring could be in a separate PR as well since it is unrelated.

Yes, it's just a utility class. It is meant to be added as an embedded method. Should I remove it ? It's a bit out of scope for this PR. Maybe another cleanup PR ?

Ok, so I removed the unneeded parts: initialize, main and call at end of file.

miq-bot · 2018-07-30T15:07:39Z

Checked commits fabiendupont/manageiq-content@83b3926~...70f5df7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

gmcculloug · 2018-07-30T17:17:31Z

content/automate/ManageIQ/Transformation/TransformationHosts/Common.class/__methods__/utils.rb

              ems_cur_runners = get_runners_count_by_ems(ems, factory_config)
-              transformation_host_hash = ems_cur_runners < ems_max_runners ? eligible_transformation_hosts(ems, factory_config).first : {}
+              transformation_host_hash = ems_cur_runners < ems_max_runners(ems, factory_config) ? eligible_transformation_hosts(ems, factory_config).first : {}


Were you planning to change this call to ems_max_runners to pass 1 as the third parameter to override max_runners? That would maintain the previous logic.

Nope. It was 1 just because I didn't know what could be a reasonable value. Now that RHV perf team has said that 10 should be the default per host, I'm setting it to 10 at the EMS level. This would allow 1 conversion host per EMS by default. Then user can override it using custom attribute.

…st_selection Fix transformation host selection (cherry picked from commit b2982c6) https://bugzilla.redhat.com/show_bug.cgi?id=1610054

simaishi · 2018-07-31T00:50:56Z

Gaprindashvili backport details:

$ git log -1
commit 99fff66d837394b3eee4bdf639c0bb44b71a7464 (HEAD -> gaprindashvili, origin/gaprindashvili)
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Jul 30 13:41:19 2018 -0400

    Merge pull request #379 from fdupont-redhat/v2v_fix_transformation_host_selection
    
    Fix transformation host selection
    (cherry picked from commit b2982c6bc856b5b6ad4af762dfa028cf9df9bb11)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1610054

Fix transformation host selection

83b3926

miq-bot added v2v bug gaprindashvili/yes blocker labels Jul 28, 2018

gmcculloug reviewed Jul 29, 2018

View reviewed changes

fabiendupont added 2 commits July 29, 2018 10:53

Separate ems_max_runners logic into a method

1040d2f

Add a separate method for max runners per host

717b0f5

gmcculloug reviewed Jul 30, 2018

View reviewed changes

Make default value more flexible for max runners

f7a8760

mkanoor reviewed Jul 30, 2018

View reviewed changes

Trim the class structure as it is only a utility class

70f5df7

gmcculloug reviewed Jul 30, 2018

View reviewed changes

gmcculloug self-assigned this Jul 30, 2018

gmcculloug merged commit b2982c6 into ManageIQ:master Jul 30, 2018

gmcculloug added this to the Sprint 91 Ending Jul 30, 2018 milestone Jul 30, 2018

simaishi pushed a commit that referenced this pull request Jul 31, 2018

Merge pull request #379 from fdupont-redhat/v2v_fix_transformation_ho…

99fff66

…st_selection Fix transformation host selection (cherry picked from commit b2982c6) https://bugzilla.redhat.com/show_bug.cgi?id=1610054

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Jul 31, 2018

ghost deleted the v2v_fix_transformation_host_selection branch July 31, 2018 07:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix transformation host selection #379

Fix transformation host selection #379

ghost commented Jul 28, 2018

ghost commented Jul 28, 2018

gmcculloug Jul 29, 2018

ghost Jul 29, 2018

gmcculloug Jul 29, 2018

ghost Jul 29, 2018

gmcculloug Jul 30, 2018

ghost Jul 30, 2018

mkanoor Jul 30, 2018

gmcculloug Jul 30, 2018

ghost Jul 30, 2018

ghost Jul 30, 2018

miq-bot commented Jul 30, 2018

gmcculloug Jul 30, 2018

ghost Jul 30, 2018

simaishi commented Jul 31, 2018

Fix transformation host selection #379

Fix transformation host selection #379

Conversation

ghost commented Jul 28, 2018

ghost commented Jul 28, 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

miq-bot commented Jul 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simaishi commented Jul 31, 2018