From eee9edd479e0e1d7b6f497e40d4eba59f31449b7 Mon Sep 17 00:00:00 2001 From: Anton Arefiev Date: Wed, 18 Feb 2015 12:44:09 +0200 Subject: [PATCH] Fix lvm manage existing volume When trying to manage command to a volume that is already managed by Cinder, the operation does not fail and you will end up in an unexpected state. The operation will succeed, the backend storage will be renamed, and there will be two Cinder volumes in the available state with only one of them being valid. This change adds volume check on driver level (since we don't know source-name, source-id or anything else used as ref for volume, each driver should be responsible for figuring out what to do with specified volume ref). Check method looking for possible existing volume in db to avoid managing already managed volume. Change-Id: I0d8d7a3cb2afce90529cd1142b413aca9ac69d4c Partial-Bug: #1364550 --- cinder/tests/unit/test_volume.py | 27 +++++++++++++ cinder/tests/unit/test_volume_utils.py | 52 ++++++++++++++++++++++++++ cinder/volume/drivers/lvm.py | 3 ++ cinder/volume/utils.py | 28 ++++++++++++++ 4 files changed, 110 insertions(+) diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py index 24520ab26..eec4b01e0 100644 --- a/cinder/tests/unit/test_volume.py +++ b/cinder/tests/unit/test_volume.py @@ -5736,6 +5736,33 @@ class LVMISCSIVolumeDriverTestCase(DriverTestCase): self.stubs.Set(self.volume.driver.vg, 'get_volume', self._get_manage_existing_lvs) + @mock.patch.object(db, 'volume_get', side_effect=exception.VolumeNotFound( + volume_id='d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1')) + def test_lvm_manage_existing_not_found(self, mock_vol_get): + self._setup_stubs_for_manage_existing() + + vol_name = 'volume-d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1' + ref = {'source-name': 'fake_lv'} + vol = {'name': vol_name, 'id': 1, 'size': 0} + + with mock.patch.object(self.volume.driver.vg, 'rename_volume'): + model_update = self.volume.driver.manage_existing(vol, ref) + self.assertIsNone(model_update) + + @mock.patch.object(db, 'volume_get') + def test_lvm_manage_existing_already_managed(self, mock_conf): + self._setup_stubs_for_manage_existing() + + mock_conf.volume_name_template = 'volume-%s' + vol_name = 'volume-d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1' + ref = {'source-name': vol_name} + vol = {'name': 'test', 'id': 1, 'size': 0} + + with mock.patch.object(self.volume.driver.vg, 'rename_volume'): + self.assertRaises(exception.ManageExistingAlreadyManaged, + self.volume.driver.manage_existing, + vol, ref) + def test_lvm_manage_existing(self): """Good pass on managing an LVM volume. diff --git a/cinder/tests/unit/test_volume_utils.py b/cinder/tests/unit/test_volume_utils.py index 604b5387b..d12977902 100644 --- a/cinder/tests/unit/test_volume_utils.py +++ b/cinder/tests/unit/test_volume_utils.py @@ -741,3 +741,55 @@ class VolumeUtilsTestCase(test.TestCase): host_2 = 'fake_host2@backend1' self.assertFalse(volume_utils.hosts_are_equivalent(host_1, host_2)) + + def test_check_managed_volume_already_managed(self): + mock_db = mock.Mock() + + result = volume_utils.check_already_managed_volume( + mock_db, 'volume-d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1') + self.assertTrue(result) + + @mock.patch('cinder.volume.utils.CONF') + def test_check_already_managed_with_vol_id_vol_pattern(self, conf_mock): + mock_db = mock.Mock() + conf_mock.volume_name_template = 'volume-%s-volume' + + result = volume_utils.check_already_managed_volume( + mock_db, 'volume-d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1-volume') + self.assertTrue(result) + + @mock.patch('cinder.volume.utils.CONF') + def test_check_already_managed_with_id_vol_pattern(self, conf_mock): + mock_db = mock.Mock() + conf_mock.volume_name_template = '%s-volume' + + result = volume_utils.check_already_managed_volume( + mock_db, 'd8cd1feb-2dcc-404d-9b15-b86fe3bec0a1-volume') + self.assertTrue(result) + + def test_check_managed_volume_not_managed_cinder_like_name(self): + mock_db = mock.Mock() + mock_db.volume_get = mock.Mock( + side_effect=exception.VolumeNotFound( + 'volume-d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1')) + + result = volume_utils.check_already_managed_volume( + mock_db, 'volume-d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1') + + self.assertFalse(result) + + def test_check_managed_volume_not_managed(self): + mock_db = mock.Mock() + + result = volume_utils.check_already_managed_volume( + mock_db, 'test-volume') + + self.assertFalse(result) + + def test_check_managed_volume_not_managed_id_like_uuid(self): + mock_db = mock.Mock() + + result = volume_utils.check_already_managed_volume( + mock_db, 'volume-d8cd1fe') + + self.assertFalse(result) diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index 3c57b632b..ab9e861b9 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -505,6 +505,9 @@ class LVMVolumeDriver(driver.VolumeDriver): lv_name = existing_ref['source-name'] self.vg.get_volume(lv_name) + if volutils.check_already_managed_volume(self.db, lv_name): + raise exception.ManageExistingAlreadyManaged(volume_ref=lv_name) + # Attempt to rename the LV to match the OpenStack internal name. try: self.vg.rename_volume(lv_name, volume['name']) diff --git a/cinder/volume/utils.py b/cinder/volume/utils.py index 29145ec36..0fe480f63 100644 --- a/cinder/volume/utils.py +++ b/cinder/volume/utils.py @@ -16,6 +16,8 @@ import math +import re +import uuid from Crypto.Random import random from oslo_concurrency import processutils @@ -27,6 +29,7 @@ from oslo_utils import units from six.moves import range from cinder.brick.local_dev import lvm as brick_lvm +from cinder import context from cinder import db from cinder import exception from cinder.i18n import _LI, _LW @@ -536,3 +539,28 @@ def read_proc_mounts(): """ with open('/proc/mounts') as mounts: return mounts.readlines() + + +def _extract_id(vol_name): + regex = re.compile( + CONF.volume_name_template.replace('%s', '(?P.+)')) + match = regex.match(vol_name) + return match.group('uuid') if match else None + + +def check_already_managed_volume(db, vol_name): + """Check cinder db for already managed volume. + + :param db: database api parameter + :param vol_name: volume name parameter + :returns: bool -- return True, if db entry with specified + volume name exist, otherwise return False + """ + vol_id = _extract_id(vol_name) + try: + if vol_id and uuid.UUID(vol_id, version=4): + db.volume_get(context.get_admin_context(), vol_id) + return True + except (exception.VolumeNotFound, ValueError): + return False + return False -- 2.45.2