Add support for pre/post-migration playbook #355

ghost · 2018-07-10T14:37:48Z

This PR adds support to call playbooks in pre and post migration phases. The implementation is generic to allow calling playbooks for other hooks. The backend provides methods to get the service template associated to pre and post hooks. The LaunchPlaybookAsAService method call create_provision_request and stores the request id in the task options hash. The CheckPlaybookAsAService method checks the status of the request. It gracefully fails if the playbook fails in post migration.

The CheckPlaybookAsAService method also updates the Ansible Tower job status in the task options hash, so that the UI can display it and also collect the job output.

Unfortunately, the test to check whether the destination VM is on was introduced inconsistency. It used the oVirt SDK gem to get the information from the provider. It was meant to be faster than wait for refresh. However, with targeted refresh, the power state is reported fast. So I changed the test to use destination_vm.power_state == 'on'. Without it the post migration playbook could be skipped due to race condition.

This PR adds add the following attributes to task options hash (example):

"pre_ansible_playbook_service_request_id" => 3,
:playbooks => {
    "pre": {
      :job_state => "finished",
      :job_status => "Ok",
      :job_id => 4
    },
    "post": {
      :job_state => "pending"
    }
}

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

ghost · 2018-07-10T14:39:18Z

@miq-bot add-label transformation

ghost · 2018-07-10T14:39:32Z

@miq-bot add-label enhancement

bzwei · 2018-07-10T18:38:42Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/checkplaybookasaservice.rb

+            service_request_task = service_request.miq_request_tasks.first
+            unless service_request_task.nil?
+              stack = service_request_task.destination.service_resources.first
+              job = stack.resource unless stack.nil?


stack is already the ansible job instance. There is no stack.resource method.

bzwei · 2018-07-10T18:50:32Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/checkplaybookasaservice.rb

+            service_request_id = task.get_option("#{transformation_hook}_ansible_playbook_service_request_id")
+            service_request = @handle.vmdb(:miq_request).find_by(:id => service_request_id)
+
+            set_ansible_job(task, service_request, transformation_hook)


You should not drill down to examine the ansible job every time. You should rely on the service request_state. Only after the service completes you can fetch the ansible job status just one time.

… job id only when the service request is finished.

ghost · 2018-07-11T08:07:09Z

@bzwei
Thanks for the advice. Indeed it can be lighter and the state and status can be those of the request. The job is only useful when the request is finished.

mturley · 2018-07-13T03:19:19Z

Part of the front-end support (the Overview page parts) for this will be merged with ManageIQ/manageiq-v2v#467 within the next day or two. The rest is already merge-ready I believe, as soon as this merges we should be all set end-to-end!

ghost · 2018-07-13T09:24:12Z

@miq-bot add-label gaprindashvili/yes

mkanoor · 2018-07-13T15:43:41Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/checkplaybookasaservice.rb

+                playbooks_status[transformation_hook][:job_status] = service_request.status
+                playbooks_status[transformation_hook][:job_id] = service_request.miq_request_tasks.first.destination.service_resources.first.resource.id
+                task.set_option(:playbooks, playbooks_status)
+                if service_request.status == 'Error' && transformation_hook == 'pre'


@fdupont-redhat is it intentional to ignore the errors for post playbooks?

@mkanoor
Yes, it is. We consider that the migration has gone far enough to not cancel it for a post migration playbook failure. The playbook or any other problem can be fixed afterwards, with a warning message. The UI knows that the playbook has failed, and it can show the log to the user.

mkanoor · 2018-07-13T15:44:38Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/checkplaybookasaservice.rb

+              if service_request.request_state == 'finished'
+                @handle.log(:info, "Ansible playbook service request (id: #{service_request_id}) is finished.")
+                playbooks_status[transformation_hook][:job_status] = service_request.status
+                playbooks_status[transformation_hook][:job_id] = service_request.miq_request_tasks.first.destination.service_resources.first.resource.id


@fdupont-redhat This is a pretty long chain of commands where we could get a nil in the middle?

@mkanoor
Good catch. I had the same feeling, but aesthetics won over security. I'll add extra tests.

mkanoor · 2018-07-17T15:06:52Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/checkplaybookasaservice.rb

+              task.set_option(:playbooks, playbooks_status)
+            end
+          rescue => e
+            @handle.set_state_var(:ae_state_progress, 'message' => e.message)


@fdupont-redhat do you want to report progress only when an error happens or should that also happen on success?
Does :ae_state_progress only mean error messages, if it does we should change it to something like :ae_state_error

@mkanoor
It's a default behavior to catch the error and log it in :ae_state_progress, so that it can be used by WeightedUpdateStatus. Here, we don't plan to add additional information because the description argument set in the state machine is explicit enough.

mkanoor · 2018-07-17T15:11:35Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/launchplaybookasaservice.rb

+              @handle.log(:info, "Service Template Id: #{service_template.id}")
+              unless service_template.nil?
+                target_host = target_host(task, transformation_hook)
+                unless target_host.nil?


@fdupont-redhat
Can this be a positive case with an 'if' statement instead of an unless, it makes it easier to read

if service_template ..... target_host = target_host(task, transformation_hook) if target_host ...... end end

Also what happens if you can't find a target_host or service_template should we report on that?

rubocop actually prefer earlier termination. In this example you can code this way

return if service_template.nil? ... return if target_host.nil? ... return unless target_host.power_state == 'on' ...

@bzwei @mkanoor
Good idea. It's better to be positive ;) And it makes the code shorter.
If service template is nil, it simply means that no playbook is expected to be run.
If target host is nil, it means that transformation_hook is neither pre, nor post. As we don't currently support any other hook, we can skip it. BTW, I don't know if we'll ever support other hooks, but the implementation is generic and could be reused for another use case.

bzwei · 2018-07-17T15:54:34Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/checkplaybookasaservice.rb

+            if service_request_id.present?
+              service_request = @handle.vmdb(:miq_request).find_by(:id => service_request_id)
+
+              playbooks_status = task.get_option(:playbooks) || {}


Don't mix string and symbol as keys at the same level. Here the examples are :playbooks and "pre_ansible_playbook_service_request_id" (BTW, the key is very long)

@bzwei
Code changed to use to_sym with the substituted string.
I know the key is very long, but it makes it really explicit.

bzwei · 2018-07-17T18:21:46Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/launchplaybookasaservice.rb

+              @handle.log(:info, "Service Template Id: #{service_template.id}")
+              unless service_template.nil?
+                target_host = target_host(task, transformation_hook)
+                unless target_host.nil?


rubocop actually prefer earlier termination. In this example you can code this way

return if service_template.nil? ... return if target_host.nil? ... return unless target_host.power_state == 'on' ...

bzwei · 2018-07-18T19:56:38Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/launchplaybookasaservice.rb

+            task = @handle.root['service_template_transformation_plan_task']
+            transformation_hook = @handle.inputs['transformation_hook']
+
+            unless transformation_hook == '_'


return if transformation_hook == '_'

bzwei · 2018-07-18T19:57:44Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/launchplaybookasaservice.rb

+              return if service_template.nil?
+              target_host = target_host(task, transformation_hook)
+              return if target_host.nil?
+              return unless target_host.power_state == 'on'


combine into return if target_host.nil? || target_host.power_state != 'on'

bzwei · 2018-07-18T19:59:42Z

content/automate/ManageIQ/Transformation/Ansible.class/__methods__/launchplaybookasaservice.rb

+            unless transformation_hook == '_'
+              service_template = task.send("#{transformation_hook}_ansible_playbook_service_template")
+              @handle.log(:info, "Service Template Id: #{service_template.id}")
+              return if service_template.nil?


if service_template.nil? then the previous line service_template.id will blow up. A simple fix is #{service_template.try(:id)}

I removed the @handle.log as it is mainly used for debug.

miq-bot · 2018-07-18T20:42:10Z

Checked commits fabiendupont/manageiq-content@5c55d63~...6db4635 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 2 offenses detected

content/automate/ManageIQ/Transformation/Ansible.class/methods/checkplaybookasaservice.rb

❗ - Line 39, Col 11 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

content/automate/ManageIQ/Transformation/Ansible.class/methods/launchplaybookasaservice.rb

❗ - Line 33, Col 11 - Style/RescueStandardError - Avoid rescuing without specifying an error class.

…aybook_support Add support for pre/post-migration playbook (cherry picked from commit f5fdc82) https://bugzilla.redhat.com/show_bug.cgi?id=1608420

simaishi · 2018-07-25T14:16:09Z

Gaprindashvili backport details:

$ git log -1
commit fb6f12ceff23a21bbdb891431ff31b43820bc8ca
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Jul 19 08:06:54 2018 -0400

    Merge pull request #355 from fdupont-redhat/v2v_pre_post_migration_playbook_support
    
    Add support for pre/post-migration playbook
    (cherry picked from commit f5fdc8293ac754a37674920607d7743392d94fcd)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1608420

fabiendupont added 2 commits July 10, 2018 10:28

Add support for pre/post-migration playbook

5c55d63

Fix typo in variable name

b63f9a3

miq-bot added the wip label Jul 10, 2018

miq-bot added the v2v label Jul 10, 2018

miq-bot added the enhancement label Jul 10, 2018

Fix source and destination VM assignment

af3cc1b

bzwei reviewed Jul 10, 2018

View reviewed changes

fabiendupont added 2 commits July 11, 2018 04:01

Refactor method to set state and status based on service request. Add…

ed89f70

… job id only when the service request is finished.

Fix rubocop errors.

f8733fa

This was referenced Jul 11, 2018

Pre & Post Playbook Support - Plan Details Page Changes ManageIQ/manageiq-v2v#400

Closed

Pre & Post Playbook Support - Overview Page Changes ManageIQ/manageiq-v2v#399

Closed

Add a check on service_request_id.

1e9afb2

miq-bot added the gaprindashvili/yes label Jul 13, 2018

Fix errors identified during testing

286e06f

ghost changed the title ~~[WIP] Add support for pre/post-migration playbook~~ Add support for pre/post-migration playbook Jul 13, 2018

miq-bot removed the wip label Jul 13, 2018

Forgot to add this file

c2c0be3

mkanoor reviewed Jul 13, 2018

View reviewed changes

michaelkro mentioned this pull request Jul 16, 2018

[#400] Plan details playbooks support ManageIQ/manageiq-v2v#442

Merged

mkanoor reviewed Jul 17, 2018

View reviewed changes

bzwei reviewed Jul 17, 2018

View reviewed changes

Changed 'if/unless' with 'return'. Use symbol for task options

3522397

bzwei suggested changes Jul 18, 2018

View reviewed changes

Return early

6db4635

bzwei approved these changes Jul 18, 2018

View reviewed changes

gmcculloug added the blocker label Jul 18, 2018

gmcculloug self-assigned this Jul 18, 2018

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

gmcculloug merged commit f5fdc82 into ManageIQ:master Jul 19, 2018

bthurber mentioned this pull request Jul 23, 2018

[WIP][#399] Pre/Post Playbook status in Overview page ManageIQ/manageiq-v2v#467

Closed

ghost mentioned this pull request Jul 24, 2018

Fix hook string passing for pre/post-migration playbooks #370

Merged

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

ghost deleted the v2v_pre_post_migration_playbook_support branch July 31, 2018 07:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for pre/post-migration playbook #355

Add support for pre/post-migration playbook #355

ghost commented Jul 10, 2018 •

edited by ghost

Loading

ghost commented Jul 10, 2018

ghost commented Jul 10, 2018

bzwei Jul 10, 2018

bzwei Jul 10, 2018

ghost commented Jul 11, 2018

mturley commented Jul 13, 2018

ghost commented Jul 13, 2018

mkanoor Jul 13, 2018

ghost Jul 18, 2018

mkanoor Jul 13, 2018

ghost Jul 18, 2018

mkanoor Jul 17, 2018

ghost Jul 18, 2018

mkanoor Jul 17, 2018

bzwei Jul 17, 2018

ghost Jul 18, 2018

bzwei Jul 17, 2018

ghost Jul 18, 2018

bzwei Jul 17, 2018

bzwei Jul 18, 2018

ghost Jul 18, 2018

bzwei Jul 18, 2018

ghost Jul 18, 2018

bzwei Jul 18, 2018

ghost Jul 18, 2018

miq-bot commented Jul 18, 2018

simaishi commented Jul 25, 2018

Add support for pre/post-migration playbook #355

Add support for pre/post-migration playbook #355

Conversation

ghost commented Jul 10, 2018 • edited by ghost Loading

ghost commented Jul 10, 2018

ghost commented Jul 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 11, 2018

mturley commented Jul 13, 2018

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

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 18, 2018

simaishi commented Jul 25, 2018

ghost commented Jul 10, 2018 •

edited by ghost

Loading