Refactor refresh new target to use save_ems_inventory #14247

agrare · 2017-03-09T16:09:55Z

By adding the ability for save_ems_inventory to work with a nil target we can delete the specialized save_new_target method which only worked for RHEV VMs and replace it with the general save_ems_inventory method.

Now any links that the caller wants to be made can be made, and any linked inventory that doesn't exist will be created just like a normal save_inventory would, so we can get rid of the specific calls to find or create linked inventory here

manageiq/app/models/ems_refresh/save_inventory_helper.rb

Lines 157 to 164 in c9e9ae1

    
           cluster = EmsCluster.find_by(:ems_ref => cluster_hash[:ems_ref], :ems_id => ems.id) 
        
           if cluster.nil? 
        
             rp = ems.resource_pools.create!(rp_hash) 
        
             cluster = ems.clusters.create!(cluster_hash) 
        
             cluster.add_resource_pool(rp) 
        
             cluster.save!

Update RHV parse_new_event (Update parse_new_target to standard refresh hashes manageiq-providers-ovirt#13)
Update VMware obj_update_to_hash Update for new refresh_new_target arguments manageiq-providers-vmware#55

miq-bot · 2017-03-09T16:56:38Z

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

agrare · 2017-03-09T20:24:01Z

@Fryguy this replaces save_new_target with generic save_ems_inventory for refresh_new_target, what do you think of the mode parameter to control the disconnects? I think it would be cleaner to not do target = ems if target.nil? everywhere but that could have some nasty side-effects if a caller is relying on that.

miq-bot · 2017-03-16T19:02:12Z

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

Fryguy · 2017-03-20T21:45:06Z

@Fryguy this replaces save_new_target with generic save_ems_inventory for refresh_new_target, what do you think of the mode parameter to control the disconnects? I think it would be cleaner to not do target = ems if target.nil? everywhere but that could have some nasty side-effects if a caller is relying on that.

It feels quite invasive, so I'm not sure. I can't see any other way to accomplish what you are trying to do though, so I'm good with it.

durandom

Looks great, from what I understand :)

TODO: need to rework the RHEV EventParser.parse_new_target to return the correct hash format

so, only RHV is using this currently anyway, right? Could we trigger them to produce a spec for their use case in order to have this PR here failing? I think their refresh_new_target would fail with this merged, right?

durandom · 2017-04-06T13:33:21Z

app/models/ems_refresh/save_inventory.rb

@@ -1,5 +1,5 @@
 module EmsRefresh::SaveInventory
-  def save_ems_inventory(ems, hashes, target = nil)
+  def save_ems_inventory(ems, hashes, target = nil, mode = :refresh)


how about a new method e.g. save_ems_inventory_no_disconnect which just calls save_ems_inventory with a mode other than :refresh. This would save you from a 4-ary method signature like

save_ems_inventory(ems, target_hash, nil, :refresh_new_target)

Good idea :) I'll give that a try, should clean this up a lot

agrare · 2017-04-06T13:47:46Z

so, only RHV is using this currently anyway, right?

I'm planning on using it for #14460 as well

Could we trigger them to produce a spec for their use case in order to have this PR here failing

YES! I'm planning on adding a RHEV save_new_target spec using one of their events

I think their refresh_new_target would fail with this merged, right?

Correct this cannot be merged until we update their event parser to work with this format

miq-bot · 2017-04-06T21:37:27Z

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

agrare · 2017-04-18T19:29:14Z

Here is the associated ovirt PR ManageIQ/manageiq-providers-ovirt#13
I'll add the tests for the ovirt event parser there, just need the RHV team to get an event hash

chessbyte · 2017-06-02T03:52:26Z

@blomquisg bump

cben · 2017-06-04T08:50:48Z

app/models/ems_refresh/save_inventory_infra.rb

-  def save_resource_pools_inventory(ems, hashes, target = nil)
-    target = ems if target.nil?
+  def save_resource_pools_inventory(ems, hashes, target = nil, disconnect = true)
+    target = ems if target.nil? && disconnect


Why && disconnect here and not elsewhere?

Because the disconnect logic for resource pools is more complex than the normal if target == ems then :use_association else []
The target has to be nil if disconnect is true so that we don't hit this case. I had to do the same for hosts and vms because their disconnect logic is special.

durandom · 2017-06-07T10:24:34Z

@agrare seems vmware doesnt have parse_new_target?!

agrare · 2017-06-07T10:53:08Z

@durandom ManageIQ/manageiq-providers-vmware#55 adds it

blomquisg

One small quibble on a method name. Otherwise, I can understand what's going on here, which is saying quite a bit :)

blomquisg · 2017-06-14T17:39:05Z

app/models/ems_refresh/save_inventory_helper.rb

-      dc = Datacenter.find_by(:ems_ref => dc_hash[:ems_ref], :ems_id => ems.id)
-      dc.add_cluster(cluster)
-      dc.save!
+  def disconnect_using_association(ems, target, disconnect = true)


The method name makes it sound like it's going to do the disconnect.

What about:

calculate_disconnects

determine_disconnects

find_disconnects (but that has an AR overlap)

Something like that?

Hmm good point, how about determine_deletes_using_association? Most callers set deletes = ... so deletes might be better than disconnects anyway.

miq-bot · 2017-06-14T18:08:39Z

Checked commits agrare/manageiq@4444d75~...bf433bb with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
11 files checked, 0 offenses detected
Everything looks fine. 👍

Now that ManageIQ/manageiq#14247 is merged we can enable the refresh_new_target test for the vmware refresher

After ManageIQ/manageiq#14247 was merged there are no more callers of obj_update_to_hash so it can be removed

agrare added providers refactoring wip labels Mar 9, 2017

miq-bot added the unmergeable label Mar 9, 2017

agrare force-pushed the refactor_refresh_new_target branch from 055482f to 0de059d Compare March 9, 2017 17:59

miq-bot removed the unmergeable label Mar 9, 2017

agrare force-pushed the refactor_refresh_new_target branch 2 times, most recently from 7d1e5a1 to addf634 Compare March 9, 2017 18:11

miq-bot added unmergeable and removed unmergeable labels Mar 16, 2017

agrare force-pushed the refactor_refresh_new_target branch from 303cdc2 to f303838 Compare March 16, 2017 19:25

agrare force-pushed the refactor_refresh_new_target branch from f303838 to 90cfdc4 Compare March 22, 2017 15:56

agrare mentioned this pull request Mar 22, 2017

New folder targeted refresh [Depends on vmware/32] #14460

Merged

chessbyte requested a review from durandom April 4, 2017 18:09

chessbyte assigned blomquisg Apr 4, 2017

durandom reviewed Apr 6, 2017

View reviewed changes

miq-bot added the unmergeable label Apr 6, 2017

agrare force-pushed the refactor_refresh_new_target branch from 90cfdc4 to dd55039 Compare April 18, 2017 18:50

miq-bot removed the unmergeable label Apr 18, 2017

agrare force-pushed the refactor_refresh_new_target branch 4 times, most recently from 2907054 to dae48ee Compare April 19, 2017 13:49

agrare changed the title ~~[WIP] Refactor refresh new target to use save_ems_inventory~~ Refactor refresh new target to use save_ems_inventory May 10, 2017

agrare removed the wip label May 10, 2017

cben reviewed Jun 4, 2017

View reviewed changes

agrare force-pushed the refactor_refresh_new_target branch from 40fbcf2 to 4a451ff Compare June 7, 2017 13:29

blomquisg approved these changes Jun 14, 2017

View reviewed changes

agrare added 14 commits June 14, 2017 13:55

Add a mode param to control disconnects

4444d75

Don't disconnect anything unless mode is refresh

55568e5

Add a way to find the new target to refresh_new_target

0accaeb

Use save_ems_inventory in refresh_new_target

c3b5ad2

Remove save_new_target method

d71a8a5

Add test to link up new VM to existing host

0e65a3d

Add test targeting an existing vm

36b33ef

Test adopting an archived vm in refresh_new_target

049e731

Add save_ems_inventory_no_disconnect method

9d0a94d

Add disconnect to save_inventory_cloud

74c6349

Add disconnect with assoc helper method

278af66

Update obj_event_to_hash for new refresh_new_target args

3bff1aa

Constantize target_class and return new target

39d9c0c

Use parse_new_target for miq_vim_broker new event

bf433bb

agrare force-pushed the refactor_refresh_new_target branch from 4a451ff to bf433bb Compare June 14, 2017 18:04

Fryguy merged commit d00fbdb into ManageIQ:master Jun 16, 2017

Fryguy added this to the Sprint 63 Ending Jun 19, 2017 milestone Jun 16, 2017

agrare deleted the refactor_refresh_new_target branch June 16, 2017 17:12

agrare added a commit to agrare/manageiq-providers-vmware that referenced this pull request Jun 19, 2017

Enable refresh_new_target test

ea65d0a

Now that ManageIQ/manageiq#14247 is merged we can enable the refresh_new_target test for the vmware refresher

agrare added a commit to agrare/manageiq-providers-vmware that referenced this pull request Jun 19, 2017

Remove compatibility method obj_update_to_hash

aae986d

After ManageIQ/manageiq#14247 was merged there are no more callers of obj_update_to_hash so it can be removed

agrare mentioned this pull request Jun 19, 2017

Enable refresh new target spec test ManageIQ/manageiq-providers-vmware#67

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor refresh new target to use save_ems_inventory #14247

Refactor refresh new target to use save_ems_inventory #14247

agrare commented Mar 9, 2017 •

edited

Loading

miq-bot commented Mar 9, 2017

agrare commented Mar 9, 2017

miq-bot commented Mar 16, 2017

Fryguy commented Mar 20, 2017

durandom left a comment

durandom Apr 6, 2017

agrare Apr 6, 2017

agrare commented Apr 6, 2017 •

edited

Loading

miq-bot commented Apr 6, 2017

agrare commented Apr 18, 2017

chessbyte commented Jun 2, 2017

cben Jun 4, 2017

agrare Jun 4, 2017

durandom commented Jun 7, 2017

agrare commented Jun 7, 2017

blomquisg left a comment

blomquisg Jun 14, 2017

agrare Jun 14, 2017

miq-bot commented Jun 14, 2017

	cluster = EmsCluster.find_by(:ems_ref => cluster_hash[:ems_ref], :ems_id => ems.id)
	if cluster.nil?
	rp = ems.resource_pools.create!(rp_hash)

	cluster = ems.clusters.create!(cluster_hash)

	cluster.add_resource_pool(rp)
	cluster.save!

Refactor refresh new target to use save_ems_inventory #14247

Refactor refresh new target to use save_ems_inventory #14247

Conversation

agrare commented Mar 9, 2017 • edited Loading

miq-bot commented Mar 9, 2017

agrare commented Mar 9, 2017

miq-bot commented Mar 16, 2017

Fryguy commented Mar 20, 2017

durandom left a comment

Choose a reason for hiding this comment

durandom Apr 6, 2017

Choose a reason for hiding this comment

agrare Apr 6, 2017

Choose a reason for hiding this comment

agrare commented Apr 6, 2017 • edited Loading

miq-bot commented Apr 6, 2017

agrare commented Apr 18, 2017

chessbyte commented Jun 2, 2017

cben Jun 4, 2017

Choose a reason for hiding this comment

agrare Jun 4, 2017

Choose a reason for hiding this comment

durandom commented Jun 7, 2017

agrare commented Jun 7, 2017

blomquisg left a comment

Choose a reason for hiding this comment

blomquisg Jun 14, 2017

Choose a reason for hiding this comment

agrare Jun 14, 2017

Choose a reason for hiding this comment

miq-bot commented Jun 14, 2017

agrare commented Mar 9, 2017 •

edited

Loading

agrare commented Apr 6, 2017 •

edited

Loading