[PATCH v1 0/4] Stroage Pool: refactor and add iSCSI support

[Zhou Zheng Sheng]

Hi all!

I'm implementing iSCSI storage pool support in kimchi. I found that the
current design and implementation is not very flexible to add new type of
pool.

Firstly it uses "if ... elif" to inspect the requested pool type and go into
the corresponding branch and create XML. This could be turn into a dispatching
dict, with the pool type as the key, and XML generating function as the value.

Secondly I think a few arguments for various type of pool can be re-used if
they are for the same purpose. For example, both NFS pool and iSCSI pool need
a remote host argument, so I renamed the "nfsserver" to "srcHost", and use
"srcHost" in both NFS and iSCSI pool. I also change the relative front end
code.

After the refactoring, I add the iSCSI support. I've tested all the code on
Fedora 19 by defining various types of storage pool with various argument.

Zhou Zheng Sheng (4):
storagepool: dispatch _get_pool_xml() to respective
_get_XXX_storagepool_xml() function
storagepool: rename and consolidate arguments of creating (back end)
storagepool: rename and consolidate arguments of creating (front end)
storagepool: Support Creating iSCSI storagepool in model.py

docs/API.md | 21 ++--
src/kimchi/model.py | 158 +++++++++++++++++++++----------
ui/js/src/kimchi.storagepool_add_main.js | 9 +-
3 files changed, 128 insertions(+), 60 deletions(-)

--
1.7.11.7

[Zhou Zheng Sheng]

In src/kimchi/model.py, _get_pool_xml() is to generate libvirt storage
pool XML definition from arguments provided by the POST data (a JSON
dict). It has to support various types of pool such as dir, netfs and
logical. Now it uses "if ... elif ... elif" to check the requested type
of pool and go in to the respective branch and generates the correct
XML.

This is OK for now but in future we'll have more types of pool, so in
this patch we turn "if ... elif" into a dispatching dict, and put the
respective XML generating code to separated functions.

Signed-off-by: Zhou Zheng Sheng <zhsh...@linux.vnet.ibm.com>
---
src/kimchi/model.py | 121 +++++++++++++++++++++++++++++++---------------------
1 file changed, 73 insertions(+), 48 deletions(-)

diff --git a/src/kimchi/model.py b/src/kimchi/model.py
index 90a9a1b..d22e02d 100644
--- a/src/kimchi/model.py
+++ b/src/kimchi/model.py
@@ -1403,60 +1403,85 @@ class LibvirtVMScreenshot(VMScreenshot):
finally:
os.close(fd)

-def _get_pool_xml(**kwargs):
+
+def _get_dir_storagepool_xml(poolArgs):
# Required parameters
# name:
# type:
# path:
+ xml = """
+ <pool type='%(type)s'>
+ <name>%(name)s</name>
+ <target>
+ <path>%(path)s</path>
+ </target>
+ </pool>
+ """ % poolArgs
+ return xml
+
+
+def _get_netfs_storagepool_xml(poolArgs):
+ # Required parameters
+ # name:
+ # type:
+ # nfsserver:
# nfspath:
- if kwargs['type'] == 'dir':
- xml = """
- <pool type='%(type)s'>
- <name>%(name)s</name>
- <target>
- <path>%(path)s</path>
- </target>
- </pool>
- """
- elif kwargs['type'] == 'netfs':
- path = '/var/lib/kimchi/nfs_mount/' + kwargs['name'];
- if not os.path.exists(path):
- os.makedirs(path)
- kwargs.update({'path': path})
- xml = """
- <pool type='%(type)s'>
- <name>%(name)s</name>
- <source>
- <host name='%(nfsserver)s'/>
- <dir path='%(nfspath)s'/>
- </source>
- <target>
- <path>%(path)s</path>
- </target>
- </pool>
- """
- elif kwargs['type'] == 'logical':
- path = '/var/lib/kimchi/logical_mount/' + kwargs['name'];
- if not os.path.exists(path):
- os.makedirs(path)
- xml = """
- <pool type='%(type)s'>
- <name>%(name)s</name>
- <source>
- %(devices)s
- </source>
- <target>
- <path>%(path)s</path>
- </target>
- </pool>
- """
- devices = ''
- for device_path in kwargs['devices']:
- devices += "<device path=\""+device_path+"\" />"
+ path = '/var/lib/kimchi/nfs_mount/' + poolArgs['name']
+ if not os.path.exists(path):
+ os.makedirs(path)
+ poolArgs['path'] = path
+ xml = """
+ <pool type='%(type)s'>
+ <name>%(name)s</name>
+ <source>
+ <host name='%(nfsserver)s'/>
+ <dir path='%(nfspath)s'/>
+ </source>
+ <target>
+ <path>%(path)s</path>
+ </target>
+ </pool>
+ """ % poolArgs
+ return xml
+
+
+def _get_logical_storagepool_xml(poolArgs):
+ # Required parameters
+ # name:
+ # type:
+ # devices:
+ path = '/var/lib/kimchi/logical_mount/' + poolArgs['name']
+ if not os.path.exists(path):
+ os.makedirs(path)
+
+ devices = []
+ for device_path in poolArgs['devices']:
+ devices.append('<device path="%s" />' % device_path)
+
+ poolArgs.update({'devices': ''.join(devices),
+ 'path': path})
+ xml = """
+ <pool type='%(type)s'>
+ <name>%(name)s</name>
+ <source>
+ %(devices)s
+ </source>
+ <target>
+ <path>%(path)s</path>
+ </target>
+ </pool>
+ """ % poolArgs
+ return xml
+
+
+def _get_pool_xml(**kwargs):
+ getPoolXml = {
+ 'dir': _get_dir_storagepool_xml,
+ 'netfs': _get_netfs_storagepool_xml,
+ 'logical': _get_logical_storagepool_xml,
+ }[kwargs['type']]
+ return getPoolXml(kwargs)

- kwargs.update({'devices': devices})
- kwargs.update({'path': path})
- return xml % kwargs

def _get_volume_xml(**kwargs):
# Required parameters
--
1.7.11.7

[Zhou Zheng Sheng]

As we are adding support to new type of storage pool, the current naming
scheme of the storage pool creating arguments should be rearranged to be
more extendable. This patch renames some arguments and consolidates the
argument of the same purposes as follow.

nfsserver -> srcHost
This is because in future patches, iSCSI pool can use this srcHost as
well. Other network backed storage pool can also make use of this
argument.

nfspath -> srcPath
This is because other netfs pool can also make use of this argument.

devices -> srcDevices
To differentiate source arguments from the target arguments, we can add
a "src" prefix to source arguments.
---
docs/API.md | 19 +++++++++++--------
src/kimchi/model.py | 17 +++++++++--------
2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/docs/API.md b/docs/API.md
index 74bc1b5..dd3d7f1 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -180,15 +180,18 @@ Represents a snapshot of the Virtual Machine's primary monitor.

* **GET**: Retrieve a summarized list of all defined Storage Pools
* **POST**: Create a new Storage Pool
- * name: The name of the Storage Pool
- * path: The path of the defined Storage Pool,
+ * name: The name of the Storage Pool.
+ * type: The type of the defined Storage Pool.
+ Supported types: 'dir', 'kimchi-iso', 'netfs', 'logical'
+ * path: The path of the defined Storage Pool.
For 'kimchi-iso' pool refers to targeted deep scan path.
- * type: The type of the defined Storage Pool,
- Supported types: 'dir', 'kimchi-iso', 'netfs'
- * nfsserver: IP or hostname of NFS server to create NFS pool.
- * nfspath: export path on nfs server for NFS pool.
- * devices: Array of devices to be used in the Storage Pool
- Exclusive to the 'logical' storage pool type.
+ Pool types: 'dir', 'kimchi-iso'.
+ * srcHost: IP or hostname of server for a pool backed from a remote host.
+ Pool types: 'nfs'.
+ * srcPath: Export path on NFS server for NFS pool.
+ Pool types: 'nfs'.
+ * srcDevices: Array of devices to be used in the Storage Pool
+ Pool types: 'logical'.

### Resource: Storage Pool

diff --git a/src/kimchi/model.py b/src/kimchi/model.py
index d22e02d..af0d728 100644
--- a/src/kimchi/model.py
+++ b/src/kimchi/model.py
@@ -1424,8 +1424,8 @@ def _get_netfs_storagepool_xml(poolArgs):

# Required parameters
# name:
# type:

- # nfsserver:
- # nfspath:
+ # srcHost:
+ # srcPath:

path = '/var/lib/kimchi/nfs_mount/' + poolArgs['name']

if not os.path.exists(path):
os.makedirs(path)
@@ -1434,8 +1434,8 @@ def _get_netfs_storagepool_xml(poolArgs):
<pool type='%(type)s'>
<name>%(name)s</name>

<source>
- <host name='%(nfsserver)s'/>
- <dir path='%(nfspath)s'/>

+ <host name='%(srcHost)s'/>
+ <dir path='%(srcPath)s'/>
</source>
<target>
<path>%(path)s</path>
@@ -1449,22 +1449,23 @@ def _get_logical_storagepool_xml(poolArgs):

# Required parameters
# name:
# type:

- # devices:
+ # srcDevices:

path = '/var/lib/kimchi/logical_mount/' + poolArgs['name']

if not os.path.exists(path):
os.makedirs(path)

devices = []
- for device_path in poolArgs['devices']:
+ for device_path in poolArgs['srcDevices']:

devices.append('<device path="%s" />' % device_path)

- poolArgs.update({'devices': ''.join(devices),
+ poolArgs.update({'srcDevices': ''.join(devices),

'path': path})
+
xml = """

<pool type='%(type)s'>
<name>%(name)s</name>
<source>
- %(devices)s
+ %(srcDevices)s
</source>
<target>
<path>%(path)s</path>
--
1.7.11.7

[Zhou Zheng Sheng]

As we are adding support to new type of storage pool, the current naming
scheme of the storage pool creating arguments should be rearranged to be
more extendable. This patch renames some arguments and consolidates the
argument of the same purposes as follow.

nfsserver -> srcHost

nfspath -> srcPath
devices -> srcDevices

This patch contains the front end changes of the consolidation work. It
just changes the formData transferred to kimchi backend, and leaves the
templates and other related scripts untouched.
---
ui/js/src/kimchi.storagepool_add_main.js | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/ui/js/src/kimchi.storagepool_add_main.js b/ui/js/src/kimchi.storagepool_add_main.js
index 7efbfa0..a5fb039 100644
--- a/ui/js/src/kimchi.storagepool_add_main.js
+++ b/ui/js/src/kimchi.storagepool_add_main.js
@@ -151,11 +151,14 @@ kimchi.addPool = function(event) {
if (!$.isArray(formData.devices)) {
var deviceObj = [];
deviceObj[0] = formData.devices;
- formData.devices = deviceObj;
+ formData.srcDevices = deviceObj;
+ } else {
+ formData.srcDevices = formData.devices;
}
+ delete formData.devices;
} else {
- formData.nfspath = $('#nfspathId').val();
- formData.nfsserver = $('#nfsserverId').val();
+ formData.srcPath = $('#nfspathId').val();
+ formData.srcHost = $('#nfsserverId').val();
}
kimchi.createStoragePool(formData, function() {
kimchi.doListStoragePools();
--
1.7.11.7

[Zhou Zheng Sheng]

This patch implements creating iSCSI storagepool for libvirt.
Each LUN in iSCSI storagepool can be used as a volume, but iSCSI
storagepool does not provide ability to create volume. For now in kimchi
we create volume for each newly created VM. Next is to implement
attaching existing volume to a new VM.

How to test

Create:
curl -u root -H 'Content-type: application/json' \
-H 'Accept: application/json' \
-d '{"srcTarget":
"targetIQN",
"srcHost": "10.0.0.X",
"type": "iscsi",
"name": "iscsipool"}' \
http://127.0.0.1:8000/storagepools

Show Info:
curl -u root -H 'Accept: application/json' \
http://127.0.0.1:8000/storagepools/iscsipool

Activate:
curl -u root -H 'Content-type: application/json' \
-H 'Accept: application/json' \
-d '' http://127.0.0.1:8000/storagepools/iscsipool/activate

Examine:
iscsiadm -m session

Deactivate:
curl -u root -H 'Content-type: application/json' \
-H 'Accept: application/json' \
-d '' http://127.0.0.1:8000/storagepools/iscsipool/deactivate

Delete:
curl -u root -X DELETE -H 'Accept: application/json' \
http://127.0.0.1:8000/storagepools/iscsipool

Signed-off-by: Zhou Zheng Sheng <zhsh...@linux.vnet.ibm.com>
---

docs/API.md | 6 ++++--
src/kimchi/model.py | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/docs/API.md b/docs/API.md
index dd3d7f1..342c583 100644
--- a/docs/API.md
+++ b/docs/API.md
@@ -182,16 +182,18 @@ Represents a snapshot of the Virtual Machine's primary monitor.

* **POST**: Create a new Storage Pool

* name: The name of the Storage Pool.

* type: The type of the defined Storage Pool.

- Supported types: 'dir', 'kimchi-iso', 'netfs', 'logical'
+ Supported types: 'dir', 'kimchi-iso', 'netfs', 'logical', 'iscsi'

* path: The path of the defined Storage Pool.
For 'kimchi-iso' pool refers to targeted deep scan path.

Pool types: 'dir', 'kimchi-iso'.

* srcHost: IP or hostname of server for a pool backed from a remote host.

- Pool types: 'nfs'.
+ Pool types: 'nfs', 'iscsi'.

* srcPath: Export path on NFS server for NFS pool.

Pool types: 'nfs'.

* srcDevices: Array of devices to be used in the Storage Pool

Pool types: 'logical'.
+ * srcTarget: Target IQN of an iSCSI pool.
+ Pool types: 'iscsi'.

### Resource: Storage Pool

diff --git a/src/kimchi/model.py b/src/kimchi/model.py

index af0d728..3888903 100644
--- a/src/kimchi/model.py
+++ b/src/kimchi/model.py
@@ -1475,11 +1475,45 @@ def _get_logical_storagepool_xml(poolArgs):
return xml


+def _get_iscsi_storagepool_xml(poolArgs):

+ # Required parameters
+ # name:
+ # type:

+ # srcHost:
+ # srcTarget:
+ #
+ # Optional parameters
+ # srcPort:
+
+ try:
+ portAttr = "port='%s'" % poolArgs['srcPort']
+ except KeyError:
+ portAttr = ""
+
+ poolArgs.update({'srcPort': portAttr,
+ 'path': '/dev/disk/by-id'})
+

+ xml = """
+ <pool type='%(type)s'>
+ <name>%(name)s</name>
+ <source>

+ <host name='%(srcHost)s' %(srcPort)s/>
+ <device path='%(srcTarget)s'/>

+ </source>
+ <target>
+ <path>%(path)s</path>
+ </target>
+ </pool>
+ """ % poolArgs
+ return xml
+
+

def _get_pool_xml(**kwargs):
getPoolXml = {
'dir': _get_dir_storagepool_xml,
'netfs': _get_netfs_storagepool_xml,
'logical': _get_logical_storagepool_xml,
+ 'iscsi': _get_iscsi_storagepool_xml,
}[kwargs['type']]
return getPoolXml(kwargs)

--
1.7.11.7

[Pradeep K Surisetty]

On Mon, 16 Dec 2013 16:01:28 +0800
Zhou Zheng Sheng <zhsh...@linux.vnet.ibm.com> wrote:

we would have to support many of these storage pools.

http://libvirt.org/storage.html

Split xml file file every type of pool, Looks good to me.

[Pradeep K Surisetty]

On Mon, 16 Dec 2013 16:01:31 +0800

Zhou Zheng Sheng <zhsh...@linux.vnet.ibm.com> wrote:

Can you provide front-end (where user give inputs), test cases
also for this.

[Zhou Zheng Sheng]

on 2013/12/16 18:09, Pradeep K Surisetty wrote:
> On Mon, 16 Dec 2013 16:01:31 +0800
> Zhou Zheng Sheng <zhsh...@linux.vnet.ibm.com> wrote:
>
> Can you provide front-end (where user give inputs), test cases
> also for this.
>

Sure. Thanks! I'll send a new patch to provide test cases. I think it
would require an existing iSCSI target or it skips the cases.

As regard of front-end, I'd like to ask the UI team for help. Maybe I
can invite them to review my patches. @ShaoHe, would you give me some
suggestions on implementing the front-end?

--
Thanks and best regards!

Zhou Zheng Sheng / 周征晟
E-mail: zhsh...@linux.vnet.ibm.com
Telephone: 86-10-82454397

[Royce Lv]

Reviewed-by: Royce Lv<lvr...@linux.vnet.ibm.com>

[Royce Lv]

Zhengsheng, param naming and choice need to extendible for types we support now and in the future according to me.
I'm not sure SCSI adapter and RBD pool hosts list can be categorized as some param below.
Also, srcHost and devices are type related params, some pool have and some not.
So what about put these in a unified param 'source' and let different pool type checking for themselves?

[Shu Ming]

Is it not the intention to replace "nfsserver" with "src_host"?

> + <dir path='%(nfspath)s'/>

Is it not the intention to replace "nfspath" with "src_path"?

> + </source>
> + <target>
> + <path>%(path)s</path>
> + </target>
> + </pool>
> + """ % poolArgs
> + return xml
> +
> +
> +def _get_logical_storagepool_xml(poolArgs):
> + # Required parameters
> + # name:
> + # type:
> + # devices:
> + path = '/var/lib/kimchi/logical_mount/' + poolArgs['name']

Can we make the path configurable? and make

'/var/lib/kimchi/*" as the default setting.

[Shu Ming]

于 2013/12/16 16:01, Zhou Zheng Sheng 写道:

Let's keep the name style as the others in this file, like "src_host" or
"srchost".

> + * srcPath: Export path on NFS server for NFS pool.
> + Pool types: 'nfs'.

The same as the above.

> + * srcDevices: Array of devices to be used in the Storage Pool
> + Pool types: 'logical'.

The same as the above.

[Shu Ming]

于 2013/12/16 16:01, Zhou Zheng Sheng 写道:

I think we also need a API to discover the target from the initiator
that the client can discover the iSCSI targets first and then select the
target.

[Royce Lv]

Zhengsheng, what is the behavior when iscsi server cannot be connected
or there is not such target on server?
Will it block other storage pool operations?

[Royce Lv]

Ming, I covered this in the storage target probe functionality.

[Zhou Zheng Sheng]

Yes. This is in another patch, "storagepool: rename and consolidate

arguments of creating (back end)"

>> + </source>
>> + <target>
>> + <path>%(path)s</path>
>> + </target>
>> + </pool>
>> + """ % poolArgs
>> + return xml
>> +
>> +
>> +def _get_logical_storagepool_xml(poolArgs):
>> + # Required parameters
>> + # name:
>> + # type:
>> + # devices:
>> + path = '/var/lib/kimchi/logical_mount/' + poolArgs['name']
> Can we make the path configurable? and make
>
> '/var/lib/kimchi/*" as the default setting.
>
>

I agree, let's do it in another patch.

[Zhou Zheng Sheng]

Thanks for the suggestion. I will change the style.

[Sheldon]

As Pradeep mentioned:
we will add more storage pools, how about split these xml to a new file?
 

Sheldon Feng(冯少合)
IBM Linux Technology Center

[Sheldon]

       

On 12/16/2013 04:01 PM, Zhou Zheng Sheng wrote:

http://libvirt.org/formatstorage.html#exampleISCSI

iSCSI based storage pool

      <pool type="iscsi">
        <name>virtimages</name>
        <source>
          <host name="iscsi.example.com"/>
          <device path="iqn.2013-06.com.example:iscsi-pool"/>
          <auth type='chap' username='myuser'>
            <secret usage='libvirtiscsi'/>
          </auth>
        </source>
        <target>
          <path>/dev/disk/by-path</path>
        </target>
      </pool>

auth tag?

 def _get_pool_xml(**kwargs):
     getPoolXml = {
         'dir': _get_dir_storagepool_xml,
         'netfs': _get_netfs_storagepool_xml,
         'logical': _get_logical_storagepool_xml,
+        'iscsi': _get_iscsi_storagepool_xml,
         }[kwargs['type']]
     return getPoolXml(kwargs)

[Sheldon]

       

On 12/16/2013 04:01 PM, Zhou Zheng Sheng wrote:

http://libvirt.org/storage.html#StorageBackendRBD
not sure we will support RBD pool.
For RBD, the srcHost should be a list.

      <pool type="rbd">
        <name>myrbdpool</name>
        <source>
          <name>rbdpool</name>
            <host name='1.2.3.4' port='6789'/>
            <host name='my.ceph.monitor' port='6789'/>
            <host name='third.ceph.monitor' port='6789'/>
            <auth username='admin' type='ceph'>
              <secret uuid='2ec115d7-3a88-3ceb-bc12-0ac909a6fd87'/>
            </auth>
        </source>
      </pool>