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

Reviewed-by: ShaoHe Feng <shaohef(a)linux.vnet.ibm.com&gt; On 12/31/2013 02:39 PM, 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 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