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 <jgilaber@redhat.com>

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:

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.

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)

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):
+    def prepare_audit_template_strategy_with_parameter(self, fake_spec=None):
-        audit.create()
+        if fake_spec is None:
-        return audit
+            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

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']
+        # check that there are no volume migrations proposed, because the input
-        # since we have not passed 'dst_pool' in the input, we should not have
+        # parameters do not have a dst_pool and dst_type==src_type
-        # a destination_node in the generated migration action
+        self.assertEqual(len(solution.actions), 0)
-        # self.assertNotIn('destination_node', migration_params)
+
-        # temporarily make the test pass, delete and use the above assert in
+    def test_execute_migrate_volume_dst_pool(self):
-        # followup
+        volume_on_src1 = self.fake_volume(host="src1@back1#pool1",
-        self.assertIsNone(migration_params['destination_node'])
+                                          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(