Allow volume and vm migrations in zone_migration

Currently, when an audit with strategy zone_migration has added at least
one volume_migration action, it will not process the instances
migrations according to the definition of the `compute_nodes` parameter.
This behavior is unexpected according to the documentation of the
strategy.

This patch is fixing that behavior and making sure that not duplicated
actions are added to the solution, to handle the case where instances
migration actions are created when analyzing the volumes if the
`with_attached_volume` parameter is enabled. The patch is also removing
the method `instances_no_attached` which is not longer used.

Finally, it's adding some unit tests for the new method and fixing the
ones to cover the mixed instances and volumes migration situation.

Closes-Bug: #2109722
Change-Id: Ief7386ab448c2711d0d8a94a77fa9ba189c8b7d2
Signed-off-by: jgilaber <jgilaber@redhat.com>

releasenotes/notes/bug-2109722-cb205216d0c1a836.yaml

@@ -0,0 +1,13 @@
+---
+fixes:
+  - |
+    Previously, when an audit was created with zone_migration strategy and
+    both storage_pools and compute_nodes parameters are passed, the audit
+    did not created the required instances migration actions if any volume
+    migration action was created.
+
+    Now, in that situation the audit will create both instance and volume
+    migrations according to the expected behavior and the limits defined
+    by the parallelization parameters.
+
+    For more information: https://bugs.launchpad.net/watcher/+bug/2109722

watcher/decision_engine/strategy/strategies/zone_migration.py

@@ -291,19 +291,11 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy):
         action_counter = ActionCounter(total_limit,
                                        per_pool_limit, per_node_limit)
 
-        for k, targets in iter(filtered_targets.items()):
+        if VOLUME in filtered_targets:
-            if k == VOLUME:
+            self.volumes_migration(filtered_targets[VOLUME], action_counter)
-                self.volumes_migration(targets, action_counter)
+        if INSTANCE in filtered_targets:
-            elif k == INSTANCE:
+            self.instances_migration(filtered_targets[INSTANCE],
-                if self.volume_count == 0 and self.volume_update_count == 0:
+                                     action_counter)
-                    # if with_attached_volume is true,
-                    # instance having attached volumes already migrated,
-                    # migrate instances which does not have attached volumes
-                    if self.with_attached_volume:
-                        targets = self.instances_no_attached(targets)
-                        self.instances_migration(targets, action_counter)
-                    else:
-                        self.instances_migration(targets, action_counter)
 
         LOG.debug("action total: %s, pools: %s, nodes %s ",
                   action_counter.total_count,
@@ -357,10 +349,6 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy):
     def is_in_use(self, volume):
         return getattr(volume, 'status') == IN_USE
 
-    def instances_no_attached(self, instances):
-        return [i for i in instances
-                if not getattr(i, "os-extended-volumes:volumes_attached")]
-
     def get_host_by_pool(self, pool):
         """Get host name from pool name
 
@@ -433,6 +421,10 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy):
     def instances_migration(self, instances, action_counter):
 
         for instance in instances:
+            if self._instance_migration_exists(instance.id):
+                LOG.debug("A migration action for instance %s already exist",
+                          instance.id)
+                continue
             src_node = getattr(instance, 'OS-EXT-SRV-ATTR:host')
 
             if action_counter.is_total_max():
@@ -480,6 +472,14 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy):
             input_parameters=parameters)
         self.planned_cold_count += 1
 
+    def _instance_migration_exists(self, instance_id):
+        for action in self.solution.actions:
+            resource_id = action['input_parameters'].get('resource_id', None)
+            if (action['action_type'] == 'migrate' and
+                    resource_id == instance_id):
+                return True
+        return False
+
     def _volume_migrate(self, volume, dst_pool):
         parameters = {"migration_type": "migrate",
                       "destination_node": dst_pool,

watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py

@@ -1051,19 +1051,35 @@ class TestZoneMigration(TestBaseStrategy):
                  'resource_name': 'volume_2',
                  'resource_id': 'a16c811e-2521-4fd3-8779-6a94ccb3be73'}}
         ]
+        expected_vm_migrations = [
+            {'action_type': 'migrate',
+             'input_parameters':
+                {'migration_type': 'live',
+                 'source_node': 'src1',
+                 'destination_node': 'dst1',
+                 'resource_name': 'INSTANCE_1',
+                 'resource_id': 'INSTANCE_1'}}
+        ]
         migrated_volumes = [action
                             for action in solution.actions
                             if action['action_type'] == 'volume_migrate']
         self.assertEqual(2, action_types.get("volume_migrate", 0))
         self.assertEqual(expected_vmigrations, migrated_volumes)
-        # TODO(amoralej) Next check should be creating a migrate action. Change
+
-        # the expected value to 1 when fixed.
+        self.assertEqual(1, action_types.get("migrate", 0))
-        self.assertEqual(0, action_types.get("migrate", 0))
+        migrated_vms = [action
+                        for action in solution.actions
+                        if action['action_type'] == 'migrate']
+        self.assertEqual(expected_vm_migrations, migrated_vms)
 
         # All the detached volumes in the pool should be migrated
         volume_indicator = [item['value'] for item in solution.global_efficacy
                             if item['name'] == "volume_migrate_ratio"][0]
         self.assertEqual(100, volume_indicator)
+        # All the instances in src1 should be migrated
+        live_ind = [item['value'] for item in solution.global_efficacy
+                    if item['name'] == "live_instance_migrate_ratio"][0]
+        self.assertEqual(100, live_ind)
 
     def test_execute_mixed_instances_volumes_with_attached(self):
         instance_on_src1_1 = self.fake_instance(
@@ -1141,7 +1157,14 @@ class TestZoneMigration(TestBaseStrategy):
                  'source_node': 'src1',
                  'destination_node': 'dst1',
                  'resource_name': 'INSTANCE_3',
-                 'resource_id': 'INSTANCE_3'}}
+                 'resource_id': 'INSTANCE_3'}},
+            {'action_type': 'migrate',
+             'input_parameters':
+                {'migration_type': 'live',
+                 'source_node': 'src1',
+                 'destination_node': 'dst1',
+                 'resource_name': 'INSTANCE_1',
+                 'resource_id': 'INSTANCE_1'}}
         ]
         migrated_volumes = [action
                             for action in solution.actions
@@ -1151,12 +1174,10 @@ class TestZoneMigration(TestBaseStrategy):
         migrated_vms = [action
                         for action in solution.actions
                         if action['action_type'] == 'migrate']
-        self.assertEqual(1, action_types.get("migrate", 0))
+        self.assertEqual(2, action_types.get("migrate", 0))
         self.assertEqual(expected_vm_migrations, migrated_vms)
 
-        # TODO(amoralej) This should be creating more migrate actions.
+        self.assertEqual(2, action_types.get("migrate", 0))
-        # Uncomment the following line when fixed.
-        # self.assertEqual(0, action_types.get("migrate", 0))
 
         # All the detached volumes in the pool should be migrated
         volume_mig_ind = [item['value'] for item in solution.global_efficacy
@@ -1166,10 +1187,29 @@ class TestZoneMigration(TestBaseStrategy):
         volume_swap_ind = [item['value'] for item in solution.global_efficacy
                            if item['name'] == "volume_update_ratio"][0]
         self.assertEqual(100, volume_swap_ind)
-        # TODO(amoralej) This should be 100. Change when fixed.
+        # All the instances in src1 should be migrated
         live_ind = [item['value'] for item in solution.global_efficacy
                     if item['name'] == "live_instance_migrate_ratio"][0]
-        self.assertEqual(50, live_ind)
+        self.assertEqual(100, live_ind)
+
+    def test_instance_migration_exists(self):
+
+        fake_actions = [
+            {'action_type': 'migrate', 'resource_id': 'instance1'},
+            {'action_type': 'some_other_action', 'resource_id': 'instance2'},
+            {'action_type': 'migrate', 'resource_id': 'instance3'}
+        ]
+
+        for action in fake_actions:
+            self.strategy.solution.add_action(
+                action['action_type'],
+                resource_id=action['resource_id'])
+        self.assertTrue(self.strategy._instance_migration_exists('instance1'))
+        self.assertTrue(self.strategy._instance_migration_exists('instance3'))
+        self.assertFalse(self.strategy._instance_migration_exists('instance2'))
+        self.assertFalse(self.strategy._instance_migration_exists('instance4'))
+        self.assertFalse(self.strategy._instance_migration_exists(None))
+        self.assertFalse(self.strategy._instance_migration_exists(''))
 
     # priority filter #