disconnect_storage should be called once #62
Conversation
|
@miq-bot add_label fine/yes |
| vm_storages = ([storage] + storages).compact.uniq | ||
| storage = vm_storages.select { |store| !vm_disks.include?(store.ems_ref) } | ||
| end | ||
| storage = if vm_disks.blank? |
There was a problem hiding this comment.
I think this will work and looks more compact:
storage = vm_storages.select { |store| !vm_disks.include?(store.ems_ref) } if vm_disks.blank?
There was a problem hiding this comment.
look reasonable, will update
There was a problem hiding this comment.
I don't think this behaves the same as the code that @pkliczewski wrote.
In @pkliczewski there will be an assignment to storage in any case.
In your suggestion, the storage might have a previous value that won't be erased by your suggested fix.
probably need to add the else part as well.
There was a problem hiding this comment.
@masayag is right after testing the change it doesn't have the same behavior.
There was a problem hiding this comment.
@masayag @pkliczewski yes you are right! maybe trinary expression then?
There was a problem hiding this comment.
@borod108 sure, we can use ternary expression
| vm_storages = ([storage] + storages).compact.uniq | ||
| return if vm_storages.empty? | ||
|
|
||
| vm_disks = collect_disks |
There was a problem hiding this comment.
@pkliczewski Is there a possibility that inside collect_disks:
disks = hardware.disks.map { |disk| "#{disk.storage.ems_ref}/disks/#{disk.filename}" }
disk.storage will return nil ? assuming a case in which vm is connected to several storages and one of them was removed.
There was a problem hiding this comment.
@masayag No, if the disk is removed it will be not listed. According to my understanding of the code disk.storage always will have a value.
pkliczewski
force-pushed
the
master
branch
2 times, most recently
from
dec2f03 to
1bdbfeb
Compare
During vm remove we used to call disconnect_storage. The first time it was called as a first step of removal flow and during refresh. This caused timing issue which caused lack of update to vm.ems_id. As a result vm state was "unknown". We need to make sure that disconnect_storage is called only once.
|
Checked commit pkliczewski@23d516a with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
Any other review is needed? Shall we merge? |
|
@oourfali I think that we have all the acks we need. |
|
Fine backport (to manageiq repo) details: |
There were 2 PRs which fixed number of issues with this flow: - ManageIQ#14572 - ManageIQ/manageiq-providers-ovirt#62 This commit backports all of above. Bug-Url: https://bugzilla.redhat.com/1497522
|
Backported to Euwe via ManageIQ/manageiq#16218 |
During vm remove we used to call disconnect_storage. The first time
it was called as a first step of removal flow and during refresh.
This caused timing issue which caused lack of update to vm.ems_id.
As a result vm state was "unknown".
We need to make sure that disconnect_storage is called only once.
Bug-Url:
https://bugzilla.redhat.com/1468201