From 058700edc93a5815bedc8c7f53cccd672618019c Mon Sep 17 00:00:00 2001 From: Alan Bishop Date: Fri, 23 Aug 2019 10:44:08 -0700 Subject: [PATCH] Fix NFS volume retype with migrate When migrating a volume to an NFS backend, don't rename the associated volume file if the original source volume was on the same backend (same provider_location). Volume migration always deletes the source volume, so the volume file should *not* be renamed to the original. Closes-Bug: #1798468 Change-Id: Iadf537521af93bbd28452a54e0e70e7ed18f606c (cherry picked from commit 33a3a4bf6fe72b093702922e2e5f102d06249375) (cherry picked from commit 6f3bec3c64e27f7daac830e62de935b2d5ffa20f) --- cinder/tests/unit/volume/drivers/test_nfs.py | 131 +++++++++---------- cinder/volume/drivers/nfs.py | 3 +- 2 files changed, 64 insertions(+), 70 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/test_nfs.py b/cinder/tests/unit/volume/drivers/test_nfs.py index 20ed172b424..a942eeede88 100644 --- a/cinder/tests/unit/volume/drivers/test_nfs.py +++ b/cinder/tests/unit/volume/drivers/test_nfs.py @@ -28,6 +28,7 @@ from cinder import context from cinder import exception from cinder.image import image_utils from cinder import test +from cinder.tests.unit import fake_constants as fake from cinder.tests.unit import fake_snapshot from cinder.tests.unit import fake_volume from cinder.volume import configuration as conf @@ -1385,6 +1386,67 @@ class NfsDriverTestCase(test.TestCase): mock_write_info_file.assert_called_with( info_path, {'active': snap_file, fake_snap.id: snap_file}) + @ddt.data({'volume_status': 'available', + 'original_provider': 'original_provider', + 'rename_side_effect': None}, + {'volume_status': 'available', + 'original_provider': 'current_provider', + 'rename_side_effect': None}, + {'volume_status': 'in-use', + 'original_provider': 'original_provider', + 'rename_side_effect': None}, + {'volume_status': 'available', + 'original_provider': 'original_provider', + 'rename_side_effect': OSError}) + @ddt.unpack + @mock.patch('os.rename') + def test_update_migrated_volume(self, + mock_rename, + rename_side_effect, + original_provider, + volume_status): + drv = nfs.NfsDriver(configuration=self.configuration) + base_dir = '/dir_base/' + current_path = base_dir + fake.VOLUME2_NAME + current_provider = 'current_provider' + mock_rename.side_effect = rename_side_effect + + volume = fake_volume.fake_volume_obj( + self.context, + id=fake.VOLUME_ID, + provider_location=original_provider, + _name_id=None) + + new_volume = fake_volume.fake_volume_obj( + self.context, + id=fake.VOLUME2_ID, + provider_location=current_provider, + _name_id=None) + + with mock.patch.object(drv, 'local_path') as local_path: + local_path.return_value = current_path + update = drv.update_migrated_volume(self.context, + volume, + new_volume, + volume_status) + + if (volume_status == 'available' and + original_provider != current_provider): + original_path = base_dir + fake.VOLUME_NAME + mock_rename.assert_called_once_with(current_path, + original_path) + else: + mock_rename.assert_not_called() + + if mock_rename.call_count > 0 and rename_side_effect is None: + self.assertEqual({'_name_id': None, + 'provider_location': current_provider}, + update) + else: + self.assertEqual({'_name_id': fake.VOLUME2_ID, + 'provider_location': current_provider}, + update) + class NfsDriverDoSetupTestCase(test.TestCase): @@ -1539,75 +1601,6 @@ class NfsDriverDoSetupTestCase(test.TestCase): check_exit_code=False, run_as_root=True)]) - @mock.patch.object(os, 'rename') - def test_update_migrated_available_volume(self, rename_volume): - self._test_update_migrated_volume('available', rename_volume) - - @mock.patch.object(os, 'rename') - def test_update_migrated_available_volume_rename_fail(self, rename_volume): - self._test_update_migrated_volume('available', rename_volume, - rename_exception=True) - - @mock.patch.object(os, 'rename') - def test_update_migrated_in_use_volume(self, rename_volume): - self._test_update_migrated_volume('in-use', rename_volume) - - def _test_update_migrated_volume(self, volume_status, rename_volume, - rename_exception=False): - drv = nfs.NfsDriver(configuration=self.configuration) - fake_volume_id = 'f51b5730-13b7-11e6-a238-fa163e67a298' - fake_new_volume_id = '12341234-13b7-11e6-a238-fa163e67a298' - fake_provider_source = 'fake_provider_source' - fake_provider = 'fake_provider' - base_dir = '/dir_base/' - volume_name_template = 'volume-%s' - original_volume_name = volume_name_template % fake_volume_id - current_name = volume_name_template % fake_new_volume_id - original_volume_path = base_dir + original_volume_name - current_path = base_dir + current_name - volume = fake_volume.fake_volume_obj( - self.context, - id=fake_volume_id, - size=1, - provider_location=fake_provider_source, - _name_id=None) - - new_volume = fake_volume.fake_volume_obj( - self.context, - id=fake_new_volume_id, - size=1, - provider_location=fake_provider, - _name_id=None) - - with mock.patch.object(drv, 'local_path') as local_path: - local_path.return_value = base_dir + current_name - if volume_status == 'in-use': - update = drv.update_migrated_volume(self.context, - volume, - new_volume, - volume_status) - self.assertEqual({'_name_id': fake_new_volume_id, - 'provider_location': fake_provider}, update) - elif rename_exception: - rename_volume.side_effect = OSError - update = drv.update_migrated_volume(self.context, - volume, - new_volume, - volume_status) - rename_volume.assert_called_once_with(current_path, - original_volume_path) - self.assertEqual({'_name_id': fake_new_volume_id, - 'provider_location': fake_provider}, update) - else: - update = drv.update_migrated_volume(self.context, - volume, - new_volume, - volume_status) - rename_volume.assert_called_once_with(current_path, - original_volume_path) - self.assertEqual({'_name_id': None, - 'provider_location': fake_provider}, update) - def test_retype_is_there(self): "Ensure that driver.retype() is there.""" diff --git a/cinder/volume/drivers/nfs.py b/cinder/volume/drivers/nfs.py index 0d57c3b9f2f..15b1fadfc17 100644 --- a/cinder/volume/drivers/nfs.py +++ b/cinder/volume/drivers/nfs.py @@ -457,7 +457,8 @@ class NfsDriver(remotefs.RemoteFSSnapDriverDistributed): :returns: model_update to update DB with any needed changes """ name_id = None - if original_volume_status == 'available': + if (original_volume_status == 'available' and + volume.provider_location != new_volume.provider_location): current_name = CONF.volume_name_template % new_volume.id original_volume_name = CONF.volume_name_template % volume.id current_path = self.local_path(new_volume)