save_vms_inventory needs to respect disconnect flag #15924
Conversation
| [target.ruby_clone] | ||
| else | ||
| [] | ||
| end | ||
| else | ||
| [] |
There was a problem hiding this comment.
I wonder if we just disconnects.to_a.index down here https://github.com/pkliczewski/manageiq/blob/3ced29d675318fbbe889b25b51aa0c75d06a29a1/app/models/ems_refresh/save_inventory.rb#L70 and then we don't need the two [] cases?
There was a problem hiding this comment.
The solution your are proposing won't work due to:
[----] E, [2017-09-04T11:47:32.216615 #5694:d81120] ERROR -- : MIQ(MiqQueue#deliver) Message id: [6897], Error: [undefined method values' for 0:Fixnum] [----] E, [2017-09-04T11:47:32.216776 #5694:d81120] ERROR -- : [NoMethodError]: undefined method values' for 0:Fixnum Method:[block in method_missing]
[----] E, [2017-09-04T11:47:32.216896 #5694:d81120] ERROR -- : /home/pkliczewski/git/manageiq/app/models/ems_refresh/save_inventory.rb:170:in `save_vms_inventory'
It looks like all the solutions are not nice. I will go with simplest approach with a comment.
| @@ -37,7 +37,8 @@ def save_vms_inventory(ems, hashes, target = nil, disconnect = true) | |||
| log_header = "EMS: [#{ems.name}], id: [#{ems.id}]" | |||
|
|
|||
| disconnects = if target.kind_of?(ExtManagementSystem) || target.kind_of?(Host) | |||
| target.vms_and_templates.reload.to_a | |||
| # make sure not to delete vms when disconnect flag set to false | |||
| disconnect ? target.vms_and_templates.reload.to_a : [] | |||
There was a problem hiding this comment.
I still worry that this doesn't fix the issue where the vm is the target, can you do
if disconnect && (target.kind_of ... )
elsif disconnect && target.kind_of?(Vm)
end
?
There was a problem hiding this comment.
Actually we do pass ems so for this specific scenario it should be fine. I will update the PR anyway to be sure for other places where it is used.
When we provision new vm we call `save_ems_inventory_no_disconnect` which updates the db with disconnect flag set to false. `save_vms_inventory` ignores the flag and disconnects the vms (ems set to nil). This patch fixes the issue.
|
Checked commit pkliczewski@5717376 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
There was a problem hiding this comment.
LGTM, great find @pkliczewski
TODO these multi-condition disconnect cases could use with some refactoring
When we provision new vm we call
save_ems_inventory_no_disconnectwhich updates the db with disconnect flag set to false.save_vms_inventoryignores the flag and disconnects the vms (ems set to nil). This patch fixes the issue.