Harden LVM driver deletion paths

During maintenance, administrators may decide
to unmount shares, snapshots or remove them
entirely prior to cleaning up LVM share resources
in manila. The driver should not fail on deletion
of missing resources.

Change-Id: Ieaf37ec10db9a8bdce6bb195b76335fea9b2b52f
Closes-Bug: #1888915
Signed-off-by: Goutham Pacha Ravi <gouthampravi@gmail.com>

manila/share/drivers/helpers.py

@@ -278,8 +278,18 @@ class NFSHelper(NASHelperBase):
                     continue
                 access_to = self._get_parsed_address_or_cidr(
                     access['access_to'])
-                self._ssh_exec(server, ['sudo', 'exportfs', '-u',
-                               ':'.join((access_to, local_path))])
+                try:
+                    self._ssh_exec(server, ['sudo', 'exportfs', '-u',
+                                            ':'.join((access_to, local_path))])
+                except exception.ProcessExecutionError as e:
+                    if "could not find" in e.stderr.lower():
+                        LOG.debug(
+                            "Client/s with IP address/es %(host)s did not "
+                            "have access to %(share)s. Nothing to deny.",
+                            {'host': access_to, 'share': share_name})
+                    else:
+                        raise
+
             if delete_rules:
                 self._sync_nfs_temp_and_perm_files(server)
             for access in add_rules:

manila/share/drivers/lvm.py

@@ -108,7 +108,9 @@ class LVMMixin(driver.ExecuteMixin):
                               (self.configuration.lvm_share_volume_group,
                                share_name), run_as_root=True)
         except exception.ProcessExecutionError as exc:
-            if "not found" not in exc.stderr:
+            err_pattern = re.compile(".*failed to find.*|.*not found.*",
+                                     re.IGNORECASE)
+            if not err_pattern.match(exc.stderr):
                 LOG.exception("Error deleting volume")
                 raise
             LOG.warning("Volume not found: %s", exc.stderr)
@@ -256,11 +258,11 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
         return location
 
     def delete_share(self, context, share, share_server=None):
-        self._unmount_device(share)
+        self._unmount_device(share, raise_if_missing=False)
         self._delete_share(context, share)
         self._deallocate_container(share['name'])
 
-    def _unmount_device(self, share_or_snapshot):
+    def _unmount_device(self, share_or_snapshot, raise_if_missing=True):
         """Unmount the filesystem of a share or snapshot LV."""
         mount_path = self._get_mount_path(share_or_snapshot)
         if os.path.exists(mount_path):
@@ -268,9 +270,13 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
             try:
                 self._execute('umount', '-f', mount_path, run_as_root=True)
             except exception.ProcessExecutionError as exc:
-                if 'device is busy' in six.text_type(exc):
+                if 'device is busy' in exc.stderr.lower():
                     raise exception.ShareBusyException(
                         reason=share_or_snapshot['name'])
+                elif 'not mounted' in exc.stderr.lower():
+                    if raise_if_missing:
+                        LOG.error('Unable to find device: %s', exc)
+                        raise
                 else:
                     LOG.error('Unable to umount: %s', exc)
                     raise
@@ -429,7 +435,7 @@ class LVMShareDriver(LVMMixin, driver.ShareDriver):
         return {'export_locations': exports}
 
     def delete_snapshot(self, context, snapshot, share_server=None):
-        self._unmount_device(snapshot)
+        self._unmount_device(snapshot, raise_if_missing=False)
 
         super(LVMShareDriver, self).delete_snapshot(context, snapshot,
                                                     share_server)

manila/tests/share/drivers/test_helpers.py

@@ -215,12 +215,41 @@ class NFSHelperTestCase(test.TestCase):
             [],
             [])
 
-    def test_update_access_delete_invalid_rule(self):
-        delete_rules = [test_generic.get_fake_access_rule(
-            'lala', 'fake_level', access_type='user'), ]
+    @ddt.data({'access_to': 'lala', 'access_type': 'user'},
+              {'access_to': '203.0.113.29'},
+              {'access_to': '2001:0DB8:7d18:c63e:5f0a:871f:83b8:d244',
+               'access_level': 'ro'})
+    @ddt.unpack
+    def test_update_access_delete_invalid_rule(
+            self, access_to, access_level='rw', access_type='ip'):
+        mount_path = '%s:/shares/%s' % (access_to, self.share_name)
+        if access_type == 'ip':
+            self._helper._get_parsed_address_or_cidr = mock.Mock(
+                return_value=access_to)
+            not_found_msg = (
+                "exportfs: Could not find '%s' to unexport.\n" % mount_path
+            )
+            exc = exception.ProcessExecutionError
+            self.mock_object(
+                self._helper,
+                '_ssh_exec',
+                mock.Mock(side_effect=[(0, 0), exc(stderr=not_found_msg)]))
+
+        delete_rules = [
+            test_generic.get_fake_access_rule(access_to,
+                                              access_level,
+                                              access_type),
+        ]
         self.mock_object(self._helper, '_sync_nfs_temp_and_perm_files')
+
         self._helper.update_access(self.server, self.share_name, [],
                                    [], delete_rules)
+
+        if access_type == 'ip':
+            self._helper._ssh_exec.assert_has_calls([
+                mock.call(self.server, ['sudo', 'exportfs']),
+                mock.call(self.server,
+                          ['sudo', 'exportfs', '-u', mount_path])])
         self._helper._sync_nfs_temp_and_perm_files.assert_called_with(
             self.server)
 

manila/tests/share/drivers/test_lvm.py

@@ -248,9 +248,12 @@ class LVMShareDriverTestCase(test.TestCase):
                           self._driver._deallocate_container,
                           self.share['name'])
 
-    def test_deallocate_container_not_found_error(self):
+    @ddt.data(
+        'Logical volume "fake/fake-volume" not found\n',
+        'Failed to find logical volume "fake/fake-volume"\n')
+    def test_deallocate_container_not_found_error(self, error_msg):
         def _fake_exec(*args, **kwargs):
-            raise exception.ProcessExecutionError(stderr="not found")
+            raise exception.ProcessExecutionError(stderr=error_msg)
 
         self.mock_object(self._driver, '_try_execute', _fake_exec)
         self._driver._deallocate_container(self.share['name'])
@@ -268,6 +271,22 @@ class LVMShareDriverTestCase(test.TestCase):
                              self._driver.get_share_stats(refresh=True))
         self._driver._update_share_stats.assert_called_once_with()
 
+    def test__unmount_device_not_mounted(self):
+        def exec_runner(*ignore_args, **ignore_kwargs):
+            umount_msg = (
+                "umount: /opt/stack/data/manila/mnt/share-fake-share: not "
+                "mounted.\n"
+            )
+            raise exception.ProcessExecutionError(stderr=umount_msg)
+        self._os.path.exists.return_value = True
+        mount_path = self._get_mount_path(self.share)
+        expected_exec = "umount -f %s" % (mount_path)
+        fake_utils.fake_execute_set_repliers([(expected_exec, exec_runner)])
+
+        self._driver._unmount_device(self.share, raise_if_missing=False)
+
+        self._os.path.exists.assert_called_with(mount_path)
+
     def test__unmount_device_is_busy_error(self):
         def exec_runner(*ignore_args, **ignore_kwargs):
             raise exception.ProcessExecutionError(stderr='device is busy')

releasenotes/notes/bug-1888915-harden-lvm-deletions-2a735ab0ee4a4903.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - |
+    The LVM driver no longer fails to delete shares, snapshots and access
+    rules that are missing from storage. See `Launchpad bug #1888915
+    <https://launchpad.net/bugs/1888915>`_ for more details.