openstack/watcher
Code Issues Proposed changes

Check result of retype action based on type and status

Browse Source 
Currently, when there is a volume_migrate action and migration_type is
`retype`, watcher assumes that the retype always triggers a migration
and checks the result of the retype based on the fields related to
the migration action (actually, it uses the same function to check the
result when `migration_type` is `retype` or `migrate`. This creates
problem in different scenarios:

- Actions keep in ONGOING status forever for volumes which have never
  being migrated as the migration fields of the volume are empty.
- Actions which were migrated anytime before, still have the old values
  so it may report the status of te retype actions wrongly.

This patch is implementing an entirely new function to check the result
of a retype action based on the final type and the status field of the
volume. This should be valid for any kind of retype action, with or
without migration. The criteria for successfull retype is that the type
for the volume is the destination one in the action and the status is
available or in-use.

Closes-Bug: #2112100

Change-Id: I76e91ed99e7a814a43a6dd906b6bcc150d471624
Signed-off-by: jgilaber <jgilaber@redhat.com>
This commit is contained in:
Alfredo Moralejo
2025-05-30 13:24:36 +02:00
committed by jgilaber
parent 59757249bb
commit 90009aac84
3 changed files with 230 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

11
releasenotes/notes/bug-2112100-c1e56173cd29a35e.yaml Normal file
View File

@@ -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

39
watcher/common/cinder_helper.py
View File

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

184
watcher/tests/common/test_cinder_helper.py
View File

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