Merge pull request #1924 from phillxnet/1923_improve_subvol_mount_cod…

…e_robustness

improve subvol mount code robustness. Fixes #1923

src/rockstor/fs/btrfs.py

@@ -19,7 +19,6 @@
 import re
 import time
 import os
-import shutil
 from system.osi import run_command, create_tmp_dir, is_share_mounted, \
     is_mounted, get_dev_byid_name, convert_to_kib, toggle_path_rw, \
     get_device_path
@@ -423,14 +422,17 @@ def mount_share(share, mnt_pt):
     mount_root(share.pool)
     pool_device = get_device_path(share.pool.disk_set.attached()
                                        .first().target_name)
-    subvol_str = 'subvol=%s' % share.subvol_name
+    qgroup = share.qgroup
+    # share.qgroup = "0/subvolid" use for subvol reference as more
+    # flexible than "subvol=share.subvol_name" (prior method).
+    subvol_str = 'subvolid={}'.format(qgroup[2:])
     create_tmp_dir(mnt_pt)
     toggle_path_rw(mnt_pt, rw=False)
     mnt_cmd = [MOUNT, '-t', 'btrfs', '-o', subvol_str, pool_device, mnt_pt]
     return run_command(mnt_cmd)
 
 
-def mount_snap(share, snap_name, snap_mnt=None):
+def mount_snap(share, snap_name, snap_qgroup, snap_mnt=None):
     pool_device = get_device_path(share.pool.disk_set.attached()
                                        .first().target_name)
     share_path = ('%s%s' % (DEFAULT_MNT_DIR, share.name))
@@ -444,8 +446,10 @@ def mount_snap(share, snap_name, snap_mnt=None):
     mount_share(share, share_path)
     if (is_subvol(snap_path)):
         create_tmp_dir(snap_mnt)
-        return run_command([MOUNT, '-o', 'subvol=%s' % rel_snap_path,
-                            pool_device, snap_mnt])
+        # snap_qgroup = "0/subvolid" use for subvol reference as more
+        # flexible than "subvol=rel_snap_path" (prior method).
+        subvol_str = 'subvolid={}'.format(snap_qgroup[2:])
+        return run_command([MOUNT, '-o', subvol_str, pool_device, snap_mnt])
 
 
 def subvol_list_helper(mnt_pt):
@@ -598,12 +602,14 @@ def snaps_info(mnt_pt, share_name):
 
 def share_id(pool, share_name):
     """
-    Returns the subvolume id, becomes the share's uuid.
+    Returns the subvolume id: becomes the share's / snapshots's qgroup.
     @todo: this should be part of add_share -- btrfs create should atomically
     Works by iterating over the output of btrfs subvolume list, received from
     subvol_list_helper() looking for a match in share_name. If found the same
-    line is parsed for the ID, example line in output:
+    line is parsed for the ID, example line in above command output:
     'ID 257 gen 13616 top level 5 path rock-ons-root'
+    consequent subvol_id return value:
+    '257'
     :param pool: a pool object.
     :param share_name: target share name to find
     :return: the id for the given share_name or an Exception stating no id
@@ -662,16 +668,15 @@ def remove_share(pool, share_name, pqgroup, force=False):
     return qgroup_destroy(pqgroup, root_pool_mnt)
 
 
-def remove_snap(pool, share_name, snap_name):
+def remove_snap(pool, share_name, snap_name, snap_qgroup):
     root_mnt = mount_root(pool)
     snap_path = ('%s/.snapshots/%s/%s' %
                  (root_mnt, share_name, snap_name))
     if (is_mounted(snap_path)):
         umount_root(snap_path)
     if (is_subvol(snap_path)):
-        qgroup = ('0/%s' % share_id(pool, snap_name))
         run_command([BTRFS, 'subvolume', 'delete', snap_path], log=True)
-        return qgroup_destroy(qgroup, root_mnt)
+        return qgroup_destroy(snap_qgroup, root_mnt)
     else:
         o, e, rc = run_command([BTRFS, 'subvolume', 'list', '-s', root_mnt])
         for l in o:
@@ -722,30 +727,6 @@ def add_snap(pool, share_name, snap_name, writable):
     return add_snap_helper(share_full_path, snap_full_path, writable)
 
 
-def rollback_snap(snap_name, sname, subvol_name, pool):
-    """
-    1. validate destination snapshot and umount the share
-    2. remove the share
-    3. move the snapshot to share location and mount it.
-    """
-    mnt_pt = ('%s%s' % (DEFAULT_MNT_DIR, sname))
-    snap_fp = ('%s/%s/.snapshots/%s/%s' % (DEFAULT_MNT_DIR, pool.name, sname,
-                                           snap_name))
-    if (not is_subvol(snap_fp)):
-        raise Exception('Snapshot(%s) does not exist. Rollback is not '
-                        'possible' % snap_fp)
-    mount_root(pool)
-    if (is_share_mounted(sname)):
-        umount_root(mnt_pt)
-    remove_share(pool, subvol_name, PQGROUP_DEFAULT)
-    shutil.move(snap_fp, '%s/%s/%s' % (DEFAULT_MNT_DIR, pool.name, sname))
-    create_tmp_dir(mnt_pt)
-    subvol_str = 'subvol=%s' % sname
-    dpath = get_device_path(pool.disk_set.attached().first().target_name)
-    mnt_cmd = [MOUNT, '-t', 'btrfs', '-o', subvol_str, dpath, mnt_pt]
-    run_command(mnt_cmd)
-
-
 def switch_quota(pool, flag='enable'):
     root_mnt_pt = mount_root(pool)
     cmd = [BTRFS, 'quota', flag, root_mnt_pt]

src/rockstor/storageadmin/tests/test_share_commands.py

@@ -33,16 +33,10 @@ class ShareCommandTests(APITestMixin, APITestCase):
     def setUpClass(cls):
         super(ShareCommandTests, cls).setUpClass()
 
-        # post mocks
-        cls.patch_update_quota = patch('storageadmin.views.share_command.'
-                                       'update_quota')
-        cls.mock_update_quota = cls.patch_update_quota.start()
-        cls.mock_update_quota.return_value = 'foo'
-
-        cls.patch_rollback_snap = patch('storageadmin.views.share_command.'
-                                        'rollback_snap')
-        cls.mock_rollback_snap = cls.patch_rollback_snap.start()
-        cls.mock_rollback_snap.return_value = True
+        cls.patch_create_repclone = patch('storageadmin.views.share_command.'
+                                          'create_repclone')
+        cls.mock_create_repclone = cls.patch_create_repclone.start()
+        cls.mock_create_repclone.return_value = Response('{"message": "ok!"}')
 
         cls.patch_create_clone = patch('storageadmin.views.share_command.'
                                        'create_clone')

src/rockstor/storageadmin/views/clone_helpers.py

@@ -20,15 +20,17 @@
 from storageadmin.util import handle_exception
 from fs.btrfs import (add_clone, share_id, update_quota, mount_share,
                       qgroup_create, set_property, remove_share,
-                      share_pqgroup_assign)
+                      share_pqgroup_assign, is_subvol)
 from rest_framework.response import Response
 from storageadmin.serializers import ShareSerializer
 import re
 import shutil
 from django.conf import settings
-
 from system.osi import run_command
 
+# The following model/db default setting is also used when quotas are disabled.
+PQGROUP_DEFAULT = settings.MODEL_DEFS['pqgroup']
+
 
 def create_repclone(share, request, logger, snapshot):
     """
@@ -49,7 +51,7 @@ def create_repclone(share, request, logger, snapshot):
     :return: response of serialized share (in it's updated form)
     """
     try:
-        logger.debug('Supplanting share ({}) with '
+        logger.info('Supplanting share ({}) with '
                      'snapshot ({}).'.format(share.name, snapshot.name))
         # We first strip our snapshot.name of any path as when we encounter the
         # initially created receive subvol it is identified as a share with a
@@ -71,8 +73,13 @@ def create_repclone(share, request, logger, snapshot):
         share_path = ('{}{}/{}'.format(settings.MNT_PT, share.pool.name,
                                        share.name))
         # eg /mnt2/poolname/sharename
-        # unmount and then subvol deletes our on disk share
-        remove_share(share.pool, share.name, '-1/-1')
+        # Passed db snap assured by caller but this does not guarantee on disk.
+        if not is_subvol(snap_path):
+            raise Exception('Subvol with path ({}) does not exist. Aborting '
+                            'replacement of share ({}).'.format(snap_path,
+                                                                share.name))
+        # unmounts and then subvol deletes our on disk share
+        remove_share(share.pool, share.name, PQGROUP_DEFAULT)
         # Remove read only flag on our snapshot subvol
         set_property(snap_path, 'ro', 'false', mount=False)
         # Ensure removed share path is clean, ie remove mount point.
@@ -126,7 +133,7 @@ def create_clone(share, new_name, request, logger, snapshot=None):
         new_share = Share(pool=share.pool, qgroup=qgroup_id, pqgroup=pqid,
                           name=new_name, size=share.size, subvol_name=new_name)
         new_share.save()
-        if pqid is not settings.MODEL_DEFS['pqgroup']:
+        if pqid is not PQGROUP_DEFAULT:
             update_quota(new_share.pool, pqid, new_share.size * 1024)
             share_pqgroup_assign(pqid, new_share)
         # Mount our new clone share.

src/rockstor/storageadmin/views/command.py

@@ -122,7 +122,7 @@ def post(self, request, command, rtcepoch=None):
             for snap in Snapshot.objects.all():
                 if (snap.uvisible):
                     try:
-                        mount_snap(snap.share, snap.real_name)
+                        mount_snap(snap.share, snap.real_name, snap.qgroup)
                     except Exception as e:
                         e_msg = ('Failed to make the snapshot ({}) visible. '
                                  'Exception: ({}).').format(snap.real_name,

src/rockstor/storageadmin/views/share_command.py

@@ -16,14 +16,12 @@
 along with this program. If not, see <http://www.gnu.org/licenses/>.
 """
 
-from rest_framework.response import Response
 from django.db import transaction
 from storageadmin.models import (Share, Snapshot, NFSExport, SambaShare)
-from fs.btrfs import (update_quota, rollback_snap)
 from storageadmin.serializers import ShareSerializer
 from storageadmin.util import handle_exception
 import rest_framework_custom as rfc
-from clone_helpers import create_clone
+from clone_helpers import create_clone, create_repclone
 from share import ShareMixin
 from django.core.exceptions import ObjectDoesNotExist
 import logging
@@ -72,11 +70,4 @@ def post(self, request, sid, command):
                              'shared via Samba. Unshare and '
                              'try again.').format(share.name)
                     handle_exception(Exception(e_msg), request)
-
-                rollback_snap(snap.real_name, share.name, share.subvol_name,
-                              share.pool)
-                update_quota(share.pool, snap.qgroup, share.size * 1024)
-                share.qgroup = snap.qgroup
-                share.save()
-                snap.delete()
-                return Response()
+                return create_repclone(share, request, logger, snap)

src/rockstor/storageadmin/views/share_helpers.py

@@ -60,20 +60,20 @@ def sftp_snap_toggle(share, mount=True):
                                     share.owner, share.name,
                                     snap.name))
         if (mount and not is_mounted(mnt_pt)):
-            mount_snap(share, snap.name, mnt_pt)
+            mount_snap(share, snap.name, snap.qgroup, mnt_pt)
         elif (is_mounted(mnt_pt) and not mount):
             umount_root(mnt_pt)
 
 
-def toggle_sftp_visibility(share, snap_name, on=True):
+def toggle_sftp_visibility(share, snap_name, snap_qgroup, on=True):
     if (not SFTP.objects.filter(share=share).exists()):
         return
 
     mnt_pt = ('%s/%s/%s/.%s' % (settings.SFTP_MNT_ROOT, share.owner,
                                 share.name, snap_name))
     if (on):
         if (not is_mounted(mnt_pt)):
-            mount_snap(share, snap_name, mnt_pt)
+            mount_snap(share, snap_name, snap_qgroup, mnt_pt)
     else:
         umount_root(mnt_pt)
 

src/rockstor/storageadmin/views/snapshot.py

@@ -66,13 +66,13 @@ def get_queryset(self, *args, **kwargs):
             return Snapshot.objects.filter(share=share).order_by('-id')
 
     @transaction.atomic
-    def _toggle_visibility(self, share, snap_name, on=True):
+    def _toggle_visibility(self, share, snap_name, snap_qgroup, on=True):
         cur_exports = list(NFSExport.objects.all())
         snap_mnt_pt = ('%s%s/.%s' % (settings.MNT_PT, share.name, snap_name))
         export_pt = snap_mnt_pt.replace(settings.MNT_PT,
                                         settings.NFS_EXPORT_ROOT)
         if (on):
-            mount_snap(share, snap_name)
+            mount_snap(share, snap_name, snap_qgroup)
 
             if (NFSExport.objects.filter(share=share).exists()):
                 se = NFSExport.objects.filter(share=share)[0]
@@ -156,7 +156,8 @@ def post(self, request, sid, snap_name, command=None):
 
                 if (uvisible):
                     try:
-                        self._toggle_visibility(share, ret.data['real_name'])
+                        self._toggle_visibility(share, ret.data['real_name'],
+                                                ret.data['qgroup'])
                     except Exception as e:
                         msg = ('Failed to make the snapshot ({}) visible. '
                                'Exception: ({}).').format(snap_name,
@@ -165,7 +166,8 @@ def post(self, request, sid, snap_name, command=None):
                         logger.exception(e)
 
                     try:
-                        toggle_sftp_visibility(share, ret.data['real_name'])
+                        toggle_sftp_visibility(share, ret.data['real_name'],
+                                               ret.data['qgroup'])
                     except Exception as e:
                         msg = ('Failed to make the snapshot ({}) visible for '
                                'SFTP. Exception: ({}).').format(snap_name,
@@ -237,10 +239,12 @@ def _delete_snapshot(self, request, sid, id=None, snap_name=None):
             handle_exception(Exception(e_msg), request)
 
         if (snapshot.uvisible):
-            self._toggle_visibility(share, snapshot.real_name, on=False)
-            toggle_sftp_visibility(share, snapshot.real_name, on=False)
+            self._toggle_visibility(share, snapshot.real_name, snapshot.qgroup,
+                                    on=False)
+            toggle_sftp_visibility(share, snapshot.real_name, snapshot.qgroup,
+                                   on=False)
 
-        remove_snap(share.pool, share.name, snapshot.name)
+        remove_snap(share.pool, share.name, snapshot.name, snapshot.qgroup)
         snapshot.delete()
         return Response()