Use OvirtSDK for the provisioning flow #2

borod108 · 2017-04-07T11:27:23Z

Use OvirtSDK for the provisioning flow, this respects use_ovirt_engine_sdk setting
and will not use OvirtSDK if it is off.

This was a common effort with big contribution from @pkliczewski
and help from @jhernand and @masayag

borod108 · 2017-04-07T11:29:18Z

@durandom Moved the PR here.
(@pkliczewski, @oourfali )

borod108 · 2017-04-07T11:41:40Z

@miq-bot assign @durandom
@miq-bot add-label fine/yes

add manageiq-providers-openstack.rb

durandom · 2017-04-10T13:45:44Z

This is a port of ManageIQ/manageiq#14402

durandom · 2017-04-10T14:11:52Z

@gmcculloug @bdunne @syncrou could you review please?

This is to enable usage of v4 of the ovirt api. Part of this was already done in
ManageIQ/manageiq#14398
ManageIQ/manageiq#14399

We should get this into fine to enable customers to opt into v4 and let it be used in general.
Although this PR is quite huge, the v3 part is still using the old v3 code - although refactored into common places.

borod108 · 2017-04-18T07:54:53Z

@durandom Hi! was wondering if there are any news regarding this PR?

mkanoor · 2017-04-18T14:13:08Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+      phase_context[:clone_task_ref] = vm.creation_status_link
+    end
+
+    def nics_for_(vm)


@borod108
is the trailing underscore in the method name intentional?

yes, i just like this style but I can remove it and write nics_for_vm(vm) instead, but the double "vm" thing...

Yes, I never saw a trailing underscore style in our code base (actually not even in ruby projects...)

well rubocop did not complain!
so should I change it?

well rubocop did not complain!

probably no one ever had the idea 😄 - I'd change it

mkanoor · 2017-04-18T14:19:48Z

app/models/manageiq/providers/redhat/infra_manager/inventory/strategies/v4.rb

@@ -17,7 +17,7 @@ def host_targeted_refresh(target)
    end

    def vm_targeted_refresh(target)
-      ems.with_provider_connection(:version => 4) do |connection|
+      @ems.with_provider_connection(:version => 4) do |connection|


@borod108
Could we have used the old code as is because ems is an attr_reader method and it would have always returned @ems.

This also applies to the other @ems changes in this file.

yes definitely, by bad.

gmcculloug · 2017-04-18T14:22:15Z

app/models/manageiq/providers/redhat/infra_manager/inventory/strategies/v4.rb

@@ -87,6 +87,8 @@ def collect_vms
    def collect_vm_by_uuid(uuid)
      vm = connection.system_service.vms_service.vm_service(uuid).get
      [VmPreloadedAttributesDecorator.new(vm, connection)]
+    rescue OvirtSDK4::Error
+      []


Seems odd to ignore this error and return an empty array. Can you explain the reasoning here?

Actually this is currently the right way to check if a VM exists using the oVirt Ruby SDK. The vm_service(uuid) method will always succeed, even if the VM doesn't exist. Calling the get method will then raise an exception, corresponding to the Not found HTTP code. The details of the error are in the exception message which isn't easy to parse.

I'd suggest to keep this code as it is. We will improve the SDK to provide a mechanism to check the kind of error, then this can be improved. For example:

begin ... rescue OvirtSDK4::Error => e if e.code == 404 [] else raise end end

I have opened the following oVirt SDK bug to track it:

[RFE] Add a mechanism to check the HTTP result code of errors
https://bugzilla.redhat.com/1443420

It would be great to rescue a more specific error like OvirtSDK4::ResourceNotFound

Unfortunately, currently this not possible, please refer to Juans answer above, we will do it in future PRs

Please open an issue so that it isn't forgotten @borod108

gmcculloug · 2017-04-18T14:25:51Z

app/models/manageiq/providers/redhat/infra_manager/inventory/strategies/v4.rb

@@ -8,7 +8,7 @@ def initialize(args)
    end

    def host_targeted_refresh(target)
-      ems.with_provider_connection(:version => 4) do |connection|
+      @ems.with_provider_connection(:version => 4) do |connection|


Since this is in the v4.rb file would it make sense to make either the 4 or the whole hash a constant.

VERSION = 4 VERSION_HASH = {:version => VERSION}.freeze ... @ems.with_provider_connection(VERSION_HASH) do |connection|

Oh, good idea!

gmcculloug · 2017-04-18T14:41:02Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+      nic_name = args[:nic_name]
+      interface = args[:interface]
+      vnic = args[:vnic]
+      logger = args[:logger]


I do not see any benefit to mapping most of these single-use variables. Just reference them directory from args in the method. The key names are clear.

Anything being referenced more then once it would make more sense to make a variable.

The benefit of writing like this is that one can easily see args this method can receive.

@borod108 If that's what you're trying to accomplish, you may want to use keyword args

gmcculloug · 2017-04-18T15:00:01Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+      _log.info("#{log_header} Completed.")
+    end
+
+    class NicsDecorator < SimpleDelegator


Suggest moving classes into their own files.

I would like to do this in future refactoring.
There are several more refactoring steps on this code and this will be one of them, but currently will be just a bit easier to do while the classes are still here.

gmcculloug · 2017-04-18T15:00:11Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+      end
+    end
+
+    class GeneralUpdateMethodNamesDecorator < SimpleDelegator


Suggest moving classes into their own files.

syncrou · 2017-04-19T16:01:19Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+
+    def shutdown_guest(operation)
+      operation.with_provider_object(&:shutdown)
+    rescue Ovirt::VmIsNotRunning


@borod108 - I'm not understanding the value in suppressing exceptions, if there is a case for it - should there be some documentation around that case?

This logic was not changed from before. I found many things I do not like but kept the same logic because this is out of the scope of this PR.

@syncrou This pattern was initially added because the desired result in this case is for the VM to be off. If we sent shutdown_guest for a VM that is already off, the API would raise this error. So, it was simplest to just rescue the error and move on.

syncrou · 2017-04-19T16:02:40Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+      vm.with_provider_object do |rhevm_vm|
+        rhevm_vm.start { |action| action.use_cloud_init(true) if cloud_init }
+      end
+    rescue Ovirt::VmAlreadyRunning


Same here in regards to suppressing exceptions

syncrou · 2017-04-19T16:02:51Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+
+    def vm_stop(vm)
+      vm.with_provider_object(&:stop)
+    rescue Ovirt::VmIsNotRunning


syncrou · 2017-04-19T17:14:30Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v4.rb

+
+    def collect_disks_by_hrefs(disks)
+      vm_disks = []
+      ext_management_system.with_provider_connection(:version => 4) do |connection|


Wondering if we should generate a constant for :version => 4 to keep the process consistent.

You are right. Done.

durandom

👍 to get it into fine

the mentioned cleanups will be done in future and smaller PRs

bdunne

In the future, it is much easier to review smaller PR's. If you feel it is necessary to keep all changes in one PR, multiple commits are helpful. I think the respond_to_missing? method is important.

bdunne · 2017-04-21T13:34:44Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+      nic_name = args[:nic_name]
+      interface = args[:interface]
+      vnic = args[:vnic]
+      logger = args[:logger]


@borod108 If that's what you're trying to accomplish, you may want to use keyword args

bdunne · 2017-04-21T13:41:45Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+
+    def shutdown_guest(operation)
+      operation.with_provider_object(&:shutdown)
+    rescue Ovirt::VmIsNotRunning


@syncrou This pattern was initially added because the desired result in this case is for the VM to be off. If we sent shutdown_guest for a VM that is already off, the API would raise this error. So, it was simplest to just rescue the error and move on.

bdunne · 2017-04-21T13:43:03Z

app/models/manageiq/providers/redhat/infra_manager/ovirt_services/strategies/v3.rb

+    end
+
+    class GeneralUpdateMethodNamesDecorator < SimpleDelegator
+      def method_missing(method_name, *args)


You need to also define respond_to_missing?

Please see the comment in the code, the problem is that the ovirt-gem does not actually define the respond_to_missing? so I can't really know without sending the method if it works or not. I do not see a clean way to do this.
In future PR we need to update the ovirt-gem and change this method_missing which is quite ugly. I will then be able to add the respond_to_missing? as well.

Please open an issue so that it is not forgotten @borod108

bdunne · 2017-04-21T14:08:04Z

spec/models/manageiq/providers/redhat/infra_manager/provision/configuration/network_spec.rb

-          {:network => network_name, :mac_address => nil},
-          {:network => network_name}
-        ])
+                                                 {:network => network_name, :mac_address => nil},


Why the whitespace addition?

this is how rubocop liked it...

bdunne · 2017-04-21T18:49:14Z

spec/models/manageiq/providers/redhat/infra_manager/provision/state_machine_spec.rb

@@ -12,6 +12,8 @@
      allow(v).to receive(:with_provider_object).and_yield(rhevm_vm)
      allow(ems).to receive(:with_disk_attachments_service).with(v).and_return(disk_attachments_service)
      allow(ems).to receive(:with_provider_connection).and_return(false)
+      # TODO: (inventory) wirte for v4


typo: write

bdunne · 2017-04-21T19:11:39Z

app/models/manageiq/providers/redhat/infra_manager/provision/configuration/network.rb

-    vnic.nil? ? get_provider_destination.create_nic(options) : vnic.apply_options!(options)
+    ems.ovirt_services.configure_vnic(
+      :vm        => destination,
+      :mac_addr  => mac_addr,


Is mac_address required now? IIRC, if we passed a nil, it would try to set it to nil. That was the reason for the delete_blanks.

It is required for v4, so the delete blanks moved into the ovirt_services/strategies/v3

bdunne · 2017-04-21T19:32:57Z

app/models/manageiq/providers/redhat/infra_manager/inventory/strategies/v4.rb

@@ -87,6 +87,8 @@ def collect_vms
    def collect_vm_by_uuid(uuid)
      vm = connection.system_service.vms_service.vm_service(uuid).get
      [VmPreloadedAttributesDecorator.new(vm, connection)]
+    rescue OvirtSDK4::Error
+      []


It would be great to rescue a more specific error like OvirtSDK4::ResourceNotFound

borod108 · 2017-04-24T06:04:55Z

Please note the PR is currently back to WIP because we want to test fully after all the changes.

miq-bot · 2017-04-24T15:04:11Z

This pull request is not mergeable. Please rebase and repush.

borod108 · 2017-04-24T19:35:39Z

@bdunne thank you for the review! I tried to address your comments.

bdunne

It looks like you merged master into your branch rather than rebasing your branch. This can cause problems when commits on master reference bugzillas since all commits in the PR that have a bugzilla reference will be updated with a link to this PR. In this case I think it's okay ¯\_(ツ)_/¯ since that one commit doesn't have a bugzilla reference, but please be aware of this in the future.

borod108 · 2017-04-25T03:44:02Z

Hi please do not merge for the next few hours, we would like to make a final round of tests and internal review.
@miq-bot add-label wip

@pkliczewski

…e_sdk setting and will not use OvirtSDK if it is off. This was a common effort with big contribution from @pkliczewski and help from @jhernand and @masayag

borod108 · 2017-04-25T04:58:20Z

@miq-bot remove-label wip

miq-bot · 2017-04-25T04:58:23Z

Some comments on commit borod108@83f887a

spec/models/manageiq/providers/redhat/infra_manager/provision/configuration_spec.rb

⚠️ - 13 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 14 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

spec/models/manageiq/providers/redhat/infra_manager/provision_spec.rb

⚠️ - 117 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2017-04-25T04:58:23Z

Some comments on commit borod108@83f887a

spec/models/manageiq/providers/redhat/infra_manager/provision/configuration_spec.rb

⚠️ - 13 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 14 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

spec/models/manageiq/providers/redhat/infra_manager/provision_spec.rb

⚠️ - 117 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2017-04-25T04:58:29Z

durandom · 2017-04-25T06:50:24Z

👏 finally 😅

everybody thanks for your patience and efforts

simaishi · 2017-04-25T12:50:04Z

Fine backport (to manageiq repo) details:

$ git log -1
commit d15e29742e87b34d13ec97904bc9d8971b668b95
Author: Marcel Hild <[email protected]>
Date:   Tue Apr 25 08:48:51 2017 +0200

    Merge pull request #2 from borod108/rfe/new_provider_provisioning
    
    Use OvirtSDK for the provisioning flow
    (cherry picked from commit 88572201c6afd204f255dafe612098e85a582d13)

miq-bot assigned durandom Apr 7, 2017

miq-bot added the fine/yes label Apr 7, 2017

bdunne pushed a commit to bdunne/manageiq-providers-ovirt that referenced this pull request Apr 7, 2017

Merge pull request ManageIQ#2 from durandom/manageiq-providers-openstack

1e0be42

add manageiq-providers-openstack.rb

gmcculloug mentioned this pull request Apr 12, 2017

New provider provisioning for RHV ManageIQ/manageiq#14402

Closed

mkanoor reviewed Apr 18, 2017

View reviewed changes

gmcculloug reviewed Apr 18, 2017

View reviewed changes

borod108 force-pushed the rfe/new_provider_provisioning branch from d540cf2 to af4b65c Compare April 19, 2017 09:49

syncrou reviewed Apr 19, 2017

View reviewed changes

borod108 force-pushed the rfe/new_provider_provisioning branch 2 times, most recently from 3d60fb1 to 940fb86 Compare April 21, 2017 12:17

durandom approved these changes Apr 21, 2017

View reviewed changes

bdunne requested changes Apr 21, 2017

View reviewed changes

borod108 changed the title ~~Use OvirtSDK for the provisioning flow~~ [WIP]Use OvirtSDK for the provisioning flow Apr 23, 2017

miq-bot added the wip label Apr 23, 2017

borod108 force-pushed the rfe/new_provider_provisioning branch 2 times, most recently from b71245b to 66cd51e Compare April 24, 2017 06:01

borod108 force-pushed the rfe/new_provider_provisioning branch from 66cd51e to dee68de Compare April 24, 2017 08:18

miq-bot added unmergeable and removed unmergeable labels Apr 24, 2017

borod108 changed the title ~~[WIP]Use OvirtSDK for the provisioning flow~~ Use OvirtSDK for the provisioning flow Apr 24, 2017

miq-bot removed the wip label Apr 24, 2017

bdunne approved these changes Apr 24, 2017

View reviewed changes

This was referenced Apr 25, 2017

Add specific handeling for exceptions from the OvirtSDK #17

Closed

GeneralUpdateMethodNamesDecorator should implement respond_to_missing? #18

Closed

miq-bot changed the title ~~Use OvirtSDK for the provisioning flow~~ [WIP] Use OvirtSDK for the provisioning flow Apr 25, 2017

miq-bot added the wip label Apr 25, 2017

Use OvirtSDK for the provisioning flow, this respects use_ovirt_engin…

83f887a

…e_sdk setting and will not use OvirtSDK if it is off. This was a common effort with big contribution from @pkliczewski and help from @jhernand and @masayag

borod108 force-pushed the rfe/new_provider_provisioning branch from a2776a7 to 83f887a Compare April 25, 2017 04:54

miq-bot changed the title ~~[WIP] Use OvirtSDK for the provisioning flow~~ Use OvirtSDK for the provisioning flow Apr 25, 2017

miq-bot removed the wip label Apr 25, 2017

durandom added the enhancement label Apr 25, 2017

durandom merged commit 8857220 into ManageIQ:master Apr 25, 2017

durandom added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 25, 2017

simaishi added fine/backported and removed fine/yes labels Apr 25, 2017

borod108 deleted the rfe/new_provider_provisioning branch December 4, 2018 14:57

Use OvirtSDK for the provisioning flow #2

Use OvirtSDK for the provisioning flow #2

Conversation

borod108 commented Apr 7, 2017

borod108 commented Apr 7, 2017

borod108 commented Apr 7, 2017

durandom commented Apr 10, 2017

durandom commented Apr 10, 2017

borod108 commented Apr 18, 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

durandom left a comment

Choose a reason for hiding this comment

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

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

borod108 commented Apr 24, 2017

miq-bot commented Apr 24, 2017

borod108 commented Apr 24, 2017

bdunne left a comment

Choose a reason for hiding this comment

borod108 commented Apr 25, 2017

borod108 commented Apr 25, 2017

miq-bot commented Apr 25, 2017

miq-bot commented Apr 25, 2017

miq-bot commented Apr 25, 2017

durandom commented Apr 25, 2017

simaishi commented Apr 25, 2017