search

LINBIT / linstor-proxmox

Integration pluging bridging LINSTOR to Proxmox VE
31 stars 7 forks source link

allow multiple SPs in RG #53

Closed dseliskar closed 2 years ago

dseliskar commented 2 years ago

should fix https://github.com/LINBIT/linstor-proxmox/issues/39

rck commented 2 years ago

currently a bit busy with other things and I have to think it through. I'm pretty sure I will get to it during this week. If you did not hear back am Mon feel free to remind me. thanks!

rck commented 2 years ago

unfortunately this looks a bit half baked and would break functionality as well as accounting.

for example placing the resource on the local node if possible would break here: https://github.com/dseliskar/linstor-proxmox/blob/master/LINBIT/Linstor.pm#L245 . You probably did not notice because the auto-place after it fixed it for you. That part of the code should use https://app.swaggerhub.com/apis-docs/Linstor/Linstor/1.14.0#/developers/resourceMakeAvailableOnNode which can deal with multiple storage pools

Using make-available is the easy part, but https://github.com/dseliskar/linstor-proxmox/blob/master/LINSTORPlugin.pm#L217 also tries to get the one and only storage pool which is then, if you search for that function, used for various storage pool accounting in "list" and "status" commands. I assume that all breaks.

dseliskar commented 2 years ago

Thanks for review!

unfortunately this looks a bit half baked and would break functionality as well as accounting.

for example placing the resource on the local node if possible would break here: https://github.com/dseliskar/linstor-proxmox/blob/master/LINBIT/Linstor.pm#L245 . You probably did not notice because the auto-place after it fixed it for you. That part of the code should use https://app.swaggerhub.com/apis-docs/Linstor/Linstor/1.14.0#/developers/resourceMakeAvailableOnNode which can deal with multiple storage pools

You are right. I missed this. Mostly because it did not show any error or malfunction on my test deployment.

Using make-available is the easy part, but https://github.com/dseliskar/linstor-proxmox/blob/master/LINSTORPlugin.pm#L217 also tries to get the one and only storage pool which is then, if you search for that function, used for various storage pool accounting in "list" and "status" commands. I assume that all breaks.

This I think was fixed by previous patch with two more general filter statements in PluginHelper's get_images and get_status.

rck commented 2 years ago

Hi @dseliskar , the make-available part looks correct now, and I guess you are right about the SP accounting, I stopped reading here too early :). I will have to play around with it and see if it works as expected. I will be AFK most of next week, so please give me some time, I will merge it or give feedback or do the last bits if I find something in the first week of October then. Thanks for your work!

rck commented 2 years ago

Hi @dseliskar I had finally time to test it a bit more.

I will do a RC1 release later today or tomorrow.

Thanks for your great contribution