From 4373e98fa0881c0bf6fa747e05742143402f4c39 Mon Sep 17 00:00:00 2001 From: Nate Potter Date: Tue, 12 Jul 2016 03:55:26 +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: Ifc9dcb20e17c60a835e9f3b38c5bffd836fa5188 Closes-bug: #1495560 --- cinder/brick/local_dev/lvm.py | 27 +++++++++++++++++++++++ cinder/exception.py | 4 ++++ cinder/tests/unit/brick/test_brick_lvm.py | 17 ++++++++++++++ 3 files changed, 48 insertions(+) diff --git a/cinder/brick/local_dev/lvm.py b/cinder/brick/local_dev/lvm.py index 5c4c2ca1dd3..871047da608 100644 --- a/cinder/brick/local_dev/lvm.py +++ b/cinder/brick/local_dev/lvm.py @@ -577,6 +577,18 @@ 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() + 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'] @@ -592,6 +604,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/cinder/exception.py b/cinder/exception.py index 0987c901c67..d847aeaeb93 100644 --- a/cinder/exception.py +++ b/cinder/exception.py @@ -821,6 +821,10 @@ class VolumeGroupCreationFailed(CinderException): message = _('Failed to create Volume Group: %(vg_name)s') +class VolumeNotDeactivated(CinderException): + message = _('Volume %(name)s was not deactivated in time.') + + class VolumeDeviceNotFound(CinderException): message = _('Volume device not found at %(device)s.') diff --git a/cinder/tests/unit/brick/test_brick_lvm.py b/cinder/tests/unit/brick/test_brick_lvm.py index 2673abf4b53..864e1c55195 100644 --- a/cinder/tests/unit/brick/test_brick_lvm.py +++ b/cinder/tests/unit/brick/test_brick_lvm.py @@ -372,3 +372,20 @@ class BrickLvmTestCase(test.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')