[Kimchi-devel] [PATCH v2 2/5] storagepool: rename and consolidate arguments of creating (back end)
Aline Manera
alinefm at linux.vnet.ibm.com
Fri Dec 27 12:32:51 UTC 2013
On 12/26/2013 11:54 PM, Zhou Zheng Sheng wrote:
> 于 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 at linux.vnet.ibm.com>
>>> ---
>>> 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 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-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?
Yes! We are using the following rule:
import ...
import ...
import ...
<2 lines>
from ... import ...
from ... import ...
<2 lines>
import kimchi...
from kimchi import ...
<2 lines>
All those blocks must be in alphabetic order
> 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())
>
More information about the Kimchi-devel
mailing list