rhv: removed the option to migrate the VMs outside of the cluster.

[matobet]

The support for cross cluster migrations was added to oVirt only as a
workaround for el6->el7 migrations but should not be exposed. Since the current
code in manageiq anyway did not work properly, removing the support for it
completely - it is a low level functionality, it is obsoleted and discouraged
to be used.

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

[matobet]

@miq-bot assign @durandom

[miq-bot]

@matobet @durandom is an invalid assignee, ignoring...

[matobet]

@miq-bot assign @himdel

[himdel]

OK, so.. task_supported? is probably the right method to add this to, even though this adds a special case in there, which looks a bit wrong .. but we're asking the provider instead of the record .. so.. yeah, I guess can't be helped :).

any? { |ems| ems.respond_to?(:supports_migrate_for_all?) && !ems.supports_migrate_for_all?(vms) }

But.. if none of the providers has a supports_migrate_for_all method, this doesn't do anything.
Only when a provider has that method and doesn't support all the VMs, it fails.

So .. sounds like surprisingly this lets you migrate the same VM on a provider that's less capable, but not on one that is more. Are you sure that's the correct behaviour? Or is this missing some extra condition?

---

Apart from that, it also feels weird that you generate a flash that it can't be done .. and then do it anyway.

[himdel]

Added pending_backend because this clearly depends on ManageIQ/manageiq#13603

[matobet]

@himdel they way I understand this, the implementation of :supports_migrate_for_all is more of a sign of incapability of a provider than one of a capability. That means "If one of the providers in question can "veto" the operation by its "incapability" the operation needs to be cancelled".

Hope that made sense :-)

[himdel]

@matobet right, makes complete sense, thanks! :) In that case, can you add a small comment explaining that please?

But a question still remains: what happens after you've created that flash message? (I mean, I would have kinda expected to see something like return javascript_render_flash((message)), not just add_flash and continue as if nothing happened, but maybe it's handled somewhere else?)

[matobet]

@himdel not sure how the flash infrastructure works but with with the current add_flash followed by an return statement we get the result we want: flash is displayed and the operation halted.

[himdel]

Alright, I'll assume there's a if flash array not empty, do nothing logic somewhere. :)

[matobet]

@miq-bot add_label euwe/yes

[matobet]

ok now I see this logic could be a bit improved also. We are passing all selected vms to each individual ems to check whether they can be migrated together but IMO each ems should have only say about his own subset of vms. Of course this accidentally works since the RHV implementation checks for the same cluster id which across providers will never hold, but that I don't believe that is the robustness we want :-) Will post a fix.

[himdel]

This These :)

[matobet]

@himdel I guess we could also teach @miq-bot to check grammar in strings of submitted PRs among other things xD

[himdel]

heheh, the horrors it would see.. :)

[himdel]

LGTM, merging when green :)

[miq-bot]

Checked commit matobet@926812b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

[simaishi]

Euwe backport (to manageiq repo) details:

$ git log -1
commit 27c5e9cbaf01ea8b03764df9f683c1017b5130e3
Author: Martin Hradil <[email protected]>
Date:   Tue Jan 24 10:17:12 2017 +0000

    Merge pull request #207 from matobet/tomas-ccm
    
    rhv: removed the option to migrate the VMs outside of the cluster.
    (cherry picked from commit 9e3eb7a95ad8092f661c1436f2c86afc066351f0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1416077