search

fog / fog-ovirt

MIT License
11 stars 21 forks source link

Fix storage domain issue when attaching LUN #107

Open sbernhard opened 9 months ago

sbernhard commented 9 months ago

Thanks @Manisha15

sbernhard commented 9 months ago

Workaround for https://projects.theforeman.org/issues/30199

Review would be nice @orrabin / @ShimShtein

sbernhard commented 5 months ago

Thoughts on this @ShimShtein ?

sbernhard commented 1 month ago

ping @ShimShtein :-)

sbernhard commented 1 month ago

I think we don't want to push 0 as the storage domain id, if storage_domains returned nil or and empty list. If we don't use the safe navigation operator here, we will end up in the rescue block (which is not desired, I think)

I can understand that in the case of an actual error, we can assume there is at least one domain, but we can't get the list, so we assume there is only one.

This would then not work. The original issue was: image

So, attachment_disk.storage_domains is nil and the access to "[0]" crashes. In this case, and only in this case it should return with "0" which would then work in case of direct attached LUN disks.

Its still better to return 0 instead of crashing.

dosas commented 1 month ago

@sbernhard There were quite some changes on the default branch. Could you rebase to enable the CI?

sbernhard commented 2 weeks ago

@ShimShtein I had a look at the code again.

  1. in case of there is 'attachment_disk_storage_domains' first entry with an id => use this id
  2. in case of there is 'attachment_disk_storage_domains' first entry but no id field is found => nil should be returned
  3. in case of NO 'attachment_disk_storage_domains' first entry, fallback to 0

This is what https://bugzilla.redhat.com/show_bug.cgi?id=1882404#c9 suggestes and works for us.

sbernhard commented 2 weeks ago

@ShimShtein can you have a look at this again, please?