[Kimchi-devel] [PATCH v3 1/5] storagepool: refactor _get_pool_xml()
Zhou Zheng Sheng
zhshzhou at linux.vnet.ibm.com
Tue Dec 31 08:23:11 UTC 2013
Really thanks a lot! I apply most your suggestions and submitted a new
patch series. Your opinions are always very helpful and thoughtful.
on 2013/12/30 15:58, Royce Lv wrote:
> I would say I really like this re-factory! Make this quite clear.
>
> Minor suggestion below.
> On 2013年12月27日 18:20, Zhou Zheng Sheng wrote:
>> In src/kimchi/model.py, _get_pool_xml() is to generate libvirt storage
>> pool XML definition from arguments provided by the POST data (a JSON
>> dict). It has to support various types of pool such as dir, netfs and
>> logical. Now it uses "if ... elif ... elif" to check the requested type
>> of pool and go in to the respective branch and generates the correct
>> XML. It also performs some preparations such as create target directory.
>> This is OK for now but in future we'll have more types of pool, and add
>> more validation and preparation code. So it needs a little refactor
>>
>> In this patch we define StoragePoolDef object along with a factory
>> methoed named "create". The factory method instantiates DirPoolDef,
>> NetfsPoolDef or LogicalPoolDef according to the requested pool type.
>> Then we can call defObj.prepare() to check NFS mount and iSCSI
>> login/password. We can call defObj.get_xml() to generate XML for the
>> respective poolType.
>>
>> v2 -> v3
>> Call pool.build() for netfs, dir and logical pool. So do not need to
>> create dirs any longer. Since NFS and iSCSI pool need validation,
>> extract a StoragePoolDef object with "prepare" and "get_xml" methods.
>>
>> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
>> ---
>> src/kimchi/model.py | 120
>> +++++++++++++++++++++++++++++++++++++++-------------
>> 1 file changed, 90 insertions(+), 30 deletions(-)
>>
>> diff --git a/src/kimchi/model.py b/src/kimchi/model.py
>> index a6790b8..4d7f9f2 100644
>> --- a/src/kimchi/model.py
>> +++ b/src/kimchi/model.py
>> @@ -994,7 +994,9 @@ class Model(object):
>>
>> if params['type'] == 'kimchi-iso':
>> task_id = self._do_deep_scan(params)
>> - xml = _get_pool_xml(**params)
>> + poolDef = StoragePoolDef.create(params)
>> + poolDef.prepare()
>> + xml = poolDef.get_xml()
>> except KeyError, key:
>> raise MissingParameter(key)
>>
>> @@ -1009,7 +1011,7 @@ class Model(object):
>> return name
>>
>> pool = conn.storagePoolDefineXML(xml, 0)
>> - if params['type'] in ['logical', 'dir']:
>> + if params['type'] in ['logical', 'dir', 'netfs']:
>> pool.build(libvirt.VIR_STORAGE_POOL_BUILD_NEW)
>> # autostart dir and logical storage pool created
>> from kimchi
>> pool.setAutostart(1)
>> @@ -1430,28 +1432,74 @@ class LibvirtVMScreenshot(VMScreenshot):
>> finally:
>> os.close(fd)
>>
>> -def _get_pool_xml(**kwargs):
>> - # Required parameters
>> - # name:
>> - # type:
>> - # path:
>> - # nfspath:
>> - if kwargs['type'] == 'dir':
>> +
>> +class StoragePoolDef(object):
>> + @classmethod
>> + def create(cls, poolArgs):
>> + for klass in cls.__subclasses__():
>> + if poolArgs['type'] == klass.poolType:
>> + return klass(poolArgs)
>> + raise OperationFailed('Unsupported pool type: %s' %
>> poolArgs['type'])
>> +
>> + def __init__(self, poolArgs):
>> + self.poolArgs = poolArgs
>> +
>> + def prepare(self):
>> + ''' Validate pool arguments and perform preparations.
>> Operation which
>> + would cause side effect should be put here. Subclasses can
>> optionally
>> + override this method, or it always succeeds by default. '''
>> + pass
>> +
>> + def get_xml(self):
>> + ''' Generate storage pool XML string. It's an idempotent
>> operation and
>> + should not cause any side effect. '''
>> + return self._get_xml_imp(copy.deepcopy(self.poolArgs))
>> +
>> + def _get_xml_imp(self, poolArgs):
>> + ''' Subclasses have to override this method to actually
>> generate the
>> + storage pool XML definition. '''
>> + raise OperationFailed('get_xml is not implemented: %s' % self)
>> +
>> +
>> +class DirPoolDef(StoragePoolDef):
>> + poolType = 'dir'
>> +
>> + def _get_xml_imp(self, poolArgs):
> How about change _get_xml_imp to:
> @property
> def pool_xml(self):
> ....
> and delete get_xml() function, So that we can get it easier and more clear.
>> + # Required parameters
>> + # name:
>> + # type:
>> + # path:
>> xml = """
>> - <pool type='%(type)s'>
>> + <pool type='dir'>
>> <name>%(name)s</name>
>> <target>
>> <path>%(path)s</path>
>> </target>
>> </pool>
>> - """
>> - elif kwargs['type'] == 'netfs':
>> - path = '/var/lib/kimchi/nfs_mount/' + kwargs['name'];
>> - if not os.path.exists(path):
>> - os.makedirs(path)
>> - kwargs.update({'path': path})
>> + """ % poolArgs
>> + return xml
>> +
>> +
>> +class NetfsPoolDef(StoragePoolDef):
>> + poolType = 'netfs'
>> +
>> + def __init__(self, poolArgs):
>> + super(NetfsPoolDef, self).__init__(poolArgs)
>> + self.path = '/var/lib/kimchi/nfs_mount/' + self.poolArgs['name']
>> +
>> + def prepare(self):
>> + # TODO Verify the NFS export can be actually mounted.
>> + pass
>> +
>> + def _get_xml_imp(self, poolArgs):
>> + # Required parameters
>> + # name:
>> + # type:
>> + # nfsserver:
>> + # nfspath:
>> + poolArgs['path'] = self.path
>> xml = """
>> - <pool type='%(type)s'>
>> + <pool type='netfs'>
>> <name>%(name)s</name>
>> <source>
>> <host name='%(nfsserver)s'/>
>> @@ -1461,13 +1509,30 @@ def _get_pool_xml(**kwargs):
>> <path>%(path)s</path>
>> </target>
>> </pool>
>> - """
>> - elif kwargs['type'] == 'logical':
>> - path = '/var/lib/kimchi/logical_mount/' + kwargs['name'];
>> - if not os.path.exists(path):
>> - os.makedirs(path)
>> + """ % poolArgs
>> + return xml
>> +
>> +
>> +class LogicalPoolDef(StoragePoolDef):
>> + poolType = 'logical'
>> +
>> + def __init__(self, poolArgs):
>> + super(LogicalPoolDef, self).__init__(poolArgs)
>> + self.path = '/var/lib/kimchi/logical_mount/' +
>> self.poolArgs['name']
>> +
>> + def _get_xml_imp(self, poolArgs):
>> + # Required parameters
>> + # name:
>> + # type:
>> + # devices:
>> + devices = []
>> + for device_path in poolArgs['devices']:
>> + devices.append('<device path="%s" />' % device_path)
>> +
>> + poolArgs.update({'devices': ''.join(devices),
>> + 'path': self.path})
>> xml = """
>> - <pool type='%(type)s'>
>> + <pool type='logical'>
>> <name>%(name)s</name>
>> <source>
>> %(devices)s
>> @@ -1476,14 +1541,9 @@ def _get_pool_xml(**kwargs):
>> <path>%(path)s</path>
>> </target>
>> </pool>
>> - """
>> - devices = ''
>> - for device_path in kwargs['devices']:
>> - devices += "<device path=\""+device_path+"\" />"
>> + """ % poolArgs
>> + return xml
>>
>> - kwargs.update({'devices': devices})
>> - kwargs.update({'path': path})
>> - return xml % kwargs
>>
>> def _get_volume_xml(**kwargs):
>> # Required parameters
>
--
Thanks and best regards!
Zhou Zheng Sheng / 周征晟
E-mail: zhshzhou at linux.vnet.ibm.com
Telephone: 86-10-82454397
More information about the Kimchi-devel
mailing list