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

diff --git a/cinder/tests/unit/test_volume.py b/cinder/tests/unit/test_volume.py
index 24520ab268966c529433b74e601d73a59944c979..eec4b01e0053ffbb44eef54c8a164feb32b8dafe 100644 (file)
--- 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 604b5387bc92f91747aa2e6ae2726c4fd193c622..d129779024355a62d909853118481aaf8330caa4 100644 (file)
--- 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 3c57b632bd203b0de079ba84cf07c7fe67fe4f9f..ab9e861b9eb314fbcf2e33985b2557c621c182a0 100644 (file)
--- 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 29145ec36026c28f9d006fe81038c834495baea0..0fe480f630a8c5098e440d2fffcd00e3f6c419a0 100644 (file)
--- 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<uuid>.+)'))
+    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