From e1f9a5462512e3acd39689ffbff67d454e5db58d Mon Sep 17 00:00:00 2001 From: Nate Potter Date: Thu, 14 Jul 2016 00:32:40 +0000 Subject: [PATCH] Remove race condition from lvextend Currently it's possible for extend_volume in lvm to return from the deactivate_lv call and try to extend the volume before the lv has actually been deactivated. This patch adds logic to make sure that the lv is deactivated before returning from deactivate_lv. Change-Id: I5c3671043df6e7474acdfcce342d655ac215a461 Closes-bug: #1495560 --- os_brick/exception.py | 4 +++ os_brick/local_dev/lvm.py | 29 ++++++++++++++++++++++ os_brick/tests/local_dev/test_brick_lvm.py | 26 +++++++++++++++++++ 3 files changed, 59 insertions(+) diff --git a/os_brick/exception.py b/os_brick/exception.py index 211084b..281fd95 100644 --- a/os_brick/exception.py +++ b/os_brick/exception.py @@ -94,6 +94,10 @@ class NoFibreChannelVolumeDeviceFound(BrickException): message = _("Unable to find a Fibre Channel volume device.") +class VolumeNotDeactivated(BrickException): + message = _('Volume %(name)s was not deactivated in time.') + + class VolumeDeviceNotFound(BrickException): message = _("Volume device not found at %(device)s.") diff --git a/os_brick/local_dev/lvm.py b/os_brick/local_dev/lvm.py index 2b80c2d..8dbf65e 100644 --- a/os_brick/local_dev/lvm.py +++ b/os_brick/local_dev/lvm.py @@ -616,6 +616,20 @@ class LVM(executor.Executor): return name return '_' + name + def _lv_is_active(self, name): + cmd = LVM.LVM_CMD_PREFIX + ['lvdisplay', '--noheading', '-C', '-o', + 'Attr', '%s/%s' % (self.vg_name, name)] + out, _err = self._execute(*cmd, + root_helper=self._root_helper, + run_as_root=True) + if out: + out = out.strip() + # An example output might be '-wi-a----'; the 4th index specifies + # the status of the volume. 'a' for active, '-' for inactive. + if (out[4] == 'a'): + return True + return False + def deactivate_lv(self, name): lv_path = self.vg_name + '/' + self._mangle_lv_name(name) cmd = ['lvchange', '-a', 'n'] @@ -631,6 +645,21 @@ class LVM(executor.Executor): LOG.error(_LE('StdErr :%s'), err.stderr) raise + # Wait until lv is deactivated to return in + # order to prevent a race condition. + self._wait_for_volume_deactivation(name) + + @utils.retry(exceptions=exception.VolumeNotDeactivated, retries=3, + backoff_rate=1) + def _wait_for_volume_deactivation(self, name): + LOG.debug("Checking to see if volume %s has been deactivated.", + name) + if self._lv_is_active(name): + LOG.debug("Volume %s is still active.", name) + raise exception.VolumeNotDeactivated(name=name) + else: + LOG.debug("Volume %s has been deactivated.", name) + def activate_lv(self, name, is_snapshot=False, permanent=False): """Ensure that logical volume/snapshot logical volume is activated. diff --git a/os_brick/tests/local_dev/test_brick_lvm.py b/os_brick/tests/local_dev/test_brick_lvm.py index f097e84..17d1ede 100644 --- a/os_brick/tests/local_dev/test_brick_lvm.py +++ b/os_brick/tests/local_dev/test_brick_lvm.py @@ -364,3 +364,29 @@ class BrickLvmTestCase(base.TestCase): self.vg.vg_name = "test-volumes" self.vg.extend_volume("test", "2G") self.assertFalse(self.vg.deactivate_lv.called) + + def test_lv_deactivate(self): + with mock.patch.object(self.vg, '_execute'): + is_active_mock = mock.Mock() + is_active_mock.return_value = False + self.vg._lv_is_active = is_active_mock + self.vg.create_volume('test', '1G') + self.vg.deactivate_lv('test') + + def test_lv_deactivate_timeout(self): + with mock.patch.object(self.vg, '_execute'): + is_active_mock = mock.Mock() + is_active_mock.return_value = True + self.vg._lv_is_active = is_active_mock + self.vg.create_volume('test', '1G') + self.assertRaises(exception.VolumeNotDeactivated, + self.vg.deactivate_lv, 'test') + + def test_lv_is_active(self): + self.vg.create_volume('test', '1G') + with mock.patch.object(self.vg, '_execute', + return_value=['owi-a---', '']): + self.assertTrue(self.vg._lv_is_active('test')) + with mock.patch.object(self.vg, '_execute', + return_value=['owi-----', '']): + self.assertFalse(self.vg._lv_is_active('test'))