-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix transformation host selection #379
Changes from 1 commit
83b3926
1040d2f
717b0f5
f7a8760
70f5df7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same concepts apply here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if Transformation::TransformationHosts::Common::Utils.get_runners_count_by_ems(destination_ems, factory_config) >= max_runners.to_i | ||
@handle.log(:info, "Too many transformations running [#{max_runners}]. Retrying.") | ||
else | ||
wrapper_options = ManageIQ::Automate::Transformation::TransformationHosts::Common::Utils.virtv2vwrapper_options(task) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect that
ems_max_runners
is an integer and we should not have to callto_i
on line 48 when you are going to evaluate it. Also10
should be a class constantDEFAULT_EMS_MAX_RUNNERS
.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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.