From a85d95084bf00a35efa40c9c3c2d38c9e73f45b8 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Fri, 20 May 2016 16:02:12 +0200 Subject: [PATCH] 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 | 4 ++ cinder/objects/base.py | 5 ++ cinder/tests/unit/test_lvm_driver.py | 5 +- cinder/tests/unit/test_volume_utils.py | 68 ++++++++++++-------------- cinder/volume/drivers/lvm.py | 2 +- cinder/volume/utils.py | 12 ++--- 6 files changed, 49 insertions(+), 47 deletions(-) diff --git a/cinder/db/api.py b/cinder/db/api.py index 605b2e8f0..7ee9b905a 100644 --- a/cinder/db/api.py +++ b/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) diff --git a/cinder/objects/base.py b/cinder/objects/base.py index a4929eed9..76a5de2b2 100644 --- a/cinder/objects/base.py +++ b/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): diff --git a/cinder/tests/unit/test_lvm_driver.py b/cinder/tests/unit/test_lvm_driver.py index 8aafb2f88..9d93059a6 100644 --- a/cinder/tests/unit/test_lvm_driver.py +++ b/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} diff --git a/cinder/tests/unit/test_volume_utils.py b/cinder/tests/unit/test_volume_utils.py index a5a667522..30e9a7df6 100644 --- a/cinder/tests/unit/test_volume_utils.py +++ b/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): diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index be7534d88..c7141646a 100644 --- a/cinder/volume/drivers/lvm.py +++ b/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. diff --git a/cinder/volume/utils.py b/cinder/volume/utils.py index 4e17e635d..53fd2bdb7 100644 --- a/cinder/volume/utils.py +++ b/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):