Skip to content
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 default cleanup state machine #382

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ def create_cleanup_request(task)
options[:class_name] = sm_uri_array.pop
options[:namespace] = sm_uri_array.join('/')
options[:user_id] = @handle.root['user_id']
options[:attrs] = { :service_template_transformation_plan_task_id => task.id }
request = @handle.execute("create_automation_request", options, @handle.root['user'].userid, true)
task.set_option(:cleanup_request_id, request.id) unless request.nil?
end
Expand Down

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
module ManageIQ
module Automate
module Transformation
module Infrastructure
module VM
module Common
class CheckPoweredOn
def initialize(handle = $evm)
@handle = handle
end

def main
if @handle.root['service_template_transformation_plan_task'].blank?
task = @handle.vmdb(:service_template_transformation_plan_task).find_by(:id => @handle.root['service_template_transformation_plan_task_id'])
vm = task.source if task.present?
else
task = @handle.root['service_template_transformation_plan_task']
vm = @handle.vmdb(:vm).find_by(:id => task.get_option(:destination_vm_id)) if task.present?
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should break this down into methods which would make the logic easier to read.

Move this first if-else block into a method that returns both task and vm:

def task_and_vm
  if @handle.root['service_template_transformation_plan_task'].blank?
    task = @handle.vmdb(:service_template_transformation_plan_task).find_by(:id => @handle.root['service_template_transformation_plan_task_id'])
    vm = task.source if task.present?
  else
    task = @handle.root['service_template_transformation_plan_task']
    vm = @handle.vmdb(:vm).find_by(:id => task.get_option(:destination_vm_id)) if task.present?
  end

  return task, vm
end

Then the first part of main becomes:

task, vm = task_and_vm

If you think this is too much change it could be in a followup refactoring, but we need to move away for having large single method scripts in general.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is identical in checkpoweredon.rb and poweron.rb. I'd propose moving task_and_vm to a embedded utility method. It's a bigger change that may require another PR. I'd go for a followup refactoring.

return if vm.blank?
@handle.log(:info, "Target VM: #{vm.name} [#{vm.vendor}]")
return if task.get_option(:source_vm_power_state) != 'on'
if vm.power_state != 'on'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For readability add spaces between lines when the context changes:

I would think something like this:

def main
  task, vm = task_and_vm
  return if vm.blank?

  @handle.log(:info, "Target VM: #{vm.name} [#{vm.vendor}]")

  return if task.get_option(:source_vm_power_state) != 'on'

  if vm.power_state != 'on'
    @handle.root["ae_result"] = "retry"
    @handle.root["ae_retry_interval"] = "15.seconds"
  end
rescue => e
  @handle.set_state_var(:ae_state_progress, 'message' => e.message)
  raise
end

@handle.root["ae_result"] = "retry"
@handle.root["ae_retry_interval"] = "15.seconds"
end
rescue => e
@handle.set_state_var(:ae_state_progress, 'message' => e.message)
raise
end
end
end
end
end
end
end
end

if $PROGRAM_NAME == __FILE__
ManageIQ::Automate::Transformation::Infrastructure::VM::Common::CheckPoweredOn.new.main
end

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ object:
on_error: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 5, description
=> "Power-on VM", task_message => "Migrating")
- State8:
value: "/Transformation/Infrastructure/VM/${state_var#destination_ems_type}/CheckPoweredOn"
value: "/Transformation/Infrastructure/VM/Common/CheckPoweredOn"
on_entry: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 40, description
=> "Power-on VM", task_message => "Migrating")
on_exit: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 40, description
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,31 +9,23 @@ object:
description:
fields:
- State2:
value: "/Transformation/Common/AssessTransformationCleanup"
on_entry: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 10, description
=> "Assess Migration Cleanup", task_message => "Cleanup")
on_exit: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 10, description
=> "Assess Migration Cleanup", task_message => "Cleanup")
on_error: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 10, description
=> "Assess Migration Cleanup", task_message => "Cleanup")
- State5:
value: "/Transformation/TransformationHosts/Common/KillVirtV2V"
on_entry: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 40, description
=> "Interrupt virt-v2v", task_message => "Cleanup")
on_exit: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 40, description
=> "Interrupt virt-v2v", task_message => "Cleanup")
on_error: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 40, description
=> "Interrupt virt-v2v", task_message => "Cleanup")
- State8:
- State5:
value: "/Transformation/Infrastructure/VM/Common/PowerOn"
on_entry: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 10, description
=> "Power-on VM", task_message => "Cleanup")
on_exit: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 10, description
=> "Power-on VM", task_message => "Cleanup")
on_error: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 10, description
=> "Power-on VM", task_message => "Cleanup")
- State11:
value: "/Transformation/Infrastructure/VM/${state_var#destination_ems_type}/CheckPoweredOn"
- State8:
value: "/Transformation/Infrastructure/VM/Common/CheckPoweredOn"
on_entry: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 40, description
=> "Power-on VM", task_message => "Cleanup")
on_exit: /System/CommonMethods/MiqAe.WeightedUpdateStatus(weight => 40, description
Expand Down