Support moving a VM to another folder during VM Migrate. #17519

lfu · 2018-06-04T18:46:09Z

RHEV v4+ supports VM Migrate. Only supported migration options for RHEV would be shown in the workflow. Folder is one of the unsupported migration options.
SCVMM does not support Migrate.

Includes ManageIQ/manageiq-providers-vmware#285.
~~Includes ManageIQ/manageiq-schema#219.~~
Includes ManageIQ/manageiq-ui-classic#4045.

https://bugzilla.redhat.com/show_bug.cgi?id=1090957

@miq-bot assign @gmcculloug
@miq-bot add_label gaprindashvili/no, enhancement

cc @agrare @bronaghs

agrare · 2018-06-07T18:33:02Z

app/models/ems_folder.rb

+    with_provider_object do |vim_folder|
+      _log.info("#{log_header} Invoking moveIntoFolder ...")
+      vim_folder.send(:moveIntoFolder, vm_mor)
+      _log.info("#{log_header} Invoking moveIntoFolder complete")


Can you use the more common Invoking moveIntoFolder... / Invoking moveIntoFolder...Complete ? That makes it easier to grep for one or both.

agrare

Tested EmsFolder#move_into_folder and it looks good to me, I'll leave the migrate task review to @gmcculloug

agrare · 2018-06-07T19:03:52Z

@lfu #17525 was merged so that should fix your refresh issues when moving the host and folder.

Fryguy · 2018-06-07T20:08:39Z

app/models/ems_folder.rb

@@ -150,6 +150,18 @@ def register_host(host)
    end
  end

+  def move_into_folder(vm)


Why is this implemented at the base ems_folder level instead of in the vmware provider?

I would be ok if this was implemented at the base as "raise NotImplementedError" and in vmware as doing the concrete work.

There isn't a folder class in the VMware provider currently

I'm good with adding one though

Fryguy · 2018-06-07T20:10:12Z

app/models/vm_migrate_task.rb

        vm.relocate(host, respool, datastore, nil, disk_transform)
      end
+
+      folder.move_into_folder(vm) if folder.present?


If you move this into the provider and don't have it at the base, this can be folder.try(:move_into_folder, vm)

lfu · 2018-06-11T18:07:14Z

Travis failed for constant ManageIQ::Providers::Vmware::InfraManager::EmsFolder that is in ManageIQ/manageiq-providers-vmware#285.

Fryguy · 2018-06-19T14:11:16Z

app/models/vm_migrate_task.rb

@@ -66,18 +71,20 @@ def do_request
    begin
      if vc_method == :migrate
        vm.migrate(host, respool)
-      else
+      elsif vc_method == :relocate


Might want to switch to a case statement, but that's minor.

Fryguy · 2018-06-19T14:11:36Z

app/models/vm_migrate_task.rb

        vm.relocate(host, respool, datastore, nil, disk_transform)
      end
+
+      folder.try(:move_into_folder, vm) if folder.present?


no need to check for presence if you are doing .try

Fryguy · 2018-06-19T14:12:27Z

app/models/vm_migrate_task.rb

@@ -57,7 +62,7 @@ def do_request
                  :relocate
                elsif respool && host.nil?
                  :relocate
-                else
+                elsif host
                  :migrate
                end


Since you've removed the catchall else, is there a new catchall?

There is no catchall case here. Only do certain thing when the condition meets.

Fryguy · 2018-06-19T14:14:36Z

spec/factories/ems_folder.rb

@@ -19,27 +19,27 @@
  # VMware specific folders
  #

-  factory :vmware_folder, :parent => :ems_folder do
+  factory :vmware_folder, :parent => :ems_folder, :class => "ManageIQ::Providers::Vmware::InfraManager::Folder" do


if the other vmware_folder objects inherit from this one, then you can set the class once.

Fryguy · 2018-06-19T14:14:50Z

spec/factories/ems_folder.rb

@@ -69,7 +69,7 @@
    hidden true
  end

-  factory :vmware_datacenter, :parent => :vmware_folder, :class => "Datacenter" do
+  factory :vmware_datacenter, :class => "Datacenter" do


I don't like the removal of this. A vmware_datacenter is a vmware_folder, and this is supposed to be used for inheriting the other attributes. Why was this necessary to remove?

Fryguy · 2018-06-19T14:15:39Z

spec/models/vm_migrate_task_spec.rb

+      subject.vm = vm
+      host = FactoryGirl.create(:host, :name => "test")
+      options = {:placement_host_name => [host.id, host.name] }
+      subject.update_attributes(:options => options)


Prefer to inline the options...

subject.update_attributes(:options =>{:placement_host_name => [host.id, host.name]})

Fryguy · 2018-06-19T14:32:29Z

I talked with @agrare about this because the introduction of a leaf-class for folders is being done only for VMware, but not for ovirt nor scvmm, and that concerns me. While we could go the path of also changing ovirt and scvmm to introduce leaf classes, and make it consistent, then update this PR to use that, I think that introduces way too much change for what's necessary.

It looks like the reason for introducing the leaf class is to have a place to put the move_into_folder method. However, I think the implementation is upside-down. While vmware itself implements a folder migrate as a method on the folder object itself, we don't have to follow the same API structuring in our AR modeling. Instead, we could have the move_into_folder method live on the Vm model, accepting an EmsFolder object as a parameter. If we do it this way we don't need to introduce any leaf classes for the folder hierarchy. By not introducing leaf classes, we don't need any data migrations nor changes to the other providers.

agrare · 2018-06-19T14:35:16Z

@Fryguy agreed, we have the VmOrTemplate::Operations::Relocation module which has methods to move the vm to other hosts and/or storages (relocate and migrate) so we could add this method there.

https://bugzilla.redhat.com/show_bug.cgi?id=1090957

miq-bot · 2018-06-19T19:32:31Z

Checked commit lfu@6ec2e3f with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

agrare · 2018-06-20T14:16:33Z

@gmcculloug I merged the VMware PR this depends on, I wonder if we should add some supports_feature_mixin entry for this so that move into folder isn't shown for all providers?

lfu · 2018-06-21T20:24:05Z

Currently SCVMM does not support VM migrate.
For RHEV 4.0+, folder is one of the fields that are excluded from VM Migrate.
So we are good with the folder visibility in the migrate workflow.

gmcculloug · 2018-06-22T20:39:28Z

Thanks @lfu.

@agrare Seems like a good idea overall, but does not appear that we need to hold up this PR to add it. Could be added as a later refactoring.

miq-bot assigned gmcculloug Jun 4, 2018

miq-bot added enhancement gaprindashvili/no labels Jun 4, 2018

lfu mentioned this pull request Jun 4, 2018

Add folder dropdown to VM Migrate. ManageIQ/manageiq-ui-classic#4045

Merged

miq-bot added the wip label Jun 4, 2018

lfu force-pushed the vm_move_folder_1090957 branch from 568b40e to b76fd96 Compare June 7, 2018 14:22

lfu changed the title ~~[WIP] Support moving a VM to another folder during VM Migrate.~~ Support moving a VM to another folder during VM Migrate. Jun 7, 2018

miq-bot removed the wip label Jun 7, 2018

agrare reviewed Jun 7, 2018

View reviewed changes

lfu force-pushed the vm_move_folder_1090957 branch 2 times, most recently from 5bf7e55 to 100569b Compare June 7, 2018 18:45

agrare approved these changes Jun 7, 2018

View reviewed changes

Fryguy reviewed Jun 7, 2018

View reviewed changes

lfu mentioned this pull request Jun 8, 2018

Add method vm_move_into_folder. ManageIQ/manageiq-providers-vmware#285

Merged

lfu force-pushed the vm_move_folder_1090957 branch from 100569b to e9f0e98 Compare June 8, 2018 14:48

lfu mentioned this pull request Jun 8, 2018

Add VMware folder types. ManageIQ/manageiq-schema#219

Closed

lfu force-pushed the vm_move_folder_1090957 branch 3 times, most recently from 20ad78c to f22fcc7 Compare June 14, 2018 16:03

Fryguy reviewed Jun 19, 2018

View reviewed changes

Support moving a VM to another folder during VM Migrate.

6ec2e3f

https://bugzilla.redhat.com/show_bug.cgi?id=1090957

lfu force-pushed the vm_move_folder_1090957 branch from f22fcc7 to 6ec2e3f Compare June 19, 2018 19:28

gmcculloug merged commit adb2759 into ManageIQ:master Jun 22, 2018

gmcculloug added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 22, 2018

gmcculloug added the providers/vmware/infra label Jun 22, 2018

lfu deleted the vm_move_folder_1090957 branch September 29, 2018 14:30

Fryguy added the providers/infrastructure label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support moving a VM to another folder during VM Migrate. #17519

Support moving a VM to another folder during VM Migrate. #17519

lfu commented Jun 4, 2018 •

edited

Loading

agrare Jun 7, 2018

agrare left a comment

agrare commented Jun 7, 2018

Fryguy Jun 7, 2018

Fryguy Jun 7, 2018

agrare Jun 7, 2018

agrare Jun 7, 2018

Fryguy Jun 7, 2018

lfu commented Jun 11, 2018

Fryguy Jun 19, 2018

Fryguy Jun 19, 2018

Fryguy Jun 19, 2018

lfu Jun 19, 2018

Fryguy Jun 19, 2018

Fryguy Jun 19, 2018

Fryguy Jun 19, 2018

Fryguy commented Jun 19, 2018

agrare commented Jun 19, 2018

miq-bot commented Jun 19, 2018

agrare commented Jun 20, 2018

lfu commented Jun 21, 2018

gmcculloug commented Jun 22, 2018

Support moving a VM to another folder during VM Migrate. #17519

Support moving a VM to another folder during VM Migrate. #17519

Conversation

lfu commented Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

agrare left a comment

Choose a reason for hiding this comment

agrare commented Jun 7, 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

lfu commented Jun 11, 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

Fryguy commented Jun 19, 2018

agrare commented Jun 19, 2018

miq-bot commented Jun 19, 2018

agrare commented Jun 20, 2018

lfu commented Jun 21, 2018

gmcculloug commented Jun 22, 2018

lfu commented Jun 4, 2018 •

edited

Loading