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

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(a)linux.vnet.ibm.com&gt; > --- > 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