diff --git a/releasenotes/notes/bug-2112100-c1e56173cd29a35e.yaml b/releasenotes/notes/bug-2112100-c1e56173cd29a35e.yaml new file mode 100644 index 000000000..66505c9f5 --- /dev/null +++ b/releasenotes/notes/bug-2112100-c1e56173cd29a35e.yaml @@ -0,0 +1,11 @@ +--- +fixes: + - | + Currently, when Watcher applies a `volume_migrate` action with value + `retype` for the `migratione_type`, it can wrongly report the result of + the action when the retype does not trigger a volume migration. + + This patch fixes the logic to validate the resulting state of the action + and reports it correctly. + + For more details: https://bugs.launchpad.net/watcher/+bug/2112100 diff --git a/watcher/common/cinder_helper.py b/watcher/common/cinder_helper.py index a686cd67d..c00eaaad5 100644 --- a/watcher/common/cinder_helper.py +++ b/watcher/common/cinder_helper.py @@ -184,6 +184,43 @@ class CinderHelper(object): {'volume': volume.id, 'host': host_name}) return True + def check_retyped(self, volume, dst_type, retry_interval=10): + volume = self.get_volume(volume) + valid_status = ('available', 'in-use') + # A volume retype is correct when the type is the dst_type + # and the status is available or in-use. Otherwise, it is + # in retyping status or the action failed + while (volume.volume_type != dst_type or + volume.status not in valid_status): + # Retype is not finished successfully, checking if the + # retype is still ongoing or failed. If status is not + # `retyping` it means something went wrong. + if volume.status != 'retyping': + LOG.error( + "Volume retype failed : " + "volume %(volume)s has now type '%(type)s' and " + "status %(status)s", + {'volume': volume.id, 'type': volume.volume_type, + 'status': volume.status}) + # If migration_status is in error, a likely reason why the + # retype failed is some problem in the migration. Report it in + # the logs if migration_status is error. + if volume.migration_status == 'error': + LOG.error("Volume migration error on volume %(volume)s.", + {'volume': volume.id}) + return False + + LOG.debug('Waiting the retype of %s', volume) + time.sleep(retry_interval) + volume = self.get_volume(volume.id) + + LOG.debug( + "Volume retype succeeded : " + "volume %(volume)s has now type '%(type)s'.", + {'volume': volume.id, 'type': dst_type}) + + return True + def migrate(self, volume, dest_node): """Migrate volume to dest_node""" volume = self.get_volume(volume) @@ -217,7 +254,7 @@ class CinderHelper(object): self.cinder.volumes.retype( volume, dest_type, "on-demand") - return self.check_migrated(volume) + return self.check_retyped(volume, dest_type) def create_volume(self, cinder, volume, dest_type, retry=120, retry_interval=10): diff --git a/watcher/tests/common/test_cinder_helper.py b/watcher/tests/common/test_cinder_helper.py index 5334d08f2..a9a11784f 100644 --- a/watcher/tests/common/test_cinder_helper.py +++ b/watcher/tests/common/test_cinder_helper.py @@ -13,6 +13,8 @@ # limitations under the License. # +import copy + from unittest import mock from http import HTTPStatus @@ -139,6 +141,8 @@ class TestCinderHelper(base.TestCase): volume.snapshot_id = kwargs.get('snapshot_id', None) volume.availability_zone = kwargs.get('availability_zone', 'nova') volume.volume_type = kwargs.get('volume_type', 'fake_type') + volume.migration_status = kwargs.get('migration_status') + volume.os_vol_host_attr_host = kwargs.get('os_vol_host_attr_host') return volume @mock.patch.object(time, 'sleep', mock.Mock()) @@ -186,12 +190,26 @@ class TestCinderHelper(base.TestCase): self.assertFalse(result) @mock.patch.object(time, 'sleep', mock.Mock()) - def test_retype_success(self, mock_cinder): + @mock.patch.object(cinder_helper.CinderHelper, 'get_volume') + def test_retype_success(self, mock_get_volume, mock_cinder): cinder_util = cinder_helper.CinderHelper() volume = self.fake_volume() setattr(volume, 'os-vol-host-attr:host', 'source_node') setattr(volume, 'migration_status', 'success') + + def side_effect(volume, status, volume_type): + result_volume = copy.deepcopy(volume) + result_volume.status = status + result_volume.volume_type = volume_type + return result_volume + + mock_get_volume.side_effect = [ + side_effect(volume, 'in-use', 'fake_type'), + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'in-use', 'notfake_type'), + side_effect(volume, 'in-use', 'notfake_type'), + ] cinder_util.cinder.volumes.get.return_value = volume result = cinder_util.retype(volume, 'notfake_type') @@ -201,6 +219,7 @@ class TestCinderHelper(base.TestCase): def test_retype_fail(self, mock_cinder): cinder_util = cinder_helper.CinderHelper() + # dest_type is the actual one volume = self.fake_volume() setattr(volume, 'os-vol-host-attr:host', 'source_node') setattr(volume, 'migration_status', 'success') @@ -211,9 +230,15 @@ class TestCinderHelper(base.TestCase): "Volume type must be different for retyping", cinder_util.retype, volume, 'fake_type') + # type is not the expected one volume = self.fake_volume() - setattr(volume, 'os-vol-host-attr:host', 'source_node') - setattr(volume, 'migration_status', 'error') + cinder_util.cinder.volumes.get.return_value = volume + + result = cinder_util.retype(volume, 'notfake_type') + self.assertFalse(result) + + # type is correct but status is error + volume = self.fake_volume(status='error') cinder_util.cinder.volumes.get.return_value = volume result = cinder_util.retype(volume, 'notfake_type') @@ -429,3 +454,156 @@ class TestCinderHelper(base.TestCase): cinder_util.get_deleting_volume = mock.MagicMock(return_value=volume) result = cinder_util.check_migrated(volume, retry_interval=1) self.assertFalse(result) + + @mock.patch.object(time, 'sleep', mock.Mock()) + @mock.patch.object(cinder_helper.LOG, 'debug') + def test_check_retyped_success_immediate(self, mock_log_debug, + mock_cinder): + + cinder_util = cinder_helper.CinderHelper() + + volume = self.fake_volume(status='in-use', volume_type='dest_type') + cinder_util.cinder.volumes.get.return_value = volume + cinder_util.check_volume_deleted = mock.MagicMock(return_value=True) + result = cinder_util.check_retyped(volume, 'dest_type') + self.assertNotIn(mock.call('Waiting the retype of %s', + volume), mock_log_debug.mock_calls) + mock_log_debug.assert_called_with( + "Volume retype succeeded : volume %(volume)s has now type " + "'%(type)s'.", {'volume': volume.id, 'type': 'dest_type'}) + self.assertTrue(result) + + @mock.patch.object(time, 'sleep', mock.Mock()) + @mock.patch.object(cinder_helper.CinderHelper, 'get_volume') + @mock.patch.object(cinder_helper.LOG, 'debug') + def test_check_retyped_success_retries(self, mock_log_debug, + mock_get_volume, mock_cinder): + + cinder_util = cinder_helper.CinderHelper() + volume = self.fake_volume(status='in-use') + + def side_effect(volume, status, volume_type): + result_volume = copy.deepcopy(volume) + result_volume.status = status + result_volume.volume_type = volume_type + return result_volume + + mock_get_volume.side_effect = [ + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'in-use', 'dest_type'), + ] + + cinder_util.cinder.volumes.get.return_value = volume + cinder_util.check_volume_deleted = mock.MagicMock(return_value=True) + result = cinder_util.check_retyped(volume, 'dest_type') + self.assertEqual(mock_get_volume.call_count, 3) + mock_log_debug.assert_any_call('Waiting the retype of %s', volume) + mock_log_debug.assert_called_with( + "Volume retype succeeded : volume %(volume)s has now type " + "'%(type)s'.", {'volume': volume.id, 'type': 'dest_type'}) + self.assertTrue(result) + + @mock.patch.object(time, 'sleep', mock.Mock()) + @mock.patch.object(cinder_helper.CinderHelper, 'get_volume') + @mock.patch.object(cinder_helper.LOG, 'debug') + def test_check_retyped_success_retries_migration( + self, mock_log_debug, mock_get_volume, mock_cinder): + + cinder_util = cinder_helper.CinderHelper() + volume = self.fake_volume(status='in-use', migration_status='success', + os_vol_host_attr_host='source_node') + + def side_effect(volume, status, volume_type): + result_volume = copy.deepcopy(volume) + result_volume.status = status + result_volume.volume_type = volume_type + return result_volume + + mock_get_volume.side_effect = [ + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'in-use', 'dest_type'), + ] + + cinder_util.cinder.volumes.get.return_value = volume + cinder_util.check_volume_deleted = mock.MagicMock(return_value=True) + cinder_util.get_deleting_volume = mock.MagicMock(return_value=volume) + result = cinder_util.check_retyped(volume, 'dest_type') + self.assertEqual(mock_get_volume.call_count, 3) + mock_log_debug.assert_any_call('Waiting the retype of %s', volume) + mock_log_debug.assert_called_with( + "Volume retype succeeded : volume %(volume)s has now type " + "'%(type)s'.", {'volume': volume.id, 'type': 'dest_type'}) + self.assertTrue(result) + + @mock.patch.object(time, 'sleep', mock.Mock()) + @mock.patch.object(cinder_helper.CinderHelper, 'get_volume') + @mock.patch.object(cinder_helper.LOG, 'debug') + @mock.patch.object(cinder_helper.LOG, 'error') + def test_check_retyped_failed_available( + self, mock_log_error, mock_log_debug, mock_get_volume, + mock_cinder): + + cinder_util = cinder_helper.CinderHelper() + volume = self.fake_volume(status='available') + + def side_effect(volume, status, volume_type): + result_volume = copy.deepcopy(volume) + result_volume.status = status + result_volume.volume_type = volume_type + return result_volume + + mock_get_volume.side_effect = [ + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'available', 'fake_type'), + ] + + cinder_util.cinder.volumes.get.return_value = volume + result = cinder_util.check_retyped( + volume, 'dest_type', retry_interval=1) + self.assertEqual(mock_get_volume.call_count, 3) + mock_log_debug.assert_any_call('Waiting the retype of %s', volume) + mock_log_error.assert_called_with( + "Volume retype failed : volume %(volume)s has now type " + "'%(type)s' and status %(status)s", + {'volume': volume.id, 'type': 'fake_type', 'status': 'available'}) + self.assertFalse(result) + + @mock.patch.object(time, 'sleep', mock.Mock()) + @mock.patch.object(cinder_helper.CinderHelper, 'get_volume') + @mock.patch.object(cinder_helper.LOG, 'debug') + @mock.patch.object(cinder_helper.LOG, 'error') + def test_check_retyped_failed_inuse(self, mock_log_error, mock_log_debug, + mock_get_volume, mock_cinder): + + cinder_util = cinder_helper.CinderHelper() + volume = self.fake_volume(status='in-use', migration_status='error') + + def side_effect(volume, status, volume_type): + result_volume = copy.deepcopy(volume) + result_volume.status = status + result_volume.volume_type = volume_type + return result_volume + + mock_get_volume.side_effect = [ + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'retyping', 'fake_type'), + side_effect(volume, 'in-use', 'fake_type'), + ] + + cinder_util.cinder.volumes.get.return_value = volume + result = cinder_util.check_retyped( + volume, 'dest_type', retry_interval=1) + self.assertEqual(mock_get_volume.call_count, 4) + mock_log_debug.assert_any_call('Waiting the retype of %s', volume) + mock_log_error.assert_any_call( + "Volume retype failed : volume %(volume)s has now type " + "'%(type)s' and status %(status)s", + {'volume': volume.id, 'type': 'fake_type', 'status': 'in-use'}) + mock_log_error.assert_called_with( + "Volume migration error on volume %(volume)s.", + {'volume': volume.id}) + self.assertFalse(result)