Changing refresh target for vm creation #10934

pkliczewski · 2016-09-01T11:05:52Z

This PR improves performance of RHEV provider when new vm is created and fixes:

https://bugzilla.redhat.com/1069707

pkliczewski · 2016-09-01T11:06:14Z

blomquisg · 2016-09-07T17:48:51Z

@pkliczewski the problem with this current approach is that the event is never saved to the database, because you're circumventing the entire event handling process.

I've looked over what this PR is doing and did a refresher course on the new event handling via automate, and I think I have a suggestion for you.

Add a new method in ems_event/automate.rb called refresh_new_target. Then, a new method in the automation engine called miq_event_action_refresh_new_target. And, then you'd have to add a new event handler fixture similar to the event action refresh handler.

Then, your handlers for user_add_vm and user_add_vm_finished_success could call this new event handler instead of event_action_refresh with the target as the source VM.

This way, you can get rid of the special handling in the event catcher runner that bypasses the standard event handling.

The new method in ems_event/automate.rb called refresh_new_target can do the special handling that's done by refresh_new in this PR. Also, the refresh_new_target method can do a couple things to be cleaner:

Look up manager object from the event, like ems = event.ext_management_system.
Guard against other types of managers accidentally using this with the Support Feature Mixin. The end result could look like:

ems = event.ext_management_system
if ems.supports_refresh_new_target?
  vm_class = ems.class::Vm
  target = vm_class.create_from_event(event)
  EmsRefresh.refresh(target)
end

Now, when the event is caught, it's parsed, saved to the DB, and handled through the normal event handling mechanism. And, this should have removed any (or most) of the RHEV-specific references in the code.

/cc @Fryguy, @durandom thoughts? Is that about right?

pkliczewski · 2016-09-07T18:09:57Z

@blomquisg Thank you for reviewing. Will follow your suggestions

blomquisg · 2016-09-07T19:02:45Z

@pkliczewski wait to see if @durandom or @Fryguy have any other ideas ... I've pinged them both to get some other insight.

durandom · 2016-09-08T14:05:34Z

the problem with this current approach is that the event is never saved to the database

Is that really the case? To me it looks like MiqQueue.put_or_update does put the event on the queue and into the db.
By doing the way through Automate, as @blomquisg suggested, this looks cleaner though and more in line with the current event system. But it still feels like a workaround. I think the Vm should be created through refresh code that generates those hashes and the calls some save_inventory methods...

It does add those methods to the Automate side of things instead of to the event code :) Does this buy us more customizability by users? Or more verbose logging? I dont know :(

pkliczewski · 2016-09-08T14:31:25Z

@durandom We are not able to create Vm through refresh code because it is too late. Limitation of targeted refresh is that we can target only existing Vms. The goal of my work is to target everything since full refresh is very costly.

What we agree on it to create it in refresh worker so we have the same guaranties of db access serialization as refresh code has.

blomquisg · 2016-09-08T14:45:37Z

@durandom the trick with MiqQueue.put_or_update is that it creates a message on the queue. But, we never get anything saved to the event_streams table. Which is where we store all the events (used to be ems_events table).

Bypassing the standard event handling means that we don't store the event in the event_streams and we'll lose it on the timeline.

agrare · 2016-09-08T16:21:00Z

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

@@ -415,6 +411,20 @@ def self.vm_inv_to_hashes(inv, _storage_inv, storage_uids, cluster_uids, host_ui
    return result, result_uids
  end

+  def self.create_vm_hash(data)


Can you pass these in as named parameters? It'll be easier to read than just as an array

@agrare Will do

Fryguy · 2016-09-08T20:16:41Z

@pkliczewski @oourfali In the interest of time, I've transferred this over to @agrare and he is completely up to speed on our discussions.

agrare · 2016-09-08T20:19:56Z

db/fixtures/ae_datastore/ManageIQ/System/Event/EmsEvent/RHEVM.class/user_add_vm.yaml

@@ -9,4 +9,4 @@ object:
    description: 
  fields:
  - rel5:
-      value: "/System/event_handlers/event_action_refresh?target=src_ems_cluster"
+      value: "/System/event_handlers/event_action_refresh?target=src_vm"


So instead of calling event_action_refresh here and having to do some special casing in the event catcher (here) a better approach would be to create a new method like event_action_new_vm

You'll need a builtin automate method similar to miq_event_action_refresh and an EmsEvent method similar to refresh which puts a new_vm action on the queue for the Refresh Worker. This way creating the VM record is done by the Refresh Worker

Sure, will do

pkliczewski · 2016-09-09T12:54:18Z

@agrare @blomquisg @Fryguy @durandom I applied your comments, please review

agrare · 2016-09-09T13:36:39Z

app/models/ems_event/automate.rb

+      ems = ext_management_system
+      if ems.supports_refresh_new_target?
+        vm_class = ems.class::Vm
+        target = vm_class.create_from_event(self)


Okay making progress :) So this is still creating the VM record in the Event Hander, the next step is to put an item on the queue for the Refresh Worker that way we don't have any race conditions with two workers creating a record for the same VM.

agrare · 2016-09-12T12:33:31Z

app/models/ems_refresh.rb

@@ -55,6 +55,10 @@ def self.queue_refresh(target, id = nil, sync = false)
    end
  end

+  def self.queue_event_for_refresh(event, ems)


You'll want to keep the method names consistent, typically the naming pattern for a queued method named refresh_new_target would be queue_refresh_new_target that way its more obvious what is going to get queued.

agrare · 2016-09-30T20:11:33Z

app/models/ems_refresh/save_inventory.rb

+
+  def save_new_target(target_hash)
+    vm_hash = target_hash[:vm]
+    existing_vm = VmOrTemplate.find_by(:ems_ref => vm_hash[:ems_ref])


As we get to creating new targets that aren't VMs we'll want to check for target_hash[:vm].nil?

I added the check

agrare · 2016-09-30T20:12:31Z

Couple of minor things but I think they can be fixed later, @Fryguy can you take a quick look?

miq-bot · 2016-09-30T21:46:12Z

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

Fryguy · 2016-10-03T16:07:27Z

app/models/ems_refresh/save_inventory.rb

+  def save_new_target(target_hash)
+    unless target_hash[:vm].nil?
+      vm_hash = target_hash[:vm]
+      existing_vm = VmOrTemplate.find_by(:ems_ref => vm_hash[:ems_ref])


This needs to be scoped by the ems_id. The ems_ref could be identical (think vms/1) in multiple EMSes.

Fryguy · 2016-10-03T16:08:03Z

app/models/ems_refresh/save_inventory_helper.rb

@@ -149,4 +149,24 @@ def relation_values(association, target)

    top_level && (target == true || target.nil? || parent == target) ? :use_association : []
  end
+
+  def get_cluster(ems, cluster_hash, rp_hash, dc_hash)
+    cluster = EmsCluster.find_by(:ems_ref => cluster_hash[:ems_ref])


Also needs scoping by ems_id

Fryguy · 2016-10-03T16:08:48Z

app/models/ems_refresh/save_inventory_helper.rb

+      cluster.add_resource_pool(rp)
+      cluster.save!
+
+      dc = Datacenter.find_by(:name => dc_hash[:name])


Same on the ems_id. (Also, is there no ems_ref in this table?)

Fryguy · 2016-10-03T16:09:13Z

app/models/ems_refresh/save_inventory_helper.rb

+    cluster = EmsCluster.find_by(:ems_ref => cluster_hash[:ems_ref])
+    if cluster.nil?
+      rp = ems.resource_pools.build(rp_hash)
+      rp.save!


If you are going to save it immediately, then instead of build/save! just call create!

Fryguy · 2016-10-03T16:11:54Z

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

+
+  def self.parse_new_cluster(cluster_ref, cluster_id, cluster_name)
+    {
+      :ems_ref     => cluster_ref,


I'm wondering if the ems_id should be in these other payloads as well for consistency. @agrare Thoughts?

@Fryguy the :ems_id for the new target is given here so that should cover the rest of these (vm, cluster, etc...)

Fryguy · 2016-10-03T16:12:45Z

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

+    dc_name = nil
+    ems.with_provider_connection do |rhevm|
+      dc_name = Ovirt::DataCenter.find_by_href(rhevm, dc_ref).try(:[], :name)
+    end


I think you should be able to do the following, and not have to play games with the variable manipulation.

dc_name = ems.with_provider_connection do |rhevm| Ovirt::DataCenter.find_by_href(rhevm, dc_ref).try(:[], :name) end

agrare · 2016-10-04T13:21:27Z

app/models/ems_refresh/save_inventory_helper.rb

+      cluster.add_resource_pool(rp)
+      cluster.save!
+
+      dc = Datacenter.find_by(:name => dc_hash[:name], :ems_id => ems.id)


@pkliczewski just to confirm, is the name of the datacenter guaranteed unique within an EMS? Also is it not feasible to get the ems_ref for the dc?

The name guarantees uniqueness of the DC in rhev. Potentially we could use ems_ref here if we wanted to.

If possible :ems_ref would be better since it's guaranteed unique within an EMS for all providers and this is common code

sure will change

Thanks @pkliczewski !

When a vm is created we want to tagret this specific vm when running referesh. We used to run full refresh which may take a lot of time. There are 3 events that we want to handle: - USER_ADD_VM - when new vm is created - USER_ADD_VM_FINISHED_SUCCESS - when a vm is created from a template Changing refresh target to vm for above events requires to create a vm before actual refresh is created. In order to mitigate race conditions we want to create new vm just before refresh in EmsRefresh.

miq-bot · 2016-10-04T15:26:59Z

Checked commit pkliczewski@7c2295b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
12 files checked, 4 offenses detected

app/models/ems_refresh/save_inventory.rb

❕ - Line 313, Col 3 - Metrics/AbcSize - Assignment Branch Condition size for save_new_target is too high. [29.48/20]

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

❕ - Line 433, Col 121 - Metrics/LineLength - Line is too long. [137/120]

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

❕ - Line 39, Col 121 - Metrics/LineLength - Line is too long. [125/120]

app/models/mixins/supports_feature_mixin.rb

❗ - Line 79, Col 5 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

agrare · 2016-10-04T16:56:15Z

Great job @pkliczewski

Fryguy · 2016-10-04T17:10:43Z

Great work @pkliczewski !! 🎉

pkliczewski · 2016-10-04T17:45:09Z

Cool!!

Changing refresh target for vm creation (cherry picked from commit 92655f2)

chessbyte · 2016-10-05T13:54:02Z

Euwe Backport details:

$ git log
commit f6b13aa59450a2202b7e5bf40628cec657d62346
Author: Adam Grare <agrare@redhat.com>
Date:   Tue Oct 4 12:55:17 2016 -0400

    Merge pull request #10934 from pkliczewski/master

    Changing refresh target for vm creation
    (cherry picked from commit 92655f24f4a8f7f9c4a6d7584583bb03c0ee6be8)

Changing refresh target for vm creation (cherry picked from commit 92655f2)

miq-bot assigned blomquisg Sep 1, 2016

pkliczewski mentioned this pull request Sep 1, 2016

Targeted refresh improvement for vm creation #10732

Closed

pkliczewski force-pushed the master branch 5 times, most recently from aa592b8 to cfcdff3 Compare September 6, 2016 12:31

Fryguy assigned agrare and unassigned blomquisg Sep 8, 2016

blomquisg added enhancement providers/rhevm labels Sep 8, 2016

agrare reviewed Sep 8, 2016
View reviewed changes

pkliczewski force-pushed the master branch 2 times, most recently from 1eae92f to 8a8eade Compare September 9, 2016 12:12

agrare reviewed Sep 9, 2016
View reviewed changes

pkliczewski force-pushed the master branch 2 times, most recently from 5d58d45 to 8c41a89 Compare September 9, 2016 20:21

agrare reviewed Sep 12, 2016
View reviewed changes

agrare reviewed Sep 30, 2016

View reviewed changes

pkliczewski force-pushed the master branch from de98fb0 to 1c9a269 Compare October 3, 2016 08:31

Fryguy reviewed Oct 3, 2016

View reviewed changes

pkliczewski force-pushed the master branch 2 times, most recently from ca19ccd to d8b252d Compare October 4, 2016 11:46

agrare reviewed Oct 4, 2016

View reviewed changes

pkliczewski force-pushed the master branch from d8b252d to 93472c9 Compare October 4, 2016 15:15

pkliczewski force-pushed the master branch from 93472c9 to 7c2295b Compare October 4, 2016 15:21

agrare merged commit 92655f2 into ManageIQ:master Oct 4, 2016

agrare added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 4, 2016

agrare added the euwe/yes label Oct 4, 2016

chessbyte pushed a commit that referenced this pull request Oct 5, 2016

Merge pull request #10934 from pkliczewski/master

f6b13aa

Changing refresh target for vm creation (cherry picked from commit 92655f2)

chessbyte added euwe/backported and removed euwe/yes labels Oct 5, 2016

agrare mentioned this pull request Nov 2, 2016

Fix openstack network object create to immediately create a manageiq … #12268

Closed

agrare added a commit to agrare/manageiq that referenced this pull request Nov 30, 2016

Merge pull request ManageIQ#10934 from pkliczewski/master

3c2b029

Changing refresh target for vm creation (cherry picked from commit 92655f2)

Fryguy added the providers/infrastructure label Jan 24, 2020

Changing refresh target for vm creation #10934

Changing refresh target for vm creation #10934

Conversation

pkliczewski commented Sep 1, 2016 • edited by miq-bot

pkliczewski commented Sep 1, 2016

blomquisg commented Sep 7, 2016 • edited

pkliczewski commented Sep 7, 2016

blomquisg commented Sep 7, 2016

durandom commented Sep 8, 2016

pkliczewski commented Sep 8, 2016

blomquisg commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Sep 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkliczewski commented Sep 9, 2016

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

agrare commented Sep 30, 2016

miq-bot commented Sep 30, 2016

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

agrare Oct 4, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 4, 2016

agrare commented Oct 4, 2016

Fryguy commented Oct 4, 2016

pkliczewski commented Oct 4, 2016

chessbyte commented Oct 5, 2016

pkliczewski commented Sep 1, 2016 •

edited by miq-bot

blomquisg commented Sep 7, 2016 •

edited

agrare Oct 4, 2016 •

edited