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

@@ -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.
 

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)

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'])

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