Support VM Reconfigure for VMware vCloud provider

[miha-plesko]

With this commit we add support to reconfigure VM hardware for VMware vCloud provider. Different configuration is needed for VMware vSphere (infra) than for VMware vCloud (cloud) so we cannot rely on vm.vendor = vmware attribute anymore, which is common to them both... Instead, we rely on VM class name.

Also, there seems to be a bug in ruby controller which tries to render flash message AFTER redirect was invoked. This results in ugly rails crash. Solved by removing the flash rendering part.

Video: http://x.k00.fr/reconfigure
Depends on: ManageIQ/manageiq-providers-vmware#231
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1572086

@miq-bot add_label enhancement,gaprindashvili/yes
@miq-bot assign @martinpovolny

[miha-plesko]

Removed WIP tag as this PR is done even if it's pending core. @martinpovolny I kindly ask for review even if core is not merged yet, to speed up the process.

[miha-plesko]

I have no idea why JavaScript spec is failing, doesn't seem to be related with this PR. @martinpovolny do you perhaps know what could be the cause?

[martinpovolny]

I restarted travis, now it's green.

@himdel, @AparnaKarve, @Hyperkid123 : can you, please, review this PR, it's out of my area of expertise.

Thx!

[himdel]

This bit is surprising, why would you remove a flash message?

[miha-plesko]

Thanks for asking. This flash rendering breaks redirect as far as I've tested because it happend AFTER redirecting to other page. See the condition in L352:

if ...
   javascript_redirect(...)
else
   javascript_redirect(...)
end

There is no way we can arrive to L357 without callind redirect first. Or am I missing something? Looking forward to your answer, I'm curious how this can work for other providers. Most probably it doesn't :)

[himdel]

Aah... I think you're right.. A few lines above, there is a flash_to_session call which saves the flash messages for later.

Since both branches do javascript_redirect, unconditionally, I agree this is broken.
And since flash_to_session I think we can expect this won't hide any flash messages that would otherwise appear.

👍

[himdel]

The angular changes look good, I don't see a problem there.

Where I do see a problem is

-            %td{"ng-if" => @reconfigitems.first.vendor == 'vmware'}
+            %td{"ng-if" => "vm.vm_type.includes('Vmware::InfraManager')"}

We should never expect the class name to be meaningful like this and making the UI depend on the exact format of class names sounds wrong.

If you need to distinguish between infra and cloud, we need a method on the provider(/vm/whatever?) class.
(Something similar to vendor really.)

[miha-plesko]

@agrare I kindly ask for your suggestion here. Question is: what field to use on Vm instance to tell whether it's VMware infra or cloud. Unfortunately they both have vendor == 'vmware'. I see two possibilities:

a) use vm.ext_management_system.ems_type which is vmware_cloud for cloud and vmwarews for infra
b) introduce auxilary method on ManageIQ::Providers::Vmware::CloudManager::Vm e.g. is_cloud?

Approach (a) will result in additional VMDB query while approach (b) pollutes the code... WDYT?

[agrare]

I'd probably go with checking the VM's ems_type, something like is_cloud? on a CloudManager::Vm just seems strange. @himdel the UI doesn't do any class/kind_of/is_a checks?

We can't backport it because it would require a data migration but we could always set the vendor to "vcloud" which would match how we handle SCVMM.

[himdel]

@himdel the UI doesn't do any class/kind_of/is_a checks?

@agrare It's more like the UI has been trying to remove any mentions of provider classes ever since Provider pluggability became a goal. You can't have provider pluggability and still rely on class names.

So, I'm not saying the check would be a new thing in the UI, but I am saying that at some point, we will need to remove it, so why would we want to introduce it in the first place.

As for CloudManager::Vm#is_cloud? - it sounds perfectly reasonable to me if there is no other way of telling.

The main point is.. the UI can't have the knowledge that ManageIQ::Provider::Vmware::CloudManager::Vm is a cloud thing, otherwise you can't really add a provider and expect it to work without changing the UI.

[agrare]

@himdel yeah I get where you're coming from there, but checking for vendor which should be 1:1 with the type when determining if you should show something seems like the same thing as checking for the type.

Not all cloud vms are going to support reconfigure, so it would have to be something like vm.vendor == "vmware" && vm.is_cloud?

The pluggable thing to do would be to ask the vm if you support reconfigure disk remove backing file before showing that slider no?

[miha-plesko]

Careful now, I'm not sure we want to have a dozen of methods on VM like:

def reconfigure_hdType?
def reconfigure_hdMode?
def reconfigure_new_controller_type?
def reconfigure_hdSize?
def reconfigure_hdUnit?
...

Or do we?

[agrare]

@miha-plesko yeah that's kind of my point, if we ask for every little thing it is going to get out of hand.

Ideally in the foreman plugin sense the provider would might their own reconfigure UI, these options are pretty specific to the vSphere provider.

[miha-plesko]

Coming back from future into present time with approaching deadline for 4.6.3 - @himdel would you agree to go with option (a) then where we'd check like this:

vm.vm_type == 'vmware_cloud'

where rails would set vm.vm_type = @reconfigitems.first.ext_management_system.ems for angular?

[himdel]

Yeah, as long as we're trying to go in the right direction... :)

vm.vm_type == 'vmware_cloud' works fine :)

(Also, if we relax the "no class" restriction a bit, we could still rely on classes defined in the core - so ManageIQ::Providers, as long as it's not ManageIQ::Providers::$Vendor)

So, another option would be to test if the Vm is a descendant of CloudManager::Vm and has vendor == 'vmware'. (If that helps with the extra db query.)

[himdel]

The pluggable thing to do would be to ask the vm if you support reconfigure disk remove backing file before showing that slider no?

That would be perfect, yes.

(I'm thinking asking for every single field is wrong, but.. most likely they can be described by a common property ... supports_reconfigure_disk_stuff? :))

[miha-plesko]

Thanks for review @himdel and discussion @agrare , PR is now updated. I need to test GUI once again when vmware PR gets merged.

[agrare]

@miha-plesko vmware provider PR merged

[miha-plesko]

Tested once again and of course there was some small error so I'm really glad I did :) Everything seems to be working now, @himdel looking forward to any further comments you might have.

[miha-plesko]

@himdel don't want to be too pushy but there is a followup PR already implemented to also support VM's network adapters reconfiguration #3972 and it depends on this one here ;)

[miha-plesko]

@miq-bot remove_label pending core

[miq-bot]

Checked commit miha-plesko@a79575d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 1 offense detected

**

• 💣 💥 🔥 🚒 - Linter/Haml - Linter::Haml STDERR:

warning: parser/current is loading parser/ruby23, which recognizes
warning: 2.3.6-compliant syntax, but you are running 2.3.3.
warning: please see https://github.com/whitequark/parser#compatibility-with-ruby-mri.

[miha-plesko]

I've slightly updated the Jasmine spec. There is plenty of room for testing the controller, but at least we can now be sure initialization went okay...

@miq-bot assign @himdel

[himdel]

LGTM, thanks! :)

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit ae5b7d1502eb0a41141a32420d7bb4680857f154
Author: Martin Hradil <himdel@seznam.cz>
Date:   Thu May 24 16:08:02 2018 +0200

    Merge pull request #3854 from miha-plesko/vcloud-reconfigure
    
    Support VM Reconfigure for VMware vCloud provider
    (cherry picked from commit 9a05d39aeef8d272fd2338ccb4fa659e849302bb)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1584699