[Kimchi-devel] [PATCH v2 5/5] test_model: test creating iSCSI storage pool
Aline Manera
alinefm at linux.vnet.ibm.com
Fri Dec 27 12:39:21 UTC 2013
On 12/27/2013 02:07 AM, Zhou Zheng Sheng wrote:
> on 2013/12/26 23:35, Aline Manera wrote:
>> On 12/22/2013 08:55 AM, Zhou Zheng Sheng wrote:
>>> This patch adds iSCSI storage pool test to test_modle.test_storagepool.
>> typo: test_modle
>>
> Get it. Thanks!
>>> It firstly checks if there exists an iSCSI target named
>>> "iqn.2013-12.localhost.kimchiUnitTest" on local host. If not, skip the
>>> test; if yes, continue to run the test already defined in
>>> test_storagepool.
>>>
>>> On RedHat family distributions, libiscsi comes with a Python binding,
>>> but not on other distributions. So in this patch it invokes iscsiadm to
>>> check iSCSI target.
>>>
>>> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
>>> ---
>>> Makefile.am | 1 +
>>> src/kimchi/Makefile.am | 1 +
>>> src/kimchi/iscsi.py | 57 +++++++++++++++++++++++++++++++
>>> tests/test_model.py | 91
>>> ++++++++++++++++++++++++++++----------------------
>>> 4 files changed, 111 insertions(+), 39 deletions(-)
>>> create mode 100644 src/kimchi/iscsi.py
>>>
>>> diff --git a/Makefile.am b/Makefile.am
>>> index cbdc128..d832fcd 100644
>>> --- a/Makefile.am
>>> +++ b/Makefile.am
>>> @@ -41,6 +41,7 @@ PEP8_WHITELIST = \
>>> src/kimchi/asynctask.py \
>>> src/kimchi/config.py.in \
>>> src/kimchi/disks.py \
>>> + src/kimchi/iscsi.py \
>>> src/kimchi/server.py \
>>> plugins/__init__.py \
>>> plugins/sample/__init__.py \
>>> diff --git a/src/kimchi/Makefile.am b/src/kimchi/Makefile.am
>>> index 47a3bd2..02b6fee 100644
>>> --- a/src/kimchi/Makefile.am
>>> +++ b/src/kimchi/Makefile.am
>>> @@ -28,6 +28,7 @@ kimchi_PYTHON = \
>>> distroloader.py \
>>> exception.py \
>>> __init__.py \
>>> + iscsi.py \
>>> isoinfo.py \
>>> netinfo.py \
>>> network.py \
>>> diff --git a/src/kimchi/iscsi.py b/src/kimchi/iscsi.py
>>> new file mode 100644
>>> index 0000000..96c77fa
>>> --- /dev/null
>>> +++ b/src/kimchi/iscsi.py
>> It is only used for tests so move it under /tests
>>
> I plan to validate iscsi backend before creating the iscsi pool. It will
> be used in model.py, so in next patch set I'd like to still put iscsi.py
> in src/kimchi.
Ok. Leave it in /src/kimchi
>>> @@ -0,0 +1,57 @@
>>> +#
>>> +# Project Kimchi
>>> +#
>>> +# Copyright IBM, Corp. 2013
>>> +#
>>> +# Authors:
>>> +# Zhou Zheng Sheng <zhshzhou at 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-1301USA
>>> +
>>> +import subprocess
>>> +
>> 2 lines
>>
> Get it. Thanks!
>>> +from kimchi.exception import OperationFailed
>>> +
>>> +
>>> +class IscsiAdmin(object):
>>> + def __init__(self, host, port=None):
>>> + self.portal = host + ("" if port is None else ":%s" % port)
>>> +
>>> + def _run_op(self, mode, target, op):
>>> + iscsiadm = subprocess.Popen(
>>> + ['iscsiadm', '--mode', mode, '--targetname', target,
>>> + '--portal', self.portal, '--' + op],
>>> + stdout=subprocess.PIPE, stderr=subprocess.PIPE)
>>> + out, err = iscsiadm.communicate()
>>> + if iscsiadm.returncode != 0:
>>> + raise OperationFailed('Error executing iscsiadm: %s' % err)
>>> + return out
>>> +
>>> + def login(self, target):
>>> + self._run_op('node', target, 'login')
>>> +
>>> + def logout(self, target):
>>> + self._run_op('node', target, 'logout')
>>> +
>>> +
>>> +def validate_iscsi_target(target, host, port=None):
>>> + adm = IscsiAdmin(host, port)
>>> + try:
>>> + adm.login(target)
>>> + except OperationFailed:
>>> + return False
>>> +
>>> + adm.logout(target)
>>> + return True
>> Let's do a python module with classes or functions - never both
>> That way we can easily identify modules which implements instances types
>> or use functions
>>
>> The function validate_iscsi_target can be into /tests/utils.py module.
>>
> I do not agree with you. Code should be divided according to the things
> they do, not according to they are classes or functions. All iscsi
> related things should be put in iscsi.py, regardless of classes or
> functions. In Python standard library, there are many examples that a
> module contains both classes and functions, as long as they serve
> related purpose. Putting all functions in utils.py is a bad idea. You
> get a big module with a lot of unrelated functions, and since each
> function do different things, you have to import a lot of unrelated
> modules in utils.py. This is an anti-pattern of modulization. Ideally,
> most classes and functions are divided into modules according to their
> logical/functional semantics. Only those small helper functions that can
> not fit into any module can be put in utils.py. Having a bit utils.py is
> a sign of bad engineered project.
So if you want to put all related to iscsi in iscsi.py so move the
function inside the class.
But let's do a module with class or functions.
>>> diff --git a/tests/test_model.py b/tests/test_model.py
>>> index fb7d6dd..82fa57d 100644
>>> --- a/tests/test_model.py
>>> +++ b/tests/test_model.py
>>> @@ -34,6 +34,7 @@ import kimchi.model
>>> import kimchi.objectstore
>>> from kimchi.exception import *
>>> from kimchi import netinfo
>>> +from kimchi.iscsi import validate_iscsi_target
>>> import utils
>>> import iso_gen
>>>
>>> @@ -112,48 +113,60 @@ class ModelTests(unittest.TestCase):
>>> def test_storagepool(self):
>>> inst = kimchi.model.Model('qemu:///system', self.tmp_store)
>>>
>>> - with utils.RollbackContext() as rollback:
>>> - path = '/tmp/kimchi-images'
>>> - name = 'test-pool'
>>> - if not os.path.exists(path):
>>> - os.mkdir(path)
>>> + poolDefs = [
>>> + {'type': 'dir',
>>> + 'name': 'unitTestDirPool',
>>> + 'path': '/tmp/kimchi-images'},
>>> + {'type': 'iscsi',
>>> + 'name': 'unitTestISCSIPool',
>>> + 'source': {'host': '127.0.0.1',
>>> + 'target':
>>> 'iqn.2013-12.localhost.kimchiUnitTest'}}]
>>>
>>> - pools = inst.storagepools_get_list()
>>> - num = len(pools) + 1
>>> + for poolDef in poolDefs:
>>> + with utils.RollbackContext() as rollback:
>>> + path = poolDef.get('path')
>>> + name = poolDef['name']
>>> + if not (path is None or os.path.exists(path)):
>>> + os.mkdir(path)
>> The backend should be able to create the path if it doesn't exist by
>> pool.build()
>>
> OK. Thanks!
>>> +
>>> + if poolDef['type'] == 'iscsi':
>>> + if not validate_iscsi_target(**poolDef['source']):
>>> + continue
>>> +
>>> + pools = inst.storagepools_get_list()
>>> + num = len(pools) + 1
>>> +
>>> + inst.storagepools_create(poolDef)
>>> + rollback.prependDefer(inst.storagepool_delete, name)
>>> +
>>> + pools = inst.storagepools_get_list()
>>> + self.assertEquals(num, len(pools))
>>> +
>>> + poolinfo = inst.storagepool_lookup(name)
>>> + if path is not None:
>>> + self.assertEquals(path, poolinfo['path'])
>>> + self.assertEquals('inactive', poolinfo['state'])
>>> + if poolinfo['type'] == 'dir':
>>> + self.assertEquals(True, poolinfo['autostart'])
>>> + else:
>>> + self.assertEquals(False, poolinfo['autostart'])
>>> +
>>> + inst.storagepool_activate(name)
>>> + rollback.prependDefer(inst.storagepool_deactivate, name)
>>>
>>> - args = {'name': name,
>>> - 'path': path,
>>> - 'type': 'dir'}
>>> - inst.storagepools_create(args)
>>> - rollback.prependDefer(inst.storagepool_delete, name)
>>> -
>>> - pools = inst.storagepools_get_list()
>>> - self.assertEquals(num, len(pools))
>>> -
>>> - poolinfo = inst.storagepool_lookup(name)
>>> - self.assertEquals(path, poolinfo['path'])
>>> - self.assertEquals('inactive', poolinfo['state'])
>>> - if poolinfo['type'] == 'dir':
>>> - self.assertEquals(True, poolinfo['autostart'])
>>> - else:
>>> - self.assertEquals(False, poolinfo['autostart'])
>>> -
>>> - inst.storagepool_activate(name)
>>> - rollback.prependDefer(inst.storagepool_deactivate, name)
>>> -
>>> - poolinfo = inst.storagepool_lookup(name)
>>> - self.assertEquals('active', poolinfo['state'])
>>> -
>>> - autostart = poolinfo['autostart']
>>> - ori_params = {'autostart': True} if autostart else
>>> {'autostart': False}
>>> - for i in [True, False]:
>>> - params = {'autostart': i}
>>> - inst.storagepool_update(name, params)
>>> - rollback.prependDefer(inst.storagepool_update, name,
>>> - ori_params)
>>> poolinfo = inst.storagepool_lookup(name)
>>> - self.assertEquals(i, poolinfo['autostart'])
>>> - inst.storagepool_update(name, ori_params)
>>> + self.assertEquals('active', poolinfo['state'])
>>> +
>>> + autostart = poolinfo['autostart']
>>> + ori_params = {'autostart': True} if autostart else
>>> {'autostart': False}
>>> + for i in [True, False]:
>>> + params = {'autostart': i}
>>> + inst.storagepool_update(name, params)
>>> + rollback.prependDefer(inst.storagepool_update, name,
>>> + ori_params)
>>> + poolinfo = inst.storagepool_lookup(name)
>>> + self.assertEquals(i, poolinfo['autostart'])
>>> + inst.storagepool_update(name, ori_params)
>>>
>>> pools = inst.storagepools_get_list()
>>> self.assertIn('default', pools)
>
More information about the Kimchi-devel
mailing list