[libvirt] [PATCH] [RFC] ISCSI transport protocol support in libvirt
Aurelien ROUGEMONT
beorn at binaries.fr
Tue Aug 3 12:21:26 UTC 2010
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 at 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!
More information about the libvir-list
mailing list