From 296856101f7cafe1b687119d9a88101e7c9e1af6 Mon Sep 17 00:00:00 2001 From: Alfredo Moralejo Date: Fri, 6 Jun 2025 12:47:27 +0200 Subject: [PATCH] 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 --- .../notes/bug-2109722-cb205216d0c1a836.yaml | 13 ++++ .../strategy/strategies/zone_migration.py | 34 +++++------ .../strategies/test_zone_migration.py | 60 +++++++++++++++---- 3 files changed, 80 insertions(+), 27 deletions(-) create mode 100644 releasenotes/notes/bug-2109722-cb205216d0c1a836.yaml 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 #