From 4e292ea884ff72341c2ab6733745fe6d35733aa8 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 (cherry picked from commit e57809fd74186714aec7dae305380a51c31fc7d5) (cherry picked from commit 8620f7efed1c2b669a06f13440add1eb20ef12e9) --- 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 c3efac256c..012cbf5b06 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 6ba844a1e2..fe30366cc1 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 a88db6e8d6..d5cc134f28 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 a61d1bd192..6ff5cc14c6 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.