diff --git a/releasenotes/notes/bug-2109722-cb205216d0c1a836.yaml b/releasenotes/notes/bug-2109722-cb205216d0c1a836.yaml new file mode 100644 index 000000000..0ff03b827 --- /dev/null +++ b/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 diff --git a/watcher/decision_engine/strategy/strategies/zone_migration.py b/watcher/decision_engine/strategy/strategies/zone_migration.py index 80d7d58e0..3af1e5086 100644 --- a/watcher/decision_engine/strategy/strategies/zone_migration.py +++ b/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 k == VOLUME: - self.volumes_migration(targets, action_counter) - elif k == INSTANCE: - if self.volume_count == 0 and self.volume_update_count == 0: - # 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) + if VOLUME in filtered_targets: + self.volumes_migration(filtered_targets[VOLUME], action_counter) + if INSTANCE in filtered_targets: + self.instances_migration(filtered_targets[INSTANCE], + 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, 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 d6d1f3101..2de745fd2 100644 --- a/watcher/tests/decision_engine/strategy/strategies/test_zone_migration.py +++ b/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(0, action_types.get("migrate", 0)) + + self.assertEqual(1, 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. - # Uncomment the following line when fixed. - # self.assertEqual(0, action_types.get("migrate", 0)) + self.assertEqual(2, 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 #