From 02163d64aa0de7c99455c351b124cec865a5cf98 Mon Sep 17 00:00:00 2001 From: Yumeng_Bao Date: Wed, 22 Nov 2017 17:08:22 +0800 Subject: [PATCH] bug fix remove volume migration type 'cold' MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Migration action 'cold' is not intuitive for the developers and users, so this patch replaces it with ‘migrate’ and 'retype'. Change-Id: I58acac741499f47e79630a6031d44088681e038a Closes-Bug: #1733247 --- watcher/applier/actions/volume_migration.py | 34 +++++++------------ watcher/common/cinder_helper.py | 17 ---------- .../applier/actions/test_volume_migration.py | 22 +++--------- watcher/tests/common/test_cinder_helper.py | 27 --------------- 4 files changed, 17 insertions(+), 83 deletions(-) diff --git a/watcher/applier/actions/volume_migration.py b/watcher/applier/actions/volume_migration.py index e19c60a47..6752e7e3c 100644 --- a/watcher/applier/actions/volume_migration.py +++ b/watcher/applier/actions/volume_migration.py @@ -36,13 +36,16 @@ class VolumeMigrate(base.BaseAction): By using this action, you will be able to migrate cinder volume. Migration type 'swap' can only be used for migrating attached volume. - Migration type 'cold' can only be used for migrating detached volume. + Migration type 'migrate' can be used for migrating detached volume to + the pool of same volume type. + Migration type 'retype' can be used for changing volume type of + detached volume. The action schema is:: schema = Schema({ 'resource_id': str, # should be a UUID - 'migration_type': str, # choices -> "swap", "cold" + 'migration_type': str, # choices -> "swap", "migrate","retype" 'destination_node': str, 'destination_type': str, }) @@ -60,7 +63,8 @@ class VolumeMigrate(base.BaseAction): MIGRATION_TYPE = 'migration_type' SWAP = 'swap' - COLD = 'cold' + RETYPE = 'retype' + MIGRATE = 'migrate' DESTINATION_NODE = "destination_node" DESTINATION_TYPE = "destination_type" @@ -85,7 +89,7 @@ class VolumeMigrate(base.BaseAction): }, 'migration_type': { 'type': 'string', - "enum": ["swap", "cold"] + "enum": ["swap", "retype", "migrate"] }, 'destination_node': { "anyof": [ @@ -127,20 +131,6 @@ class VolumeMigrate(base.BaseAction): def destination_type(self): return self.input_parameters.get(self.DESTINATION_TYPE) - def _cold_migrate(self, volume, dest_node, dest_type): - if not self.cinder_util.can_cold(volume, dest_node): - raise exception.Invalid( - message=(_("Invalid state for cold migration"))) - - if dest_node: - return self.cinder_util.migrate(volume, dest_node) - elif dest_type: - return self.cinder_util.retype(volume, dest_type) - else: - raise exception.Invalid( - message=(_("destination host or destination type is " - "required when migration type is cold"))) - def _can_swap(self, volume): """Judge volume can be swapped""" @@ -212,12 +202,14 @@ class VolumeMigrate(base.BaseAction): try: volume = self.cinder_util.get_volume(volume_id) - if self.migration_type == self.COLD: - return self._cold_migrate(volume, dest_node, dest_type) - elif self.migration_type == self.SWAP: + if self.migration_type == self.SWAP: if dest_node: LOG.warning("dest_node is ignored") return self._swap_volume(volume, dest_type) + elif self.migration_type == self.RETYPE: + return self.cinder_util.retype(volume, dest_type) + elif self.migration_type == self.MIGRATE: + return self.cinder_util.migrate(volume, dest_node) else: raise exception.Invalid( message=(_("Migration of type '%(migration_type)s' is not " diff --git a/watcher/common/cinder_helper.py b/watcher/common/cinder_helper.py index f43207a05..3a43489e0 100644 --- a/watcher/common/cinder_helper.py +++ b/watcher/common/cinder_helper.py @@ -111,23 +111,6 @@ class CinderHelper(object): return True return False - def can_cold(self, volume, host=None): - """Judge volume can be migrated""" - can_cold = False - status = self.get_volume(volume).status - snapshot = self._has_snapshot(volume) - - same_host = False - if host and getattr(volume, 'os-vol-host-attr:host') == host: - same_host = True - - if (status == 'available' and - snapshot is False and - same_host is False): - can_cold = True - - return can_cold - def get_deleting_volume(self, volume): volume = self.get_volume(volume) all_volume = self.get_volume_list() diff --git a/watcher/tests/applier/actions/test_volume_migration.py b/watcher/tests/applier/actions/test_volume_migration.py index 53e965b9d..e21fd6686 100644 --- a/watcher/tests/applier/actions/test_volume_migration.py +++ b/watcher/tests/applier/actions/test_volume_migration.py @@ -84,7 +84,7 @@ class TestMigration(base.TestCase): self.action_swap.input_parameters = self.input_parameters_swap self.input_parameters_migrate = { - "migration_type": "cold", + "migration_type": "migrate", "destination_node": "storage1-poolname", "destination_type": "", baction.BaseAction.RESOURCE_ID: self.VOLUME_UUID, @@ -93,7 +93,7 @@ class TestMigration(base.TestCase): self.action_migrate.input_parameters = self.input_parameters_migrate self.input_parameters_retype = { - "migration_type": "cold", + "migration_type": "retype", "destination_node": "", "destination_type": "storage1-typename", baction.BaseAction.RESOURCE_ID: self.VOLUME_UUID, @@ -130,7 +130,7 @@ class TestMigration(base.TestCase): def test_parameters_migrate(self): params = {baction.BaseAction.RESOURCE_ID: self.VOLUME_UUID, - self.action.MIGRATION_TYPE: 'cold', + self.action.MIGRATION_TYPE: 'migrate', self.action.DESTINATION_NODE: 'node-1', self.action.DESTINATION_TYPE: None} self.action_migrate.input_parameters = params @@ -139,7 +139,7 @@ class TestMigration(base.TestCase): def test_parameters_retype(self): params = {baction.BaseAction.RESOURCE_ID: self.VOLUME_UUID, - self.action.MIGRATION_TYPE: 'cold', + self.action.MIGRATION_TYPE: 'retype', self.action.DESTINATION_NODE: None, self.action.DESTINATION_TYPE: 'type-1'} self.action_retype.input_parameters = params @@ -157,7 +157,6 @@ class TestMigration(base.TestCase): def test_migrate_success(self): volume = self.fake_volume() - self.m_c_helper.can_cold.return_value = True self.m_c_helper.get_volume.return_value = volume result = self.action_migrate.execute() self.assertTrue(result) @@ -166,16 +165,9 @@ class TestMigration(base.TestCase): "storage1-poolname" ) - def test_migrate_fail(self): - self.m_c_helper.can_cold.return_value = False - result = self.action_migrate.execute() - self.assertFalse(result) - self.m_c_helper.migrate.assert_not_called() - def test_retype_success(self): volume = self.fake_volume() - self.m_c_helper.can_cold.return_value = True self.m_c_helper.get_volume.return_value = volume result = self.action_retype.execute() self.assertTrue(result) @@ -184,12 +176,6 @@ class TestMigration(base.TestCase): "storage1-typename", ) - def test_retype_fail(self): - self.m_c_helper.can_cold.return_value = False - result = self.action_migrate.execute() - self.assertFalse(result) - self.m_c_helper.migrate.assert_not_called() - def test_swap_success(self): volume = self.fake_volume( status='in-use', attachments=[{'server_id': 'server_id'}]) diff --git a/watcher/tests/common/test_cinder_helper.py b/watcher/tests/common/test_cinder_helper.py index 872307466..3892c44a1 100644 --- a/watcher/tests/common/test_cinder_helper.py +++ b/watcher/tests/common/test_cinder_helper.py @@ -136,33 +136,6 @@ class TestCinderHelper(base.TestCase): volume.volume_type = kwargs.get('volume_type', 'fake_type') return volume - def test_can_cold_success(self, mock_cinder): - cinder_util = cinder_helper.CinderHelper() - - volume = self.fake_volume() - cinder_util.cinder.volumes.get.return_value = volume - result = cinder_util.can_cold(volume) - self.assertTrue(result) - - def test_can_cold_fail(self, mock_cinder): - cinder_util = cinder_helper.CinderHelper() - - volume = self.fake_volume(status='in-use') - cinder_util.cinder.volumes.get.return_value = volume - result = cinder_util.can_cold(volume) - self.assertFalse(result) - - volume = self.fake_volume(snapshot_id='snapshot_id') - cinder_util.cinder.volumes.get.return_value = volume - result = cinder_util.can_cold(volume) - self.assertFalse(result) - - volume = self.fake_volume() - setattr(volume, 'os-vol-host-attr:host', 'host@backend#pool') - cinder_util.cinder.volumes.get.return_value = volume - result = cinder_util.can_cold(volume, 'host@backend#pool') - self.assertFalse(result) - @mock.patch.object(time, 'sleep', mock.Mock()) def test_migrate_success(self, mock_cinder):