Merge ~ack/maas:1915715-lxd-multidisk-size-fix into maas:master

[Alberto Donato]

LP #1915715 - LXD: consider tags/size for disks in get_discovered_machine

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1915715-lxd-multidisk-size-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: 0009786e76e0fbbf72bf550f1099a05d00d56abd

review: Approve

[MAAS Lander (maas-lander)]

UNIT TESTS
-b 1915715-lxd-multidisk-size-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: d4bd357580d93256e8c03c095c9c4d1e7983eb12

review: Approve

[Björn Tillenius (bjornt)]

+1

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b 1915715-lxd-multidisk-size-fix lp:~ack/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci.internal:8080/job/maas/job/branch-tester/9254/consoleText

diff --git a/src/provisioningserver/drivers/pod/lxd.py b/src/provisioningserver/drivers/pod/lxd.py
index 16929e3..665873a 100644
--- a/src/provisioningserver/drivers/pod/lxd.py
+++ b/src/provisioningserver/drivers/pod/lxd.py
@@ -78,6 +78,7 @@ LXD_BYTE_SUFFIXES = {
 def convert_lxd_byte_suffixes(value, divisor=None):
     """Takes the value and converts to a proper integer
     using LXD_BYTE_SUFFIXES."""
+    value = str(value)
     result = re.match(
         r"(?P<size>[0-9]+)(?P<unit>%s)" % "|".join(LXD_BYTE_SUFFIXES.keys()),
         value,
@@ -615,64 +616,44 @@ class LXDPodDriver(PodDriver):
             )
             power_state = "unknown"
 
-        expanded_config = machine.expanded_config
-        expanded_devices = machine.expanded_devices
+        def _get_discovered_block_device(name, device, requested_device=None):
+            tags = requested_device.tags if requested_device else []
+            # When LXD creates a QEMU disk the serial is always
+            # lxd_{device name}. The device_name is defined by
+            # the LXD profile or when adding a device. This is
+            # commonly "root" for the first disk. The model and
+            # serial must be correctly defined here otherwise
+            # MAAS will delete the disk created during composition
+            # which results in losing the storage pool link. Without
+            # the storage pool link MAAS can't determine how much
+            # of the storage pool has been used.
+            serial = f"lxd_{name}"
+            source = device.get("source")
+            if source:
+                pool = client.storage_pools.get(device["pool"])
+                volume = pool.volumes.get("custom", source)
+                size = volume.config.get("size")
+            else:
+                size = device.get("size")
+            # Default disk size is 10GB in LXD
+            size = convert_lxd_byte_suffixes(size or "10GB")
+            return DiscoveredMachineBlockDevice(
+                model="QEMU HARDDISK",
+                serial=serial,
+                id_path=f"/dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_{serial}",
+                size=size,
+                tags=tags,
+                storage_pool=device.get("pool"),
+            )
 
-        # Discover block devices.
-        block_devices = []
-        for device in expanded_devices:
-            # Block device.
-            # When request is provided map the tags from the request block
-            # devices to the discovered block devices. This ensures that
-            # composed machine has the requested tags on the block device.
-
-            tags = []
-            if (
-                request is not None
-                and expanded_devices[device]["type"] == "disk"
-            ):
-                tags = request.block_devices[0].tags
-
-            device_info = expanded_devices[device]
-            if device_info["type"] == "disk":
-                # When LXD creates a QEMU disk the serial is always
-                # lxd_{device name}. The device_name is defined by
-                # the LXD profile or when adding a device. This is
-                # commonly "root" for the first disk. The model and
-                # serial must be correctly defined here otherwise
-                # MAAS will delete the disk created during composition
-                # which results in losing the storage pool link. Without
-                # the storage pool link MAAS can't determine how much
-                # of the storage pool has been used.
-                serial = f"lxd_{device}"
-                # Default disk size is 10GB.
-                size = convert_lxd_byte_suffixes(
-                    device_info.get("size", "10GB")
-                )
-                storage_pool = device_info.get("pool")
-                block_devices.append(
-                    DiscoveredMachineBlockDevice(
-                        model="QEMU HARDDISK",
-                        serial=serial,
-                        id_path=f"/dev/disk/by-id/scsi-0QEMU_QEMU_HARDDISK_{serial}",
-                        size=size,
-                        tags=tags,
-                        storage_pool=storage_pool,
-                    )
-                )
+        expanded_config = machine.expanded_config
+        iface_to_mac = {
+            key.split(".")[1]: value
+            for key, value in expanded_config.items()
+            if key.endswith("hwaddr")
+        }
 
-        # Discover interfaces.
-        interfaces = []
-        boot = True
-        config_mac_address = {}
-        for configuration in expanded_config:
-            if configuration.endswith("hwaddr"):
-                mac = expanded_config[configuration]
-                name = configuration.split(".")[1]
-                config_mac_address[name] = mac
-        for name, device in expanded_devices.items():
-            if device["type"] != "nic":
-                continue
+        def _get_discovered_interface(name, device, boot):
             if "network" in device:
                 # Try finding the nictype from the networks.
                 # XXX: This should work for "bridge" networks,
@@ -691,18 +672,40 @@ class LXDPodDriver(PodDriver):
                 )
             mac = device.get("hwaddr")
             if mac is None:
-                mac = config_mac_address.get(name)
-
-            interfaces.append(
-                DiscoveredMachineInterface(
-                    mac_address=mac,
-                    vid=int(device.get("vlan", get_vid_from_ifname(name))),
-                    boot=boot,
-                    attach_type=attach_type,
-                    attach_name=attach_name,
-                )
+                mac = iface_to_mac.get(name)
+            return DiscoveredMachineInterface(
+                mac_address=mac,
+                vid=int(device.get("vlan", get_vid_from_ifname(name))),
+                boot=boot,
+                attach_type=attach_type,
+                attach_name=attach_name,
             )
-            boot = False
+
+        extra_block_devices = 0
+        block_devices = []
+        interfaces = []
+        for name, device in machine.expanded_devices.items():
+            if device["type"] == "disk":
+                requested_device = None
+                if request:
+                    # for composed VMs, the root disk is always the first
+                    # one. Adjust the index so that it matches the requested
+                    # device
+                    if name == "root":
+                        index = 0
+                    else:
+                        extra_block_devices += 1
+                        index = extra_block_devices
+                    requested_device = request.block_devices[index]
+                block_devices.append(
+                    _get_discovered_block_device(
+                        name, device, requested_device=requested_device
+                    )
+                )
+            elif device["type"] == "nic":
+                interfaces.append(
+                    _get_discovered_interface(name, device, not interfaces)
+                )
 
         # LXD uses different suffixes to store memory so make
         # sure we convert to MiB, which is what MAAS uses.
diff --git a/src/provisioningserver/drivers/pod/tests/test_lxd.py b/src/provisioningserver/drivers/pod/tests/test_lxd.py
index 3045029..d6e69ec 100644
--- a/src/provisioningserver/drivers/pod/tests/test_lxd.py
+++ b/src/provisioningserver/drivers/pod/tests/test_lxd.py
@@ -44,7 +44,8 @@ from provisioningserver.utils.network import generate_mac_address
 def make_requested_machine(num_disks=1, **kwargs):
     block_devices = [
         RequestedMachineBlockDevice(
-            size=random.randint(1024 ** 3, 4 * 1024 ** 3)
+            size=random.randint(1024 ** 3, 4 * 1024 ** 3),
+            tags=[factory.make_name("tag")],
         )
         for _ in range(num_disks)
     ]
@@ -749,6 +750,80 @@ class TestLXDPodDriver(MAASTestCase):
         self.assertTrue(discovered_machine.hugepages_backed)
         self.assertEqual(discovered_machine.pinned_cores, [0, 1, 2])
 
+    def test_get_discovered_machine_with_request(self):
+        request = make_requested_machine(num_disks=2)
+        driver = lxd_module.LXDPodDriver()
+        Client = self.patch(driver, "_get_client")
+        client = Client.return_value
+        mock_profile = Mock()
+        mock_profile.name = random.choice(["maas", "default"])
+        profile_devices = {
+            "eth0": {
+                "name": "eth0",
+                "nictype": "bridged",
+                "parent": "lxdbr0",
+                "type": "nic",
+            },
+        }
+        mock_profile.devices = profile_devices
+        client.profiles.get.return_value = mock_profile
+        mock_storage_pools = Mock()
+        client.storage_pools.all.return_value = mock_storage_pools
+        mock_get_usable_storage_pool = self.patch(
+            driver, "_get_usable_storage_pool"
+        )
+        # a volume is created for the second disk
+        volume = Mock()
+        volume.name = factory.make_name("vol")
+        volume.config = {"size": request.block_devices[1].size}
+        usable_pool = Mock()
+        usable_pool.name = factory.make_name("pool")
+        usable_pool.volumes.create.return_value = volume
+        usable_pool.volumes.get.return_value = volume
+        mock_get_usable_storage_pool.return_value = usable_pool
+        client.storage_pools.get.return_value = usable_pool
+        mock_machine = Mock(architecture="x86_64")
+        expanded_config = {
+            "limits.cpu": "2",
+            "limits.memory": "1024",
+            "volatile.eth0.hwaddr": "00:16:3e:78:be:04",
+        }
+        expanded_devices = {
+            "root": {
+                "path": "/",
+                "type": "disk",
+                "pool": usable_pool.name,
+                "size": str(request.block_devices[0].size),
+                "boot.priority": "0",
+            },
+            "disk1": {
+                "path": "",
+                "type": "disk",
+                "pool": usable_pool.name,
+                "source": volume.name,
+            },
+            "eth0": {
+                "boot.priority": "1",
+                "name": "eth0",
+                "nictype": "bridged",
+                "parent": "lxdbr0",
+                "type": "nic",
+            },
+        }
+        mock_machine.expanded_config = expanded_config
+        mock_machine.expanded_devices = expanded_devices
+        client.virtual_machines.create.return_value = mock_machine
+        discovered_machine = driver._get_discovered_machine(
+            client, mock_machine, [usable_pool], request
+        )
+        # invert sort as the root device shows up last because of name ordering
+        discovered_devices = sorted(
+            discovered_machine.block_devices, reverse=True
+        )
+        for idx, device in enumerate(discovered_devices):
+            self.assertEqual(device.size, request.block_devices[idx].size)
+            self.assertEqual(device.tags, request.block_devices[idx].tags)
+
     def test_get_hugepages_info_int_value_as_bool(self):
         driver = lxd_module.LXDPodDriver()
         Client = self.patch(lxd_module, "Client")