Merge "Harden LVM driver deletion paths" into stable/rocky

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

@@ -118,7 +118,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)
@@ -269,11 +271,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):
@@ -281,9 +283,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
@@ -442,7 +448,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

@@ -268,9 +268,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'])
@@ -288,6 +291,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.