Merge "Check result of retype action based on type and status"

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

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):

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')
+        cinder_util.cinder.volumes.get.return_value = volume
-        setattr(volume, 'migration_status', 'error')
+
+        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)