[Kimchi-devel] [PATCH 3/3] Github bug #327: NFS pool workaround: model changes

[Aline Manera]

On 02/25/2014 01:33 PM, Daniel Barboza wrote:
> From: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
>
> Implementing a workaround in '_get_storagepool_vols_num',
> 'lookup', 'activate' and 'deactivate' methods to avoid
> libvirt calls in NFS pools when the corresponding NFS server
> is unreachable.
>
> In the 'lookup' method the state of the pool returned in
> such scenario is 'inaccessible' to warn the UI that the pool
> isn't available for normal operation.
>
> Signed-off-by: Daniel Henrique Barboza <danielhb at linux.vnet.ibm.com>
> ---
>   src/kimchi/model/storagepools.py | 75 +++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py
> index 011feb0..956ab08 100644
> --- a/src/kimchi/model/storagepools.py
> +++ b/src/kimchi/model/storagepools.py
> @@ -168,6 +168,20 @@ class StoragePoolModel(object):
>                   raise
>
>       def _get_storagepool_vols_num(self, pool):
> +        #FIXME: nfs workaround - return 0 if 'pool' is a NFS pool
> +        # and the corresponding NFS server is not reachable.
> +        xml = pool.XMLDesc(0)
> +        pool_type = xmlutils.xpath_get_text(xml, "/pool/@type")[0]
> +        if pool_type == 'netfs':
> +            source = self._get_storage_source(pool_type, xml)
> +            nfs_params = {}
> +            nfs_params['name'] = name
> +            nfs_params['type'] = pool_type
> +            nfs_params['source'] = {'path': source['path'],
> +                                    'host': source['addr']}
> +            if not self._nfs_status_online(pool, nfs_params):
> +                return 0
> +

This function is only called on lookup() in which you have already added 
the workaround.
I think we can leave it just there

>           try:
>               if pool.isActive():
>                   pool.refresh(0)
> @@ -194,15 +208,42 @@ class StoragePoolModel(object):
>
>           return source
>
> +    def _nfs_status_online(self, pool, poolArgs):
> +        conn = self.conn.get()
> +        poolDef = StoragePoolDef.create(poolArgs)
> +        try:
> +            poolDef.prepare(conn)
> +            return True
> +        except Exception as e:
> +            return False
> +
>       def lookup(self, name):
>           pool = self.get_storagepool(name, self.conn)
>           info = pool.info()
> -        nr_volumes = self._get_storagepool_vols_num(pool)
>           autostart = True if pool.autostart() else False
>           xml = pool.XMLDesc(0)
>           path = xmlutils.xpath_get_text(xml, "/pool/target/path")[0]
>           pool_type = xmlutils.xpath_get_text(xml, "/pool/@type")[0]
>           source = self._get_storage_source(pool_type, xml)
> +        #FIXME: nfs workaround - prevent any libvirt operation
> +        # for a nfs if the corresponding NFS server is down.
> +        if pool_type == 'netfs':
> +            nfs_params = {}
> +            nfs_params['name'] = name
> +            nfs_params['type'] = pool_type
> +            nfs_params['source'] = {'path': source['path'],
> +                                    'host': source['addr']}
> +            if not self._nfs_status_online(pool, nfs_params):
> +                kimchi_log.debug("NFS pool %s is offline, reason: NFS "
> +                                 "server %s is unreachable.", name,
> +                                 source['addr'])
> +                # Mark state as '4' => inaccessible.
> +                info[0] = 4
> +                # skip calculating volumes
> +                nr_volumes = 0
> +        else:
> +            nr_volumes = self._get_storagepool_vols_num(pool)
> +
>           res = {'state': POOL_STATE_MAP[info[0]],
>                  'path': path,
>                  'source': source,
> @@ -267,6 +308,22 @@ class StoragePoolModel(object):
>
>       def activate(self, name):
>           pool = self.get_storagepool(name, self.conn)
> +        #FIXME: nfs workaround - do not activate a NFS pool
> +        # if the NFS server is not reachable.

> +        xml = pool.XMLDesc(0)
> +        pool_type = xmlutils.xpath_get_text(xml, "/pool/@type")[0]
> +        if pool_type == 'netfs':
> +            source = self._get_storage_source(pool_type, xml)
> +            nfs_params = {}
> +            nfs_params['name'] = name
> +            nfs_params['type'] = pool_type
> +            nfs_params['source'] = {'path': source['path'],
> +                                    'host': source['addr']}
> +            if not self._nfs_status_online(pool, nfs_params):
> +                # block the user from activating the pool.
> +                raise OperationFailed("KCHPOOL0032E",
> +                                      {'name': name, 'server': source['addr']})

All the above coding is being used in some places.
What about create a function for it? _check_nfs_server() ?

def _check_nfs_server(self, pool):
     ....
     return True|False

> +                return
>           try:
>               pool.create(0)
>           except libvirt.libvirtError as e:
> @@ -275,6 +332,22 @@ class StoragePoolModel(object):
>
>       def deactivate(self, name):
>           pool = self.get_storagepool(name, self.conn)
> +        #FIXME: nfs workaround - do not try to deactivate a NFS pool
> +        # if the NFS server is not reachable.
> +        xml = pool.XMLDesc(0)
> +        pool_type = xmlutils.xpath_get_text(xml, "/pool/@type")[0]
> +        if pool_type == 'netfs':
> +            source = self._get_storage_source(pool_type, xml)
> +            nfs_params = {}
> +            nfs_params['name'] = name
> +            nfs_params['type'] = pool_type
> +            nfs_params['source'] = {'path': source['path'],
> +                                    'host': source['addr']}
> +            if not self._nfs_status_online(pool, nfs_params):
> +                # block the user from dactivating the pool.
> +                raise OperationFailed("KCHPOOL0033E",
> +                                      {'name': name, 'server': source['addr']})
> +                return
>           try:
>               pool.destroy()
>           except libvirt.libvirtError as e: