Re: [Kimchi-devel] [PATCH v4 1/5] storagepool: refactor _get_pool_xml()

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 inspect defObj.xml to get the generated 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. v3 -> v4 Turn 'defObj.get_xml()' into a lazily calculated property 'defObj.xml'. Signed-off-by: Zhou Zheng Sheng <zhshzhou(a)linux.vnet.ibm.com&gt; --- src/kimchi/model.py | 124 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 94 insertions(+), 30 deletions(-) diff --git a/src/kimchi/model.py b/src/kimchi/model.py index a6790b8..107aa6c 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.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,77 @@ 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 + self._xml = None + + 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 + + @property + def xml(self): + if self._xml is None: + self._xml = self._get_xml() + return self._xml + + def _get_xml(self): + ''' 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(self): + # 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}) + """ % self.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(self): + # Required parameters + # name: + # type: + # nfsserver: + # nfspath: + poolArgs = copy.deepcopy(self.poolArgs) + poolArgs['path'] = self.path xml = """ - <pool type='%(type)s'> + <pool type='netfs'> <name>%(name)s</name> <source> <host name='%(nfsserver)s'/> @@ -1461,13 +1512,31 @@ 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(self): + # Required parameters + # name: + # type: + # devices: + poolArgs = copy.deepcopy(self.poolArgs) + 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 +1545,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