From fe56660c444216bf235b2404ddeeba5b91c9cb8c Mon Sep 17 00:00:00 2001 From: jgilaber Date: Fri, 16 May 2025 13:46:57 +0200 Subject: [PATCH] Handle missing dst_pool parameter in zone_migration Unlike Nova, Cinder does not support calling the 'os-migrate_volume'[1] action without a host or a cluster. For volume migrations of type 'migrate' in watcher the dst_pool is required, but for other migrations that migrate the volumes to different types is not needed. This change checks if the dst_pool is defined and prevents some migrations when it's misssing information. Adds testing for creating audits with the Zone Migration status, validating the schema changes. [1] https://docs.openstack.org/api-ref/block-storage/v3/index.html#migrate-a-volume Closes-Bug: 2108988 Change-Id: I305c58e47093c4a884e86f1d91fdc15ef2a1cfba Signed-off-by: jgilaber --- doc/source/strategies/zone_migration.rst | 3 + ...ion-missing-dst-node-bd0377af1f1ed245.yaml | 5 + .../strategy/strategies/zone_migration.py | 7 + watcher/tests/api/v1/test_audits.py | 190 ++++++++++++++---- .../strategies/test_zone_migration.py | 40 +++- 5 files changed, 198 insertions(+), 47 deletions(-) diff --git a/doc/source/strategies/zone_migration.rst b/doc/source/strategies/zone_migration.rst index 67f67cf75..c29b3381f 100644 --- a/doc/source/strategies/zone_migration.rst +++ b/doc/source/strategies/zone_migration.rst @@ -112,6 +112,9 @@ parameter type default required description instances migrate. ``dst_node`` string None Optional Compute node to which instances migrate. + If omitted, nova will + choose the destination + node automatically. ============= ======= ======== ========= ======================== The elements of storage_pools array are: diff --git a/releasenotes/notes/zone-migration-missing-dst-node-bd0377af1f1ed245.yaml b/releasenotes/notes/zone-migration-missing-dst-node-bd0377af1f1ed245.yaml index 0cfaf4d9b..12e29d11a 100644 --- a/releasenotes/notes/zone-migration-missing-dst-node-bd0377af1f1ed245.yaml +++ b/releasenotes/notes/zone-migration-missing-dst-node-bd0377af1f1ed245.yaml @@ -6,4 +6,9 @@ fixes: This brings the implementation of the strategy in line with the the api schema where dest_node is optional. + If a dst_pool is not passed, the strategy will not migrate some volumes, as Cinder can't compute a + destination host when migrating available volumes like Nova does. If src_type and dst_type are equal, + a migration is only performed if a dst_pool is provided, otherwise the volume will be skipped/ignored. + If src_type and dst_type are different, the strategy will retype the volumes. + See: https://bugs.launchpad.net/watcher/+bug/2108988 for more details. diff --git a/watcher/decision_engine/strategy/strategies/zone_migration.py b/watcher/decision_engine/strategy/strategies/zone_migration.py index e8876754a..fbd6e4aef 100644 --- a/watcher/decision_engine/strategy/strategies/zone_migration.py +++ b/watcher/decision_engine/strategy/strategies/zone_migration.py @@ -412,6 +412,13 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy): LOG.debug("%s %s", dst_pool, dst_type) if src_type == dst_type: + if dst_pool is None: + LOG.warning("volume %s will not be migrated because " + "it already has type %s and 'dst_pool'" + " was not specified for src_pool %s", + volume.name, dst_type, + pool) + continue self._volume_migrate(volume, dst_pool) else: self._volume_retype(volume, dst_type) diff --git a/watcher/tests/api/v1/test_audits.py b/watcher/tests/api/v1/test_audits.py index c754c4918..7627f3e4a 100644 --- a/watcher/tests/api/v1/test_audits.py +++ b/watcher/tests/api/v1/test_audits.py @@ -26,6 +26,7 @@ from watcher.api.controllers.v1 import audit as api_audit from watcher.common import utils from watcher.db import api as db_api from watcher.decision_engine import rpcapi as deapi +from watcher.decision_engine.strategy import strategies from watcher import objects from watcher.tests.api import base as api_base from watcher.tests.api import utils as api_utils @@ -534,10 +535,14 @@ class TestPatchStateTransitionOk(api_base.FunctionalTest): self.assertEqual(test_time, return_updated_at) -class TestPost(api_base.FunctionalTest): +class TestPostBase(api_base.FunctionalTest): + + def _simulate_rpc_audit_create(self, audit): + audit.create() + return audit def setUp(self): - super(TestPost, self).setUp() + super(TestPostBase, self).setUp() obj_utils.create_test_goal(self.context) obj_utils.create_test_strategy(self.context) obj_utils.create_test_audit_template(self.context) @@ -547,9 +552,43 @@ class TestPost(api_base.FunctionalTest): self._simulate_rpc_audit_create) self.addCleanup(p.stop) - def _simulate_rpc_audit_create(self, audit): - audit.create() - return audit + def prepare_audit_template_strategy_with_parameter(self, fake_spec=None): + if fake_spec is None: + fake_spec = { + "properties": { + "fake1": { + "description": "number parameter example", + "type": "number", + "minimum": 1.0, + "maximum": 10.2, + } + }, + 'required': ['fake1'] + } + template_uuid = 'e74c40e0-d825-11e2-a28f-0800200c9a67' + strategy_uuid = 'e74c40e0-d825-11e2-a28f-0800200c9a68' + template_name = 'my template' + strategy_name = 'my strategy' + strategy_id = 3 + strategy = db_utils.get_test_strategy(parameters_spec=fake_spec, + id=strategy_id, + uuid=strategy_uuid, + name=strategy_name) + obj_utils.create_test_strategy(self.context, + parameters_spec=fake_spec, + id=strategy_id, + uuid=strategy_uuid, + name=strategy_name) + obj_utils.create_test_audit_template(self.context, + strategy_id=strategy_id, + uuid=template_uuid, + name='name') + audit_template = db_utils.get_test_audit_template( + strategy_id=strategy['id'], uuid=template_uuid, name=template_name) + return audit_template + + +class TestPost(TestPostBase): @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') @mock.patch('oslo_utils.timeutils.utcnow') @@ -912,40 +951,6 @@ class TestPost(api_base.FunctionalTest): self.assertIn(expected_error_msg, response.json['error_message']) assert not mock_trigger_audit.called - def prepare_audit_template_strategy_with_parameter(self): - fake_spec = { - "properties": { - "fake1": { - "description": "number parameter example", - "type": "number", - "minimum": 1.0, - "maximum": 10.2, - } - }, - 'required': ['fake1'] - } - template_uuid = 'e74c40e0-d825-11e2-a28f-0800200c9a67' - strategy_uuid = 'e74c40e0-d825-11e2-a28f-0800200c9a68' - template_name = 'my template' - strategy_name = 'my strategy' - strategy_id = 3 - strategy = db_utils.get_test_strategy(parameters_spec=fake_spec, - id=strategy_id, - uuid=strategy_uuid, - name=strategy_name) - obj_utils.create_test_strategy(self.context, - parameters_spec=fake_spec, - id=strategy_id, - uuid=strategy_uuid, - name=strategy_name) - obj_utils.create_test_audit_template(self.context, - strategy_id=strategy_id, - uuid=template_uuid, - name='name') - audit_template = db_utils.get_test_audit_template( - strategy_id=strategy['id'], uuid=template_uuid, name=template_name) - return audit_template - @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') @mock.patch('oslo_utils.timeutils.utcnow') def test_create_audit_with_name(self, mock_utcnow, mock_trigger_audit): @@ -1228,3 +1233,110 @@ class TestAuditEnforcementWithAdminContext(TestListAudit, "audit:get": "rule:default", "audit:get_all": "rule:default", "audit:update": "rule:default"}) + + +class TestAuditZoneMigration(TestPostBase): + def setUp(self): + super(TestAuditZoneMigration, self).setUp() + + # create strategy having the same spec as Zone Migration + self.zm_spec = strategies.ZoneMigration.get_schema() + + def _prepare_audit_params(self, parameters): + audit_templ = self.prepare_audit_template_strategy_with_parameter( + fake_spec=self.zm_spec + ) + audit_dict = api_utils.audit_post_data(parameters=parameters) + audit_dict['audit_template_uuid'] = audit_templ['uuid'] + del_keys = ['uuid', 'goal_id', 'strategy_id', 'state', + 'interval', 'scope', 'next_run_time', 'hostname'] + for k in del_keys: + del audit_dict[k] + return audit_dict + + @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') + def test_create_audit_zone_migration_without_dst_pool(self, + mock_trigger_audit): + mock_trigger_audit.return_value = mock.ANY + zm_params = { + 'storage_pools': [ + {"src_pool": "src_pool_name", + "src_type": "src_type_name", + "dst_type": "dst_type_name"} + ] + } + + audit_input_dict = self._prepare_audit_params(zm_params) + + response = self.post_json('/audits', audit_input_dict) + self.assertEqual("application/json", response.content_type) + self.assertEqual(HTTPStatus.CREATED, response.status_int) + + @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') + def test_create_audit_zone_migration_without_src_pool(self, + mock_trigger_audit): + mock_trigger_audit.return_value = mock.ANY + zm_params = { + 'storage_pools': [ + {"dst_pool": "dst_pool_name", + "src_type": "src_type_name", + "dst_type": "dst_type_name"} + ] + } + + audit_input_dict = self._prepare_audit_params(zm_params) + + response = self.post_json('/audits', audit_input_dict, + expect_errors=True) + self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) + self.assertEqual("application/json", response.content_type) + expected_error_msg = ("'src_pool' is a required property") + self.assertTrue(response.json['error_message']) + self.assertIn(expected_error_msg, response.json['error_message']) + assert not mock_trigger_audit.called + + @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') + def test_create_audit_zone_migration_without_dst_type(self, + mock_trigger_audit): + mock_trigger_audit.return_value = mock.ANY + zm_params = { + 'storage_pools': [ + {"src_pool": "src_pool_name", + "src_type": "src_type_name", + "dst_pool": "dst_pool_name"} + ] + } + + audit_input_dict = self._prepare_audit_params(zm_params) + + response = self.post_json('/audits', audit_input_dict, + expect_errors=True) + self.assertEqual("application/json", response.content_type) + self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) + expected_error_msg = ("'dst_type' is a required property") + self.assertTrue(response.json['error_message']) + self.assertIn(expected_error_msg, response.json['error_message']) + assert not mock_trigger_audit.called + + @mock.patch.object(deapi.DecisionEngineAPI, 'trigger_audit') + def test_create_audit_zone_migration_without_src_type(self, + mock_trigger_audit): + mock_trigger_audit.return_value = mock.ANY + zm_params = { + 'storage_pools': [ + {"dst_pool": "dst_pool_name", + "src_pool": "src_pool_name", + "dst_type": "dst_type_name"} + ] + } + + audit_input_dict = self._prepare_audit_params(zm_params) + + response = self.post_json('/audits', audit_input_dict, + expect_errors=True) + self.assertEqual("application/json", response.content_type) + self.assertEqual(HTTPStatus.BAD_REQUEST, response.status_int) + expected_error_msg = ("'src_type' is a required property") + self.assertTrue(response.json['error_message']) + self.assertIn(expected_error_msg, response.json['error_message']) + assert not mock_trigger_audit.called diff --git a/watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py b/watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py index c02b21cd1..fee9d8d22 100644 --- a/watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py +++ b/watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py @@ -316,20 +316,44 @@ class TestZoneMigration(TestBaseStrategy): name="volume_1") self.m_c_helper.get_volume_list.return_value = [ volume_on_src1, + ] self.m_migrate_storage_pools.return_value = [ {"src_pool": "src1@back1#pool1", - "src_type": "type1", "dst_type": "type1"}, + "src_type": "type1", + "dst_type": "type1"}, + ] self.m_n_helper.get_instance_list.return_value = [] solution = self.strategy.execute() - migration_params = solution.actions[0]['input_parameters'] - # since we have not passed 'dst_pool' in the input, we should not have - # a destination_node in the generated migration action - # self.assertNotIn('destination_node', migration_params) - # temporarily make the test pass, delete and use the above assert in - # followup - self.assertIsNone(migration_params['destination_node']) + # check that there are no volume migrations proposed, because the input + # parameters do not have a dst_pool and dst_type==src_type + self.assertEqual(len(solution.actions), 0) + + def test_execute_migrate_volume_dst_pool(self): + volume_on_src1 = self.fake_volume(host="src1@back1#pool1", + id=volume_uuid_mapping["volume_1"], + name="volume_1") + self.m_c_helper.get_volume_list.return_value = [ + volume_on_src1, + + ] + self.m_migrate_storage_pools.return_value = [ + {"src_pool": "src1@back1#pool1", + "src_type": "type1", + "dst_pool": "back2", + "dst_type": "type1"}, + + ] + self.m_n_helper.get_instance_list.return_value = [] + solution = self.strategy.execute() + # check that there is one volume migration proposed + migration_types = collections.Counter( + [action.get('input_parameters')['migration_type'] + for action in solution.actions]) + self.assertEqual(1, migration_types.get("migrate", 0)) + global_efficacy_value = solution.global_efficacy[2].get('value', 0) + self.assertEqual(100, global_efficacy_value) def test_execute_migrate_volume_no_compute_nodes(self): instance_on_src1 = self.fake_instance(