Add automate methods for VM import between providers #36

matobet · 2017-02-07T16:19:18Z

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1404920

Added automate methods required to back the transform vm service dialog (ManageIQ/manageiq#13794 ManageIQ/manageiq-ui-classic#383) facilitating v2v conversion of VMs between two infra providers.

The dialog consists of 3 select boxes (target infra provider, cluster, storage domain) and a single checkbox that controls whether thin provisioning is enabled. To supply those select boxes we provided the list_infra_providers, list_clusters and list_storages methods respectively.

matobet · 2017-02-14T16:47:56Z

Depends on ManageIQ/manageiq#13916

mkanoor · 2017-02-14T19:26:45Z

@matobet
Could you also add a spec for the automate methods in this PR

For the newer methods we are following a module approach so that we can test them outside of the Automate Engine. A good example to follow would be a method and spec like

Method

https://github.com/ManageIQ/manageiq-content/blob/master/content/automate/ManageIQ/Service/Generic/StateMachines/GenericLifecycle.class/__methods__/check_completed.rb

Spec

https://github.com/ManageIQ/manageiq-content/blob/master/spec/content/automate/ManageIQ/Service/Generic/StateMachines/GenericLifecycle.class/__methods__/check_completed_spec.rb

matobet · 2017-02-15T16:05:15Z

@mkanoor Posted new (modularized) version of the scripts, can you please tell me if I'm on the right track?

mkanoor · 2017-02-15T20:55:07Z

...nt/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/import_infra_vm.rb

+                  :storage_id => storage_id,
+                  :sparse     => sparse
+                )
+                exit MIQ_OK


@matobet
We don't need the exit you can use @handle.root['ae_result'] = 'ok'

mkanoor · 2017-02-15T20:58:16Z

...nt/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/import_infra_vm.rb

+
+                provider.import_vm(
+                  vm.id,
+                  :name       => name,


@matobet
Could we not use the local variables and directly use the attribute values from root

provider.import_vm( @handle.root['vm'].id, :name => @handle.root['dialog_name'], :cluster_id => @handle.root['dialog_cluster'], :storage_id => @handle.root['dialog_storage'], :sparse => @handle.root['dialog_sparse'])

matobet · 2017-02-16T17:20:04Z

...tomate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_infra_providers.rb

+                values_hash = {}
+                values_hash[''] = '-- select target infrastructure provider from list --'
+
+                managers = @handle.vmdb('ManageIQ_Providers_Redhat_InfraManager').all.select(&:validate_import_vm)


@mkanoor When writing the specs for this script, how should I approach to mocking this call to vmdb(ManageIQ_Providers_Redhat_InfraManager)? I tried looking in other specs and found no example of this..

From what I understand the basic idea is to create the backing AR model, e.g.

let(:provider) { FactoryGirl.create(:ems_redhat) }

and then create the "wrapping"

let(:svc_model_provider) { MiqAeMethodService::MiqAeServiceManageIQ_Providers_Redhat_InfraManager.find(provider.id) }

but I'm not sure how to proceed in this case... (tests are currently failing with "Service Model not found".

mkanoor · 2017-02-17T14:28:36Z

content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_clusters.rb

+                  end
+                end
+                list_values = {
+                  'sort_by'   => :value,


@matobet
Do you want the Dialogs to show items sorted by ID or by description/name?
If you want it to displayed sorted by name description the sort_by => 'description'

mkanoor · 2017-02-17T14:29:17Z

content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_clusters.rb

+                    values_hash['!'] = '-- No clusters found --'
+                  else
+                    provider.ems_clusters.each do |cluster|
+                      values_hash[cluster.id.to_s] = cluster.name


@matobet
could the key here be just id and not id string

mkanoor · 2017-02-17T14:33:17Z

content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_clusters.rb

+                if !provider_id.nil? && !provider_id.empty? && provider_id != '!'
+                  provider = @handle.vmdb(:ext_management_system, provider_id)
+                  if provider.nil?
+                    values_hash['!'] = '-- No clusters found --'


@matobet
We typically use nil => "None" as the default for
https://github.com/ManageIQ/manageiq-content/blob/master/content/automate/ManageIQ/Cloud/Orchestration/Operations/Methods.class/__methods__/available_flavors.rb#L27

mkanoor · 2017-02-17T14:34:47Z

content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_clusters.rb

+                values_hash = {}
+                values_hash[''] = '-- select cluster from list --'
+
+                provider_id = @handle.root['dialog_provider']


@matobet
Are you expecting the @handle.root['dialog_provider'] to be ! at some point as a default value

@mkanoor The '!' value of provider_id is being handled at line 17, if that is your question.

mkanoor · 2017-02-21T16:03:57Z

content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_storages.rb

+
+              def main
+                values_hash = {}
+                values_hash[''] = '-- select storage from list --'


@matobet
I think the key here should be nil
value_hash[nil] = '-- select storage from list --'
On line 21 you are setting nil to None if no providers are found.

mkanoor · 2017-02-21T18:04:44Z

content/automate/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/list_clusters.rb

@@ -0,0 +1,45 @@
+module ManageIQ


@matobet
can you please add a spec for this method and all the other methods.
https://github.com/ManageIQ/manageiq-content/blob/master/spec/content/automate/ManageIQ/AutomationManagement/AnsibleTower/Operations/Methods.class/__methods__/available_credentials_spec.rb

mkanoor

we are requesting that every automate method have an associated spec. Please add the missing specs for the list methods

matobet · 2017-02-22T12:28:13Z

@mkanoor done, but now this patch also depends on the core patch adding required factories for the specs ManageIQ/manageiq#14017

matobet · 2017-05-16T16:34:38Z

@tinaafitz pushed new version, added missing tests and hopefully addressed your comments :)

tinaafitz · 2017-05-17T15:14:14Z

...nt/automate/ManageIQ/Infrastructure/VM/Transform/StateMachines/VmImport.class/__class__.yaml

@@ -0,0 +1,53 @@
+---


@matobet We're working on documenting guidelines for adding State Machines to the ManageIQ domain.

Can you modify your ImportVM State Machine as follows:

Minimum Requirements for ManageIQ domain State Machines:
(In addition to the specific processing states required by the new state machine)

Minimum of 3 Pre states(pre1, pre2, pre3)

Minimum of 3 Post states(post1, post2, post3)

update_*_status method required for every states on_entry, on_exit, on_error
a. * update request with enhanced message including
updated_message = "[#{$evm.root['miq_server'].name}] "
updated_message += "Step [#{$evm.root['ae_state']}] "
updated_message += "Status [#{status}] "
updated_message += "Message [#{prov.message}] "
updated_message += "Current Retry Number [#{$evm.root['ae_state_retries']}]" if
$evm.root['ae_result'] == 'retry'
b. * set user_message in request:
prov.miq_request.user_message = updated_message

c. * create notification and error log message on error
if $evm.root['ae_result'] == "error"
$evm.create_notification(:level => "error"
:message => "VM Provision Error: #{updated_message}")
$evm.log(:error, "VM Provision Error: #{updated_message}")
end

Finished state
Can use CommonMethods finished method:
/System/CommonMethods/StateMachineMethods/your_instance_name_here
(Add an instance to the /System/CommonMethods/StateMachineMethods class with your instance name. Can optionally modify task_finished method in that class for special processing)

@tinaafitz ok, 1) and 2) seems pretty straightforward (I assume the states to be of type string)
but regarding 3) what is exactly necessary? add 3 methods for each state of the state machine (with the new pre and post that would total to 8*3=24 new methods)?

@tinaafitz and regarding 4) the state will be part of the ImportVm state machine (after the last "post" state)?

@tinaafitz Done, please review at your nearest convenience :)

tinaafitz

@matobet - Great job on all of your changes!
I ran all of the spec tests and they look good.
I made just a few comments and I'm finished with the review. The next step is I'll ask @mkanoor for a final review.
Could you provide a brief explanation of setup/prerequisites/use cases for documentation purposes?

tinaafitz · 2017-05-22T17:32:22Z

...omate/ManageIQ/System/Event/EmsEvent/RHEVM.class/__methods__/update_vm_import_status_spec.rb

+  let(:miq_server) { EvmSpecHelper.local_miq_server }
+  let(:ems)        { FactoryGirl.create(:ems_redhat) }
+  let(:vm)         { FactoryGirl.create(:vm_redhat) }
+  let(:miq_event)  { FactoryGirl.create(:miq_event, :event_type => event_type) }


@matobet miq_event is not needed here.

tinaafitz · 2017-05-22T20:44:00Z

...te/ManageIQ/Infrastructure/VM/Transform/Import.class/__methods__/create_vm_import_request.rb

+              end
+
+              def main
+                options = {


@matobet - Could you add basic validation that the required arguments exist before calling execute to create the automation request?

Here's a sample basic validation method:
def validate_root_args(arg_names)
arg_names.each do |name|
unless $evm.root[name].present?
msg = "Error, required root attribute: #{name} not found"
$evm.log(:error, msg)
raise msg
end
end
end

and you can call it prior to calling execute....

validate_root_args(%w(vm dialog_name dialog_provider dialog_cluster dialog_sparse))

tinaafitz · 2017-05-22T20:52:42Z

...eIQ/Infrastructure/VM/Transform/StateMachines/VmImport.class/__methods__/submit_vm_import.rb

+
+              def main
+                provider = @handle.vmdb(:ext_management_system, @handle.root['provider_id'])
+                @handle.log(:info, 'Submitting Import')


@matobet - Could you add basic validation that the required arguments exist before calling submit_import_vm?

Here's a sample basic validation method:
def validate_root_args(arg_names)
arg_names.each do |name|
unless $evm.root[name].present?
msg = "Error, required root attribute: #{name} not found"
$evm.log(:error, msg)
raise msg
end
end
end

and you can call it prior to calling execute....

validate_root_args(%w(vm name cluster_id storage_id sparse))

matobet · 2017-05-23T11:16:54Z

Hi @tinaafitz thank you for the review, I addressed all your comments but I'm not sure what exactly do you mean by setup/prerequisites/use cases for documentation purposes?

tinaafitz · 2017-05-23T13:25:06Z

@matobet Thanks for making the changes. I'll ask @mkanoor to review.
You probably already have all of the necessary documentation information in your PRs.
The purpose is to answer the question of how do I use this feature? Although I've reviewed your PR, I wouldn't know how best to guide someone who wanted to use this new feature.

I think the information is all here, maybe if you could provide a brief explanation to a new user wanting to use v2.
https://docs.google.com/document/d/1TnAjYJd041vCWu71ks4oxT_jjSrlDE2nZWLBXK4ZKkg/edit
https://trello.com/c/uh3V62OZ/11-v2v-in-miq

tinaafitz · 2017-05-23T13:26:08Z

@mkanoor @matobet has made all of the requested changes. Please review.

mkanoor · 2017-05-23T14:36:57Z

@matobet Can you fix the rubocop warning, we can use a next if the variable is present else raise an error

mkanoor · 2017-05-23T14:41:08Z

@matobet

              def validate_root_args(arg_names)
                arg_names.each do |name|
                  next if @handle.root[name].present?
                  raise "Error, required root attribute: #{name} not found"
                 end
              end

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1404920

matobet · 2017-05-23T15:04:10Z

@mkanoor done

miq-bot · 2017-05-23T15:08:45Z

Checked commit matobet@62030d8 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
15 files checked, 0 offenses detected
Everything looks fine. 🍰

matobet · 2017-05-23T15:17:20Z

@tinaafitz as per our discussion, posting here short feature description:

Feature Overview

Allow migration of user workloads (VMs) from VMware infra provider to RHV provider
So far bulk operations not supported, need to individual VMs one-by-one
User chooses target (RHV) provider, cluster and storage domain
So far only the Vmware -> RHV is direction is supported (no other permutations of source & destination)

REST

Added a new action to the providers REST resource that allows for importing vms.
POST /api/providers/:id

{
  "action" : "import_vm", 
  "resource" : {
      "source": { "href": "http://localhost:3000/api/vms/:source_vm_id" },
      "target": {
          "name": "<name of newly created VM in RHEV>",
          "data_store": { "href": "http://localhost:3000/api/:target_data_store_id" },
          "cluster": { "href": "http://localhost:3000/api/:target_cluster_id" },
          "sparse": "[true|false]"
      }
  }
}

For now only the combination of a Vmware source VM and a RHEV as the target provider is supported.

UI

New button in the “vm detail” (“Transform this VM to RHV”)
It should be visible only for VMware VMs
It should be enabled (not greyed out) only when there exists a suitable target provider in the system (in this case RHV infra provider of version 4.0+)
Spawns dialog with the following options
- Name
- Cluster
- Storage domain
- Thin provisioning (yes/no checkbox)
  For the created VM.
After submission the status and progress of the operation should be tracked along with other automate requests.

mkanoor · 2017-05-23T16:38:32Z

@matobet Looks good.
👍

Add automate methods for VM import between providers (cherry picked from commit bd5bb97) https://bugzilla.redhat.com/show_bug.cgi?id=1459996

simaishi · 2017-06-09T16:05:10Z

Fine backport details:

$ git log -1
commit 868138f7b46eb73d639d8c8bed337cea78da34c7
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Wed May 24 08:28:35 2017 -0400

    Merge pull request #36 from matobet/v2v-automate
    
    Add automate methods for VM import between providers
    (cherry picked from commit bd5bb97383ea203cc2bde9d740a09624a08e01f2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1459996

matobet force-pushed the v2v-automate branch 3 times, most recently from dd03e99 to 7780a59 Compare February 14, 2017 12:43

matobet mentioned this pull request Feb 14, 2017

Add Transform VM service dialog ManageIQ/manageiq#13794

Closed

matobet force-pushed the v2v-automate branch from 7780a59 to a385c08 Compare February 14, 2017 16:59

matobet force-pushed the v2v-automate branch from a385c08 to 86417e3 Compare February 15, 2017 15:37

mkanoor reviewed Feb 15, 2017

View reviewed changes

matobet force-pushed the v2v-automate branch from 86417e3 to 07756c3 Compare February 16, 2017 15:26

matobet commented Feb 16, 2017

View reviewed changes

matobet force-pushed the v2v-automate branch from 07756c3 to a52ae6c Compare February 17, 2017 10:35

jhernand approved these changes Feb 17, 2017

View reviewed changes

mkanoor reviewed Feb 17, 2017

View reviewed changes

matobet force-pushed the v2v-automate branch from a52ae6c to 6241c3e Compare February 17, 2017 16:12

mkanoor reviewed Feb 21, 2017

View reviewed changes

matobet force-pushed the v2v-automate branch from 6241c3e to 00f6d5b Compare February 21, 2017 16:27

mkanoor reviewed Feb 21, 2017

View reviewed changes

mkanoor suggested changes Feb 21, 2017

View reviewed changes

matobet force-pushed the v2v-automate branch from 00f6d5b to 9a70856 Compare February 22, 2017 12:12

matobet force-pushed the v2v-automate branch 2 times, most recently from 05e728f to 3fe9d9d Compare March 1, 2017 15:19

matobet force-pushed the v2v-automate branch from 3fe9d9d to 2121c5b Compare March 16, 2017 11:08

matobet force-pushed the v2v-automate branch from 2121c5b to 65cf9e0 Compare April 4, 2017 14:35

matobet force-pushed the v2v-automate branch 2 times, most recently from 8dce9b5 to 1cacf6e Compare May 16, 2017 16:27

matobet force-pushed the v2v-automate branch from 1cacf6e to 7d8acde Compare May 17, 2017 12:46

tinaafitz reviewed May 17, 2017

View reviewed changes

matobet force-pushed the v2v-automate branch 3 times, most recently from a9202e8 to 1a9520f Compare May 19, 2017 16:27

tinaafitz reviewed May 22, 2017

View reviewed changes

matobet force-pushed the v2v-automate branch from 1a9520f to 0026e90 Compare May 23, 2017 11:35

Add automate methods for VM import between providers

62030d8

Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1404920

matobet force-pushed the v2v-automate branch from 0026e90 to 62030d8 Compare May 23, 2017 15:04

mkanoor approved these changes May 23, 2017

View reviewed changes

mkanoor assigned gmcculloug May 23, 2017

gmcculloug merged commit bd5bb97 into ManageIQ:master May 24, 2017

gmcculloug added this to the Sprint 62 Ending Jun 5, 2017 milestone May 24, 2017

gmcculloug added the enhancement label May 24, 2017

simaishi pushed a commit that referenced this pull request Jun 9, 2017

Merge pull request #36 from matobet/v2v-automate

868138f

Add automate methods for VM import between providers (cherry picked from commit bd5bb97) https://bugzilla.redhat.com/show_bug.cgi?id=1459996

simaishi added fine/backported and removed fine/yes labels Jun 9, 2017

Add automate methods for VM import between providers #36

Add automate methods for VM import between providers #36

Conversation

matobet commented Feb 7, 2017 • edited Loading

matobet commented Feb 14, 2017

mkanoor commented Feb 14, 2017

matobet commented Feb 15, 2017

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

matobet Feb 16, 2017 • edited Loading

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

mkanoor left a comment

Choose a reason for hiding this comment

matobet commented Feb 22, 2017

matobet commented May 16, 2017

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

tinaafitz left a 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

matobet commented May 23, 2017

tinaafitz commented May 23, 2017

tinaafitz commented May 23, 2017

mkanoor commented May 23, 2017

mkanoor commented May 23, 2017

matobet commented May 23, 2017

miq-bot commented May 23, 2017

matobet commented May 23, 2017

Feature Overview

REST

UI

mkanoor commented May 23, 2017

simaishi commented Jun 9, 2017

matobet commented Feb 7, 2017 •

edited

Loading

matobet Feb 16, 2017 •

edited

Loading