iSCSI support for ZoL #1099
iSCSI support for ZoL #1099
Conversation
|
I'm curious to know why this was not merged and no comment was added. Does nobody care about iSCSI? |
|
It's scheduled for milestone 0.7.0 so it's not considered high priority right now. It does do a couple of things that are objectionable, such as modifying the So yeah, there's some rather serious problems with it. I wouldn't merge it as-is. Someone should look at it in even more detail though. |
|
When I started investigate iSCSI, I found a number of specifications about the IQN. All said it's supposed to be 'iqn.[date (YYYY-MM) of creating share].[host sharing the device]'. That's why I'm auto-generating it. But the latest version can have this set using either options to 'shareiscsi' or config file... |
|
Updated for latest master. I think I got rid of the META modification (wasn't supposed to be there). Also merged it as ONE commit. |
|
I'm working on updating it to latest HEAD, but in the mean time edit That should do it... |
|
Ok, updated (again) to HEAD. |
|
Are you sure you're running the new module and binaries? |
|
Did you run ./autogen.sh and the configure line again before you built? |
|
Well, I just tried it myself, and it wouldn't compile because there was an erraneous space in a function call. So you'll have to make absolutly sure that there is no rpm in your directory before continuing. I'm quite sure you installed a previous, non-iscsi enabled package... |
|
Is there support for scst too? |
|
@pyavdr Yes there is. With the latest push, it compiles and installs just fine: |
|
Yes i got it now. Will do some tests with it. Thank you! |
|
By the way, in your command line above, use && instead of ;. That way the next command won't continue if the previous failed... |
|
@FransUrbo |
|
@DeHackEd Could you take another look at the patch, I got rid of most (all?) of you issues. Regarding the IQN thing. According to http://en.wikipedia.org/wiki/ISCSI#Addressing, the naming scheme is:
So the 'reversed domain name' can be read in two ways - either MY domain name (i.e., the domain name I'm using myself at home/at the site) and the second interpretation is ZoL... Technically, so is the date - when was I started using my domain (I choose to interpret it as when I initially stared sharing the volume). But the possibility to hardcode it in I'm not very happy with forcing/hardcoding it to |
|
@DeHackEd I'm looking into adding support for tgtd, but it doesn't seem to be possible to create a target with |
|
I'm running CentOS 6. Their version of tgtd doesn't read a config file, the startup script launches tgtd and then invokes I don't know what you're running, but that's something you might want to look into. CentOS's current package name/version is scsi-target-utils-1.0.24-10.el6 |
|
I'm starting to thing we're talking about two different softwares... The one I've been playing with is called (in Debian GNU/Linux) But I also found The first one clearly states to be (all!) user space, the other needs a kernel module. That one does however have |
|
I've started adding support for the latter (STGT) now in any case. Should only take a day or two hopefully... |
|
Ok, STGT (from http://stgt.berlios.de) support added. Not complete - works in simple tests but better/more testing is needed. |
|
@DeHackEd If STGT is the one you're talking about (it sure looks like it though), is it true it's not possible to add a backing store to LUN0?!? No matter what I try, I fail: Instead I resorted to add it as LUN1 which works, but what's the point of having an 'empty' LUN? |
|
stgt appears to be what CentOS 6 ships. As for LUN0, it's not an empty LUN, it's the controller. It's SCSI, after all. |
|
Ok. Doesn't really matter. None of the other implementations I've tried had anything like that. There the device is on LUN0 'directly'... Ah, well. Now all I/we need is more eyes on this :) |
|
There's something very weird with this patch. Something is missing, but I can't for the life of me figure out what! http://bayour.com/misc/screenlog_share+destry.txt That is a log of creating three volumes, set Sometimes it works, sometimes it doesn't. And when it fails, it's completely random WHERE it will fail (i.e., on which volume). It will always work if I issue the destroy multiple times though. Since |
|
Ok, with a lot of trial-and-error, I managed to figure out that STGT actually takes 'a while' from issuing the delete, until it's actually deleted. So adding a UPDATE: Using |
|
Have to settle for half a second. That worked when the machine was loaded at 3.something... Don't like it, but it works... If anyone have a better idea, I'm listening! |
|
I have considered to just spawn tgtadm (or whatever admin command is in use) and wait for it to fail when looking for a TID. But that's going to take longer... Might be better though (no matter how loaded, it won't claim unshared until it really is)... UPDATE: Well, that didn't work anyway :(. The tgtadm command, when asked for a TID that don't exist will return > 0, but zfs destroy still reports |
|
Ok, even worse problem: A 'zfs rename' doesn't work most of the times. So I've separated what I was planning/hoping on being a fix for that, but that just made things worse. So if someone have an idea of the correct fix, I'd appreciate it. |
|
The problem is that in a 'zfs rename', the old share isn't unshared before the new dataset name is shared, leaving TWO shares (one broken, because the ZVOL have been renamed). Strangely enough, this seems to work just fine for both NFS and SMB. I've been looking at this for two days now, but I just can't find it... |
|
So basically, if I put the unshare anywhere BEFORE the IOCTL, zfs hangs on the IOCTL. The few times I got it to work, it was eventually unshared in sa_fini(). But I don't know how that can be, it is called after changelist_rename(), which basically removes the old entry and overwrites it with the new name, which can't be unshared, since it technically isn't shared (because it doesn't exist). However, the sharetab is correctly updated in sa_fini() (the old entry is removed correctly). |
|
Wait, what hapened here? @FransUrbo why is this closed? Now both issues are closed, and your branch is deleted? |
It's quite clear that this isn't going in… It's been waiting for two and a half years (granted, I've rewritten it twice since then, but still) and now it's been pushed to the next tag AGAIN. I simply don't have any interested in pursuing this any more. Brian wants ZED to do this, so anyone that want/need iSCSI support in ZoL will just have to wait for it to be done 'correctly' (in zed). |
|
@FransUrbo Yes, I did and I'm sorry about that. But the absolute last thing I want to do is accidentally break something subtle before a tag. A tag which is already overdue. I should have gotten this pushed over the finish line and merged right after 0.6.3 as planned then it would have had sufficient time to soak in master. But I didn't. My concern is that this patch touches a considerable bit of It's true I would like to explore the idea of having the ZED take care of this but I'm not hell bent on that solution. |
|
@FransUrbo @behlendorf Can we get some momentum on this and get this patch going again and properly reviewed as suggested in the comments? I think I'm not the only one that wants native iSCSI support in ZOL :) |
|
@behlendorf: IMHO, ZED is going to be its own can of sandworms, and pushing something which has had this much testing among the community into a shallow grave because it doesnt meet the aspirations of how everything in ZFS should work is a mistake. First off, persistent iSCSI settings in ZFS allow for fast failover to snapshot recipient hosts. Secondly, given how much work @FransUrbo has put into this, it would probably not help in the overall effort of attracting more developers to the project if viable code is canned like this. Can we open this back up and at least leave it for people to use from github? Better yet, push out 0.6.4 and make this the first commit in 0.6.5's roadmap. |
|
here here! |
@sempervictus I'd like nothing better, it wasn't my intention to kill this off and I definitely should have handled this better. When I commented that I wasn't comfortable merging this before the 0.6.4 tag I didn't expect it to be closed. I just hoped to delay the merge until right after the tag. But since I did this exact same thing before the 0.6.3 tag and then didn't merge it I can understand why @FransUrbo is justifiably upset. I should have been clearer about my reservations. I still feel this patch is too risky to merge before the 0.6.4 tag since it does make some non-trivial changes to common code in the utilities. However, I think we can definitely merge it right after the tag. That will give us a few months to shake it out in master and discover if I'm just being overly cautious (hopefully I am). For those who need this patch it's still available in #3116. I opened this pull request a few days ago to run it through a fresh round of testing and so I could start carefully looking over the code. Again, if we could get another set of eyes on this I'd feel much better about it. I'm not an iSCSI expert, and once it's in the tree I feel obligated to support it. Hopefully with @FransUrbo's help! As for doing this in the ZED I agree that is absolutely going to be a ton of work. We still need to build some needed infrastructure even to make it possible. And then to get it working and tested will take a considerable amount of effort. That's not something which is going to happen soon. |
|
+1 |
|
@behlendorf now that 0.6.4 is out: maybe reopen this one so it will not get lost? |
|
@kpande sharenfs/sharesmb/shareiscsi are standard features on other ZFS implementations. Also, you might consider toning down the hyperbole a little. |
|
@kpande fine, FreeBSD doesn't support it, but that's probably only because they didn't have a target until the 10.0 release, clearly it's not considered out of scope though. To answer some of your other questions, go look at the PR. Bringing systemd into the conversation is straight-up flame-bait. Should we also drop sharenfs and sharesmb because you don't want ZFS to 'control every aspect of file services'? ZFS has been accused of layering violations before, but IMO those 'violations' are some of the reasons it's nice to administer. |
|
@kpande you're entitled to your opinion, however there are real advantages to having shares defined on the filesystem, like being able to |
@kpande @pdf this is a bit of a digression but this isn't too far from where I've suggested things should go. The |
|
@behlendorf this sounds like a good long-term plan. Do you have a link to the discussion/design notes? |
|
@pdf I'm sure I've had this discussion before in the tracker but at the moment I can't seem to find it. @dun and I have fleshed out a nice design for this but we haven't posted it anywhere, we should so it can be commented on. But the gist of it is we need to extend the existing event infrastructure to minimally allow:
|
|
+1 for an awesomeness
|
NOTE: 'Unfortunately' (mostly because I'm lazy :), it require my
iSCSI patch to ZoL.
openzfs/zfs#1099
If anyone actually MUST have manual iSCSI add/remove, feel free
to supply me with patches. It's not going to happen othervise..
|
@behlendorf is this shelved due to the branch being closed, the preference of using ZED or some other reason? |
|
Anyone who's still interested in this, give a thumbs up on the top/initial comment so we can judge how many's interested. |
|
Seeing this via email, so can't thumbs up far as I know, but I am very On Jul 15, 2016 08:13, "Turbo Fredriksson" notifications@github.com wrote: Anyone who's still interested in this, give a thumbs up on the top/initial — |
|
@sempervictus This works with LIO as well. It haven't had as much testing as SCST (which I've used in production together with this patch for 3+ years). If you're interested in Cinder, you might want to have a look at "my" Openstack-ZFS driver: https://github.com/FransUrbo/Openstack-ZFS |
|
We generally use scst too, but turns out it doesn't work in lxc whereas lio
|
|
On Jul 20, 2016, at 6:33 AM, RageLtMan wrote:
Thanx :). But I'm taking to many short cuts (chief among them is to use "shareiscsi", which isn't officially available yet). I can't justify getting this in Cinder proper with that 'limitation'. But I'm not discounting the possibility for the future. I just want much more eyes on it before I even consider it - "with enough eyeballs, all bugs are shallow" I think HE said.. :) |
|
On my phone, so not seeing the thumbs up either, but add me to the count. Would love to see this integrated. |
|
Hi there I would like to try @FransUrbo his OpenStack-Cinder-ZFS-Mothership-Godlike-Driver :) ... using on Ubuntu 16.04 with Root ( / ) on ZFS. Do I have to apply this patch and recompile zfsonlinux on Ubuntu? Or it is now inside master? root@sackgesicht: |
There was a problem hiding this comment.
I would also (as some commented above) want to have the logic in iscsi_[LIO|SCST|...].c move to an external (script) helper that gets called from ZFS with the full dataset path and the contents of its shareiscsi (or 'off' for the unshare case) property. Hard-baking the implementation into ZFS dosn't seem like a good idea.
I personally don't care if this would be called from ZFS directly or delegated through ZED (which might have the upside of blocking helpers that wait for a timeout wouldn't hang the system completely), please just in a way that /path/to/zfs_iscsi_helper is allowed to be (symlinked or not):
- a wrapper that does auto-detection of the iSCSI subsystem and delegates to the actual iscsi_helper_[LIO|SCST|...] implementation
- the actual implementation used on the system (no autodetection)
- /bin/true to completely neuter shareiscsi on the system (without any need to adjust dataset properties, should be useful for servers without an iSCSI subsystem or possible consumers)
- a user supplied wrapper/implementation to add more features or modify/restrict the share
Putting all the options as a string into the property might be the initial design for share[smb|nfs] and most likely is a case of we have always done it like this so it won't change but IMHO it is a bad design decision.
A helper would allow to replace set shareiscsi="<long list of all needed options>" with multiple shareiscsi:<option>="value" properties (that can be inherited from the parent dataset(s) to avoid having to retype them) and have my_iscsi_helper work from these (collect into a single string and feed the actual iscsi_helper_[LIO|SCST|...] implementation, whatever) and use the shareiscsi property just as a basic on/off toggle.
With the implementation in a replaceable script the user could decide how it should be handled on his system, the ones who like it as-is could simply stick with the default configuration. Since this would not impact the way of others, hopefully no one will be against allowing me to do something like this on my systems.
TL;DR: Sharing through 3rd party deamons shouldn't be baked in but managed with helper scripts.
This also applies to sharenfs and sharesmb (where it is worse as the current implementation also auto-shares child datasets).
| @@ -0,0 +1,118 @@ | |||
| #!/bin/bash | |||
|
|
|||
| # !! This script is for ISCSI-SCST !! | |||
There was a problem hiding this comment.
should be 'ISCSI-LIO'.
|
does this PR apply to master or 0.7.x? |
…s#1099) Bumps [openssl](https://github.com/sfackler/rust-openssl) from 0.10.55 to 0.10.56. - [Release notes](https://github.com/sfackler/rust-openssl/releases) - [Commits](sfackler/rust-openssl@openssl-v0.10.55...openssl-v0.10.56) --- updated-dependencies: - dependency-name: openssl dependency-type: indirect update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Add support for the
shareiscsiproperty.It currently (as of Sept 2, 2014) supports four different iSCSI target implementations (in this order of discovery):
Requires that SCST was compiled with /sys support (the default).
It supports the following (official) options to the property: