From e57809fd74186714aec7dae305380a51c31fc7d5 Mon Sep 17 00:00:00 2001 From: Goutham Pacha Ravi Date: Mon, 27 Jul 2020 18:02:51 -0700 Subject: [PATCH] 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 --- manila/share/drivers/helpers.py | 14 ++++++-- manila/share/drivers/lvm.py | 16 ++++++--- manila/tests/share/drivers/test_helpers.py | 35 +++++++++++++++++-- manila/tests/share/drivers/test_lvm.py | 23 ++++++++++-- ...harden-lvm-deletions-2a735ab0ee4a4903.yaml | 6 ++++ 5 files changed, 82 insertions(+), 12 deletions(-) create mode 100644 releasenotes/notes/bug-1888915-harden-lvm-deletions-2a735ab0ee4a4903.yaml diff --git a/manila/share/drivers/helpers.py b/manila/share/drivers/helpers.py index b69d0267a2..085e97d520 100644 --- a/manila/share/drivers/helpers.py +++ b/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: diff --git a/manila/share/drivers/lvm.py b/manila/share/drivers/lvm.py index e991e50fe8..91d532b61c 100644 --- a/manila/share/drivers/lvm.py +++ b/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) diff --git a/manila/tests/share/drivers/test_helpers.py b/manila/tests/share/drivers/test_helpers.py index 873c3c7a5c..d38aaf2b01 100644 --- a/manila/tests/share/drivers/test_helpers.py +++ b/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) diff --git a/manila/tests/share/drivers/test_lvm.py b/manila/tests/share/drivers/test_lvm.py index 175e9415fc..4c0aa96068 100644 --- a/manila/tests/share/drivers/test_lvm.py +++ b/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') diff --git a/releasenotes/notes/bug-1888915-harden-lvm-deletions-2a735ab0ee4a4903.yaml b/releasenotes/notes/bug-1888915-harden-lvm-deletions-2a735ab0ee4a4903.yaml new file mode 100644 index 0000000000..537c320c3a --- /dev/null +++ b/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 + `_ for more details.