mgr / volumes: improve error handling

This cleans up lots of return-fu statements and make the source
look much more pythonic. Functions should raise an instance of
VolumeException() class wherever necessary (error handling).

Fixes: http://tracker.ceph.com/issues/39969
Signed-off-by: Venky Shankar <vshankar@redhat.com>
(cherry picked from commit 241c9cea7c1e489461c17fc43eb77ba28a4fd2c0)

diff --git a/src/pybind/mgr/volumes/fs/exception.py b/src/pybind/mgr/volumes/fs/exception.py
new file mode 100644 (file)
index 0000000..a9e8818
--- /dev/null
+++ b/src/pybind/mgr/volumes/fs/exception.py
@@ -0,0 +1,10 @@
+class VolumeException(Exception):
+    def __init__(self, error_code, error_message):
+        self.errno = error_code
+        self.error_str = error_message
+
+    def to_tuple(self):
+        return self.errno, "", self.error_str
+
+    def __str__(self):
+        return "{0} ({1})".format(self.errno, self.error_str)

diff --git a/src/pybind/mgr/volumes/fs/subvolspec.py b/src/pybind/mgr/volumes/fs/subvolspec.py
index 63e5094ce8333f8c6c79ba49215096243617af20..6efb24ca8679fb1159f6845fc6636dc074086132 100644 (file)
--- a/src/pybind/mgr/volumes/fs/subvolspec.py
+++ b/src/pybind/mgr/volumes/fs/subvolspec.py
@@ -29,6 +29,20 @@ class SubvolumeSpec(object):
         """
         return self.groupid == SubvolumeSpec.NO_GROUP_NAME
 
+    @property
+    def subvolume_id(self):
+        """
+        Return the subvolume-id from the subvolume specification
+        """
+        return self.subvolumeid
+
+    @property
+    def group_id(self):
+        """
+        Return the group-id from the subvolume secification
+        """
+        return self.groupid
+
     @property
     def subvolume_path(self):
         """

diff --git a/src/pybind/mgr/volumes/fs/subvolume.py b/src/pybind/mgr/volumes/fs/subvolume.py
index 01767cf331ae5b70d19ef17970bd23291c4aaa0e..6849278ff97850dafd63e4513e5801e90ad3b208 100644 (file)
--- a/src/pybind/mgr/volumes/fs/subvolume.py
+++ b/src/pybind/mgr/volumes/fs/subvolume.py
@@ -6,10 +6,12 @@ LGPL2.1.  See file COPYING.
 
 import logging
 import os
+import errno
 
 import cephfs
 
 from .subvolspec import SubvolumeSpec
+from .exception import VolumeException
 
 log = logging.getLogger(__name__)
 
@@ -54,6 +56,8 @@ class SubVolume(object):
                 self.fs.stat(subpath)
             except cephfs.ObjectNotFound:
                 self.fs.mkdir(subpath, mode)
+            except cephfs.Error as e:
+                raise VolumeException(e.args[0], e.args[1])
 
     ### basic subvolume operations
 
@@ -91,13 +95,14 @@ class SubVolume(object):
         # TODO: handle error...
         self.fs.setxattr(subvolpath, xattr_key, xattr_val.encode('utf-8'), 0)
 
-    def remove_subvolume(self, spec):
+    def remove_subvolume(self, spec, force):
         """
         Make a subvolume inaccessible to guests.  This function is idempotent.
         This is the fast part of tearing down a subvolume: you must also later
         call purge_subvolume, which is the slow part.
 
         :param spec: subvolume path specification
+        :param force: flag to ignore non-existent path (never raise exception)
         :return: None
         """
 
@@ -109,7 +114,14 @@ class SubVolume(object):
         self._mkdir_p(trashdir)
 
         trashpath = spec.trash_path
-        self.fs.rename(subvolpath, trashpath)
+        try:
+            self.fs.rename(subvolpath, trashpath)
+        except cephfs.ObjectNotFound:
+            if not force:
+                raise VolumeException(
+                    -errno.ENOENT, "Subvolume '{0}' not found, cannot remove it".format(spec.subvolume_id))
+        except cephfs.Error as e:
+            raise VolumeException(e.args[0], e.args[1])
 
     def purge_subvolume(self, spec):
         """
@@ -123,6 +135,8 @@ class SubVolume(object):
                 dir_handle = self.fs.opendir(root_path)
             except cephfs.ObjectNotFound:
                 return
+            except cephfs.Error as e:
+                raise VolumeException(e.args[0], e.args[1])
             d = self.fs.readdir(dir_handle)
             while d:
                 d_name = d.d_name.decode('utf-8')
@@ -149,6 +163,8 @@ class SubVolume(object):
             self.fs.stat(path)
         except cephfs.ObjectNotFound:
             return None
+        except cephfs.Error as e:
+            raise VolumeException(e.args[0]. e.args[1])
         return path
 
     ### group operations
@@ -157,9 +173,15 @@ class SubVolume(object):
         path = spec.group_path
         self._mkdir_p(path, mode)
 
-    def remove_group(self, spec):
+    def remove_group(self, spec, force):
         path = spec.group_path
-        self.fs.rmdir(path)
+        try:
+            self.fs.rmdir(path)
+        except cephfs.ObjectNotFound:
+            if not force:
+                raise VolumeException(-errno.ENOENT, "Subvolume group '{0}' not found".format(spec.group_id))
+        except cephfs.Error as e:
+            raise VolumeException(e.args[0], e.args[1])
 
     def get_group_path(self, spec):
         path = spec.group_path
@@ -197,31 +219,39 @@ class SubVolume(object):
             self.fs.stat(snappath)
         except cephfs.ObjectNotFound:
             self.fs.mkdir(snappath, mode)
+        except cephfs.Error as e:
+            raise VolumeException(e.args[0], e.args[1])
         else:
             log.warn("Snapshot '{0}' already exists".format(snappath))
 
-    def _snapshot_delete(self, snappath):
+    def _snapshot_delete(self, snappath, force):
         """
         Remove a snapshot, or do nothing if it doesn't exist.
         """
-        self.fs.stat(snappath)
-        self.fs.rmdir(snappath)
+        try:
+            self.fs.stat(snappath)
+            self.fs.rmdir(snappath)
+        except cephfs.ObjectNotFound:
+            if not force:
+                raise VolumeException(-errno.ENOENT, "Snapshot '{0}' not found, cannot remove it".format(snappath))
+        except cephfs.Error as e:
+            raise VolumeException(e.args[0], e.args[1])
 
     def create_subvolume_snapshot(self, spec, snapname, mode=0o755):
         snappath = spec.make_subvol_snap_path(self.rados.conf_get('client_snapdir'), snapname)
         self._snapshot_create(snappath, mode)
 
-    def remove_subvolume_snapshot(self, spec, snapname):
+    def remove_subvolume_snapshot(self, spec, snapname, force):
         snappath = spec.make_subvol_snap_path(self.rados.conf_get('client_snapdir'), snapname)
-        self._snapshot_delete(snappath)
+        self._snapshot_delete(snappath, force)
 
     def create_group_snapshot(self, spec, snapname, mode=0o755):
         snappath = spec.make_group_snap_path(self.rados.conf_get('client_snapdir'), snapname)
         self._snapshot_create(snappath, mode)
 
-    def remove_group_snapshot(self, spec, snapname):
+    def remove_group_snapshot(self, spec, snapname, force):
         snappath = spec.make_group_snap_path(self.rados.conf_get('client_snapdir'), snapname)
-        return self._snapshot_delete(snappath)
+        return self._snapshot_delete(snappath, force)
 
     ### context manager routines
 

diff --git a/src/pybind/mgr/volumes/fs/volume.py b/src/pybind/mgr/volumes/fs/volume.py
index a2f4672b52d78b759c295daed3107e376d857d01..7b936b71b2e400a30cc1c17ca198f48b4468d937 100644 (file)
--- a/src/pybind/mgr/volumes/fs/volume.py
+++ b/src/pybind/mgr/volumes/fs/volume.py
@@ -7,6 +7,7 @@ import orchestrator
 
 from .subvolspec import SubvolumeSpec
 from .subvolume import SubVolume
+from .exception import VolumeException
 
 log = logging.getLogger(__name__)
 
@@ -36,6 +37,12 @@ class VolumeClient(object):
     def volume_exists(self, volname):
         return self.get_fs(volname) is not None
 
+    def volume_exception_to_retval(self, ve):
+        """
+        return a tuple representation from a volume exception
+        """
+        return ve.to_tuple()
+
     def create_pool(self, pool_name, pg_num, pg_num_min=None, pg_autoscale_factor=None):
         # create the given pool
         command = {'prefix': 'osd pool create', 'pool': pool_name, 'pg_num': pg_num}
@@ -165,183 +172,187 @@ class VolumeClient(object):
     ### subvolume operations
 
     def create_subvolume(self, volname, subvolname, groupname, size):
-        if not self.volume_exists(volname):
-            return -errno.ENOENT, "", \
-                "Volume '{0}' not found, create it with `ceph fs volume create` " \
-                "before trying to create subvolumes".format(volname)
-
-        # TODO: validate that subvol size fits in volume size
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec(subvolname, groupname)
-            if not self.group_exists(sv, spec):
-                return -errno.ENOENT, "", \
-                    "Subvolume group '{0}' not found, create it with `ceph fs subvolumegroup create` " \
-                    "before creating subvolumes".format(groupname)
-            return sv.create_subvolume(spec, size)
+        ret = 0, "", ""
+        try:
+            if not self.volume_exists(volname):
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, create it with `ceph fs " \
+                    "volume create` before trying to create subvolumes".format(volname))
+            with SubVolume(self.mgr, fs_name=volname) as sv:
+                spec = SubvolumeSpec(subvolname, groupname)
+                if not self.group_exists(sv, spec):
+                    raise VolumeException(
+                        -errno.ENOENT, "Subvolume group '{0}' not found, create it with " \
+                        "`ceph fs subvolumegroup create` before creating subvolumes".format(groupname))
+                sv.create_subvolume(spec, size)
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     def remove_subvolume(self, volname, subvolname, groupname, force):
-        fs = self.get_fs(volname)
-        if not fs:
-            if force:
-                return 0, "", ""
-            else:
-                return -errno.ENOENT, "", \
-                    "Volume '{0}' not found, cannot remove subvolume '{1}'".format(volname, subvolname)
-
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec(subvolname, groupname)
-            if not self.group_exists(sv, spec):
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume group '{0}' not found, cannot remove subvolume '{1}'".format(groupname, subvolname)
-            try:
-                sv.remove_subvolume(spec)
-            except cephfs.ObjectNotFound:
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume '{0}' not found, cannot remove it".format(subvolname)
-            sv.purge_subvolume(spec)
-            return 0, "", ""
+        ret = 0, "", ""
+        try:
+            fs = self.get_fs(volname)
+            if fs:
+                with SubVolume(self.mgr, fs_name=volname) as sv:
+                    spec = SubvolumeSpec(subvolname, groupname)
+                    if self.group_exists(sv, spec):
+                        sv.remove_subvolume(spec, force)
+                        sv.purge_subvolume(spec)
+                    elif not force:
+                        raise VolumeException(
+                            -errno.ENOENT, "Subvolume group '{0}' not found, cannot remove " \
+                            "subvolume '{1}'".format(groupname, subvolname))
+            elif not force:
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, cannot remove subvolume " \
+                    "'{1}'".format(volname, subvolname))
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     def subvolume_getpath(self, volname, subvolname, groupname):
-        if not self.volume_exists(volname):
-            return -errno.ENOENT, "", "Volume '{0}' not found".format(volname)
-
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec(subvolname, groupname)
-            if not self.group_exists(sv, spec):
-                return -errno.ENOENT, "", "Subvolume group '{0}' not found".format(groupname)
-            path = sv.get_subvolume_path(spec)
-            if not path:
-                return -errno.ENOENT, "", "Subvolume '{0}' not found".format(groupname)
-            return 0, path, ""
+        ret = None
+        try:
+            if not self.volume_exists(volname):
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found".format(volname))
+
+            with SubVolume(self.mgr, fs_name=volname) as sv:
+                spec = SubvolumeSpec(subvolname, groupname)
+                if not self.group_exists(sv, spec):
+                    raise VolumeException(
+                        -errno.ENOENT, "Subvolume group '{0}' not found".format(groupname))
+                path = sv.get_subvolume_path(spec)
+                if not path:
+                    raise VolumeException(
+                        -errno.ENOENT, "Subvolume '{0}' not found".format(subvolname))
+                ret = 0, path, ""
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     ### subvolume snapshot
 
     def create_subvolume_snapshot(self, volname, subvolname, snapname, groupname):
-        if not self.volume_exists(volname):
-            return -errno.ENOENT, "", \
-                   "Volume '{0}' not found, cannot create snapshot '{1}'".format(volname, snapname)
-
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec(subvolname, groupname)
-            if not self.group_exists(sv, spec):
-                return -errno.ENOENT, "", \
-                    "Subvolume group '{0}' not found, cannot create snapshot '{1}'".format(groupname, snapname)
-            if not sv.get_subvolume_path(spec):
-                return -errno.ENOENT, "", \
-                       "Subvolume '{0}' not found, cannot create snapshot '{1}'".format(subvolname, snapname)
-            sv.create_subvolume_snapshot(spec, snapname)
-        return 0, "", ""
+        ret = 0, "", ""
+        try:
+            if not self.volume_exists(volname):
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, cannot create snapshot " \
+                    "'{1}'".format(volname, snapname))
+
+            with SubVolume(self.mgr, fs_name=volname) as sv:
+                spec = SubvolumeSpec(subvolname, groupname)
+                if not self.group_exists(sv, spec):
+                    raise VolumeException(
+                        -errno.ENOENT, "Subvolume group '{0}' not found, cannot create " \
+                        "snapshot '{1}'".format(groupname, snapname))
+                if not sv.get_subvolume_path(spec):
+                    raise VolumeException(
+                        -errno.ENOENT, "Subvolume '{0}' not found, cannot create snapshot " \
+                        "'{1}'".format(subvolname, snapname))
+                sv.create_subvolume_snapshot(spec, snapname)
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     def remove_subvolume_snapshot(self, volname, subvolname, snapname, groupname, force):
-        if not self.volume_exists(volname):
-            if force:
-                return 0, "", ""
-            else:
-                return -errno.ENOENT, "", \
-                    "Volume '{0}' not found, cannot remove subvolumegroup snapshot '{1}'".format(volname, snapname)
-
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec(subvolname, groupname)
-            if not self.group_exists(sv, spec):
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume group '{0}' already removed, cannot remove subvolume snapshot '{1}'".format(groupname, snapname)
-            if not sv.get_subvolume_path(spec):
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume '{0}' not found, cannot remove subvolume snapshot '{1}'".format(subvolname, snapname)
-            try:
-                sv.remove_subvolume_snapshot(spec, snapname)
-            except cephfs.ObjectNotFound:
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume snapshot '{0}' not found, cannot remove it".format(snapname)
-        return 0, "", ""
+        ret = 0, "", ""
+        try:
+            if self.volume_exists(volname):
+                with SubVolume(self.mgr, fs_name=volname) as sv:
+                    spec = SubvolumeSpec(subvolname, groupname)
+                    if self.group_exists(sv, spec):
+                        if sv.get_subvolume_path(spec):
+                            sv.remove_subvolume_snapshot(spec, snapname, force)
+                        elif not force:
+                            raise VolumeException(
+                                -errno.ENOENT, "Subvolume '{0}' not found, cannot remove " \
+                                "subvolume snapshot '{1}'".format(subvolname, snapname))
+                    elif not force:
+                        raise VolumeException(
+                            -errno.ENOENT, "Subvolume group '{0}' already removed, cannot " \
+                            "remove subvolume snapshot '{1}'".format(groupname, snapname))
+            elif not force:
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, cannot remove subvolumegroup " \
+                    "snapshot '{1}'".format(volname, snapname))
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     ### group operations
 
     def create_subvolume_group(self, volname, groupname):
-        if not self.volume_exists(volname):
-            return -errno.ENOENT, "", \
-                   "Volume '{0}' not found, create it with `ceph fs volume create` " \
-                   "before trying to create subvolume groups".format(volname)
-
-        # TODO: validate that subvol size fits in volume size
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec("", groupname)
-            sv.create_group(spec)
-        return 0, "", ""
+        ret = 0, "", ""
+        try:
+            if not self.volume_exists(volname):
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, create it with `ceph fs " \
+                    "volume create` before trying to create subvolume groups".format(volname))
+
+            # TODO: validate that subvol size fits in volume size
+            with SubVolume(self.mgr, fs_name=volname) as sv:
+                spec = SubvolumeSpec("", groupname)
+                sv.create_group(spec)
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     def remove_subvolume_group(self, volname, groupname, force):
-        if not self.volume_exists(volname):
-            if force:
-                return 0, "", ""
-            else:
-                return -errno.ENOENT, "", \
-                    "Volume '{0}' not found, cannot remove subvolume group '{0}'".format(volname, groupname)
-
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            # TODO: check whether there are no subvolumes in the group
-            spec = SubvolumeSpec("", groupname)
-            try:
-                sv.remove_group(spec)
-            except cephfs.ObjectNotFound:
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume group '{0}' not found".format(groupname)
-        return 0, "", ""
+        ret = 0, "", ""
+        try:
+            if self.volume_exists(volname):
+                with SubVolume(self.mgr, fs_name=volname) as sv:
+                    # TODO: check whether there are no subvolumes in the group
+                    spec = SubvolumeSpec("", groupname)
+                    sv.remove_group(spec, force)
+            elif not force:
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, cannot remove subvolume " \
+                    "group '{0}'".format(volname, groupname))
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     ### group snapshot
 
     def create_subvolume_group_snapshot(self, volname, groupname, snapname):
-        if not self.volume_exists(volname):
-            return -errno.ENOENT, "", \
-                   "Volume '{0}' not found, cannot create snapshot '{1}'".format(volname, snapname)
-
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec("", groupname)
-            if not self.group_exists(sv, spec):
-                return -errno.ENOENT, "", \
-                    "Subvolume group '{0}' not found, cannot create snapshot '{1}'".format(groupname, snapname)
-            sv.create_group_snapshot(spec, snapname)
-        return 0, "", ""
+        ret = 0, "", ""
+        try:
+            if not self.volume_exists(volname):
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, cannot create snapshot " \
+                    "'{1}'".format(volname, snapname))
+
+            with SubVolume(self.mgr, fs_name=volname) as sv:
+                spec = SubvolumeSpec("", groupname)
+                if not self.group_exists(sv, spec):
+                    raise VolumeException(
+                        -errno.ENOENT, "Subvolume group '{0}' not found, cannot create " \
+                        "snapshot '{1}'".format(groupname, snapname))
+                sv.create_group_snapshot(spec, snapname)
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret
 
     def remove_subvolume_group_snapshot(self, volname, groupname, snapname, force):
-        if not self.volume_exists(volname):
-            if force:
-                return 0, "", ""
-            else:
-                return -errno.ENOENT, "", \
-                    "Volume '{0}' not found, cannot remove subvolumegroup snapshot '{1}'".format(volname, snapname)
-
-        with SubVolume(self.mgr, fs_name=volname) as sv:
-            spec = SubvolumeSpec("", groupname)
-            if not self.group_exists(sv, spec):
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume group '{0}' not found, cannot remove it".format(groupname)
-            try:
-                sv.remove_group_snapshot(spec, snapname)
-            except:
-                if force:
-                    return 0, "", ""
-                else:
-                    return -errno.ENOENT, "", \
-                        "Subvolume group snapshot '{0}' not found, cannot remove it".format(snapname)
-        return 0, "", ""
+        ret = 0, "", ""
+        try:
+            if self.volume_exists(volname):
+                with SubVolume(self.mgr, fs_name=volname) as sv:
+                    spec = SubvolumeSpec("", groupname)
+                    if self.group_exists(sv, spec):
+                        sv.remove_group_snapshot(spec, snapname, force)
+                    elif not force:
+                        raise VolumeException(
+                            -errno.ENOENT, "Subvolume group '{0}' not found, cannot " \
+                            "remove it".format(groupname))
+            elif not force:
+                raise VolumeException(
+                    -errno.ENOENT, "Volume '{0}' not found, cannot remove subvolumegroup " \
+                    "snapshot '{1}'".format(volname, snapname))
+        except VolumeException as ve:
+            ret = self.volume_exception_to_retval(ve)
+        return ret