Re: [pve-devel] [PATCH V3 storage 0/4] updated Linux targetcli/LIO support
On Wed, 2018-07-11 at 19:50 +0200, Stoiko Ivanov wrote: > Hi, > > I finally got around to give your patches a spin, and can report that > they work for my preliminary tests. > > One thing, which I think is not a good idea is the reuse of the > comstar_tg property for the tpg of LIO (from my understanding they do > serve somewhat different purposes). Should we introduce it like that, > we will be stuck with supporting this syntax for backwards > compatibility. We should probably introduce a different optional > property "lio_tpg" and initially just present it along with the > others > in the GUI dialog. What would your opinion be on that? > > Most possible places for some refactoring we saw apply to the iet > implementation you based your patches on as well, but if you feel > like > it, it would be great to use the new implementation for an initial > cleanup. > > One thing which would probably be nice is to remove the created zvol, > if the subsequent creation of the lun fails (I stumbled upon that by > having a typo in the iqn of the target). I've now had a little time to refactor the implementation according to your remarks: * new tpg_lio property instead of using the comstar_tg property While this works of course, it has added another optional input field to the Add: ZFS over iSCSI" dialog. Ideally options like these should only be visible depending on the state of another field (in this case, if "LIO" has been selected as the iSCSI provider), but unfortunately I didn't find an (easy) way to implement this. Looks like this is a feature missing in general from the proxmox UI :) * Removing a created zvol in case the target has not been set up correctly The best way to handle this for any ZFS via iSCSI implementation would be to verify the connection after the user has entered the connection data, preventing the scenario you describe all together. While this doesn't look to difficult to implement, I decided against it because this would require changes to all other iSCSI providers as well. Unfortunately I don't have access to other iSCSI servers but LIO based ones, and so I don't really feel comfortable changing code for the other providers. The rest of your remarks has been incorporated. Cheers Udo -- Udo Rader, GF/CEO BestSolution.at EDV Systemhaus GmbH Eduard-Bodem-Gasse 5-7, A-6020 Innsbruck http://www.bestsolution.at/ Reg. Nr. FN 222302s am Firmenbuchgericht Innsbruck signature.asc Description: This is a digitally signed message part ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH V3 storage 0/4] updated Linux targetcli/LIO support
Hi, I finally got around to give your patches a spin, and can report that they work for my preliminary tests. One thing, which I think is not a good idea is the reuse of the comstar_tg property for the tpg of LIO (from my understanding they do serve somewhat different purposes). Should we introduce it like that, we will be stuck with supporting this syntax for backwards compatibility. We should probably introduce a different optional property "lio_tpg" and initially just present it along with the others in the GUI dialog. What would your opinion be on that? Most possible places for some refactoring we saw apply to the iet implementation you based your patches on as well, but if you feel like it, it would be great to use the new implementation for an initial cleanup. One thing which would probably be nice is to remove the created zvol, if the subsequent creation of the lun fails (I stumbled upon that by having a typo in the iqn of the target). I'll post some comments to the individual patches. Thanks again for your work! On Fri, 29 Jun 2018 10:00:51 +0200 Udo Rader wrote: > fixed a typo that prevented LIO.pm from loading > > Udo Rader (4): > adding linux LIO support > adding linux LIO support > expiring cached iSCSI configuration after 15 seconds, after which it > is re-read from the portal > fixed syntax error, preventing LIO.pm from loading > > PVE/Storage/LunCmd/LIO.pm | 407 > PVE/Storage/LunCmd/Makefile | > 2 +- PVE/Storage/ZFSPlugin.pm| 7 +- > 3 files changed, 414 insertions(+), 2 deletions(-) > create mode 100644 PVE/Storage/LunCmd/LIO.pm > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
Re: [pve-devel] [PATCH V3 storage 0/4] updated Linux targetcli/LIO support
Hi, Thank you very much for the patches and your work! I'll try to review/test them next week and provide some feedback. Regarding the last patch (fixing of the typo) - you could squash that into the previous commit, which would have the upside that all commits are syntactically correct. The article at https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History gives a nice overview of the possibilities of interactive rebasing. However this is just a suggestion, and not material to these patches - we will happily do those tiny fixups when applying the patches. Thanks again, stoiko On Fri, 29 Jun 2018 10:00:51 +0200 Udo Rader wrote: > fixed a typo that prevented LIO.pm from loading > > Udo Rader (4): > adding linux LIO support > adding linux LIO support > expiring cached iSCSI configuration after 15 seconds, after which it > is re-read from the portal > fixed syntax error, preventing LIO.pm from loading > > PVE/Storage/LunCmd/LIO.pm | 407 > PVE/Storage/LunCmd/Makefile | > 2 +- PVE/Storage/ZFSPlugin.pm| 7 +- > 3 files changed, 414 insertions(+), 2 deletions(-) > create mode 100644 PVE/Storage/LunCmd/LIO.pm > ___ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
