Use the new OvirtSDK for refresh #14398

borod108 · 2017-03-19T12:40:09Z

If the ovirt provider supports version 4 of the api use the new
OvirtSDK to do refresh and targeted refresh.

(This is a first in a series of PRs that will allow using the OvirtSDK with providers that support version 4 for refresh, event handling and provisioning.)

If the ovirt provider supports version 4 of the api use the new OvirtSDK to do refresh and targeted refresh.

borod108 · 2017-03-19T21:11:41Z

@pkliczewski @masayag please review.

Add a setting that by default will disable the use of the OvirtSDK for refresh and provisioning.

Fryguy · 2017-03-20T15:05:56Z

config/settings.yml

@@ -101,6 +101,7 @@
      :omit_default_port: true
      :read_timeout: 60
  :ems_redhat:
+    :use_ovirt_engine_sdk: false


Not a fan of adding another option. When would a customer ever turn this on?

The idea is that we want to get the code in and allow people who want to try it to use it but if there are problems they can turn it off. Its off by default as a safety measure.
One might want to turn this on if he wants better performance due to the improved sdk for example.
When it is tested enough the default will be true and when the old api is not supported anymore it will just be removed.

@Fryguy @blomquisg exactly. This feature toggle is basically the same approach we took for the graph refresh in amazon.

durandom

Basically ok with the feature toggle approach.

@borod108 could you explain a bit, how the current refresh code is still used? Although you dont delete much existing code you still introduce a bunch of V3 code...

other suggestions as comments

@blomquisg this is intended to go into Fine. In the end this is you call, but we discussed it's ok with the feature toggle approach

durandom · 2017-03-20T16:53:15Z

config/settings.yml

@@ -101,6 +101,7 @@
      :omit_default_port: true
      :read_timeout: 60
  :ems_redhat:
+    :use_ovirt_engine_sdk: false


@Fryguy @blomquisg exactly. This feature toggle is basically the same approach we took for the graph refresh in amazon.

durandom · 2017-03-20T16:54:25Z

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_4_spec.rb

@@ -9,6 +9,7 @@
    @ems.default_endpoint.path = "/ovirt-engine/api"
    allow(@ems).to receive(:supported_api_versions).and_return([3, 4])
    allow(@ems).to receive(:resolve_ip_address).with(ip_address).and_return(ip_address)
+    ::Settings.ems.ems_redhat.use_ovirt_engine_sdk = true


this should use stub_settings - otherwise you are modifying the Settings singleton and it will be true for others as well

durandom · 2017-03-20T16:54:57Z

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_4_1_spec.rb

@@ -5,6 +5,7 @@
                              :port => 8443)
    @ems.update_authentication(:default => {:userid => "admin@internal", :password => "123456"})
    allow(@ems).to receive(:supported_api_versions).and_return([3, 4])
+    ::Settings.ems.ems_redhat.use_ovirt_engine_sdk = true


stub_settings too - see below

durandom · 2017-03-20T16:55:25Z

spec/models/manageiq/providers/redhat/infra_manager/refresh/parse/parser_builder_spec.rb

+  subject { described_class.new(ems, options).build }
+  describe 'chooses the right parsing strategy' do
+    before do
+      ::Settings.ems.ems_redhat.use_ovirt_engine_sdk = use_ovirt_engine_sdk


stub_settings - see below

durandom · 2017-03-20T17:01:03Z

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

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


I'd use the attr_reader ems if you already have it

durandom · 2017-03-20T17:04:01Z

...els/manageiq/providers/redhat/infra_manager/refresh/parse/strategies/datacenter_inventory.rb

@@ -0,0 +1,63 @@
+module ManageIQ::Providers::Redhat::InfraManager::Refresh::Parse::Strategies
+  class DatacenterInventory
+    attr_reader :datacenter_inv, :_log


_log will mask _log here

miq-bot · 2017-03-22T10:51:31Z

Some comments on commits borod108/manageiq@ccb1cf8~...7e639d3

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_4_1_spec.rb

⚠️ - 23 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 25 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 27 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 29 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 31 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 33 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 35 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 37 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 38 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_target_vm_4_spec.rb

⚠️ - 27 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 29 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 31 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 33 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 35 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 37 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
⚠️ - 38 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2017-03-22T10:52:47Z

Checked commits borod108/manageiq@ccb1cf8~...7e639d3 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
22 files checked, 41 offenses detected

app/models/manageiq/providers/redhat/infra_manager/refresh/parse/strategies/vm_inventory.rb

❗ - Line 186, Col 7 - Style/MultilineBlockChain - Avoid multi-line chains of blocks.

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

spec/models/manageiq/providers/redhat/infra_manager/refresh/parse/parser_builder_spec.rb

❗ - Line 28, Col 46 - Style/ExtraSpacing - Unnecessary spacing detected.
❗ - Line 34, Col 1 - Style/EmptyLinesAroundBlockBody - Extra empty line detected at block body end.
❗ - Line 47, Col 46 - Style/ExtraSpacing - Unnecessary spacing detected.

spec/models/manageiq/providers/redhat/infra_manager/refresh/refresher_4_1_spec.rb

❗ - Line 653, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 655, Col 13 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 671, Col 9 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 675, Col 17 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 676, Col 17 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 677, Col 17 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 678, Col 17 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 679, Col 17 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 680, Col 17 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

borod108 · 2017-03-24T17:40:08Z

@Fryguy @blomquisg @durandom please note that when the setting use_ovirt_engine_sdk is set to false it the code that will be used is the old code for both collecting th einventory and parsing.

A little more details: this: https://github.com/ManageIQ/manageiq/pull/14398/files#diff-d6125c21d43685840429020f8c5a57aaR233
will return "3"
and so this: https://github.com/ManageIQ/manageiq/pull/14398/files#diff-119cc54f84ee5e4e6e981fc798938f8aR12 will use the parser api3 version which is exactly the old code for parsing.
and for refreshing the same happens here: https://github.com/ManageIQ/manageiq/pull/14398/files#diff-56e54a726f15e610eec31cddfb4b0682
the refresher just hold the targeted_refresh functions which were moved there and inventory_from_ovirt(ems) is overridden in api4 of the refresher but returns the old Ovirt::Inventory for
version api3 of the refresher.
So when the switch is turned off (as by default) the code that does the refresh and parsing is actually the same.
The uni-tests pass for both versions and we have being testing this manually as well for a while now.

durandom

👍
QE also tested this part extensively and found no regressions

@blomquisg please merge

durandom · 2017-03-27T10:35:51Z

@miq-bot add_labels enhancement

@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 This PR is based on: ManageIQ#14398

borod108 force-pushed the rfe/new_provider_refresh branch from 5c2dab5 to dd43a5f Compare March 19, 2017 14:13

Use the new OvitSDK for refresh

ccb1cf8

If the ovirt provider supports version 4 of the api use the new OvirtSDK to do refresh and targeted refresh.

borod108 force-pushed the rfe/new_provider_refresh branch from dd43a5f to ccb1cf8 Compare March 19, 2017 14:19

borod108 changed the title ~~[WIP] Use the new OvitSDK for refresh~~ Use the new OvitSDK for refresh Mar 19, 2017

borod108 mentioned this pull request Mar 19, 2017

New provider event parsing #14399

Merged

Add setting to disable OvirtSDK

d2cbf43

Add a setting that by default will disable the use of the OvirtSDK for refresh and provisioning.

borod108 force-pushed the rfe/new_provider_refresh branch from 12c5514 to d2cbf43 Compare March 20, 2017 11:29

chessbyte changed the title ~~Use the new OvitSDK for refresh~~ Use the new OvirtSDK for refresh Mar 20, 2017

chessbyte assigned oourfali Mar 20, 2017

chessbyte added the providers/rhevm label Mar 20, 2017

Fryguy reviewed Mar 20, 2017

View reviewed changes

durandom suggested changes Mar 20, 2017

View reviewed changes

Fix tests that use ::Settigns and some of the rubocops

7e639d3

borod108 mentioned this pull request Mar 27, 2017

New provider provisioning for RHV #14402

Closed

durandom approved these changes Mar 27, 2017

View reviewed changes

miq-bot added the enhancement label Mar 27, 2017

blomquisg merged commit e7656ce into ManageIQ:master Mar 27, 2017

blomquisg added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 27, 2017

durandom mentioned this pull request Apr 10, 2017

Use OvirtSDK for the provisioning flow ManageIQ/manageiq-providers-ovirt#2

Merged

Fryguy added the providers/infrastructure label Jan 24, 2020

Fryguy unassigned oourfali Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the new OvirtSDK for refresh #14398

Use the new OvirtSDK for refresh #14398

borod108 commented Mar 19, 2017

borod108 commented Mar 19, 2017

Fryguy Mar 20, 2017

borod108 Mar 20, 2017

Fryguy Mar 20, 2017

durandom Mar 20, 2017

durandom left a comment

durandom Mar 20, 2017

durandom Mar 20, 2017

durandom Mar 20, 2017

borod108 Mar 22, 2017

durandom Mar 20, 2017

borod108 Mar 22, 2017

durandom Mar 20, 2017

borod108 Mar 22, 2017

durandom Mar 20, 2017

borod108 Mar 22, 2017

miq-bot commented Mar 22, 2017

miq-bot commented Mar 22, 2017

borod108 commented Mar 24, 2017

durandom left a comment

durandom commented Mar 27, 2017

Use the new OvirtSDK for refresh #14398

Use the new OvirtSDK for refresh #14398

Conversation

borod108 commented Mar 19, 2017

borod108 commented Mar 19, 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

durandom 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

miq-bot commented Mar 22, 2017

miq-bot commented Mar 22, 2017

borod108 commented Mar 24, 2017

durandom left a comment

Choose a reason for hiding this comment

durandom commented Mar 27, 2017