Add Migrate support to RHEV provider #11366

jelkosz · 2016-09-19T14:46:03Z

A simple implementation which adds the support to migrating of virtual machines in the ovirt/rhev.

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

Fryguy · 2016-09-19T15:16:53Z

app/models/manageiq/providers/redhat/infra_manager.rb

+  def vm_migrate(vm, options = {})
+    host_id = URI(options[:host]).path.split('/').last
+
+    mig_options = {


What is mig? Is that migration? If so, prefer spelling it out for readability/clarity.

yes, migration. Changed.

Fryguy · 2016-09-19T15:17:27Z

app/models/manageiq/providers/redhat/infra_manager.rb

+            :id => host_id
+        }
+    }
+    connection = connect(:version => 4)


Prefer with_provider_connection helper which auto-closes.

Fryguy · 2016-09-19T15:18:30Z

app/models/vm_migrate_workflow.rb

+  end
+
+  def field_unsupported(key)
+    @manager.respond_to? :unsupported_migration_options and


Prefer && over and

Fryguy · 2016-09-19T15:19:33Z

@gmcculloug Please review. I'm not sure about the changes for unsupported fields. I feel like they should be at the base class, but you might know better.

jelkosz · 2016-09-20T13:31:19Z

Implement migrate VM MVP

jelkosz · 2016-09-20T13:35:20Z

app/models/manageiq/providers/redhat/infra_manager.rb

+      }
+    }
+
+    with_provider_connection(:version => 4) do |connection|


@borod108 do you know how to behave in case of old api level?

jelkosz · 2016-09-22T07:20:42Z

@borod108 have you ever seen this travis failure? I have no idea if that has anything to do with my patch, it fails on
Errno::ENOENT: No such file or directory @ rb_sysopen - /home/travis/build/ManageIQ/manageiq/log/last_settings.txt

borod108 · 2016-09-22T10:47:36Z

@jelkosz nope, did you try to run tests on local?
To me it does not seem anything you did might cause this, so I would try to fetch master rebase and push again.

miq-bot · 2016-09-26T20:28:23Z

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

jelkosz · 2016-09-29T07:49:08Z

@miq-bot add_label euwe/yes

durandom · 2016-10-10T10:43:19Z

app/models/vm_migrate_workflow.rb

@@ -22,6 +22,8 @@ def get_source_and_targets(refresh = false)
    return @target_resource = {} if ems.length != 1

    result = {:ems => ci_to_hash_struct(ems.first)}
+    @manager = ems.first


I think you can remove this here and inline that below in field_unsupported
Because like this its adding a side effect to this method

I don't see any good way of doing it since the ems is a result of
ems = @values[:src_ids].to_miq_a.collect { |v_id| v = Vm.find_by_id(v_id); v.ext_management_system }.uniq.compact

which I don't really want to repeat in field_unsupported.

But this method anyway have side effects so not sure this is an issue.

how about moving ems into its own method with memoization then?

But lets wait for @mkanoor to chime in

@bdunne
Can you please help review this it relates to RHEVM

durandom · 2016-10-10T10:46:59Z

app/models/vm_migrate_workflow.rb

+    super unless field_unsupported(key)
+  end
+
+  def field_unsupported(key)


@mkanoor can you have a look at this or somebody from your team that has experience with this?
I'm not sure if this is how we handle partially supported fields

oourfali · 2016-10-10T13:10:08Z

live migration

jelkosz · 2016-10-13T07:35:59Z

@miq-bot assign @mkanoor

bdunne

This needs tests

bdunne · 2016-10-18T16:59:56Z

app/models/manageiq/providers/redhat/infra_manager.rb

@@ -100,6 +100,20 @@ def prepare_disk(disk_spec, ems_storage_uid)
    }
  end

+  def vm_migrate(vm, options = {})


Shouldn't this live under the VM operations instead of the manager?

bdunne · 2016-10-18T17:01:12Z

app/models/manageiq/providers/redhat/infra_manager.rb

@@ -118,4 +132,8 @@ def remove_disks(disks, vm)
      disks.each { |disk_id| service.attachment_service(disk_id).remove }
    end
  end
+
+  def unsupported_migration_options


bdunne · 2016-10-18T17:57:48Z

app/models/vm_migrate_workflow.rb

@@ -38,6 +40,15 @@ def get_source_and_targets(refresh = false)
    @target_resource = result
  end

+  def add_target(dialog_key, key, klass, result)
+    super unless field_unsupported(key)


It would be great if logic was positive case rather than the current double negative "unless unsupported"

bdunne · 2016-10-26T20:14:52Z

app/models/manageiq/providers/redhat/infra_manager.rb

@@ -119,4 +119,23 @@ def remove_disks(disks, vm)
      disks.each { |disk_id| service.attachment_service(disk_id).remove }
    end
  end
+
+  def vm_migrate(vm, options = {})


I was expecting migrate to be a VM operation. Any reason that this is on the infra manager instead?

I was trying to make it similar to the vmware provider where it is also placed in the infra manager. Also, it needs to be called from the vm_migrate_workflow

yes, this how our api currently works. VmOrTemaplate#migrate calls VmOrTemaplate#raw_migrate which calls Ems#vm_migrate - so this should work

jelkosz · 2016-10-31T09:12:02Z

@durandom @bdunne so what you guys think?

durandom · 2016-10-31T17:07:29Z

@bdunne I'm fine with the non-automate part. If you think the workflow part is good, I think this can be merged.

bdunne · 2016-10-31T18:46:26Z

@jelkosz There are a couple of remaining rubocop comments. Otherwise looks good 👍

durandom · 2016-11-01T09:54:38Z

@jelkosz thanks for addressing the rubocop stuff

@gmcculloug can you merge this please? @bdunne and me are 👍

miq-bot · 2016-11-03T11:58:19Z

Checked commits jelkosz/manageiq@1581e45~...bc4c103 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
6 files checked, 0 offenses detected
Everything looks good. 🏆

jelkosz · 2016-11-03T13:53:13Z

@gmcculloug @bdunne Only rebased and added a check to allow this code only for api version 4. The travis failure is not related, all commits are failing on this same issue.

jelkosz · 2016-11-03T14:03:14Z

@miq-bot add_label blocker

gmcculloug · 2016-11-03T18:44:19Z

@jelkosz Is there a BZ related to this change?

jelkosz · 2016-11-03T19:43:18Z

@gmcculloug here you go: https://bugzilla.redhat.com/show_bug.cgi?id=1373079

chessbyte · 2016-11-08T17:35:39Z

Euwe BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1392005

Add Migrate support to RHEV provider (cherry picked from commit 99adc9d) https://bugzilla.redhat.com/show_bug.cgi?id=1392005

chessbyte · 2016-11-08T17:44:53Z

Euwe Backport details:

$ git log -1
commit ab440c5fe20e3e35ceb5e2443b0ae1250b032ee5
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Nov 3 14:43:17 2016 -0400

    Merge pull request #11366 from jelkosz/add-migrate-to-rhev-provider

    Add Migrate support to RHEV provider
    (cherry picked from commit 99adc9df0da7a25bfd220a153680cef6fc9e767c)

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

Fryguy reviewed Sep 19, 2016

View reviewed changes

Fryguy assigned gmcculloug Sep 19, 2016

jelkosz force-pushed the add-migrate-to-rhev-provider branch 2 times, most recently from 447c1e4 to 7914d29 Compare September 20, 2016 11:18

jelkosz commented Sep 20, 2016

View reviewed changes

jelkosz force-pushed the add-migrate-to-rhev-provider branch from 7914d29 to 503c80e Compare September 21, 2016 11:04

chessbyte changed the title ~~added support of migrate to redhat provider~~ added support of migrate to rhev provider Sep 22, 2016

chessbyte changed the title ~~added support of migrate to rhev provider~~ Add Migrate support to RHEV provider Sep 22, 2016

chessbyte added providers/rhevm enhancement labels Sep 22, 2016

jelkosz force-pushed the add-migrate-to-rhev-provider branch 3 times, most recently from bcae041 to 80b556c Compare September 23, 2016 12:26

jelkosz force-pushed the add-migrate-to-rhev-provider branch from 80b556c to 97c459c Compare September 29, 2016 07:07

miq-bot added the euwe/yes label Sep 29, 2016

durandom reviewed Oct 10, 2016

View reviewed changes

miq-bot assigned mkanoor and unassigned gmcculloug Oct 13, 2016

bdunne requested changes Oct 18, 2016

View reviewed changes

gmcculloug unassigned mkanoor Oct 18, 2016

gmcculloug self-assigned this Oct 18, 2016

jelkosz force-pushed the add-migrate-to-rhev-provider branch 2 times, most recently from 028ab6d to b8283f7 Compare October 25, 2016 08:11

bdunne reviewed Oct 26, 2016

View reviewed changes

bdunne approved these changes Oct 31, 2016

View reviewed changes

jelkosz force-pushed the add-migrate-to-rhev-provider branch from b8283f7 to 3866681 Compare November 1, 2016 08:42

Tomas Jelinek added 2 commits November 3, 2016 12:53

Add Migrate support to RHEV provider

1581e45

Allow migrate only for api level 4

bc4c103

jelkosz force-pushed the add-migrate-to-rhev-provider branch from 3866681 to bc4c103 Compare November 3, 2016 11:55

miq-bot added the blocker label Nov 3, 2016

gmcculloug merged commit 99adc9d into ManageIQ:master Nov 3, 2016

gmcculloug added this to the Sprint 49 Ending Nov 14, 2016 milestone Nov 3, 2016

chessbyte pushed a commit that referenced this pull request Nov 8, 2016

Merge pull request #11366 from jelkosz/add-migrate-to-rhev-provider

ab440c5

Add Migrate support to RHEV provider (cherry picked from commit 99adc9d) https://bugzilla.redhat.com/show_bug.cgi?id=1392005

chessbyte added euwe/backported and removed euwe/yes labels Nov 8, 2016

Fryguy added the providers/infrastructure label Jan 24, 2020

Add Migrate support to RHEV provider #11366

Add Migrate support to RHEV provider #11366

Conversation

jelkosz commented Sep 19, 2016 • edited by gmcculloug 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

Fryguy commented Sep 19, 2016

jelkosz commented Sep 20, 2016

Choose a reason for hiding this comment

jelkosz commented Sep 22, 2016

borod108 commented Sep 22, 2016

miq-bot commented Sep 26, 2016

jelkosz commented Sep 29, 2016

durandom Oct 10, 2016 • 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

oourfali commented Oct 10, 2016

jelkosz commented Oct 13, 2016

bdunne 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

jelkosz commented Oct 31, 2016

durandom commented Oct 31, 2016

bdunne commented Oct 31, 2016

durandom commented Nov 1, 2016

miq-bot commented Nov 3, 2016

jelkosz commented Nov 3, 2016

jelkosz commented Nov 3, 2016

gmcculloug commented Nov 3, 2016

jelkosz commented Nov 3, 2016

chessbyte commented Nov 8, 2016

chessbyte commented Nov 8, 2016

jelkosz commented Sep 19, 2016 •

edited by gmcculloug

Loading

durandom Oct 10, 2016 •

edited

Loading