Improve DB volume check in LVM manage volume

LVM driver makes a call to volume.utils.check_alredy_managed_volume when
managing a new volume in method manage_existing, and this check method
is trying to retrieve the volume for the check, which means getting all
the volume information with joined tables and then converting it to an
ORM object.

This patch improves this check_already_managed_volume by using a simpler
EXISTS DB query.

Change-Id: I132b6bdd05524c955fcd404ed45624e1b1e98320

cinder/db/api.py

@@ -1204,6 +1204,10 @@ def message_destroy(context, message_id):
 ###################
 
 
+def resource_exists(context, model, resource_id):
+    return IMPL.resource_exists(context, model, resource_id)
+
+
 def get_model_for_versioned_object(versioned_object):
     return IMPL.get_model_for_versioned_object(versioned_object)
 

cinder/objects/base.py

@@ -416,6 +416,11 @@ class CinderPersistentObject(object):
                     setattr(self, field, current_field)
         self.obj_reset_changes()
 
+    @classmethod
+    def exists(cls, context, id_):
+        model = db.get_model_for_versioned_object(cls)
+        return db.resource_exists(context, model, id_)
+
 
 class CinderComparableObject(base.ComparableVersionedObject):
     def __eq__(self, obj):

cinder/tests/unit/test_lvm_driver.py

@@ -660,11 +660,10 @@ class LVMVolumeDriverTestCase(DriverTestCase):
             model_update = self.volume.driver.manage_existing(vol, ref)
             self.assertIsNone(model_update)
 
-    @mock.patch.object(db.sqlalchemy.api, 'volume_get')
-    def test_lvm_manage_existing_already_managed(self, mock_conf):
+    @mock.patch('cinder.db.sqlalchemy.api.resource_exists', return_value=True)
+    def test_lvm_manage_existing_already_managed(self, exists_mock):
         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}

cinder/tests/unit/test_volume_utils.py

@@ -25,6 +25,7 @@ from oslo_concurrency import processutils
 from oslo_config import cfg
 
 from cinder import context
+from cinder.db.sqlalchemy import models
 from cinder import exception
 from cinder.objects import fields
 from cinder import test
@@ -757,56 +758,51 @@ 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')
+    @mock.patch('cinder.db.sqlalchemy.api.resource_exists', return_value=True)
+    def test_check_managed_volume_already_managed(self, exists_mock):
+        id_ = 'd8cd1feb-2dcc-404d-9b15-b86fe3bec0a1'
+        vol_id = 'volume-' + id_
+        result = volume_utils.check_already_managed_volume(vol_id)
         self.assertTrue(result)
+        exists_mock.assert_called_once_with(mock.ANY, models.Volume, id_)
 
-    @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'
+    @mock.patch('cinder.db.sqlalchemy.api.resource_exists', return_value=True)
+    def test_check_already_managed_with_vol_id_vol_pattern(self, exists_mock):
+        template = 'volume-%s-volume'
+        self.override_config('volume_name_template', template)
+        id_ = 'd8cd1feb-2dcc-404d-9b15-b86fe3bec0a1'
+        vol_id = template % id_
 
-        result = volume_utils.check_already_managed_volume(
-            mock_db, 'volume-d8cd1feb-2dcc-404d-9b15-b86fe3bec0a1-volume')
+        result = volume_utils.check_already_managed_volume(vol_id)
         self.assertTrue(result)
+        exists_mock.assert_called_once_with(mock.ANY, models.Volume, id_)
 
-    @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'
+    @mock.patch('cinder.db.sqlalchemy.api.resource_exists', return_value=True)
+    def test_check_already_managed_with_id_vol_pattern(self, exists_mock):
+        template = '%s-volume'
+        self.override_config('volume_name_template', template)
+        id_ = 'd8cd1feb-2dcc-404d-9b15-b86fe3bec0a1'
+        vol_id = template % id_
 
-        result = volume_utils.check_already_managed_volume(
-            mock_db, 'd8cd1feb-2dcc-404d-9b15-b86fe3bec0a1-volume')
+        result = volume_utils.check_already_managed_volume(vol_id)
         self.assertTrue(result)
+        exists_mock.assert_called_once_with(mock.ANY, models.Volume, id_)
 
-    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')
-
+    @mock.patch('cinder.db.sqlalchemy.api.resource_exists', return_value=False)
+    def test_check_managed_volume_not_managed_cinder_like_name(self,
+                                                               exists_mock):
+        id_ = 'd8cd1feb-2dcc-404d-9b15-b86fe3bec0a1'
+        vol_id = 'volume-' + id_
+        result = volume_utils.check_already_managed_volume(vol_id)
         self.assertFalse(result)
+        exists_mock.assert_called_once_with(mock.ANY, models.Volume, id_)
 
     def test_check_managed_volume_not_managed(self):
-        mock_db = mock.Mock()
-
-        result = volume_utils.check_already_managed_volume(
-            mock_db, 'test-volume')
-
+        result = volume_utils.check_already_managed_volume('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')
-
+        result = volume_utils.check_already_managed_volume('volume-d8cd1fe')
         self.assertFalse(result)
 
     def test_convert_config_string_to_dict(self):

cinder/volume/drivers/lvm.py

@@ -586,7 +586,7 @@ 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):
+        if volutils.check_already_managed_volume(lv_name):
             raise exception.ManageExistingAlreadyManaged(volume_ref=lv_name)
 
         # Attempt to rename the LV to match the OpenStack internal name.

cinder/volume/utils.py

@@ -38,6 +38,7 @@ from cinder import context
 from cinder import db
 from cinder import exception
 from cinder.i18n import _, _LI, _LW, _LE
+from cinder import objects
 from cinder import rpc
 from cinder import utils
 from cinder.volume import throttling
@@ -670,22 +671,19 @@ def _extract_id(vol_name):
     return match.group('uuid') if match else None
 
 
-def check_already_managed_volume(db, vol_name):
+def check_already_managed_volume(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 (vol_id and uuid.UUID(vol_id, version=4) and
+                objects.Volume.exists(context.get_admin_context(), vol_id))
+    except ValueError:
         return False
-    return False
 
 
 def convert_config_string_to_dict(config_string):