Re: [Kimchi-devel] [PATCH v2 2/5] storagepool: rename and consolidate arguments of creating (back end)

于 2013年12月26日 23:17, Aline Manera 写道: > On 12/22/2013 08:55 AM, Zhou Zheng Sheng wrote: >> 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. This patch moves storage source >> related arguments under "source" key, while "source" itself is a >> dictionary. >> >> nfsserver -> source.host >> This is because in future patches, iSCSI pool can use this host info as >> well. Other network backed storage pool can also make use of this >> argument. >> >> nfspath -> source.path >> This is because other netfs pool can also make use of this argument. >> >> devices -> source.devices >> To differentiate source arguments from the target arguments, we move >> this argument under "source" key. >> >> The patch also adds unit test to check the generated XML is correct. >> >> Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; >> --- >> Makefile.am | 1 + >> docs/API.md | 20 ++++++----- >> src/kimchi/model.py | 45 ++++++++++++----------- >> tests/Makefile.am | 1 + >> tests/test_storagepool.py | 92 >> +++++++++++++++++++++++++++++++++++++++++++++++ >> 5 files changed, 130 insertions(+), 29 deletions(-) >> create mode 100644 tests/test_storagepool.py >> >> diff --git a/Makefile.am b/Makefile.am >> index e57d3b6..cbdc128 100644 >> --- a/Makefile.am >> +++ b/Makefile.am >> @@ -46,6 +46,7 @@ PEP8_WHITELIST = \ >> plugins/sample/__init__.py \ >> plugins/sample/model.py \ >> tests/test_plugin.py \ >> + tests/test_storagepool.py \ >> $(NULL) >> >> check-local: >> diff --git a/docs/API.md b/docs/API.md >> index 9edc551..d5c8ff6 100644 >> --- a/docs/API.md >> +++ b/docs/API.md >> @@ -180,15 +180,19 @@ 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'. >> + * source: Dictionary containing source information of the pool. >> + * host: IP or hostname of server for a pool backed from a >> remote host. >> + Pool types: 'nfs'. > It should be 'netfs', ritgh? > >> + * path: Export path on NFS server for NFS pool. >> + Pool types: 'nfs'. > It should be 'netfs', ritgh? > Yes. My mistake. I'll correct it. Thanks very much! >> + * devices: 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 0a497b5..a2fe76c 100644 >> --- a/src/kimchi/model.py >> +++ b/src/kimchi/model.py >> @@ -1437,13 +1437,13 @@ def _get_dir_storagepool_xml(poolArgs): >> # type: >> # path: >> xml = """ >> - <pool type='%(type)s'> >> - <name>%(name)s</name> >> + <pool type='{type}'> >> + <name>{name}</name> >> <target> >> - <path>%(path)s</path> >> + <path>{path}</path> >> </target> >> </pool> >> - """ % poolArgs >> + """.format(**poolArgs) >> return xml >> >> >> @@ -1451,24 +1451,24 @@ def _get_netfs_storagepool_xml(poolArgs): >> # Required parameters >> # name: >> # type: >> - # nfsserver: >> - # nfspath: >> + # source[host]: >> + # source[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> >> + <pool type='{type}'> >> + <name>{name}</name> >> <source> >> - <host name='%(nfsserver)s'/> >> - <dir path='%(nfspath)s'/> >> + <host name='{source[host]}'/> >> + <dir path='{source[path]}'/> >> </source> >> <target> >> - <path>%(path)s</path> >> + <path>{path}</path> >> </target> >> </pool> >> - """ % poolArgs >> + """.format(**poolArgs) >> return xml >> >> >> @@ -1476,32 +1476,35 @@ def _get_logical_storagepool_xml(poolArgs): >> # Required parameters >> # name: >> # type: >> - # devices: >> + # source[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']: >> + for device_path in poolArgs['source']['devices']: >> devices.append('<device path="%s" />' % device_path) >> >> - poolArgs.update({'devices': ''.join(devices), >> - 'path': path}) >> + poolArgs['source']['devices'] = ''.join(devices) >> + poolArgs['path'] = path >> + >> xml = """ >> - <pool type='%(type)s'> >> - <name>%(name)s</name> >> + <pool type='{type}'> >> + <name>{name}</name> >> <source> >> - %(devices)s >> + {source[devices]} >> </source> >> <target> >> - <path>%(path)s</path> >> + <path>{path}</path> >> </target> >> </pool> >> - """ % poolArgs >> + """.format(**poolArgs) >> return xml >> >> >> def _get_pool_xml(**kwargs): >> + # REMIND: When add new pool type, also add the related test in >> + # tests/test_storagepool.py >> getPoolXml = { >> 'dir': _get_dir_storagepool_xml, >> 'netfs': _get_netfs_storagepool_xml, >> diff --git a/tests/Makefile.am b/tests/Makefile.am >> index 5fb00c0..55d3ab4 100644 >> --- a/tests/Makefile.am >> +++ b/tests/Makefile.am >> @@ -32,6 +32,7 @@ EXTRA_DIST = \ >> test_plugin.py \ >> test_rest.py \ >> test_server.py \ >> + test_storagepool.py \ >> test_vmtemplate.py \ >> utils.py \ >> $(NULL) >> diff --git a/tests/test_storagepool.py b/tests/test_storagepool.py >> new file mode 100644 >> index 0000000..5d5a7cd >> --- /dev/null >> +++ b/tests/test_storagepool.py >> @@ -0,0 +1,92 @@ >> +# >> +# 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-1301 USA >> + >> +import unittest >> + > This blank line isn't needed > Also organize it in alphabetic order > Thanks. I think it needs a blank line. According to PEP 8[1], it says, """ Imports should be grouped in the following order: standard library imports related third party imports local application/library specific imports You should put a blank line between each group of imports. """. unittest is in standard library, and libxml2 is third party library, so libxml2 comes after unittest and a blank line is needed. Agree? [1] http://www.python.org/dev/peps/pep-0008/#imports >> +import libxml2 >> + > 2 lines here > According to PEP 8 mentioned above, one blank line between import sections is enough. Is it a kimchi customized rule to organize imports in such a way that mix standard and third party imports in alphabetic order, and need two blank lines between standard library and local imports?

If this is the rule I'll follow it, but in my humble opinion, the rule in PEP 8 seems more reasonable. >> +import kimchi.model >> +import utils >> + >> + >> +class stroagepoolTests(unittest.TestCase): > typo: stroagepoolTests > Ahh! Thanks! I should have be more careful. >> + def test_get_storagepool_xml(self): >> + poolDefs = [ >> + {'def': >> + {'type': 'dir', >> + 'name': 'unitTestDirPool', >> + 'path': '/var/temp/images'}, >> + 'xml': >> + """ >> + <pool type='dir'> >> + <name>unitTestDirPool</name> >> + <target> >> + <path>/var/temp/images</path> >> + </target> >> + </pool> >> + """}, >> + {'def': >> + {'type': 'netfs', >> + 'name': 'unitTestNFSPool', >> + 'source': {'host': '127.0.0.1', >> + 'path': '/var/export'}}, >> + 'xml': >> + """ >> + <pool type='netfs'> >> + <name>unitTestNFSPool</name> >> + <source> >> + <host name='127.0.0.1'/> >> + <dir path='/var/export'/> >> + </source> >> + <target> >> + <path>/var/lib/kimchi/nfs_mount/unitTestNFSPool</path> >> + </target> >> + </pool> >> + """}, >> + {'def': >> + {'type': 'logical', >> + 'name': 'unitTestLogicalPool', >> + 'source': {'devices': ['/dev/hda', '/dev/hdb']}}, >> + 'xml': >> + """ >> + <pool type='logical'> >> + <name>unitTestLogicalPool</name> >> + <source> >> + <device path="/dev/hda" /> >> + <device path="/dev/hdb" /> >> + </source> >> + <target> >> + >> <path>/var/lib/kimchi/logical_mount/unitTestLogicalPool</path> >> + </target> >> + </pool> >> + """}] >> + >> + for poolDef in poolDefs: >> + xmlstr = kimchi.model._get_pool_xml(**poolDef['def']) >> + with utils.RollbackContext() as rollback: >> + t1 = libxml2.readDoc(xmlstr, URL='', encoding='UTF-8', >> + options=libxml2.XML_PARSE_NOBLANKS) >> + rollback.prependDefer(t1.freeDoc) >> + t2 = libxml2.readDoc(poolDef['xml'], URL='', >> encoding='UTF-8', >> + options=libxml2.XML_PARSE_NOBLANKS) >> + rollback.prependDefer(t2.freeDoc) >> + self.assertEquals(t1.serialize(), t2.serialize())