From d1869d0b1cf090b33e436171cc2844b936ebad52 Mon Sep 17 00:00:00 2001 From: Yuriy Nesenenko Date: Thu, 3 Dec 2015 16:28:31 +0200 Subject: [PATCH] Add synchronization in Block Device driver Multiple calls can get the same device. This patch adds synchronization to create_volume, delete_volume and create_cloned_volume to avoid race conditions. The delete_volume method is modified to avoid code duplication in the implementation of delete_snapshot method for snapshot in the next patch. Also it adds update provider_location and host to mark device as used to avoid race with other threads. It uses object notation instead of dict notation to a volume and snapshot. Change-Id: I0b8fb28dde367c3f34dd0b6b9d98372cfff65d45 Closes-Bug: #1335904 --- cinder/tests/unit/test_block_device.py | 112 +++++++++++++++---------- cinder/volume/drivers/block_device.py | 79 ++++++++++------- 2 files changed, 121 insertions(+), 70 deletions(-) diff --git a/cinder/tests/unit/test_block_device.py b/cinder/tests/unit/test_block_device.py index 464b66bcf..cd83c8b30 100644 --- a/cinder/tests/unit/test_block_device.py +++ b/cinder/tests/unit/test_block_device.py @@ -17,8 +17,10 @@ import mock from oslo_config import cfg from cinder import context -from cinder.db.sqlalchemy import api +from cinder import db import cinder.exception +from cinder.objects import snapshot as obj_snap +from cinder.objects import volume as obj_volume import cinder.test from cinder.volume import configuration as conf from cinder.volume.drivers import block_device @@ -37,15 +39,14 @@ class TestBlockDeviceDriver(cinder.test.TestCase): self.configuration.volume_dd_blocksize = 1234 self.drv = block_device.BlockDeviceDriver( configuration=self.configuration, - host='localhost') + host='localhost', db=db) def test_initialize_connection(self): - TEST_VOLUME1 = {'host': 'localhost1', - 'provider_location': '1 2 3 /dev/loop1', - 'provider_auth': 'a b c', - 'attached_mode': 'rw', - 'id': 'fake-uuid'} - + TEST_VOLUME1 = obj_volume.Volume(host='localhost1', + provider_location='1 2 3 /dev/loop1', + provider_auth='a b c', + attached_mode='rw', + id='fake-uuid') TEST_CONNECTOR = {'host': 'localhost1'} data = self.drv.initialize_connection(TEST_VOLUME1, TEST_CONNECTOR) @@ -57,11 +58,11 @@ class TestBlockDeviceDriver(cinder.test.TestCase): @mock.patch('cinder.volume.driver.ISCSIDriver.initialize_connection') def test_initialize_connection_different_hosts(self, _init_conn): TEST_CONNECTOR = {'host': 'localhost1'} - TEST_VOLUME2 = {'host': 'localhost2', - 'provider_location': '1 2 3 /dev/loop2', - 'provider_auth': 'd e f', - 'attached_mode': 'rw', - 'id': 'fake-uuid-2'} + TEST_VOLUME2 = obj_volume.Volume(host='localhost2', + provider_location='1 2 3 /dev/loop2', + provider_auth='d e f', + attached_mode='rw', + id='fake-uuid-2') _init_conn.return_value = 'data' data = self.drv.initialize_connection(TEST_VOLUME2, TEST_CONNECTOR) @@ -82,14 +83,18 @@ class TestBlockDeviceDriver(cinder.test.TestCase): @mock.patch('cinder.volume.utils.clear_volume') def test_delete_not_volume_provider_location(self, _clear_volume, _local_path): - TEST_VOLUME2 = {'provider_location': None} + TEST_VOLUME2 = obj_volume.Volume(provider_location=None) self.drv.delete_volume(TEST_VOLUME2) _local_path.assert_called_once_with(TEST_VOLUME2) @mock.patch('os.path.exists', return_value=True) @mock.patch('cinder.volume.utils.clear_volume') def test_delete_volume_path_exist(self, _clear_volume, _exists): - TEST_VOLUME1 = {'provider_location': '1 2 3 /dev/loop1'} + TEST_VOLUME = obj_volume.Volume(name_id='1234', + size=1, + provider_location='/dev/loop1', + display_name='vol1', + status='available') with mock.patch.object(self.drv, 'local_path', return_value='/dev/loop1') as lp_mocked: @@ -98,29 +103,43 @@ class TestBlockDeviceDriver(cinder.test.TestCase): gds_mocked: volutils.clear_volume(gds_mocked, lp_mocked) - self.drv.delete_volume(TEST_VOLUME1) + self.drv.delete_volume(TEST_VOLUME) - lp_mocked.assert_called_once_with(TEST_VOLUME1) + lp_mocked.assert_called_once_with(TEST_VOLUME) gds_mocked.assert_called_once_with(['/dev/loop1']) self.assertTrue(_exists.called) self.assertTrue(_clear_volume.called) def test_delete_path_is_not_in_list_of_available_devices(self): - TEST_VOLUME2 = {'provider_location': '1 2 3 /dev/loop0'} + TEST_VOLUME2 = obj_volume.Volume(provider_location='/dev/loop0') with mock.patch.object(self.drv, 'local_path', return_value='/dev/loop0') as lp_mocked: self.drv.delete_volume(TEST_VOLUME2) lp_mocked.assert_called_once_with(TEST_VOLUME2) + def test__update_provider_location(self): + TEST_VOLUME = obj_volume.Volume(name_id='1234', + size=1, + display_name='vol1') + with mock.patch.object(obj_volume.Volume, 'update') as update_mocked, \ + mock.patch.object(obj_volume.Volume, 'save') as save_mocked: + self.drv._update_provider_location(TEST_VOLUME, 'dev_path') + self.assertEqual(1, update_mocked.call_count) + save_mocked.assert_called_once_with() + def test_create_volume(self): - TEST_VOLUME = {'size': 1, 'name': 'vol1'} + TEST_VOLUME = obj_volume.Volume(name_id='1234', + size=1, + display_name='vol1') with mock.patch.object(self.drv, 'find_appropriate_size_device', return_value='dev_path') as fasd_mocked: - result = self.drv.create_volume(TEST_VOLUME) - self.assertEqual({'provider_location': 'dev_path'}, result) - fasd_mocked.assert_called_once_with(TEST_VOLUME['size']) + with mock.patch.object(self.drv, '_update_provider_location') as \ + upl_mocked: + self.drv.create_volume(TEST_VOLUME) + fasd_mocked.assert_called_once_with(TEST_VOLUME.size) + upl_mocked.assert_called_once_with(TEST_VOLUME, 'dev_path') def test_update_volume_stats(self): @@ -147,10 +166,13 @@ class TestBlockDeviceDriver(cinder.test.TestCase): @mock.patch('cinder.volume.utils.copy_volume') def test_create_cloned_volume(self, _copy_volume): - TEST_SRC = {'id': '1', - 'size': 1, - 'provider_location': '1 2 3 /dev/loop1'} - TEST_VOLUME = {} + TEST_SRC = obj_volume.Volume(id=1, + name_id='5678', + size=1, + provider_location='/dev/loop1') + TEST_VOLUME = obj_volume.Volume(name_id='1234', + size=1, + display_name='vol1') with mock.patch.object(self.drv, 'find_appropriate_size_device', return_value='/dev/loop2') as fasd_mocked: @@ -160,20 +182,24 @@ class TestBlockDeviceDriver(cinder.test.TestCase): with mock.patch.object(self.drv, 'local_path', return_value='/dev/loop1') as \ lp_mocked: - volutils.copy_volume('/dev/loop1', fasd_mocked, 2, - mock.sentinel, - execute=self.drv._execute) - - self.assertEqual({'provider_location': '/dev/loop2'}, - self.drv.create_cloned_volume(TEST_VOLUME, - TEST_SRC)) - fasd_mocked.assert_called_once_with(TEST_SRC['size']) - lp_mocked.assert_called_once_with(TEST_SRC) - gds_mocked.assert_called_once_with(['/dev/loop2']) + with mock.patch.object(self.drv, + '_update_provider_location') as \ + upl_mocked: + volutils.copy_volume('/dev/loop1', fasd_mocked, 2, + mock.sentinel, + execute=self.drv._execute) + self.drv.create_cloned_volume(TEST_VOLUME, TEST_SRC) + fasd_mocked.assert_called_once_with(TEST_SRC.size) + lp_mocked.assert_called_once_with(TEST_SRC) + gds_mocked.assert_called_once_with(['/dev/loop2']) + upl_mocked.assert_called_once_with( + TEST_VOLUME, '/dev/loop2') @mock.patch.object(cinder.image.image_utils, 'fetch_to_raw') def test_copy_image_to_volume(self, _fetch_to_raw): - TEST_VOLUME = {'provider_location': '1 2 3 /dev/loop1', 'size': 1} + TEST_VOLUME = obj_volume.Volume(name_id='1234', + size=1, + provider_location='/dev/loop1') TEST_IMAGE_SERVICE = "image_service" TEST_IMAGE_ID = "image_id" @@ -188,7 +214,7 @@ class TestBlockDeviceDriver(cinder.test.TestCase): 1234, size=1) def test_copy_volume_to_image(self): - TEST_VOLUME = {'provider_location': '1 2 3 /dev/loop1'} + TEST_VOLUME = {'provider_location': '/dev/loop1'} TEST_IMAGE_SERVICE = "image_service" TEST_IMAGE_META = "image_meta" @@ -208,15 +234,17 @@ class TestBlockDeviceDriver(cinder.test.TestCase): def test_get_used_devices(self): TEST_VOLUME1 = {'host': 'localhost', - 'provider_location': '1 2 3 /dev/loop1'} + 'provider_location': '/dev/loop1'} TEST_VOLUME2 = {'host': 'localhost', - 'provider_location': '1 2 3 /dev/loop2'} + 'provider_location': '/dev/loop2'} def fake_local_path(vol): return vol['provider_location'].split()[-1] - with mock.patch.object(api, 'volume_get_all_by_host', - return_value=[TEST_VOLUME1, TEST_VOLUME2]): + with mock.patch.object(obj_volume.VolumeList, 'get_all_by_host', + return_value=[TEST_VOLUME1, TEST_VOLUME2]),\ + mock.patch.object(obj_snap.SnapshotList, 'get_by_host', + return_value=[]): with mock.patch.object(context, 'get_admin_context'): with mock.patch.object(self.drv, 'local_path', return_value=fake_local_path): diff --git a/cinder/volume/drivers/block_device.py b/cinder/volume/drivers/block_device.py index 6a54e657b..dfc7d58a3 100644 --- a/cinder/volume/drivers/block_device.py +++ b/cinder/volume/drivers/block_device.py @@ -21,10 +21,11 @@ from oslo_utils import importutils from oslo_utils import units from cinder import context -from cinder.db.sqlalchemy import api from cinder import exception -from cinder.i18n import _, _LI +from cinder.i18n import _, _LI, _LW from cinder.image import image_utils +from cinder import objects +from cinder import utils from cinder.volume import driver from cinder.volume import utils as volutils @@ -43,7 +44,7 @@ CONF.register_opts(volume_opts) class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, driver.CloneableImageVD, driver.TransferVD): - VERSION = '2.1.0' + VERSION = '2.2.0' def __init__(self, *args, **kwargs): super(BlockDeviceDriver, self).__init__(*args, **kwargs) @@ -61,17 +62,27 @@ class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, def check_for_setup_error(self): pass + def _update_provider_location(self, object, device): + # We update provider_location and host to mark device as used to + # avoid race with other threads. + # TODO(ynesenenko): need to remove DB access from driver + object.update({'provider_location': device, 'host': self.host}) + object.save() + + @utils.synchronized('block_device', external=True) def create_volume(self, volume): - device = self.find_appropriate_size_device(volume['size']) - LOG.info(_LI("Create %(volume)s on %(device)s"), - {"volume": volume['name'], "device": device}) - return { - 'provider_location': device, - } + device = self.find_appropriate_size_device(volume.size) + LOG.info(_LI("Creating %(volume)s on %(device)s"), + {"volume": volume.name, "device": device}) + self._update_provider_location(volume, device) def delete_volume(self, volume): """Deletes a logical volume.""" - dev_path = self.local_path(volume) + self._clear_block_device(volume) + + def _clear_block_device(self, device): + """Deletes a block device.""" + dev_path = self.local_path(device) if not dev_path or dev_path not in \ self.configuration.available_devices: return @@ -82,10 +93,17 @@ class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, dev_size[dev_path], dev_path, volume_clear=self.configuration.volume_clear, volume_clear_size=self.configuration.volume_clear_size) + else: + LOG.warning(_LW("The device %s won't be cleared."), device) + + if device.status == "error_deleting": + msg = _("Failed to delete device.") + LOG.error(msg, resource=device) + raise exception.VolumeDriverException(msg) def local_path(self, device): - if device['provider_location']: - path = device['provider_location'].rsplit(" ", 1) + if device.provider_location: + path = device.provider_location.rsplit(" ", 1) return path[-1] else: return None @@ -97,7 +115,7 @@ class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, image_id, self.local_path(volume), self.configuration.volume_dd_blocksize, - size=volume['size']) + size=volume.size) def copy_volume_to_image(self, context, volume, image_service, image_meta): """Copy the volume to the specified image.""" @@ -106,18 +124,17 @@ class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, image_meta, self.local_path(volume)) + @utils.synchronized('block_device', external=True) def create_cloned_volume(self, volume, src_vref): - LOG.info(_LI('Creating clone of volume: %s'), src_vref['id']) - device = self.find_appropriate_size_device(src_vref['size']) + LOG.info(_LI('Creating clone of volume: %s.'), src_vref.id) + device = self.find_appropriate_size_device(src_vref.size) dev_size = self._get_devices_sizes([device]) volutils.copy_volume( self.local_path(src_vref), device, dev_size[device], self.configuration.volume_dd_blocksize, execute=self._execute) - return { - 'provider_location': device, - } + self._update_provider_location(volume, device) def get_volume_stats(self, refresh=False): if refresh: @@ -135,7 +152,7 @@ class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, free_size += size total_size += size - LOG.debug("Updating volume stats") + LOG.debug("Updating volume stats.") backend_name = self.configuration.safe_get('volume_backend_name') data = {'total_capacity_gb': total_size / units.Ki, 'free_capacity_gb': free_size / units.Ki, @@ -148,15 +165,21 @@ class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, self._stats = data - def _get_used_devices(self): - lst = api.volume_get_all_by_host(context.get_admin_context(), - self.host) - used_devices = set() - for volume in lst: - local_path = self.local_path(volume) + def _get_used_paths(self, lst): + used_dev = set() + for item in lst: + local_path = self.local_path(item) if local_path: - used_devices.add(local_path) - return used_devices + used_dev.add(local_path) + return used_dev + + def _get_used_devices(self): + lst = objects.VolumeList.get_all_by_host(context.get_admin_context(), + self.host) + used_devices = self._get_used_paths(lst) + snp_lst = objects.SnapshotList.get_by_host(context.get_admin_context(), + self.host) + return used_devices.union(self._get_used_paths(snp_lst)) def _get_devices_sizes(self, dev_paths): """Return devices' sizes in Mb""" @@ -231,7 +254,7 @@ class BlockDeviceDriver(driver.BaseVD, driver.LocalVD, self.target_driver.remove_export(context, volume) def initialize_connection(self, volume, connector): - if connector['host'] != volutils.extract_host(volume['host'], 'host'): + if connector['host'] != volutils.extract_host(volume.host, 'host'): return self.target_driver.initialize_connection(volume, connector) else: return { -- 2.45.2