[libvirt] iSCSI Multi-IQN (Libvirt Support)
The following patch set realizes the multi-IQN concept discussed in an earlier thread http://www.mail-archive.com/libvir-list@redhat.com/msg16706.html The patch realizes an XML schema like the one below and allows libvirt to read through it to create storage pools. These XMLs when created using a virtualization manager realize unique VM to storage LUN mappings through a single console and thus opening up possibilities for the following scenarios - * possibility of multiple IQNs for a single Guest * option for hypervisor's initiator to use these IQNs on behalf of the guest Example Usages: Usage 1: VM1 - > <Init iqn1> <------------------------> <Target iqn1> <Init iqn2> <------------------------> <Target iqn1> <Init iqn3> <------------------------> <Target iqn1> <Init iqn4> <------------------------> <Target iqn1> Usage 2: VM1 - > <Init iqn1> <------------------------> <Target iqn1> <Init iqn2> <------------------------> <Target iqn2> <Init iqn3> <------------------------> <Target iqn3> <Init iqn4> <------------------------> <Target iqn4> Usage 3: VM1 - > <Init iqn1> <------------------------> <Target iqn1> VM2 - > <Init iqn2> <------------------------> <Target iqn1> Usage 4: VM1 - > <Init iqn1> <------------------------> <Target iqn1> VM2 - > <Init iqn2> <------------------------> <Target iqn2> Example XML schema for an iSCSI storage pool created -- <pool type="iscsi"> <name>dell</name> <uuid>cf354733-b01b-8870-2040-94888640e24d</uuid> <capacity>0</capacity> <allocation>0</allocation> <available>0</available> - <source> <initiator> iqnname = "<initiator IQN1>"> iqnname = "<initiator IQN2>"> </initiator> ........................................ ........................................ <host name="<iSCSI target hostname or Target IP address>" /> <device path="<iSCSI Target IQN name>" /> </source> - <target> <path>/dev/disk/by-path</path> - <permissions> <mode>0700</mode> <owner>0</owner> <group>0</group> </permissions> </target> </pool> Each initiator iqn name can be parsed to create the unique sessions. TODO: 1) Virt Manager GUI support to allow a visual mapping of iqn->storage pool -> VM and thus creating the XML for consumption by libvirt. Libvirt support added above can realize the same using virsh options. 2) option for Guest's own BIOS & initiator to use the initiator IQNs (iSCSI in Guest) a) Libvirt support to allow iqn as an attribute for a VM's XML. b) Qemu Support to allow Guest's bios & initiator to use these IQNs. Signed-off-by: Sudhir Bellad <sudhir_bellad@dell.com> Signed-off-by: Shyam Iyer <shyam_iyer@dell.com> diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..3f2a79d 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -159,13 +163,57 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator[0].iqnname != NULL) { + int i = 0; + while(pool->def->source.initiator[i].iqnname != NULL){ + if (!(dir = opendir(IFACES_DIR))) { + if (errno == ENOENT) + return 0; + virReportSystemError(conn, errno, _("Failed to open dir '%s'"), + IFACES_DIR); + return -1; + } + while ((entry = readdir(dir))) { + FILE *fp; + char path[PATH_MAX]; + + if (entry->d_name[0] == '.') + continue; + + sprintf(path,"%s", IFACES_DIR); + strcat(path,(const char *)entry->d_name); + + if ((fp = fopen(path, "r"))){ + char buff[256]; + while (fp != NULL && fgets(buff, sizeof(buff), fp) != NULL) { + if (strstr(buff, pool->def->source.initiator[i].iqnname) != NULL) { + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, "-I", entry->d_name, action, NULL + }; + + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } + } + } + fclose(fp); + } + i++; + } + } + else{ + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, action, NULL + }; + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } return 0; } diff --git a/src/storage_backend_iscsi.h b/src/storage_backend_iscsi.h index 665ed13..14c2c5c 100644 --- a/src/storage_backend_iscsi.h +++ b/src/storage_backend_iscsi.h @@ -25,7 +25,7 @@ #define __VIR_STORAGE_BACKEND_ISCSI_H__ #include "storage_backend.h" - extern virStorageBackend virStorageBackendISCSI; +#define IFACES_DIR "/var/lib/iscsi/ifaces/" #endif /* __VIR_STORAGE_BACKEND_ISCSI_H__ */ diff --git a/src/storage_conf.c b/src/storage_conf.c index 788de15..0f2ace9 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -106,12 +106,13 @@ struct _virStorageVolOptions { /* Flags to indicate mandatory components in the pool source */ enum { - VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), - VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), - VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), - VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), - VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), -}; + VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), + VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), + VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), + VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), + VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (1<<5), + }; @@ -179,7 +180,8 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { { .poolType = VIR_STORAGE_POOL_ISCSI, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_DEVICE), + VIR_STORAGE_POOL_SOURCE_DEVICE | + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN), }, .volOptions = { .formatToString = virStoragePoolFormatDiskTypeToString, @@ -532,6 +534,34 @@ virStoragePoolDefParseXML(virConnectPtr conn, goto cleanup; } } + + if(options->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) { + xmlNodePtr *nodeset = NULL; + int niqn, i; + + if((niqn = virXPathNodeSet(conn, "./initiator/iqnname", ctxt, &nodeset)) < 0) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("cannot extract storage pool source devices")); + goto cleanup; + } + if (VIR_ALLOC_N(ret->source.initiator, niqn) < 0) { + VIR_FREE(nodeset); + virReportOOMError(conn); + goto cleanup; + } + for (i = 0 ; i < niqn ; i++) { + xmlChar *name = xmlGetProp(nodeset[i], BAD_CAST "name"); + if (name == NULL) { + VIR_FREE(nodeset); + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing storage pool source device path")); + goto cleanup; + } + ret->source.initiator[i].iqnname = (char *)name; + } + VIR_FREE(nodeset); + } + if (options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) { xmlNodePtr *nodeset = NULL; int nsource, i; diff --git a/src/storage_conf.h b/src/storage_conf.h index 9fedb12..c77d6ae 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -182,6 +182,13 @@ struct _virStoragePoolSourceDeviceExtent { int type; /* free space type */ }; +typedef struct _virStoragePoolSourceInitiatorAttr virStoragePoolSourceInitiatorAttr; +typedef virStoragePoolSourceInitiatorAttr *virStoragePoolSourceInitiatorAttrPtr; +struct _virStoragePoolSourceInitiatorAttr { + /* Initiator iqn name */ + char *iqnname; +}; + /* * Pools can be backed by one or more devices, and some * allow us to track free space on underlying devices. @@ -223,6 +230,9 @@ struct _virStoragePoolSource { /* Or a name */ char *name; + /* One or more initiator names */ + virStoragePoolSourceInitiatorAttrPtr initiator; + int authType; /* virStoragePoolAuthType */ union { virStoragePoolAuthChap chap;
Shyam_Iyer@Dell.com wrote:
The following patch set realizes the multi-IQN concept discussed in an earlier thread http://www.mail-archive.com/libvir-list@redhat.com/msg16706.html
The patch realizes an XML schema like the one below and allows libvirt to read through it to create storage pools. These XMLs when created using a virtualization manager realize unique VM to storage LUN mappings through a single console and thus opening up possibilities for the following scenarios -
* possibility of multiple IQNs for a single Guest * option for hypervisor's initiator to use these IQNs on behalf of the guest
Example Usages: Usage 1: VM1 - > <Init iqn1> <------------------------> <Target iqn1> <Init iqn2> <------------------------> <Target iqn1> <Init iqn3> <------------------------> <Target iqn1> <Init iqn4> <------------------------> <Target iqn1>
Usage 2: VM1 - > <Init iqn1> <------------------------> <Target iqn1> <Init iqn2> <------------------------> <Target iqn2> <Init iqn3> <------------------------> <Target iqn3> <Init iqn4> <------------------------> <Target iqn4>
Usage 3: VM1 - > <Init iqn1> <------------------------> <Target iqn1>
VM2 - > <Init iqn2> <------------------------> <Target iqn1>
Usage 4: VM1 - > <Init iqn1> <------------------------> <Target iqn1>
VM2 - > <Init iqn2> <------------------------> <Target iqn2>
Example XML schema for an iSCSI storage pool created -- <pool type="iscsi"> <name>dell</name> <uuid>cf354733-b01b-8870-2040-94888640e24d</uuid> <capacity>0</capacity> <allocation>0</allocation> <available>0</available> - <source> <initiator> iqnname = "<initiator IQN1>"> iqnname = "<initiator IQN2>"> </initiator> ........................................ ........................................ <host name="<iSCSI target hostname or Target IP address>" /> <device path="<iSCSI Target IQN name>" /> </source> - <target> <path>/dev/disk/by-path</path> - <permissions> <mode>0700</mode> <owner>0</owner> <group>0</group> </permissions> </target> </pool>
Each initiator iqn name can be parsed to create the unique sessions.
TODO: 1) Virt Manager GUI support to allow a visual mapping of iqn->storage pool -> VM and thus creating the XML for consumption by libvirt. Libvirt support added above can realize the same using virsh options.
2) option for Guest's own BIOS & initiator to use the initiator IQNs (iSCSI in Guest) a) Libvirt support to allow iqn as an attribute for a VM's XML. b) Qemu Support to allow Guest's bios & initiator to use these IQNs.
Signed-off-by: Sudhir Bellad <sudhir_bellad@dell.com> Signed-off-by: Shyam Iyer <shyam_iyer@dell.com>
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..3f2a79d 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -159,13 +163,57 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator[0].iqnname != NULL) { + int i = 0; + while(pool->def->source.initiator[i].iqnname != NULL){ + if (!(dir = opendir(IFACES_DIR))) { + if (errno == ENOENT) + return 0; + virReportSystemError(conn, errno, _("Failed to open dir '%s'"), + IFACES_DIR); + return -1; + } + while ((entry = readdir(dir))) { + FILE *fp; + char path[PATH_MAX]; + + if (entry->d_name[0] == '.') + continue; + + sprintf(path,"%s", IFACES_DIR); + strcat(path,(const char *)entry->d_name); + + if ((fp = fopen(path, "r"))){ + char buff[256]; + while (fp != NULL && fgets(buff, sizeof(buff), fp) != NULL) { + if (strstr(buff, pool->def->source.initiator[i].iqnname) != NULL) { + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, "-I", entry->d_name, action, NULL + }; + + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } + } + } + fclose(fp); + } + i++; + } + } + else{ + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, action, NULL + }; + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + }
return 0; } diff --git a/src/storage_backend_iscsi.h b/src/storage_backend_iscsi.h index 665ed13..14c2c5c 100644 --- a/src/storage_backend_iscsi.h +++ b/src/storage_backend_iscsi.h @@ -25,7 +25,7 @@ #define __VIR_STORAGE_BACKEND_ISCSI_H__
#include "storage_backend.h" - extern virStorageBackend virStorageBackendISCSI; +#define IFACES_DIR "/var/lib/iscsi/ifaces/"
#endif /* __VIR_STORAGE_BACKEND_ISCSI_H__ */ diff --git a/src/storage_conf.c b/src/storage_conf.c index 788de15..0f2ace9 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -106,12 +106,13 @@ struct _virStorageVolOptions {
/* Flags to indicate mandatory components in the pool source */ enum { - VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), - VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), - VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), - VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), - VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), -}; + VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), + VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), + VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), + VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), + VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (1<<5), + };
@@ -179,7 +180,8 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { { .poolType = VIR_STORAGE_POOL_ISCSI, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_DEVICE), + VIR_STORAGE_POOL_SOURCE_DEVICE | + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN), }, .volOptions = { .formatToString = virStoragePoolFormatDiskTypeToString, @@ -532,6 +534,34 @@ virStoragePoolDefParseXML(virConnectPtr conn, goto cleanup; } } + + if(options->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) { + xmlNodePtr *nodeset = NULL; + int niqn, i; + + if((niqn = virXPathNodeSet(conn, "./initiator/iqnname", ctxt, &nodeset)) < 0) { + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("cannot extract storage pool source devices")); + goto cleanup; + } + if (VIR_ALLOC_N(ret->source.initiator, niqn) < 0) { + VIR_FREE(nodeset); + virReportOOMError(conn); + goto cleanup; + } + for (i = 0 ; i < niqn ; i++) { + xmlChar *name = xmlGetProp(nodeset[i], BAD_CAST "name"); + if (name == NULL) { + VIR_FREE(nodeset); + virStorageReportError(conn, VIR_ERR_XML_ERROR, + "%s", _("missing storage pool source device path")); + goto cleanup; + } + ret->source.initiator[i].iqnname = (char *)name; + } + VIR_FREE(nodeset); + } + if (options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) { xmlNodePtr *nodeset = NULL; int nsource, i; diff --git a/src/storage_conf.h b/src/storage_conf.h index 9fedb12..c77d6ae 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -182,6 +182,13 @@ struct _virStoragePoolSourceDeviceExtent { int type; /* free space type */ };
+typedef struct _virStoragePoolSourceInitiatorAttr virStoragePoolSourceInitiatorAttr; +typedef virStoragePoolSourceInitiatorAttr *virStoragePoolSourceInitiatorAttrPtr; +struct _virStoragePoolSourceInitiatorAttr { + /* Initiator iqn name */ + char *iqnname; +}; + /* * Pools can be backed by one or more devices, and some * allow us to track free space on underlying devices. @@ -223,6 +230,9 @@ struct _virStoragePoolSource { /* Or a name */ char *name;
+ /* One or more initiator names */ + virStoragePoolSourceInitiatorAttrPtr initiator; + int authType; /* virStoragePoolAuthType */ union { virStoragePoolAuthChap chap;
I like the functionality. To restate what I said when you first proposed it, though, each pool is currently based on one iSCSI session, so to preserve that paradigm, you should only allow zero or one initiator IQNs per pool. If the pool contains an initiator IQN, it will be used when creating the session. If it does not contain an initiator IQN, the system default initiator IQN will be used. If you require multiple initiator IQNs, you create multiple pools, one per initiator IQN each with the same target. I think that approach will result in a very small patch. Do you have a specific use case for which that approach would not work? Dave
I like the functionality. To restate what I said when you first proposed it, though, each pool is currently based on one iSCSI session, so to preserve that paradigm, you should only allow zero or one initiator IQNs per pool. If the pool contains an initiator IQN, it will be used when creating the session. If it does not contain an initiator IQN, the system default initiator IQN will be used. If you require multiple initiator IQNs, you create multiple pools, one per initiator IQN each with the same target. I think that approach will result in a very small patch. Do you have a specific use case for which that approach would not work?
Dave
Yes. Let us say I want to consider the LUNs behind a Target IQN as pool A.(Target centric terminology) If each initiator iqn creates separate pools than there will be duplicity of the same set of LUNs. And this is a side effect not necessarily a deliberate one. In this case the user knows that Pool A and Pool B are the same set of LUNs and it is a deliberate result. Possible usecase - Live Migration scenarios ... Increasing the initiator IQNs can be more effective in Load balancing, fault tolerance scenarios and the choice of the pools can be easily identified using Target IQNs. Also, with the above design if there is a need to create separate pools out of the same set of LUNs behind a Target IQN then that can be done explicitly by creating a fresh XML conf for each initiator IQN.
Shyam_Iyer@Dell.com wrote:
I like the functionality. To restate what I said when you first proposed it, though, each pool is currently based on one iSCSI session, so to preserve that paradigm, you should only allow zero or one initiator IQNs per pool. If the pool contains an initiator IQN, it will be used when creating the session. If it does not contain an initiator IQN, the system default initiator IQN will be used. If you require multiple initiator IQNs, you create multiple pools, one per initiator IQN each with the same target. I think that approach will result in a very small patch. Do you have a specific use case for which that approach would not work?
Dave
Yes.
Let us say I want to consider the LUNs behind a Target IQN as pool A.(Target centric terminology)
If each initiator iqn creates separate pools than there will be duplicity of the same set of LUNs. And this is a side effect not necessarily a deliberate one.
I'm not sure I understood you. If a LUN is visible on multiple sessions, there's going to be duplication of LUNs regardless of whether you use one pool with multiple sessions or multiple pools with one session per pool, because you're still establishing those sessions. You're also not guaranteed to have the same set of LUNs on both sessions, so you can't trust that the set of LUNs you get on one session is the same as the set on another session.
In this case the user knows that Pool A and Pool B are the same set of LUNs and it is a deliberate result.
Possible usecase - Live Migration scenarios ...
Increasing the initiator IQNs can be more effective in Load balancing, fault tolerance scenarios and the choice of the pools can be easily identified using Target IQNs.
If you can search for one pool with a particular target IQN, you can search for multiple pools with that IQN, right?
Also, with the above design if there is a need to create separate pools out of the same set of LUNs behind a Target IQN then that can be done explicitly by creating a fresh XML conf for each initiator IQN.
-----Original Message----- From: Dave Allan [mailto:dallan@redhat.com] Sent: Friday, October 23, 2009 2:55 AM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: iSCSI Multi-IQN (Libvirt Support)
Shyam_Iyer@Dell.com wrote:
I like the functionality. To restate what I said when you first proposed it, though, each pool is currently based on one iSCSI session, so to preserve that paradigm, you should only allow zero or one initiator IQNs per pool. If the pool contains an initiator IQN, it will be used when creating the session. If it does not contain an initiator IQN, the system default initiator IQN will be used. If you require multiple initiator IQNs, you create multiple pools, one per initiator IQN each with the same target. I think that approach will result in a very small patch. Do you have a specific use case for which that approach would not work?
Dave
Yes.
Let us say I want to consider the LUNs behind a Target IQN as pool A.(Target centric terminology)
If each initiator iqn creates separate pools than there will be duplicity of the same set of LUNs. And this is a side effect not necessarily a deliberate one.
I'm not sure I understood you. If a LUN is visible on multiple sessions, there's going to be duplication of LUNs regardless of whether you use one pool with multiple sessions or multiple pools with one session per pool, because you're still establishing those sessions. You're also not guaranteed to have the same set of LUNs on both sessions, so you can't trust that the set of LUNs you get on one session is the same as the set on another session.
Sorry. I wasn't clear. The present design allows the following - 1) ----------LUN A | ---------Initiator IQN1----Session 1--------------<Target IQN A>------------------LUN B | | ----------LUN A | | Pool A---------------Initiator IQN2----Session 2--------------<Target IQN A>------------------LUN B | | ---------Initiator IQN3----Session 3--------------<Target IQN B>------------------LUN C Target IQN A, B and C Or, ----------LUN A 2) | ---------Initiator IQN1----Session 1--------------<Target IQN A>------------------LUN B | | ----------LUN C | | Pool A---------------Initiator IQN2----Session 2--------------<Target IQN B>------------------LUN D | | ---------Initiator IQN3----Session 3--------------<Target IQN C>------------------LUN E And also, the one that you are describing. One pool for each initiator IQN 3) ------------------------------------ ----------LUN A | | Pool A----------------Initiator IQN1---Session 1--------------<Target IQN A>------------------LUN B | | ------------------------------------ ----------LUN C Today, the pool concept abstracts multiple LUNs behind a Target IQN into a common storage pool with a single session. The advantage of doing that with one pool is that managing the multiple LUNs with one pool becomes easy. By also abstracting multiple initiator iqns into a pool concept, the management of storage pools becomes easy for the same reason - "LUN management". At the same time it allows flexibility to realize a one pool per initiator iqn scenario that exists today. Consider the following example. If we use a separate pool for each initiator IQNs. #virsh <virsh> pool create <StorageArray_1_initiatoriqn1> <virsh> pool create <StorageArray_1_initiatoriqn2> ........................................... ........................................... <virsh> pool create <StorageArray_1_initiatoriqnN> If all pools associated with StorageArray_1 had to be destroyed then the following would happen. <virsh> pool destroy <StorageArray_1_initiatoriqn1> <virsh> pool destroy <StorageArray_1_initiatoriqn2> ........................................... ........................................... <virsh> pool destroy <StorageArray_1_initiatoriqnN> In the design that allows multiple initiator IQNs for a pool. #virsh <virsh> pool create <StorageArray_1> In this design the XML contains Multiple initiator IQNs and multiple sessions can be established for the pool "StorageArray_1". <virsh> pool destroy <StorageArray_1> With this design all the sessions for this pool will get destroyed with one effort.
Shyam_Iyer@Dell.com wrote:
-----Original Message----- From: Dave Allan [mailto:dallan@redhat.com] Sent: Friday, October 23, 2009 2:55 AM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: iSCSI Multi-IQN (Libvirt Support)
Shyam_Iyer@Dell.com wrote:
I like the functionality. To restate what I said when you first proposed it, though, each pool is currently based on one iSCSI session, so to preserve that paradigm, you should only allow zero or one initiator IQNs per pool. If the pool contains an initiator IQN, it will be used when creating the session. If it does not contain an initiator IQN, the system default initiator IQN will be used. If you require multiple initiator IQNs, you create multiple pools, one per initiator IQN each with the same target. I think that approach will result in a very small patch. Do you have a specific use case for which that approach would not work?
Dave
Yes.
Let us say I want to consider the LUNs behind a Target IQN as pool A.(Target centric terminology)
If each initiator iqn creates separate pools than there will be duplicity of the same set of LUNs. And this is a side effect not necessarily a deliberate one. I'm not sure I understood you. If a LUN is visible on multiple sessions, there's going to be duplication of LUNs regardless of whether you use one pool with multiple sessions or multiple pools with one session per pool, because you're still establishing those sessions. You're also not guaranteed to have the same set of LUNs on both sessions, so you can't trust that the set of LUNs you get on one session is the same as the set on another session.
Sorry. I wasn't clear.
The present design allows the following -
Hi Shyam, Your ASCII diagram got mangled by the emailing process. Would you mind sending a text document with it? I think I see what you're saying, but I'd like to confirm with your diagram before I comment further. Dave
1) ----------LUN A
| ---------Initiator IQN1----Session 1--------------<Target IQN A>------------------LUN B | | ----------LUN A | | Pool A---------------Initiator IQN2----Session 2--------------<Target IQN A>------------------LUN B | | ---------Initiator IQN3----Session 3--------------<Target IQN B>------------------LUN C
Target IQN A, B and C
Or,
----------LUN A 2) | ---------Initiator IQN1----Session 1--------------<Target IQN A>------------------LUN B | | ----------LUN C | | Pool A---------------Initiator IQN2----Session 2--------------<Target IQN B>------------------LUN D | | ---------Initiator IQN3----Session 3--------------<Target IQN C>------------------LUN E
And also, the one that you are describing. One pool for each initiator IQN
3) ------------------------------------ ----------LUN A | | Pool A----------------Initiator IQN1---Session 1--------------<Target IQN A>------------------LUN B | | ------------------------------------ ----------LUN C
Today, the pool concept abstracts multiple LUNs behind a Target IQN into a common storage pool with a single session. The advantage of doing that with one pool is that managing the multiple LUNs with one pool becomes easy.
By also abstracting multiple initiator iqns into a pool concept, the management of storage pools becomes easy for the same reason - "LUN management". At the same time it allows flexibility to realize a one pool per initiator iqn scenario that exists today.
Consider the following example.
If we use a separate pool for each initiator IQNs. #virsh <virsh> pool create <StorageArray_1_initiatoriqn1> <virsh> pool create <StorageArray_1_initiatoriqn2> ........................................... ........................................... <virsh> pool create <StorageArray_1_initiatoriqnN>
If all pools associated with StorageArray_1 had to be destroyed then the following would happen. <virsh> pool destroy <StorageArray_1_initiatoriqn1> <virsh> pool destroy <StorageArray_1_initiatoriqn2> ........................................... ........................................... <virsh> pool destroy <StorageArray_1_initiatoriqnN>
In the design that allows multiple initiator IQNs for a pool. #virsh <virsh> pool create <StorageArray_1> In this design the XML contains Multiple initiator IQNs and multiple sessions can be established for the pool "StorageArray_1". <virsh> pool destroy <StorageArray_1> With this design all the sessions for this pool will get destroyed with one effort.
I like the functionality. To restate what I said when you first proposed it, though, each pool is currently based on one iSCSI session, so to preserve that paradigm, you should only allow zero or one initiator IQNs per pool. If the pool contains an initiator IQN, it will be used when creating the session. If it does not contain an initiator IQN, the system default initiator IQN will be used. If you require multiple initiator IQNs, you create multiple pools, one per initiator IQN each with the same target. I think that approach will result in a very small patch. Do you have a specific use case for which that approach would not work?
Dave
Yes.
Let us say I want to consider the LUNs behind a Target IQN as
Shyam_Iyer@Dell.com wrote: pool
A.(Target centric terminology)
If each initiator iqn creates separate pools than there will be duplicity of the same set of LUNs. And this is a side effect not necessarily a deliberate one. I'm not sure I understood you. If a LUN is visible on multiple sessions, there's going to be duplication of LUNs regardless of whether you use one pool with multiple sessions or multiple pools with one session per pool, because you're still establishing those sessions. You're also not guaranteed to have the same set of LUNs on both sessions, so you can't trust that the set of LUNs you get on one session is the same as the set on another session.
Sorry. I wasn't clear.
The present design allows the following -
Hi Shyam,
Your ASCII diagram got mangled by the emailing process. Would you mind sending a text document with it? I think I see what you're saying, but I'd like to confirm with your diagram before I comment further.
Dave
Dave- Attached diagram doc describes the use cases. Thanks -Shyam
Shyam_Iyer@Dell.com wrote:
I like the functionality. To restate what I said when you first proposed it, though, each pool is currently based on one iSCSI session, so to preserve that paradigm, you should only allow zero or one initiator IQNs per pool. If the pool contains an initiator IQN, it will be used when creating the session. If it does not contain an initiator IQN, the system default initiator IQN will be used. If you require multiple initiator IQNs, you create multiple pools, one per initiator IQN each with the same target. I think that approach will result in a very small patch. Do you have a specific use case for which that approach would not work?
Dave
Yes.
Let us say I want to consider the LUNs behind a Target IQN as
Shyam_Iyer@Dell.com wrote: pool
A.(Target centric terminology)
If each initiator iqn creates separate pools than there will be duplicity of the same set of LUNs. And this is a side effect not necessarily a deliberate one. I'm not sure I understood you. If a LUN is visible on multiple sessions, there's going to be duplication of LUNs regardless of whether you use one pool with multiple sessions or multiple pools with one session per pool, because you're still establishing those sessions. You're also not guaranteed to have the same set of LUNs on both sessions, so you can't trust that the set of LUNs you get on one session is the same as the set on another session.
Sorry. I wasn't clear.
The present design allows the following - Hi Shyam,
Your ASCII diagram got mangled by the emailing process. Would you mind sending a text document with it? I think I see what you're saying, but I'd like to confirm with your diagram before I comment further.
Dave
Dave- Attached diagram doc describes the use cases.
Thanks -Shyam
Ok, you're proposing what I thought you were proposing. My objection to what you want to do is that I think this sort of complexity is better done in the client application than in libvirt. In other words, I think that *some code*, *somewhere* is going to have to loop through all the sessions and create and delete each one and enumerate the LUs on each session. I think we are only debating whether that loop should be in the client application or in libvirt. Why do you think we should put that complexity into libvirt? The way I see a client using the API is: 1) The client application enumerates the initiator IQNs it wants to use to establish sessions 2) The client application enumerates the target IQNs it wants to use to establish sessions 3) For each session: a) The client constructs the XML describing the session and b) calls create pool The way you see the client using the API is: 1) The client application enumerates the initiator IQNs it wants to use to establish sessions 2) The client application enumerates the target IQNs it wants to use to establish sessions 3) The client constructs the XML describing all the sessions and 4) calls create pool Is there a functional difference? Dave
-----Original Message----- From: Dave Allan [mailto:dallan@redhat.com] Sent: Tuesday, October 27, 2009 2:57 AM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: iSCSI Multi-IQN (Libvirt Support)
<Snip>
Ok, you're proposing what I thought you were proposing. My objection to what you want to do is that I think this sort of complexity is better done in the client application than in libvirt. In other words, I think that *some code*, *somewhere* is going to have to loop through all the sessions and create and delete each one and enumerate the LUs on each session. I think we are only debating whether that loop should be in the client application or in libvirt. Why do you think we should put that complexity into libvirt?
Ok. I was looking at giving an api that will facilitate this for the client easily but I see your point as well in the context of the present design.. So, we will stick to the following paradigm- 1) No initiator iqn in the xml would mean use of the default initiator iqn name 2) Initiator iqn provided would mean a unique session to be created using the provided iqn name. 3) Single iSCSI session per pool A few points though - The virt-manager GUI will need more intelligence built around easy manageability of pools. 1) More information on the pools. 2) Ability to group multiple pools. 2) Ability to delete multiple pools for easy administration. * This would be useful in an environment where there may be tens of arrays, hundred pools and a few more hundred LUNs. * For the visual experience if we stick to the 1session per pool design we would find multiple pools for the same target iqn and also pointing to the same luns and filing up the GUI screen unnecessarily sometimes ... Should this be discussed in the virt-manager lists also ?
Shyam_Iyer@Dell.com wrote:
-----Original Message----- From: Dave Allan [mailto:dallan@redhat.com] Sent: Tuesday, October 27, 2009 2:57 AM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: iSCSI Multi-IQN (Libvirt Support)
<Snip>
Ok, you're proposing what I thought you were proposing. My objection to what you want to do is that I think this sort of complexity is better done in the client application than in libvirt. In other words, I think that *some code*, *somewhere* is going to have to loop through all the sessions and create and delete each one and enumerate the LUs on each session. I think we are only debating whether that loop should be in the client application or in libvirt. Why do you think we should put that complexity into libvirt?
Ok. I was looking at giving an api that will facilitate this for the client easily but I see your point as well in the context of the present design..
So, we will stick to the following paradigm-
1) No initiator iqn in the xml would mean use of the default initiator iqn name 2) Initiator iqn provided would mean a unique session to be created using the provided iqn name. 3) Single iSCSI session per pool
Sounds good--thanks.
A few points though -
The virt-manager GUI will need more intelligence built around easy manageability of pools.
1) More information on the pools. 2) Ability to group multiple pools. 2) Ability to delete multiple pools for easy administration.
* This would be useful in an environment where there may be tens of arrays, hundred pools and a few more hundred LUNs. * For the visual experience if we stick to the 1session per pool design we would find multiple pools for the same target iqn and also pointing to the same luns and filing up the GUI screen unnecessarily sometimes ...
You should see the same LUNs multiple times if some LUNs are visible on multiple sessions, regardless of how the pool is implemented as far as one or multiple sessions. Am I understanding correctly that you want some sort of uniqueness filter on LUNs?
Should this be discussed in the virt-manager lists also ?
I think this should be discussed primarily on the virt-manager lists. If there's work required in libvirt to implement what you want to see in virt-manager, then discuss in both places, but you should get a design hammered out there and then see what may make sense to implement in libvirt. Dave
The following patch set realizes the multi-IQN concept discussed in an earlier thread http://www.mail-archive.com/libvir-list@redhat.com/msg16706.html And here .. http://www.mail-archive.com/libvir-list@redhat.com/msg17499.html The patch realizes an XML schema like the one below and allows libvirt to read through it to create storage pools. These XMLs when created using a virtualization manager realize unique VM to storage LUN mappings through a single console and thus opening up possibilities for the following scenarios - * possibility of multiple IQNs for a single Guest * option for hypervisor's initiator to use these IQNs on behalf of the guest Change Log from v0.3: 1) Set default tab space to 4 2) Use Case Description for Commit Log 3) Review comments from Dave Allan a) No initiator iqn in the xml would mean use of the default initiator iqn name b) Initiator iqn provided would mean a unique session to be created using the provided iqn name. c) Single iSCSI session per pool 4) Added syntax in doc/schemas/storagepool.rng There are no new errors introduced by this patch with "make check" and "make syntax-check" tests. Signed-off-by: Sudhir Bellad <sudhir_bellad@dell.com> Signed-off-by: Shyam Iyer <shyam_iyer@dell.com>
On Fri, Nov 13, 2009 at 05:18:54PM +0530, Sudhir_Bellad@Dell.com wrote:
The following patch set realizes the multi-IQN concept discussed in an earlier thread http://www.mail-archive.com/libvir-list@redhat.com/msg16706.html
And here .. http://www.mail-archive.com/libvir-list@redhat.com/msg17499.html
The patch realizes an XML schema like the one below and allows libvirt to read through it to create storage pools. These XMLs when created using a virtualization manager realize unique VM to storage LUN mappings through a single console and thus opening up possibilities for the following scenarios -
* possibility of multiple IQNs for a single Guest * option for hypervisor's initiator to use these IQNs on behalf of the guest
Change Log from v0.3: 1) Set default tab space to 4
I don't know what you did with indentation but the patch looks completely broken in this respect ...
2) Use Case Description for Commit Log 3) Review comments from Dave Allan a) No initiator iqn in the xml would mean use of the default initiator iqn name b) Initiator iqn provided would mean a unique session to be created using the provided iqn name. c) Single iSCSI session per pool 4) Added syntax in doc/schemas/storagepool.rng
There are no new errors introduced by this patch with "make check" and "make syntax-check" tests.
Please attach patches with mime type text/plain or something so that they can be viewed and commented in line even if attached :-)
From 7d76cba952e67317dc6f19b480a89d0077299cd8 Mon Sep 17 00:00:00 2001 From: Sudhir Bellad <sudhir_bellad@dell.com> Date: Fri, 13 Nov 2009 14:42:49 +0530 Subject: [PATCH 2/2] ISCSI Multi-IQN
The following patch set realizes the multi-IQN concept discussed in an earlier thread http://www.mail-archive.com/libvir-list@redhat.com/msg16706.html
And here .. http://www.mail-archive.com/libvir-list@redhat.com/msg17499.html
The patch realizes an XML schema like the one below and allows libvirt to read through it to create storage pools. These XMLs when created using a virtualization manager realize unique VM to storage LUN mappings through a single console and thus opening up possibilities for the following scenarios -
* possibility of multiple IQNs for a single Guest * option for hypervisor's initiator to use these IQNs on behalf of the guest
Change Log from v0.3: 1) Set default tab space to 4 2) Use Case Description for Commit Log 3) Review comments from Dave Allan a) No initiator iqn in the xml would mean use of the default initiator iqn name b) Initiator iqn provided would mean a unique session to be created using the provided iqn name. c) Single iSCSI session per pool 4) Added syntax in doc/schemas/storagepool.rng
There are no new errors introduced by this patch with "make check" and "make syntax-check" tests.
Signed-off-by: Sudhir Bellad <sudhir_bellad@dell.com> Signed-off-by: Shyam Iyer <shyam_iyer@dell.com>
--- docs/schemas/storagepool.rng | 10 +++++++ src/storage_backend_iscsi.c | 59 +++++++++++++++++++++++++++++++++++++----- src/storage_backend_iscsi.h | 2 + src/storage_conf.c | 20 ++++++++++---- src/storage_conf.h | 9 ++++++ 5 files changed, 87 insertions(+), 13 deletions(-)
diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index d225f97..ea14f31 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -175,6 +175,15 @@ </element> </define>
+ <define name='initiatorinfoiqnname'> + <element name='iqnname'> + <attribute name='name'> + <text/>
need indent
+ </attribute> + <empty/> + </element> + </define> + <define name='devextents'> <oneOrMore> <element name='freeExtent'> @@ -328,6 +337,7 @@ <element name='source'> <ref name='sourceinfohost'/> <ref name='sourceinfodev'/> + <ref name='initiatorinfoiqnname'/> </element> </define>
looks fine from a schemas POV
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..1fb21a5 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -159,13 +163,54 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator.iqnname != NULL) { + if (!(dir = opendir(IFACES_DIR))) { + if (errno == ENOENT) + return 0; + virReportSystemError(conn, errno, _("Failed to open dir '%s'"), + IFACES_DIR); + return -1; + } + while ((entry = readdir(dir))) { + FILE *fp; + char path[PATH_MAX];
we try to avoid allocating 4K arrays on the stack
+ if (entry->d_name[0] == '.') + continue; + + sprintf(path,"%s", IFACES_DIR); + strcat(path,(const char *)entry->d_name);
better to use virAsprintf() from util.h
+ if ((fp = fopen(path, "r"))){ + char buff[256]; + while (fp != NULL && fgets(buff, sizeof(buff), fp) != NULL) { + if (strstr(buff, pool->def->source.initiator.iqnname) != NULL) { + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, "-I", entry->d_name, action, NULL + }; + + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } + }
I'm not sure I fully understand the loop (indentation doesn't help !) First you look for all entries under IFACES_DIR, then for each of them you look for the iqnname initiator in it, then if it contains the substring (strange matching) then you execute ISCSIADM ... but you continue with the loop. So possibly you could call ISCSIADM many time when calling virStorageBackendISCSIConnection(). Is that really expected ?
+ } + fclose(fp); + } + closedir(dir); + } + else{ + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, action, NULL + }; + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + }
return 0; }
indentation is completely broken, apparently your editor doesn't show properly tab from spaces nor extra spaces at end of lines.
diff --git a/src/storage_backend_iscsi.h b/src/storage_backend_iscsi.h index 665ed13..1bb8892 100644 --- a/src/storage_backend_iscsi.h +++ b/src/storage_backend_iscsi.h @@ -28,4 +28,6 @@
extern virStorageBackend virStorageBackendISCSI;
+#define IFACES_DIR "/var/lib/iscsi/ifaces/" + #endif /* __VIR_STORAGE_BACKEND_ISCSI_H__ */ diff --git a/src/storage_conf.c b/src/storage_conf.c index 1633aac..db191e6 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -106,11 +106,12 @@ struct _virStorageVolOptions {
/* Flags to indicate mandatory components in the pool source */ enum { - VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), - VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), - VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), - VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), - VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), + VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), + VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), + VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), + VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (1<<5), };
@@ -179,7 +180,8 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { { .poolType = VIR_STORAGE_POOL_ISCSI, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_DEVICE), + VIR_STORAGE_POOL_SOURCE_DEVICE | + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN), }, .volOptions = { .formatToString = virStoragePoolFormatDiskTypeToString, @@ -283,6 +285,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) { VIR_FREE(source->dir); VIR_FREE(source->name); VIR_FREE(source->adapter); + VIR_FREE(source->initiator.iqnname);
if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { VIR_FREE(source->auth.chap.login); @@ -532,6 +535,11 @@ virStoragePoolDefParseXML(virConnectPtr conn, goto cleanup; } } + + if (options->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) { + ret->source.initiator.iqnname = virXPathString(conn, "string(./initiator/iqnname/@name)", ctxt); + } +
need to cut long line I would use "string(./initiator/iqnname[1]/@name)" in case there is an error with extra iqnname element(s). So if the option is set but there is no iqnname, the structure just get NULL and it's fine, right ?
if (options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) { xmlNodePtr *nodeset = NULL; int nsource, i; diff --git a/src/storage_conf.h b/src/storage_conf.h index 9fedb12..4152e18 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -182,6 +182,12 @@ struct _virStoragePoolSourceDeviceExtent { int type; /* free space type */ };
+typedef struct _virStoragePoolSourceInitiatorAttr virStoragePoolSourceInitiatorAttr; +struct _virStoragePoolSourceInitiatorAttr { + /* Initiator iqn name */ + char *iqnname; +}; + /* * Pools can be backed by one or more devices, and some * allow us to track free space on underlying devices. @@ -223,6 +229,9 @@ struct _virStoragePoolSource { /* Or a name */ char *name;
+ /* initiator iqn name */ + virStoragePoolSourceInitiatorAttr initiator; + int authType; /* virStoragePoolAuthType */ union { virStoragePoolAuthChap chap;
Using a full struct for just embbedding the initiator name may be a bit overkill, but that's just cosmetic. 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/
Sudhir_Bellad@Dell.com wrote:
The following patch set realizes the multi-IQN concept discussed in an earlier thread http://www.mail-archive.com/libvir-list@redhat.com/msg16706.html
And here .. http://www.mail-archive.com/libvir-list@redhat.com/msg17499.html
The patch realizes an XML schema like the one below and allows libvirt to read through it to create storage pools. These XMLs when created using a virtualization manager realize unique VM to storage LUN mappings through a single console and thus opening up possibilities for the following scenarios -
* possibility of multiple IQNs for a single Guest * option for hypervisor's initiator to use these IQNs on behalf of the guest
Change Log from v0.3: 1) Set default tab space to 4 2) Use Case Description for Commit Log 3) Review comments from Dave Allan a) No initiator iqn in the xml would mean use of the default initiator iqn name b) Initiator iqn provided would mean a unique session to be created using the provided iqn name. c) Single iSCSI session per pool 4) Added syntax in doc/schemas/storagepool.rng
There are no new errors introduced by this patch with "make check" and "make syntax-check" tests.
Signed-off-by: Sudhir Bellad <sudhir_bellad@dell.com> Signed-off-by: Shyam Iyer <shyam_iyer@dell.com>
--- docs/schemas/storagepool.rng | 10 +++++++ src/storage_backend_iscsi.c | 59 +++++++++++++++++++++++++++++++++++++----- src/storage_backend_iscsi.h | 2 + src/storage_conf.c | 20 ++++++++++---- src/storage_conf.h | 9 ++++++ 5 files changed, 87 insertions(+), 13 deletions(-) diff --git a/docs/schemas/storagepool.rng b/docs/schemas/storagepool.rng index d225f97..ea14f31 100644 --- a/docs/schemas/storagepool.rng +++ b/docs/schemas/storagepool.rng @@ -175,6 +175,15 @@ </element> </define> + <define name='initiatorinfoiqnname'> + <element name='iqnname'> Please make this element iqn, not iqnname. IQN is iSCSI Qualified Name so iqnname is redundant. + <attribute name='name'> + <text/> + </attribute> + <empty/> + </element> + </define> + <define name='devextents'> <oneOrMore> <element name='freeExtent'> @@ -328,6 +337,7 @@ <element name='source'> <ref name='sourceinfohost'/> <ref name='sourceinfodev'/> + <ref name='initiatorinfoiqnname'/> </element> </define> diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..1fb21a5 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h> #define VIR_FROM_THIS VIR_FROM_STORAGE @@ -159,13 +163,54 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator.iqnname != NULL) { What's the point of this loop? At best, it's unneeded complexity, at worst it will match multiple interface names which will create the multiple sessions per pool scenario that I explicitly want to avoid. Secondly, if you want to do some sort of validation of the iqn, why are you reading from a hardcoded directory? Can you use the output of iscsiadm? That is likely to be a more stable interface than the directory which I would expect is a compile time option to the iscsi initiator. + if (!(dir = opendir(IFACES_DIR))) { + if (errno == ENOENT) + return 0; It's not success if the directory doesn't exist; what's the purpose of this special case? + virReportSystemError(conn, errno, _("Failed to open dir '%s'"), + IFACES_DIR); + return -1; + } + while ((entry = readdir(dir))) { + FILE *fp; + char path[PATH_MAX]; Agreed with DV, don't allocate this memory on the stack. + if (entry->d_name[0] == '.') + continue; + + sprintf(path,"%s", IFACES_DIR); + strcat(path,(const char *)entry->d_name); + + if ((fp = fopen(path, "r"))){ + char buff[256]; Don't allocate this buffer on the stack, either. + while (fp != NULL && fgets(buff, sizeof(buff), fp) != NULL) { + if (strstr(buff, pool->def->source.initiator.iqnname) != NULL) { + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, "-I", entry->d_name, action, NULL + }; + + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } + } + } + fclose(fp); + } + closedir(dir); + } + else{ + const char *const cmdargv[] = { + ISCSIADM, "--mode", "node", "--portal", portal, + "--targetname", pool->def->source.devices[0].path, action, NULL + }; + if (virRun(conn, cmdargv, NULL) < 0) + return -1; + } return 0; } I'm still puzzled as to why this patch isn't limited to extending the schema and the 5 or so lines of code required create the iscsiadm command with an additional -I parameter. diff --git a/src/storage_backend_iscsi.h b/src/storage_backend_iscsi.h index 665ed13..1bb8892 100644 --- a/src/storage_backend_iscsi.h +++ b/src/storage_backend_iscsi.h @@ -28,4 +28,6 @@ extern virStorageBackend virStorageBackendISCSI; +#define IFACES_DIR "/var/lib/iscsi/ifaces/" + #endif /* __VIR_STORAGE_BACKEND_ISCSI_H__ */ diff --git a/src/storage_conf.c b/src/storage_conf.c index 1633aac..db191e6 100644 --- a/src/storage_conf.c +++ b/src/storage_conf.c @@ -106,11 +106,12 @@ struct _virStorageVolOptions { /* Flags to indicate mandatory components in the pool source */ enum { - VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), - VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), - VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), - VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), - VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_HOST = (1<<0), + VIR_STORAGE_POOL_SOURCE_DEVICE = (1<<1), + VIR_STORAGE_POOL_SOURCE_DIR = (1<<2), + VIR_STORAGE_POOL_SOURCE_ADAPTER = (1<<3), + VIR_STORAGE_POOL_SOURCE_NAME = (1<<4), + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN = (1<<5), }; @@ -179,7 +180,8 @@ static virStoragePoolTypeInfo poolTypeInfo[] = { { .poolType = VIR_STORAGE_POOL_ISCSI, .poolOptions = { .flags = (VIR_STORAGE_POOL_SOURCE_HOST | - VIR_STORAGE_POOL_SOURCE_DEVICE), + VIR_STORAGE_POOL_SOURCE_DEVICE | + VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN), }, .volOptions = { .formatToString = virStoragePoolFormatDiskTypeToString, @@ -283,6 +285,7 @@ virStoragePoolSourceFree(virStoragePoolSourcePtr source) { VIR_FREE(source->dir); VIR_FREE(source->name); VIR_FREE(source->adapter); + VIR_FREE(source->initiator.iqnname); if (source->authType == VIR_STORAGE_POOL_AUTH_CHAP) { VIR_FREE(source->auth.chap.login); @@ -532,6 +535,11 @@ virStoragePoolDefParseXML(virConnectPtr conn, goto cleanup; } } + + if (options->flags & VIR_STORAGE_POOL_SOURCE_INITIATOR_IQN) { + ret->source.initiator.iqnname = virXPathString(conn, "string(./initiator/iqnname/@name)", ctxt); + } + if (options->flags & VIR_STORAGE_POOL_SOURCE_DEVICE) { xmlNodePtr *nodeset = NULL; int nsource, i; diff --git a/src/storage_conf.h b/src/storage_conf.h index 9fedb12..4152e18 100644 --- a/src/storage_conf.h +++ b/src/storage_conf.h @@ -182,6 +182,12 @@ struct _virStoragePoolSourceDeviceExtent { int type; /* free space type */ }; +typedef struct _virStoragePoolSourceInitiatorAttr virStoragePoolSourceInitiatorAttr; +struct _virStoragePoolSourceInitiatorAttr { + /* Initiator iqn name */ + char *iqnname; +}; + /* * Pools can be backed by one or more devices, and some * allow us to track free space on underlying devices. @@ -223,6 +229,9 @@ struct _virStoragePoolSource { /* Or a name */ char *name; + /* initiator iqn name */ + virStoragePoolSourceInitiatorAttr initiator; + int authType; /* virStoragePoolAuthType */ union { virStoragePoolAuthChap chap; -- 1.6.2.2
On Mon, Nov 16, 2009 at 01:58:08PM -0500, Dave Allan wrote:
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..1fb21a5 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -159,13 +163,54 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator.iqnname != NULL) {
What's the point of this loop? At best, it's unneeded complexity, at worst it will match multiple interface names which will create the multiple sessions per pool scenario that I explicitly want to avoid.
Secondly, if you want to do some sort of validation of the iqn, why are you reading from a hardcoded directory? Can you use the output of iscsiadm? That is likely to be a more stable interface than the directory which I would expect is a compile time option to the iscsi initiator.
I'm really wondering much the same here - I don't see the purpose in iterating over this directory. The iqn given in the XML ought to be able to be passed straight to iscsadm's -I parameter Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Tuesday, November 17, 2009 11:21 PM To: Dave Allan Cc: Bellad, Sudhir; libvir-list@redhat.com; Iyer, Shyam; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Mon, Nov 16, 2009 at 01:58:08PM -0500, Dave Allan wrote:
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..1fb21a5 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -159,13 +163,54 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator.iqnname != NULL) {
What's the point of this loop? At best, it's unneeded complexity, at worst it will match multiple interface names which will create the multiple sessions per pool scenario that I explicitly want to avoid.
Secondly, if you want to do some sort of validation of the iqn, why are you reading from a hardcoded directory? Can you use the output of iscsiadm? That is likely to be a more stable interface than the directory which I would expect is a compile time option to the iscsi initiator.
I'm really wondering much the same here - I don't see the purpose in iterating over this directory. The iqn given in the XML ought to be able to be passed straight to iscsadm's -I parameter
Iscsiadm's -I parameter takes iface name as the parameter value and not the iqn name. So I believe this approach could be taken - 1) Get the iqn for the corresponding iface name using the following command #iscsiadm -m iface Example output: [root@localhost libvirt-0.7.1-15-org]# iscsiadm -m iface default tcp,default,default,unknown iser iser,default,default,unknown bnx2i bnx2i,default,default,unknown iface1 tcp,default,default,iqn.1994-05.com.fedora:iqnBellad iface3 tcp,default,default,iqn.dell iface0 tcp,unknown,unknown,iqn.1994-05.com.fedora:iqnSudhir The last value is the initiator iqn name.
Shyam_Iyer@Dell.com wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Tuesday, November 17, 2009 11:21 PM To: Dave Allan Cc: Bellad, Sudhir; libvir-list@redhat.com; Iyer, Shyam; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Mon, Nov 16, 2009 at 01:58:08PM -0500, Dave Allan wrote:
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..1fb21a5 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -159,13 +163,54 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator.iqnname != NULL) {
What's the point of this loop? At best, it's unneeded complexity, at worst it will match multiple interface names which will create the multiple sessions per pool scenario that I explicitly want to avoid.
Secondly, if you want to do some sort of validation of the iqn, why are you reading from a hardcoded directory? Can you use the output of iscsiadm? That is likely to be a more stable interface than the directory which I would expect is a compile time option to the iscsi initiator. I'm really wondering much the same here - I don't see the purpose in iterating over this directory. The iqn given in the XML ought to be able to be passed straight to iscsadm's -I parameter
Iscsiadm's -I parameter takes iface name as the parameter value and not the iqn name.
So I believe this approach could be taken -
1) Get the iqn for the corresponding iface name using the following command #iscsiadm -m iface
Example output:
[root@localhost libvirt-0.7.1-15-org]# iscsiadm -m iface default tcp,default,default,unknown iser iser,default,default,unknown bnx2i bnx2i,default,default,unknown iface1 tcp,default,default,iqn.1994-05.com.fedora:iqnBellad iface3 tcp,default,default,iqn.dell iface0 tcp,unknown,unknown,iqn.1994-05.com.fedora:iqnSudhir
The last value is the initiator iqn name.
Oh, ok, that makes sense. If you use the output of iscsiadm and STREQ on a portion of the output instead of strstr to match the iqn, that should be fine. You should also break out of the loop once you've matched the iqn. Dave
On Wed, Nov 18, 2009 at 01:53:50AM +0530, Shyam_Iyer@Dell.com wrote:
-----Original Message-----
Iscsiadm's -I parameter takes iface name as the parameter value and not the iqn name.
So I believe this approach could be taken -
1) Get the iqn for the corresponding iface name using the following command #iscsiadm -m iface
Example output:
[root@localhost libvirt-0.7.1-15-org]# iscsiadm -m iface default tcp,default,default,unknown iser iser,default,default,unknown bnx2i bnx2i,default,default,unknown iface1 tcp,default,default,iqn.1994-05.com.fedora:iqnBellad iface3 tcp,default,default,iqn.dell iface0 tcp,unknown,unknown,iqn.1994-05.com.fedora:iqnSudhir
Ah, so where does this mapping come from ? Does the administrator have to create the mapping between interfaces & iqns, or is this totally automatic somehow ? Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Wednesday, November 18, 2009 3:50 PM To: Iyer, Shyam Cc: dallan@redhat.com; Bellad, Sudhir; libvir-list@redhat.com; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Wed, Nov 18, 2009 at 01:53:50AM +0530, Shyam_Iyer@Dell.com wrote:
-----Original Message-----
Iscsiadm's -I parameter takes iface name as the parameter value and
not
the iqn name.
So I believe this approach could be taken -
1) Get the iqn for the corresponding iface name using the following command #iscsiadm -m iface
Example output:
[root@localhost libvirt-0.7.1-15-org]# iscsiadm -m iface default tcp,default,default,unknown iser iser,default,default,unknown bnx2i bnx2i,default,default,unknown iface1 tcp,default,default,iqn.1994-05.com.fedora:iqnBellad iface3 tcp,default,default,iqn.dell iface0 tcp,unknown,unknown,iqn.1994-05.com.fedora:iqnSudhir
Ah, so where does this mapping come from ? Does the administrator have to create the mapping between interfaces & iqns, or is this totally automatic somehow ?
Today the iface file is created by by iscsi administrators using either of the following methods 1) By hand by editing the iface parameters in /var/lib/iscsi/ifaces/<iface_file> Iface.iscsi_ifacename = <iface name> Iface.net_ifacename = <default, eth0 etc> Iface.hwaddress = <default, mac address etc> Iface.transport_name = <tcp, bnx2i etc> Iface.initiatorname = <iqn name> 2) By using iscsiadm commands like the following # iscsiadm -m iface -I <iface_name> -o new # iscsiadm -m iface -I iface1 --op=update -n iface.initiatorname -v <iqnname> We don't change the default initiatorname with libvirt/virt manager today right ? So, we haven't added any apis to manage the iface file name using libvirt as such. But if required by virt-manager(to create a single window of management) the iface file could be constructed using libvirt apis that call the iscsiadm commands detailed in method 2) Does that sound reasonable ?
On Wed, Nov 18, 2009 at 04:01:53PM +0530, Shyam_Iyer@Dell.com wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Wednesday, November 18, 2009 3:50 PM To: Iyer, Shyam Cc: dallan@redhat.com; Bellad, Sudhir; libvir-list@redhat.com; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Wed, Nov 18, 2009 at 01:53:50AM +0530, Shyam_Iyer@Dell.com wrote:
Iscsiadm's -I parameter takes iface name as the parameter value and not the iqn name.
So I believe this approach could be taken -
1) Get the iqn for the corresponding iface name using the following command #iscsiadm -m iface
Example output:
[root@localhost libvirt-0.7.1-15-org]# iscsiadm -m iface default tcp,default,default,unknown iser iser,default,default,unknown bnx2i bnx2i,default,default,unknown iface1 tcp,default,default,iqn.1994-05.com.fedora:iqnBellad iface3 tcp,default,default,iqn.dell iface0 tcp,unknown,unknown,iqn.1994-05.com.fedora:iqnSudhir
Ah, so where does this mapping come from ? Does the administrator have to create the mapping between interfaces & iqns, or is this totally automatic somehow ?
Today the iface file is created by by iscsi administrators using either of the following methods
1) By hand by editing the iface parameters in /var/lib/iscsi/ifaces/<iface_file>
Iface.iscsi_ifacename = <iface name> Iface.net_ifacename = <default, eth0 etc> Iface.hwaddress = <default, mac address etc> Iface.transport_name = <tcp, bnx2i etc> Iface.initiatorname = <iqn name>
This approach isn't really useful for us, since libvirt API is intended to be usable remotely without any administrator login.
2) By using iscsiadm commands like the following # iscsiadm -m iface -I <iface_name> -o new # iscsiadm -m iface -I iface1 --op=update -n iface.initiatorname -v <iqnname>
In that case, why don't we make libvirt automatically set things up this way whenever it sees the IQN in the XML file.
We don't change the default initiatorname with libvirt/virt manager today right ? So, we haven't added any apis to manage the iface file name using libvirt as such.
But if required by virt-manager(to create a single window of management) the iface file could be constructed using libvirt apis that call the iscsiadm commands detailed in method 2)
We don't need to add any new APIs for this - just automatically add the mapping whenever logging into a target with a IQN that we don't already know about Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Wednesday, November 18, 2009 4:12 PM To: Iyer, Shyam Cc: dallan@redhat.com; Bellad, Sudhir; libvir-list@redhat.com; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Wed, Nov 18, 2009 at 04:01:53PM +0530, Shyam_Iyer@Dell.com wrote:
-----Original Message----- From: Daniel P. Berrange [mailto:berrange@redhat.com] Sent: Wednesday, November 18, 2009 3:50 PM To: Iyer, Shyam Cc: dallan@redhat.com; Bellad, Sudhir; libvir-list@redhat.com; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Wed, Nov 18, 2009 at 01:53:50AM +0530, Shyam_Iyer@Dell.com wrote:
Iscsiadm's -I parameter takes iface name as the parameter value and not the iqn name.
So I believe this approach could be taken -
1) Get the iqn for the corresponding iface name using the following command #iscsiadm -m iface
Example output:
[root@localhost libvirt-0.7.1-15-org]# iscsiadm -m iface default tcp,default,default,unknown iser iser,default,default,unknown bnx2i bnx2i,default,default,unknown iface1 tcp,default,default,iqn.1994-05.com.fedora:iqnBellad iface3 tcp,default,default,iqn.dell iface0 tcp,unknown,unknown,iqn.1994-05.com.fedora:iqnSudhir
Ah, so where does this mapping come from ? Does the administrator have to create the mapping between interfaces & iqns, or is this totally automatic somehow ?
Today the iface file is created by by iscsi administrators using either of the following methods
1) By hand by editing the iface parameters in /var/lib/iscsi/ifaces/<iface_file>
Iface.iscsi_ifacename = <iface name> Iface.net_ifacename = <default, eth0 etc> Iface.hwaddress = <default, mac address etc> Iface.transport_name = <tcp, bnx2i etc> Iface.initiatorname = <iqn name>
This approach isn't really useful for us, since libvirt API is intended to be usable remotely without any administrator login.
2) By using iscsiadm commands like the following # iscsiadm -m iface -I <iface_name> -o new # iscsiadm -m iface -I iface1 --op=update -n iface.initiatorname -v <iqnname>
In that case, why don't we make libvirt automatically set things up this way whenever it sees the IQN in the XML file.
We don't change the default initiatorname with libvirt/virt manager today right ? So, we haven't added any apis to manage the iface file name using libvirt as such.
But if required by virt-manager(to create a single window of management) the iface file could be constructed using libvirt apis that call the iscsiadm commands detailed in method 2)
We don't need to add any new APIs for this - just automatically add
the
mapping whenever logging into a target with a IQN that we don't already know about
How do we ensure that the iqn is not already known and libvirt is creating the corresponding iface file for the first time? That would mean searching through the IFACES_DIR which has been opposed in the thread... I guess we should assume that the iqn entered into the xml has already been checked for uniqueness ... Is that a fair assumption? To enable a user to make that unique choice virt-manager should be able to show iface file configurations. We can go to that route later by adding libvirt apis that show the already configured iface file configurations to the user ... Also, how do we go about creating unique iface filenames when automatically creating them using libvirt ?
On Wed, Nov 18, 2009 at 04:37:02PM +0530, Shyam_Iyer@Dell.com wrote:
2) By using iscsiadm commands like the following # iscsiadm -m iface -I <iface_name> -o new # iscsiadm -m iface -I iface1 --op=update -n iface.initiatorname -v <iqnname>
In that case, why don't we make libvirt automatically set things up this way whenever it sees the IQN in the XML file.
We don't change the default initiatorname with libvirt/virt manager today right ? So, we haven't added any apis to manage the iface file name using libvirt as such.
But if required by virt-manager(to create a single window of management) the iface file could be constructed using libvirt apis that call the iscsiadm commands detailed in method 2)
We don't need to add any new APIs for this - just automatically add
the
mapping whenever logging into a target with a IQN that we don't already know about
How do we ensure that the iqn is not already known and libvirt is creating the corresponding iface file for the first time? That would mean searching through the IFACES_DIR which has been opposed in the thread...
Now that you've explained why this searching in IFACE_DIR was needed, I've no problem with that approach. We should search through IFACES_DIR to find the iface mapping, if none is found, then run those 2 commands you show above in order to create a new mapping
Also, how do we go about creating unique iface filenames when automatically creating them using libvirt ?
I don't know what naming convention is used in IFACES_DIR, but I'd say just follow whatever's used already Regards, Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
-----Original Message----- From: libvir-list-bounces@redhat.com [mailto:libvir-list- bounces@redhat.com] On Behalf Of Daniel P. Berrange Sent: Wednesday, November 18, 2009 4:47 PM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Wed, Nov 18, 2009 at 04:37:02PM +0530, Shyam_Iyer@Dell.com wrote:
2) By using iscsiadm commands like the following # iscsiadm -m iface -I <iface_name> -o new # iscsiadm -m iface -I iface1 --op=update -n iface.initiatorname
<iqnname>
In that case, why don't we make libvirt automatically set things up this way whenever it sees the IQN in the XML file.
We don't change the default initiatorname with libvirt/virt manager today right ? So, we haven't added any apis to manage the iface file name using libvirt as such.
But if required by virt-manager(to create a single window of management) the iface file could be constructed using libvirt apis that call
-v the
iscsiadm commands detailed in method 2)
We don't need to add any new APIs for this - just automatically add the mapping whenever logging into a target with a IQN that we don't already know about
How do we ensure that the iqn is not already known and libvirt is creating the corresponding iface file for the first time? That would mean searching through the IFACES_DIR which has been opposed in the thread...
Now that you've explained why this searching in IFACE_DIR was needed, I've no problem with that approach. We should search through IFACES_DIR to find the iface mapping, if none is found, then run those 2 commands you show above in order to create a new mapping
Ok. Thanks.
Also, how do we go about creating unique iface filenames when automatically creating them using libvirt ?
I don't know what naming convention is used in IFACES_DIR, but I'd say just follow whatever's used already
What I mean here is how do I create unique file names through libvirt. Today a unique iface file name is created by the user manually. So, should mkstemp be used... with a template feed of the initiator iqn name ?
Shyam_Iyer@Dell.com wrote:
-----Original Message----- From: libvir-list-bounces@redhat.com [mailto:libvir-list- bounces@redhat.com] On Behalf Of Daniel P. Berrange Sent: Wednesday, November 18, 2009 4:47 PM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] Re: [Patch v0.4] iSCSI Multi-IQN (Libvirt Support)
On Wed, Nov 18, 2009 at 04:37:02PM +0530, Shyam_Iyer@Dell.com wrote:
2) By using iscsiadm commands like the following # iscsiadm -m iface -I <iface_name> -o new # iscsiadm -m iface -I iface1 --op=update -n iface.initiatorname
<iqnname> In that case, why don't we make libvirt automatically set things up this way whenever it sees the IQN in the XML file.
We don't change the default initiatorname with libvirt/virt manager today right ? So, we haven't added any apis to manage the iface file name using libvirt as such.
But if required by virt-manager(to create a single window of management) the iface file could be constructed using libvirt apis that call
-v the
iscsiadm commands detailed in method 2) We don't need to add any new APIs for this - just automatically add the mapping whenever logging into a target with a IQN that we don't already know about
How do we ensure that the iqn is not already known and libvirt is creating the corresponding iface file for the first time? That would mean searching through the IFACES_DIR which has been opposed in the thread... Now that you've explained why this searching in IFACE_DIR was needed, I've no problem with that approach. We should search through IFACES_DIR to find the iface mapping, if none is found, then run those 2 commands you show above in order to create a new mapping
Ok. Thanks.
Dan and I took another look at this, and we think that parsing the output of iscsiadm is the right way to go, since the relevant information is available there. It's what we're doing in the rest of the code, as well as (IMO) less likely to change between builds of iscsiadm/distros/etc. Sorry about the conflicting advice.
Also, how do we go about creating unique iface filenames when automatically creating them using libvirt ? I don't know what naming convention is used in IFACES_DIR, but I'd say just follow whatever's used already
What I mean here is how do I create unique file names through libvirt. Today a unique iface file name is created by the user manually.
So, should mkstemp be used... with a template feed of the initiator iqn name ?
Just to make sure I'm understanding correctly, the filename is the interface name, so what we're talking about here is generating a unique interface name? Assuming that's the case, you could use a randomly generated string, although personally I find those things somewhat difficult to keep in mind. You could also just append a monotonically increasing value, but I don't have too strong an opinion about how you do it. Any way you generate the interface name, you should use iscsiadm to create the interface. Dave
On 10/22/2009 01:50 PM, Shyam_Iyer@Dell.com wrote: <snip>
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..3f2a79d 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -159,13 +163,57 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator[0].iqnname != NULL) {
You are using 8 space indentation here which does not match the libvirt convention of 4 spaces, please fix that. Also, be sure both 'make check' and 'make syntax-check' pass with the patch applied.
+ int i = 0; + while(pool->def->source.initiator[i].iqnname != NULL){ + if (!(dir = opendir(IFACES_DIR))) { + if (errno == ENOENT) + return 0; + virReportSystemError(conn, errno, _("Failed to open dir '%s'"), + IFACES_DIR); + return -1; + }
Looks like the patch was mangled. Just pasting the patch into your client probably won't be sufficient. I'd recommend git format-patch and git send-email, or just attach the patch file. Thanks, Cole
On 10/22/2009 03:05 PM, Cole Robinson wrote:
Looks like the patch was mangled. Just pasting the patch into your client probably won't be sufficient. I'd recommend git format-patch and git send-email, or just attach the patch file.
Actually, I found out earlier today that when a patch is sent as an attachment, and I reply to that message message using Thunderbird, the attachment isn't included in the reply (and so obviously isn't sitting there quoted), making it more difficult to comment. So as long as you've got an smtp server that will work without authentication (or possibly git send-email can be covered to deal with this too?), I think git send-email is the way to go (btw, I don't know if it was added recently or I just never noticed before, but the other day I found out I can call git send-email directly on my git repository - "git send-email HEAD~3" for example.)
On Thu, Oct 22, 2009 at 03:33:37PM -0400, Laine Stump wrote:
On 10/22/2009 03:05 PM, Cole Robinson wrote:
Looks like the patch was mangled. Just pasting the patch into your client probably won't be sufficient. I'd recommend git format-patch and git send-email, or just attach the patch file.
Actually, I found out earlier today that when a patch is sent as an attachment, and I reply to that message message using Thunderbird, the attachment isn't included in the reply (and so obviously isn't sitting there quoted), making it more difficult to comment.
Either that's a Thunderbird bug, or the attachment isn't text/plain. Sometimes mailers use silly things like text/x-patch and that confuses things. Sending them inline is safest if using a decent editor which can insert files without line wrapping, or using git-send-email Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On 10/22/2009 03:38 PM, Daniel P. Berrange wrote:
On Thu, Oct 22, 2009 at 03:33:37PM -0400, Laine Stump wrote:
On 10/22/2009 03:05 PM, Cole Robinson wrote:
Looks like the patch was mangled. Just pasting the patch into your client probably won't be sufficient. I'd recommend git format-patch and git send-email, or just attach the patch file.
Actually, I found out earlier today that when a patch is sent as an attachment, and I reply to that message message using Thunderbird, the attachment isn't included in the reply (and so obviously isn't sitting there quoted), making it more difficult to comment.
Either that's a Thunderbird bug, or the attachment isn't text/plain.
The message wsa one that DV sent this morning. I just checked and it is a text/plain attachment. Thunderbird was happy to *display* the attachment inline, but didn't want to include it inline in a reply. I did just figure out a workaround - select the entire displayed message prior to hitting reply, and it will include (and quote) the attachment contents in the reply.
Sometimes mailers use silly things like text/x-patch and that confuses things. Sending them inline is safest if using a decent editor
Thunderbird isn't "decent" in that respect either ;-) I've been unable to prevent it from mangling stuff I paste into messages (of course, I haven't tried very hard either).
which can insert files without line wrapping, or using git-send-email
-----Original Message----- From: Cole Robinson [mailto:crobinso@redhat.com] Sent: Friday, October 23, 2009 12:35 AM To: Iyer, Shyam Cc: libvir-list@redhat.com; Bellad, Sudhir; Domsch, Matt; KM, Paniraja Subject: Re: [libvirt] iSCSI Multi-IQN (Libvirt Support)
On 10/22/2009 01:50 PM, Shyam_Iyer@Dell.com wrote:
<snip>
diff --git a/src/storage_backend_iscsi.c b/src/storage_backend_iscsi.c index b516add..3f2a79d 100644 --- a/src/storage_backend_iscsi.c +++ b/src/storage_backend_iscsi.c @@ -39,6 +39,10 @@ #include "storage_backend_iscsi.h" #include "util.h" #include "memory.h" +#include <sys/types.h> +#include <sys/stat.h> +#include <fcntl.h> +#include <unistd.h>
#define VIR_FROM_THIS VIR_FROM_STORAGE
@@ -159,13 +163,57 @@ virStorageBackendISCSIConnection(virConnectPtr conn, const char *portal, const char *action) { - const char *const cmdargv[] = { - ISCSIADM, "--mode", "node", "--portal", portal, - "--targetname", pool->def->source.devices[0].path, action, NULL - }; - - if (virRun(conn, cmdargv, NULL) < 0) - return -1; + DIR *dir; + struct dirent *entry; + + + if (pool->def->source.initiator[0].iqnname != NULL) {
You are using 8 space indentation here which does not match the
libvirt
convention of 4 spaces, please fix that.
Ok. Also, be sure both 'make
check' and 'make syntax-check' pass with the patch applied.
Thanks for pointing these tests. Got a few fail tests here. Will fix them up and resend.
+ int i = 0; + while(pool->def->source.initiator[i].iqnname != NULL){ + if (!(dir = opendir(IFACES_DIR))) { + if (errno == ENOENT) + return 0; + virReportSystemError(conn, errno, _("Failed to open dir '%s'"), + IFACES_DIR); + return -1; + }
Looks like the patch was mangled. Just pasting the patch into your client probably won't be sufficient. I'd recommend git format-patch
and
git send-email, or just attach the patch file.
Sure. Thanks.
participants (7)
-
Cole Robinson -
Daniel P. Berrange -
Daniel Veillard -
Dave Allan -
Laine Stump -
Shyam_Iyer＠Dell.com -
Sudhir_Bellad＠Dell.com