Merge "Fix zone migration dst pool and type selection"

releasenotes/notes/bug-2129692-a1f941b8fa5a71be.yaml

@@ -0,0 +1,15 @@
+---
+fixes:
+  - |
+    Fixed zone migration strategy destination pool and type selection.
+    Previously, the get_dst_pool_and_type method was always selecting the
+    first matching storage pool based only on src_pool, ignoring the
+    src_type parameter. This caused incorrect volume migrations when
+    users supplied multiple values for storage_pools with the same
+    source pool but different source types.
+
+    The method now implements a two-pass matching approach:
+    first attempting to match both src_pool and src_type for exact matches,
+    then falling back to matching just src_pool if no exact match is found.
+
+    For more info see: https://bugs.launchpad.net/watcher/+bug/2129692

watcher/decision_engine/strategy/strategies/zone_migration.py

@@ -380,16 +380,36 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy):
             if node.get("src_node") == src_node:
                 return node.get("dst_node")
 
-    def get_dst_pool_and_type(self, src_pool):
+    def get_dst_pool_and_type(self, src_pool, src_type):
         """Get destination pool and type from self.migration_storage_pools
 
         :param src_pool: storage pool name
-        :returns: set of storage pool name and volume type name
+        :param src_type: storage volume type
+        :returns: set of storage dst pool name and dst volume type name, any
+        of those values can be None if no match for it is found in the
+        input_parameters
         """
+        src_pool_type_inputs = []
+        src_pool_inputs = []
         for pool in self.migrate_storage_pools:
+            if pool.get("src_type") is None:
+                src_pool_inputs.append(pool)
+            else:
+                src_pool_type_inputs.append(pool)
+
+        pool_to_check = (src_pool, src_type)
+        # First pass: prefer exact match (both src_pool and src_type)
+        for pool in src_pool_type_inputs:
+            user_pool = (pool.get("src_pool"), pool.get("src_type"))
+            if pool_to_check == user_pool:
+                return (pool.get("dst_pool"), pool.get("dst_type"))
+
+        # Second pass: fall back to matching just src_pool
+        for pool in src_pool_inputs:
             if pool.get("src_pool") == src_pool:
-                return (pool.get("dst_pool", None),
-                        pool.get("dst_type"))
+                return (pool.get("dst_pool"), pool.get("dst_type"))
+
+        return (None, None)
 
     def volumes_migration(self, volumes, action_counter, instance_targets=[]):
         instance_target_ids = {instance.uuid for instance in instance_targets}
@@ -405,7 +425,7 @@ class ZoneMigration(base.ZoneMigrationBaseStrategy):
                 continue
 
             src_type = volume.volume_type
-            dst_pool, dst_type = self.get_dst_pool_and_type(pool)
+            dst_pool, dst_type = self.get_dst_pool_and_type(pool, src_type)
             LOG.debug(src_type)
             LOG.debug("%s %s", dst_pool, dst_type)
 

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

@@ -1766,29 +1766,6 @@ class TestZoneMigration(test_utils.NovaResourcesMixin, TestBaseStrategy):
                          [volume_on_src2, volume_on_src1])
 
     def test_get_dst_pool_and_type_matching_src_pool_type(self):
-        # check that returns the right pool and type matching both  src_pool
-        # and src_type
-        pool = "src1@back1#pool1"
-        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(pool)
-        self.assertEqual(dst_pool, "dst1@back1#pool1")
-        self.assertEqual(dst_type, "type1")
-
-    def test_get_dst_pool_and_type_matching_src_pool(self):
-        # check that after not matching type it falls backs to matching
-        # src_pool
-        pool = "src2@back1#pool1"
-        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(pool)
-        self.assertEqual(dst_pool, "dst2@back2#pool1")
-        self.assertEqual(dst_type, "type3")
-
-    def test_get_dst_pool_and_type_not_matching_pool(self):
-        # check not matching pool
-        pool = "src3@back1#pool1"
-        result = self.strategy.get_dst_pool_and_type(pool)
-        self.assertIsNone(result)
-
-    def test_get_dst_pool_and_type_same_src_pool_different_src_type(self):
-        # check that it chooses the first pool listed
         self.input_parameters["storage_pools"] = [
             {
                 "src_pool": "src1@back1#pool1",
@@ -1801,11 +1778,74 @@ class TestZoneMigration(test_utils.NovaResourcesMixin, TestBaseStrategy):
             }
         ]
         pool = "src1@back1#pool1"
-        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(pool)
-        # self.assertEqual(dst_pool, "dst3@back1#pool1")
-        # self.assertEqual(dst_type, "type3")
-        # temporarily assert that the first input is selected, until the bug is
-        # fixed, then it should select the second input based on the values of
-        # src_type and src_pool
+        src_type = "type1"
+        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(
+            pool, src_type
+        )
+        self.assertEqual(dst_pool, "dst1@back1#pool1")
+        self.assertIsNone(dst_type)
+
+    def test_get_dst_pool_and_type_matching_src_pool(self):
+        self.input_parameters["storage_pools"] = [
+            {
+                "src_pool": "src1@back1#pool1",
+                "src_type": "type1",
+                "dst_pool": "dst1@back1#pool1",
+            },
+            {
+                "src_pool": "src1@back1#pool1",
+                "dst_pool": "dst2@back2#pool2"
+            }
+        ]
+        pool = "src1@back1#pool1"
+        src_type = "type1"
+        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(
+            pool, src_type
+        )
+        self.assertEqual(dst_pool, "dst1@back1#pool1")
+        self.assertIsNone(dst_type)
+
+        # check that after not matching type it falls backs to matching
+        # src_pool
+        pool = "src1@back1#pool1"
+        src_type = "typet"
+        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(
+            pool, src_type
+        )
+        self.assertEqual(dst_pool, "dst2@back2#pool2")
+        self.assertIsNone(dst_type)
+
+    def test_get_dst_pool_and_type_not_matching_pool(self):
+        pool = "src3@back1#pool1"
+        src_type = "type5"
+        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(
+            pool, src_type
+        )
+        self.assertIsNone(dst_pool)
+        self.assertIsNone(dst_type)
+
+    def test_get_dst_pool_and_type_same_src_pool_different_src_type(self):
+        self.input_parameters["storage_pools"] = [
+            {
+                "src_pool": "src1@back1#pool1",
+                "src_type": "type1",
+                "dst_pool": "dst1@back1#pool1",
+            },
+            {
+                "src_pool": "src1@back1#pool1",
+                "src_type": "type2", "dst_type": "type3"
+            }
+        ]
+        pool = "src1@back1#pool1"
+        src_type = "type2"
+        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(
+            pool, src_type
+        )
+        self.assertIsNone(dst_pool)
+        self.assertEqual(dst_type, "type3")
+        src_type = "type1"
+        dst_pool, dst_type = self.strategy.get_dst_pool_and_type(
+            pool, src_type
+        )
         self.assertEqual(dst_pool, "dst1@back1#pool1")
         self.assertIsNone(dst_type)