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

[Zhou Zheng Sheng]

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'.

v4 -> v5
  Do not cache generated xml, have subclass of StoragePoolDef override
  the '.xml' property. This makes the interface cleaner.

Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
---
 src/kimchi/model.py | 122 +++++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 92 insertions(+), 30 deletions(-)

diff --git a/src/kimchi/model.py b/src/kimchi/model.py
index a6790b8..269fc97 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,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
+
+    @property
+    def xml(self):
+        ''' Subclasses have to override this method to actually generate the
+        storage pool XML definition. Should cause no side effect and be
+        idempotent'''
+        raise OperationFailed('self.xml is not implemented: %s' % self)
+
+
+class DirPoolDef(StoragePoolDef):
+    poolType = 'dir'
+
+    @property
+    def 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
+
+    @property
+    def 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 +1509,32 @@ 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']
+
+    @property
+    def 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 +1543,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
-- 
1.7.11.7