From 4ae678e030db8a052a64a302d92313d95783fed7 Mon Sep 17 00:00:00 2001 From: Venky Shankar Date: Sun, 19 May 2019 13:34:57 -0400 Subject: [PATCH] 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 (cherry picked from commit 241c9cea7c1e489461c17fc43eb77ba28a4fd2c0) --- src/pybind/mgr/volumes/fs/exception.py | 10 + src/pybind/mgr/volumes/fs/subvolspec.py | 14 + src/pybind/mgr/volumes/fs/subvolume.py | 52 +++- src/pybind/mgr/volumes/fs/volume.py | 327 ++++++++++++------------ 4 files changed, 234 insertions(+), 169 deletions(-) create mode 100644 src/pybind/mgr/volumes/fs/exception.py diff --git a/src/pybind/mgr/volumes/fs/exception.py b/src/pybind/mgr/volumes/fs/exception.py new file mode 100644 index 0000000000000..a9e8818f981ac --- /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 63e5094ce8333..6efb24ca8679f 100644 --- 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 01767cf331ae5..6849278ff9785 100644 --- 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 a2f4672b52d78..7b936b71b2e40 100644 --- 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 -- 2.39.5