Merge ~alexsander-souza/maas:lp1887558_fix_condense_luns into maas:master

[Alexsander de Souza]

improve multipath storage device support

include support to FiberChannel, JBOD, SCSI, iSCSI and HyperV multipath devices

fixes LP#1887558

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1887558_fix_condense_luns lp:~alexsander-souza/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 3006b75a6b183c5ca9a96e179b733a32a4e71653

review: Approve

[Jack Lloyd-Walters (lloydwaltersj)]

As best I can see, this looks good!
The compiled regex at the start is very neat, good work!

review: Approve

diff --git a/src/metadataserver/builtin_scripts/hooks.py b/src/metadataserver/builtin_scripts/hooks.py
index 9904086..e34f760 100644
--- a/src/metadataserver/builtin_scripts/hooks.py
+++ b/src/metadataserver/builtin_scripts/hooks.py
@@ -735,6 +735,32 @@ def _get_matching_block_device(block_devices, serial=None, id_path=None):
     return None
 
 
+_MP_PATH_ID = {
+    "fc": [re.compile(r"^(?P<port>\w+)-(?P<lun>lun-(0x)?[\da-fA-F]+)$")],
+    "vmbus": [re.compile(r"^(?P<guid>\w+)-(?P<lun>lun-(0x)?[\da-fA-F]+)$")],
+    "sas": [
+        re.compile(
+            r"^(?P<sas_addr>0x[\da-fA-F]+)-(?P<lun>lun-(0x)?[\da-fA-F]+)$"
+        ),
+        re.compile(
+            r"^exp0x[\da-fA-F]+-phy(?P<phy_id>(0x)?[\da-fA-F]+)-(?P<lun>lun-(0x)?[\da-fA-F]+)$"
+        ),
+        re.compile(
+            r"^phy(?P<phy_id>(0x)?[\da-fA-F]+)-(?P<lun>lun-(0x)?[\da-fA-F]+)$"
+        ),
+    ],
+    "ip": [
+        re.compile(
+            r"^[\.\-\w:]+-iscsi-(?P<target>[\.\-\w:]+)-(?P<lun>lun-(0x)?[\da-fA-F]+)$"
+        )
+    ],
+}
+
+_DEV_PATH = re.compile(
+    r"^(?P<bus>\w+)-(?P<bus_addr>[\da-fA-F:\.]+)-(?P<proto>\w+)-(?P<device>.*)$"
+)
+
+
 def _condense_luns(disks):
     """Condense disks by LUN.
 
@@ -747,46 +773,33 @@ def _condense_luns(disks):
     serial_lun_map = defaultdict(list)
     processed_disks = []
     for disk in disks:
-        # split the device path from the form "key1-value1-key2-value2..." into
-        # a dict
-        tokens = disk.get("device_path", "").split("-")
-        device_path = dict(zip(tokens[::2], tokens[1::2]))
-        # LXD does not currently give a pci_address, it's included in the
-        # device_path. Add it if it isn't there. A USB disk include the
-        # USB device and PCI device the USB controller is connected to.
-        # Ignore USB devices as they are removable which MAAS doesn't
-        # model.
-        if (
-            "pci" in device_path
-            and "usb" not in device_path
-            and "pci_address" not in disk
-        ):
-            disk["pci_address"] = device_path["pci"]
-        if device_path.get("lun") not in ("0", None) and disk.get("serial"):
-            # multipath devices have LUN different from 0
-            serial_lun_map[(disk["serial"], device_path["lun"])].append(disk)
+        dev_match = _DEV_PATH.match(disk.get("device_path", ""))
+        if dev_match is None or dev_match["proto"] == "usb":
+            processed_disks.append(disk)
+            continue
+
+        proto = dev_match["proto"]
+        device = dev_match["device"]
+
+        if dev_match["bus"] == "pci" and "pci_address" not in disk:
+            disk["pci_address"] = dev_match["bus_addr"]
+
+        if disk["serial"] and (rexp_list := _MP_PATH_ID.get(proto)):
+            for r in rexp_list:
+                if m := r.match(device):
+                    serial_lun_map[(disk["serial"], m["lun"])].append(disk)
+                    break
+            else:
+                processed_disks.append(disk)
         else:
             processed_disks.append(disk)
 
     for (serial, lun), paths in serial_lun_map.items():
-        # The first disk is usually the smallest id however doesn't usually
-        # have the device_id associated with it.
-        condensed_disk = paths[0]
-        if len(paths) > 1:
-            # Only tag a disk as multipath if it actually has multiple paths to
-            # it.
+        mpaths = sorted(paths, key=itemgetter("id"))
+        condensed_disk = mpaths[0]
+        if len(mpaths) > 1:
             condensed_disk["maas_multipath"] = True
-        for path in paths[1:]:
-            # Make sure the disk processed has the lowest id. Each path is
-            # given a name variant on a normal id. e.g sda, sdaa, sdab, sdb
-            # sdba, sdbb results in just sda and sdb for two multipath disks.
-            if path["id"] < condensed_disk["id"]:
-                condensed_disk["id"] = path["id"]
-                # The device differs per id, at least on IBM Z. MAAS doesn't
-                # use the device but keep it consistent anyway.
-                condensed_disk["device"] = path["device"]
-            # Only one path is given the device_id. Make sure the disk that
-            # is processed has it for the id_path.
+        for path in mpaths[1:]:
             if not condensed_disk.get("device_id") and path.get("device_id"):
                 condensed_disk["device_id"] = path["device_id"]
         processed_disks.append(condensed_disk)
diff --git a/src/metadataserver/builtin_scripts/tests/test_hooks.py b/src/metadataserver/builtin_scripts/tests/test_hooks.py
index 9aa07e3..c37eacf 100644
--- a/src/metadataserver/builtin_scripts/tests/test_hooks.py
+++ b/src/metadataserver/builtin_scripts/tests/test_hooks.py
@@ -3805,6 +3805,140 @@ class TestUpdateNodePhysicalBlockDevices(MAASServerTestCase):
             },
         )
 
+    def test_condenses_luns_jbod(self):
+        node = factory.make_Node()
+        resources = deepcopy(SAMPLE_LXD_RESOURCES)
+        expander1 = f"pci-0000:81:00.0-sas-exp0x{factory.make_hex_string(16)}"
+        expander2 = f"pci-0000:81:00.0-sas-exp0x{factory.make_hex_string(16)}"
+        lun1_model = factory.make_name("lun1_model")
+        lun1_size = 1024**3 * random.randint(5, 100)
+        lun1_block_size = random.choice([512, 1024, 4096])
+        lun1_firmware_version = factory.make_name("lun1_firmware_version")
+        lun1_serial = factory.make_name("lun1_serial")
+        lun2_model = factory.make_name("lun2_model")
+        lun2_size = 1024**3 * random.randint(5, 100)
+        lun2_block_size = random.choice([512, 1024, 4096])
+        lun2_firmware_version = factory.make_name("lun2_firmware_version")
+        lun2_serial = factory.make_name("lun2_serial")
+        resources["storage"]["disks"] = [
+            {
+                "id": "sda",
+                "device": "8:0",
+                "model": lun1_model,
+                "type": "scsi",
+                "read_only": False,
+                "size": lun1_size,
+                "removable": False,
+                "numa_node": 0,
+                "device_path": f"{expander1}-phy2-lun-0",
+                "block_size": lun1_block_size,
+                "firmware_version": lun1_firmware_version,
+                "rpm": 0,
+                "serial": lun1_serial,
+                "device_id": "",
+                "partitions": [],
+            },
+            {
+                "id": "sdb",
+                "device": "8:16",
+                "model": lun2_model,
+                "type": "scsi",
+                "read_only": False,
+                "size": lun2_size,
+                "removable": False,
+                "numa_node": 0,
+                "device_path": f"{expander1}-phy5-lun-0",
+                "block_size": lun2_block_size,
+                "firmware_version": lun2_firmware_version,
+                "rpm": 0,
+                "serial": lun2_serial,
+                "device_id": "",
+                "partitions": [],
+            },
+            {
+                "id": "sdc",
+                "device": "8:112",
+                "model": lun1_model,
+                "type": "scsi",
+                "read_only": False,
+                "size": lun1_size,
+                "removable": False,
+                "numa_node": 0,
+                "device_path": f"{expander2}-phy2-lun-0",
+                "block_size": lun1_block_size,
+                "firmware_version": lun1_firmware_version,
+                "rpm": 0,
+                "serial": lun1_serial,
+                "device_id": "scsi-LUN1",
+                "partitions": [],
+            },
+            {
+                "id": "sdd",
+                "device": "8:118",
+                "model": lun2_model,
+                "type": "scsi",
+                "read_only": False,
+                "size": lun2_size,
+                "removable": False,
+                "numa_node": 0,
+                "device_path": f"{expander2}-phy5-lun-0",
+                "block_size": lun2_block_size,
+                "firmware_version": lun2_firmware_version,
+                "rpm": 0,
+                "serial": lun2_serial,
+                "device_id": "scsi-LUN2",
+                "partitions": [],
+            },
+        ]
+
+        _update_node_physical_block_devices(
+            node, resources, create_numa_nodes(node)
+        )
+
+        self.assertEqual(2, node.physicalblockdevice_set.count())
+        sda = node.physicalblockdevice_set.get(name="sda")
+        self.assertEqual(
+            {
+                "model": lun1_model,
+                "serial": lun1_serial,
+                "id_path": "/dev/disk/by-id/scsi-LUN1",
+                "size": lun1_size,
+                "block_size": lun1_block_size,
+                "firmware_version": lun1_firmware_version,
+                "tags": ["multipath"],
+            },
+            {
+                "model": sda.model,
+                "serial": sda.serial,
+                "id_path": sda.id_path,
+                "size": sda.size,
+                "block_size": sda.block_size,
+                "firmware_version": sda.firmware_version,
+                "tags": sda.tags,
+            },
+        )
+        sdb = node.physicalblockdevice_set.get(name="sdb")
+        self.assertEqual(
+            {
+                "model": lun2_model,
+                "serial": lun2_serial,
+                "id_path": "/dev/disk/by-id/scsi-LUN2",
+                "size": lun2_size,
+                "block_size": lun2_block_size,
+                "firmware_version": lun2_firmware_version,
+                "tags": ["multipath"],
+            },
+            {
+                "model": sdb.model,
+                "serial": sdb.serial,
+                "id_path": sdb.id_path,
+                "size": sdb.size,
+                "block_size": sdb.block_size,
+                "firmware_version": sdb.firmware_version,
+                "tags": sdb.tags,
+            },
+        )
+
     def test_no_condense_luns_different_serial(self):
         node = factory.make_Node()
         resources = deepcopy(SAMPLE_LXD_RESOURCES)