Re: [Kimchi-devel] [PATCH v2 5/5] test_model: test creating iSCSI storage pool

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(a)linux.vnet.ibm.com&gt; >> --- >> 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.

>> @@ -0,0 +1,57 @@ >> +# >> +# Project Kimchi >> +# >> +# Copyright IBM, Corp. 2013 >> +# >> +# Authors: >> +# Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; >> +# >> +# 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.

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