[libvirt] [PATCH] [RFC] ISCSI transport protocol support in libvirt
Hi everyone, This my first post on this list Context : Some days ago I have decided to use infiniband for ISCSI streams. Infiniband adds a new wonderful transport protocol : ISER. This protocols is added to the well known the default TCP. (NB: ISER = ISCSI + RDMA). I could not see any ISCSI transport protocol modification support in libivirt (the default protocol tcp is even hardcoded in the regex) What i have done : - did the shell testing for the iscsiadm - the attached patch that corrects 2 typos in the original code and switches completely the iscsi transport protocol from tcp to iser (which is not ideal at all) What should be done (imho): - add iscsi transport protocol support (using my patch as a basis) - add a new XML property/whatever_fits_the_projects_policy to the storagepool object that allows user to pick the iscsi transport protocol (default is tcp) I was thinking of having something like : <pool type="iscsi"> <name>volumename</name> <source> <host name="1.2.3.4"/> <device path="IQNOFTARGET"/> <transport protocol="iser"/> </source> <target> <path>/dev/disk/by-path</path> </target> </pool> Any comment on this ? Any help on the XML part ? Best regards, Aurélien NB: the current iscsi transport protocols available are : tcp(default), iser, qla4xxx, bnx2, and icxgb3i. PS: i'm still doing extensive testing of my patch !DSPAM:4c46f49d90977882820711!
On Wed, Jul 21, 2010 at 03:22:36PM +0200, Aurelien ROUGEMONT wrote:
Hi everyone,
Hi Aurelien,
This my first post on this list
Sorry for coming back so late, I accumulated a bit of backlog and since this clearly wasn't for 0.8.3 it ended up on the backburner...
Context : Some days ago I have decided to use infiniband for ISCSI streams. Infiniband adds a new wonderful transport protocol : ISER. This protocols is added to the well known the default TCP. (NB: ISER = ISCSI + RDMA). I could not see any ISCSI transport protocol modification support in libivirt (the default protocol tcp is even hardcoded in the regex)
What i have done : - did the shell testing for the iscsiadm - the attached patch that corrects 2 typos in the original code and switches completely the iscsi transport protocol from tcp to iser (which is not ideal at all)
The typo should be applied separately, no need to wait for them
What should be done (imho): - add iscsi transport protocol support (using my patch as a basis) - add a new XML property/whatever_fits_the_projects_policy to the storagepool object that allows user to pick the iscsi transport protocol (default is tcp)
I was thinking of having something like :
<pool type="iscsi"> <name>volumename</name> <source> <host name="1.2.3.4"/> <device path="IQNOFTARGET"/> <transport protocol="iser"/> </source> <target> <path>/dev/disk/by-path</path> </target> </pool>
Any comment on this ? Any help on the XML part ?
the XML addition of a transport element with a protocol attribute looks fine to me. Values would for now be 'tcp' by default and optionally 'iser'
Best regards,
Aurélien
NB: the current iscsi transport protocols available are : tcp(default), iser, qla4xxx, bnx2, and icxgb3i.
what about the other protocols ? Can you detect availability ? Can you automatically set things up from libvirt ?
PS: i'm still doing extensive testing of my patch
!DSPAM:4c46f49d90977882820711!
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index f34123f..7c21ad7 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -113,11 +113,13 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, * # iscsiadm --mode session * tcp: [1] 192.168.122.170:3260,1 demo-tgt-b * tcp: [2] 192.168.122.170:3260,1 demo-tgt-a + * iser: [5] 10.111.140.1:3260,1 demo-tgt-d + * * Pull out 2nd and 4th fields */ const char *regexes[] = { - "^tcp:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$" + "^iser:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$" };
Since regexes are a list of allowed regexps you should just add "^iser:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$" as the second element in the array
int vars[] = { 2, @@ -230,7 +232,7 @@ virStorageBackendIQNFound(virStoragePoolObjPtr pool,
out: if (ret == IQN_MISSING) { - VIR_DEBUG("Could not find interface witn IQN '%s'", iqn); + VIR_DEBUG("Could not find interface with IQN '%s'", iqn); }
VIR_FREE(line); @@ -293,7 +295,24 @@ virStorageBackendCreateIfaceIQN(virStoragePoolObjPtr pool, if (virRun(cmdargv2, &exitstatus) < 0) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), - cmdargv1[0], pool->def->source.initiator.iqn); + cmdargv2[0], pool->def->source.initiator.iqn);
Those 2 typo need to be fixed, I pushed this to git, no need to wait
+ goto out; + } + + /* This part switches iscsi transport protocol from default (tcp) to iser + * TODO: add some XML variable variable for enabling/disabling this block*/ + const char *const cmdargv3[] = { + ISCSIADM, "--mode", "iface", "--interface", &temp_ifacename[0], + "--op", "update", "--name iface.transport_name", "--value iser", NULL + }; + + /* Note that we ignore the exitstatus. Older versions of iscsiadm tools + * returned an exit status of > 0, even if they succeeded. We will just + * rely on whether iface file got updated properly. */ + if (virRun(cmdargv3, &exitstatus) < 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), + cmdargv3[0], pool->def->source.initiator.iqn); goto out; }
Well if you run this here you prevent normal tcp operations it seems. The big problem is to know when you need to do this, and either this can be found from the environment settings on the node (possible ?) or fallback to the user indicating this. Doing the reading from configuration should not be hard look at the existing conf code doing it, if needed grab me on IRC for help on the XML parsing/saving code, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
On 08/02/2010 09:58 PM, Daniel Veillard wrote:
Hi Aurelien, Hi Daniel, Sorry for coming back so late, I accumulated a bit of backlog and since this clearly wasn't for 0.8.3 it ended up on the backburner...
no worries, everyone has priorities.
Context : Some days ago I have decided to use infiniband for ISCSI streams. Infiniband adds a new wonderful transport protocol : ISER. This protocols is added to the well known the default TCP. (NB: ISER = ISCSI + RDMA). I could not see any ISCSI transport protocol modification support in libivirt (the default protocol tcp is even hardcoded in the regex)
What i have done : - did the shell testing for the iscsiadm - the attached patch that corrects 2 typos in the original code and switches completely the iscsi transport protocol from tcp to iser (which is not ideal at all)
The typo should be applied separately, no need to wait for them
Cool, i have noticed they were included in my last git clone.
What should be done (imho): - add iscsi transport protocol support (using my patch as a basis) - add a new XML property/whatever_fits_the_projects_policy to the storagepool object that allows user to pick the iscsi transport protocol (default is tcp)
I was thinking of having something like :
<pool type="iscsi"> <name>volumename</name> <source> <host name="1.2.3.4"/> <device path="IQNOFTARGET"/> <transport protocol="iser"/> </source> <target> <path>/dev/disk/by-path</path> </target> </pool>
Any comment on this ? Any help on the XML part ?
the XML addition of a transport element with a protocol attribute looks fine to me. Values would for now be 'tcp' by default and optionally 'iser'
This would be perfect
Best regards,
Aurélien
NB: the current iscsi transport protocols available are : tcp(default), iser, qla4xxx, bnx2, and icxgb3i.
what about the other protocols ? Can you detect availability ? Can you automatically set things up from libvirt ?
Yes : root@mdc-lab-twin01-d:/sys/module/libiscsi/holders# ls ib_iser iscsi_tcp Meaning mdc-lab-twin01-d handles tcp and iser.
PS: i'm still doing extensive testing of my patch
diff --git a/src/storage/storage_backend_iscsi.c b/src/storage/storage_backend_iscsi.c index f34123f..7c21ad7 100644 --- a/src/storage/storage_backend_iscsi.c +++ b/src/storage/storage_backend_iscsi.c @@ -113,11 +113,13 @@ virStorageBackendISCSISession(virStoragePoolObjPtr pool, * # iscsiadm --mode session * tcp: [1] 192.168.122.170:3260,1 demo-tgt-b * tcp: [2] 192.168.122.170:3260,1 demo-tgt-a + * iser: [5] 10.111.140.1:3260,1 demo-tgt-d + * * Pull out 2nd and 4th fields */ const char *regexes[] = { - "^tcp:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$" + "^iser:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$" };
Since regexes are a list of allowed regexps you should just add "^iser:\\s+\\[(\\S+)\\]\\s+\\S+\\s+(\\S+)\\s*$" as the second element in the array
Absolutely true !
int vars[] = { 2, @@ -230,7 +232,7 @@ virStorageBackendIQNFound(virStoragePoolObjPtr pool,
out: if (ret == IQN_MISSING) { - VIR_DEBUG("Could not find interface witn IQN '%s'", iqn); + VIR_DEBUG("Could not find interface with IQN '%s'", iqn); }
VIR_FREE(line); @@ -293,7 +295,24 @@ virStorageBackendCreateIfaceIQN(virStoragePoolObjPtr pool, if (virRun(cmdargv2,&exitstatus)< 0) { virStorageReportError(VIR_ERR_INTERNAL_ERROR, _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), - cmdargv1[0], pool->def->source.initiator.iqn); + cmdargv2[0], pool->def->source.initiator.iqn);
Those 2 typo need to be fixed, I pushed this to git, no need to wait
+ goto out; + } + + /* This part switches iscsi transport protocol from default (tcp) to iser + * TODO: add some XML variable variable for enabling/disabling this block*/ + const char *const cmdargv3[] = { + ISCSIADM, "--mode", "iface", "--interface",&temp_ifacename[0], + "--op", "update", "--name iface.transport_name", "--value iser", NULL + }; + + /* Note that we ignore the exitstatus. Older versions of iscsiadm tools + * returned an exit status of> 0, even if they succeeded. We will just + * rely on whether iface file got updated properly. */ + if (virRun(cmdargv3,&exitstatus)< 0) { + virStorageReportError(VIR_ERR_INTERNAL_ERROR, + _("Failed to run command '%s' to update iscsi interface with IQN '%s'"), + cmdargv3[0], pool->def->source.initiator.iqn); goto out; }
Well if you run this here you prevent normal tcp operations it seems.
True, but if we modifiy just a bit making the code replacing the value (which is iser in this patch) with the xml attribute we discussed before it would work for any transport protocol.
The big problem is to know when you need to do this, and either this can be found from the environment settings on the node (possible ?) or fallback to the user indicating this. cf last answer
Doing the reading from configuration should not be hard look at the existing conf code doing it, if needed grab me on IRC for help on the XML parsing/saving code,
If i'm not mistaking to add the whole feature we just need to: - add row(s) to the regexes table, or even widen the current regex to accept \w instead of a given string (such as tcp:// or iser://) - add the transport_protocol attribute - replace the hardcoded "iser" string by the value of this new attribute - add an availability check of the transport protocol using the /sys/module/libiscsi/holders/ folder Then everything should be ok. I'll contact you right away about the XML attribute thingy, i should be able to deal with the rest.
Daniel
Aurélien !DSPAM:4c5809c690974020910970!
participants (2)
-
Aurelien ROUGEMONT -
Daniel Veillard