wow. cool, Bing Bu. You should send out this patch easily, then we can avoid so many repetitions work. To Aline: Bing Bu not only implement iscsi and nfs pool, but also implement a test case for iscsi and nfs pool. On 12/20/2013 06:54 AM, Bing Bu Cao wrote:

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

...

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(-)

Before I leave kimchi, I written one patch set which refactoring the storage pool part. But Adam did not allow me to send this patch out because he thought it is not important at that moment. Whatever, I hope this can give you some other ideas for storage pool refactoring. But I am afraid you should make a big rebase because they are really so old...

From 6cd5a7900a4f9794c97a1cf86f32128f046bf526 Mon Sep 17 00:00:00 2001 From: Bing Bu Cao <mars@linux.vnet.ibm.com> Date: Thu, 22 Aug 2013 10:00:00 +0800 Subject: [PATCH 1/4] Generate the storage object XML in storage.py

Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> --- src/kimchi/Makefile.am | 1 + src/kimchi/storage.py | 168 ++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 0 deletions(-) create mode 100644 src/kimchi/storage.py diff --git a/src/kimchi/Makefile.am b/src/kimchi/Makefile.am index c5cf5cc..b4e92d4 100644 --- a/src/kimchi/Makefile.am +++ b/src/kimchi/Makefile.am @@ -36,6 +36,7 @@ kimchi_PYTHON = \ screenshot.py \ server.py \ sslcert.py \ + storage.py \ template.py \ vmtemplate.py \ vnc.py \ diff --git a/src/kimchi/storage.py b/src/kimchi/storage.py new file mode 100644 index 0000000..1114756 --- /dev/null +++ b/src/kimchi/storage.py @@ -0,0 +1,168 @@ +# +# Project Burnet +# +# Copyright IBM, Corp. 2013 +# +# Authors: +# Bing Bu Cao <mars@linux.vnet.ibm.com> +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +import copy +import os +from kimchi.exception import * + +STOR_TYPE_POOL = 'pool' +STOR_TYPE_VOLUME = 'volume' +STOR_TYPE_LIST = [STOR_TYPE_POOL, STOR_TYPE_VOLUME] + +DIR = "dir" +NETFS = "netfs" +ISCSI = "iscsi" +POOL_TYPE_LIST = [DIR, NETFS, ISCSI] + +DEFAULT_DIR_TARGET_BASE = "/var/lib/libvirt/images/" +DEFAULT_SCSI_TARGET = "/dev/disk/by-path" + + +class storage(object): + def __init__(self, storage_type, pool_type=None, **kwargs): + """ + @param storage_type: 'pool' or 'volume' + @param pool_type: 'pool' or 'volume' + @param kwargs: + """ + if storage_type not in STOR_TYPE_LIST: + raise InvalidParameter( + "Unknown Storage Type: %s" % storage_type) + if storage_type == 'volume' and pool_type not in POOL_TYPE_LIST: + raise InvalidParameter( + "Invalid Pool Type: %s" % pool_type) + + self.pool_type = pool_type + self.params = copy.copy(kwargs) + self.type = storage_type + + def dumpxml(self): + return getattr(self, "_get_%s_xml" % self.type)(self.pool_type) + + def _get_pool_xml(self, pool_type): + return getattr(self, "_get_%(type)s_pool_xml" % self.params)() + + def _get_dir_pool_xml(self): + # Required parameters + # name: + # type: + # Optional parameters + # path: + kwargs = self.params + kwargs.setdefault('path', DEFAULT_DIR_TARGET_BASE) + xml = """ + <pool type='%(type)s'> + <name>%(name)s</name> + <target> + <path>%(path)s</path> + </target> + </pool> + """ % kwargs + return xml + + def _get_netfs_pool_xml(self): + # Required parameters + # name: + # type: + # Optional parameters + # path: + kwargs = self.params + formats = ["auto", "nfs", "glusterfs"] + kwargs.setdefault('path', os.path.join(DEFAULT_DIR_TARGET_BASE, + kwargs['name'])) + kwargs.setdefault('format', "auto") + format = kwargs['format'] + if not format in formats: + raise InvalidParameter( + "Unknown Network Filesystem format: %s" % format) + xml = """ + <pool type='%(type)s'> + <name>%(name)s</name> + <source> + <host name='%(host)s'/> + <dir path='%(src_path)s'/> + <format type='%(format)s'/> + </source> + <target> + <path>%(path)s</path> + </target> + </pool> + """ % kwargs + return xml + + def _get_iscsi_pool_xml(self): + # Required parameters + # name: + # type: + # Optional parameters + # path: + kwargs = self.params + kwargs.setdefault('path', DEFAULT_SCSI_TARGET) + xml = """ + <pool type='%(type)s'> + <name>%(name)s</name> + <source> + <host name='%(host)s'/> + <device path='%(src_path)s'/> + </source> + <target> + <path>%(path)s</path> + </target> + </pool> + """ % kwargs + return xml + + def _get_volume_xml(self, pool_type): + # Required parameters + # name: + # capacity: + # path: + # Optional parameters + # allocation: + # format(not used for iscsi pool): + if pool_type == ISCSI: + raise InvalidOperation( + "Creating volume is not supported for iscsi pool") + kwargs = self.params + kwargs.setdefault('allocation', 0) + kwargs.setdefault('format', 'qcow2') + target = self._get_vol_target_xml(kwargs.get('format')) + kwargs.update({'target': target}) + xml = """ + <volume> + <name>%(name)s</name> + <allocation unit="MiB">%(allocation)s</allocation> + <capacity unit="MiB">%(capacity)s</capacity> + <source> + </source>%(target)s + </volume> + """ % kwargs + return xml + + def _get_vol_target_xml(self, format): + target_xml = "" + if self.pool_type == DIR: + target_xml = """ + <target> + <format type='%s'/> + </target>""" % format + return target_xml -- 1.7.7.6

From 93dd8c5d78706d7444ad54cf52845812aa8d7173 Mon Sep 17 00:00:00 2001 From: Bing Bu Cao <mars@linux.vnet.ibm.com> Date: Thu, 22 Aug 2013 10:02:02 +0800 Subject: [PATCH 2/4] Use new xml generator in model for StoragePool and StorageVolume operation

Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> --- src/kimchi/model.py | 53 +++++++------------------------------------------- 1 files changed, 8 insertions(+), 45 deletions(-) diff --git a/src/kimchi/model.py b/src/kimchi/model.py index f05b532..670fb7e 100644 --- a/src/kimchi/model.py +++ b/src/kimchi/model.py @@ -54,6 +54,7 @@ from kimchi.exception import * from kimchi.utils import kimchi_log, is_digit from kimchi.distroloader import DistroLoader +from kimchi.storage import storage ISO_POOL_NAME = u'kimchi_isos' STATS_INTERVAL = 5 @@ -581,7 +582,8 @@ class Model(object): name = params['name'] if name in (ISO_POOL_NAME, ): raise InvalidOperation("StoragePool already exists") - xml = _get_pool_xml(**params) + storpool = storage('pool', **params) + xml = storpool.dumpxml() except KeyError, key: raise MissingParameter(key) if name in self.storagepools_get_list(): @@ -689,10 +691,13 @@ class Model(object): raise def storagevolumes_create(self, pool, params): - info = self.storagepool_lookup(pool) + pool_obj = self._get_storagepool(pool) + pooltype = xmlutils.xpath_get_text(pool_obj.XMLDesc(0), + "/pool/@type")[0] try: name = params['name'] - xml = _get_volume_xml(**params) + storvol = storage('volume', pool_type=pooltype, **params) + xml = storvol.dumpxml() except KeyError, key: raise MissingParameter(key) pool = self._get_storagepool(pool) @@ -826,48 +831,6 @@ class LibvirtVMScreenshot(VMScreenshot): os.close(fd) -def _get_pool_xml(**kwargs): - # Required parameters - # name: - # type: - # path: - xml = """ - <pool type='%(type)s'> - <name>%(name)s</name> - <target> - <path>%(path)s</path> - </target> - </pool> - """ % kwargs - return xml - - -def _get_volume_xml(**kwargs): - # Required parameters - # name: - # capacity: - # - # Optional: - # allocation: - # format: - kwargs.setdefault('allocation', 0) - kwargs.setdefault('format', 'qcow2') - - xml = """ - <volume> - <name>%(name)s</name> - <allocation unit="MiB">%(allocation)s</allocation> - <capacity unit="MiB">%(capacity)s</capacity> - <source> - </source> - <target> - <format type='%(format)s'/> - </target> - </volume> - """ % kwargs - return xml - - class LibvirtConnection(object): def __init__(self, uri): self.uri = uri -- 1.7.7.6

From 21433a52d1b135d696e187d0e29b68d5954749f5 Mon Sep 17 00:00:00 2001 From: Bing Bu Cao <mars@linux.vnet.ibm.com> Date: Thu, 22 Aug 2013 10:03:39 +0800 Subject: [PATCH 3/4] Add testcases in test_model.py to test the ISCSI and NFS storagepool

Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> --- tests/test_model.py | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 123 insertions(+), 0 deletions(-) diff --git a/tests/test_model.py b/tests/test_model.py index dcabbcb..3f6a10e 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -26,6 +26,10 @@ import threading import os import time import tempfile +import shlex +import subprocess +import random +import shutil import kimchi.model import kimchi.objectstore @@ -256,6 +260,125 @@ class ModelTests(unittest.TestCase): for key in params.keys(): self.assertEquals(params[key], info[key]) + def _execute_cmd(self, cmd): + print cmd + args = shlex.split(cmd) + p = subprocess.Popen(args, close_fds=True, stdout=subprocess.PIPE, + stderr=subprocess.PIPE) + out, err = p.communicate() + print out, err + + def _create_iscsi_target(self, path, iqn, capacity): + self._execute_cmd("dd if=/dev/zero of=%s bs=1M count=%i" % (path, capacity)) + conf = """ + <target %s> + backing-store %s + </target> + """ % (iqn, path) + conf_file = '/etc/tgt/targets.conf' + with open(conf_file, 'a') as file: + file.write(conf) + + self._execute_cmd("tgt-admin --update %s" % iqn) + + def _get_target(self, iqn): + self._execute_cmd("tgt-admin --show") + + def _clean_iscsi_target(self, iqn): + self._execute_cmd("tgt-admin --delete %s" % iqn) + #remove the lines added before + conf = open('/etc/tgt/targets.conf') + lines = conf.readlines() + conf.close() + conf = open('/etc/tgt/targets.conf', 'w') + lines = lines[:-4] + conf.writelines(lines) + conf.close() + self._execute_cmd("exportfs -rf") + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_iscsi_pool(self): + inst = kimchi.model.Model('qemu:///system', self.tmp_store) + path = '/srv/images' + name = 'iscsi-test-pool' + img = 'kimchi-%s.img' % random.getrandbits(16) + iqn = 'iqn.2013.08.org.foo.bar:%s' % random.getrandbits(16) + cap = random.getrandbits(8) + if not os.path.exists(path): + os.makedirs(path) + + self._create_iscsi_target(os.path.join(path, img), iqn, cap) + self._get_target(iqn) + + with utils.RollbackContext() as rollback: + args = {'name': name, + 'type': 'iscsi', + 'host': '127.0.0.1', + 'src_path': iqn} + inst.storagepools_create(args) + rollback.prependDefer(inst.storagepool_delete, name) + inst.storagepool_activate(name) + rollback.prependDefer(inst.storagepool_deactivate, name) + poolinfo = inst.storagepool_lookup(name) + self.assertEquals(cap, poolinfo['capacity']) + + self._clean_iscsi_target(iqn) + shutil.rmtree(path) + + def _create_nfs(self, path): + conf = '%s 127.0.0.1(rw,sync,root_squash)\n' % path + conf_file = '/etc/exports' + with open(conf_file, 'a') as file: + file.write(conf) + + self._execute_cmd("exportfs -rv") + + def _clean_nfs(self, path): + #remove the line added in _create_nfs() + conf = open('/etc/exports') + output = [] + for line in conf: + if not path in line: + output.append(line) + conf.close() + conf = open('/etc/exports', 'w') + conf.writelines(output) + conf.close() + self._execute_cmd("exportfs -rf") + + @unittest.skipUnless(utils.running_as_root(), 'Must be run as root') + def test_nfs_pool(self): + inst = kimchi.model.Model('qemu:///system', self.tmp_store) + src_path = '/tmp/nfs-%s' % random.getrandbits(16) + src_file = 'kimchi-%s.img' % random.getrandbits(16) + name = "pool-nfs-test" + cap = 32 * 1024 * 1024 + if not os.path.exists(src_path): + os.makedirs(src_path) + fd = open(os.path.join(src_path, src_file), 'w') + fd.truncate(cap) + self._create_nfs(src_path) + + dst_path = '/mnt/nfs' + if not os.path.exists(dst_path): + os.makedirs(dst_path) + with utils.RollbackContext() as rollback: + args = {"type": "netfs", + "name": name, + "host": "127.0.0.1", + "format": "nfs", + "src_path": src_path, + "path": dst_path} + inst.storagepools_create(args) + rollback.prependDefer(inst.storagepool_delete, name) + inst.storagepool_activate(name) + rollback.prependDefer(inst.storagepool_deactivate, name) + volinfo = inst.storagevolume_lookup(name, src_file) + self.assertEquals(cap >> 20, volinfo['capacity']) + + self._clean_nfs(src_path) + shutil.rmtree(src_path) + def test_multithreaded_connection(self): def worker(): for i in xrange(100): -- 1.7.7.6

From 74c089a3a8174e16f2b03795b5c759334f4ce2ec Mon Sep 17 00:00:00 2001 From: Bing Bu Cao <mars@linux.vnet.ibm.com> Date: Thu, 22 Aug 2013 10:17:00 +0800 Subject: [PATCH 4/4] Redefine the storagepool 'POST' description in API.md

Signed-off-by: Bing Bu Cao <mars@linux.vnet.ibm.com> --- docs/API.md | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/docs/API.md b/docs/API.md index 0c187e0..73d3396 100644 --- a/docs/API.md +++ b/docs/API.md @@ -163,8 +163,12 @@ 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 + * path: The target path of the defined Storage Pool + * src_path: The source path of the defined Storage pool * type: The type of the defined Storage Pool + * host: The remote server which provide the pool source + Only and necessary for netfs and iscsi storage pool + * format: The format of the pool, only valid for netfs storage pool ### Resource: Storage Pool -- 1.7.7.6 -- Sheldon Feng(冯少合)<shaohef@linux.vnet.ibm.com> IBM Linux Technology Center