Merge lp:~danilo/maas/virsh-storage-units into lp:maas/2.2

[Данило Шеган]

Consider size units for storage when evaluating virsh pod storage.

This changes .get_key_value() on the Virsh driver to not strip the unit, and introduces a "_unitless" variant that still does that. This highlights a few potential future problems as well (memory size in KiB, CPU speed in Mhz, if those units change, we'll have similar problems).

I wonder why did we not end up using libvirt instead which would provide us stable units, but I imagine they might not be py3 bindings available or something along those lines.

Testing instructions:

I have not tested this yet (I only have one system where I have >1TiB LVM volume), so it seems easier to test this with a small storage partition instead (eg. MiB to ensure that still works), but generally you need to:

 - set up a libvirt on a system and add a storage pool of >1TiB
 - ensure "virsh pool-info" lists the storage pool details in TiB
 - add a pod to MAAS and look at available storage: it should still be correct

[Mike Pontillo (mpontillo)]

This looks fine, but I'm not sure why we aren't using "virsh pool-dumpxml", which would report the units consistently in the first place. For example, I see the following:

...
  <capacity unit='bytes'>61966548992</capacity>
  <allocation unit='bytes'>35070156800</allocation>
  <available unit='bytes'>26896392192</available>
...

I'm going to approve anyway, since it seems like that work would be well beyond the scope of this branch.

review: Approve

[Mike Pontillo (mpontillo)]

Actually, upon closer inspection, I think this needs fixing, because _convert_to_bytes() doesn't have unit tests that cover the entire function.

Also, I think _convert_to_bytes() should be placed in a more common location; it's more generally useful than a private function inside the virsh driver. I would find a home for it somewhere in src/provisioningserver/utils, and add unit tests for it to ensure it handles each unit correctly.

Finally, consider raising an exception for an unknown unit rather than just logging and carrying on.

review: Needs Fixing

[Mike Pontillo (mpontillo)]

While we're at it, we could also future-proof ourselves, such as:

CAPACITY_UNITS = {
    "KiB": 2 ** 10,
    "MiB": 2 ** 20,
    "GiB": 2 ** 30,
    "TiB": 2 ** 40,
    "PiB": 2 ** 50,
    "EiB": 2 ** 60,
    "ZiB": 2 ** 70,
    "YiB": 2 ** 80,
}

[MAAS Lander (maas-lander)]

Transitioned to Git.

lp:maas/2.2 has now moved from Bzr to Git.
Please propose your branches with Launchpad using Git.

git clone -b 2.2 https://git.launchpad.net/maas

Unmerged revisions

6066. By Данило Шеган on 2017-06-19

Fix test data so it adds up.

6065. By Данило Шеган on 2017-06-19

Interpret units for virsh calls.

=== modified file 'src/provisioningserver/drivers/pod/tests/test_virsh.py'
--- src/provisioningserver/drivers/pod/tests/test_virsh.py	2017-05-19 03:37:56 +0000
+++ src/provisioningserver/drivers/pod/tests/test_virsh.py	2017-06-19 10:17:32 +0000
@@ -105,6 +105,17 @@
     Available:      173.84 GiB
     """)
 
+SAMPLE_POOLINFO_TB = dedent("""
+    Name:           default
+    UUID:           59edc0cb-4635-449a-80e2-2c8a59afa327
+    State:          running
+    Persistent:     yes
+    Autostart:      yes
+    Capacity:       2.21 TiB
+    Allocation:     880.64 GiB
+    Available:      1.35 TiB
+    """)
+
 SAMPLE_IFLIST = dedent("""
     Interface  Type       Source     Model       MAC
     -------------------------------------------------------
@@ -506,6 +517,18 @@
             for pool in pools_mock.return_value
         }, expected)
 
+    def test_get_pod_pool_size_map_terabytes(self):
+        conn = self.configure_virshssh(SAMPLE_POOLINFO_TB)
+        pools_mock = self.patch(virsh.VirshSSH, 'list_pools')
+        pools_mock.return_value = [
+            factory.make_name('pool') for _ in range(3)]
+        expected = conn.get_pod_pool_size_map('Capacity')
+        capacity = int(2.21 * 2**40)
+        self.assertEqual({
+            pool: capacity
+            for pool in pools_mock.return_value
+        }, expected)
+
     def test_get_pod_local_storage(self):
         conn = self.configure_virshssh(SAMPLE_POOLINFO)
         pools_mock = self.patch(virsh.VirshSSH, 'list_pools')
 
=== modified file 'src/provisioningserver/drivers/pod/virsh.py'
--- src/provisioningserver/drivers/pod/virsh.py	2017-05-19 03:57:03 +0000
+++ src/provisioningserver/drivers/pod/virsh.py	2017-06-19 10:17:32 +0000
@@ -273,7 +273,13 @@
         data = data.strip().splitlines()
         for d in data:
             if key == d.split(':')[0].strip():
-                return d.split(':')[1].split()[0]
+                return d.split(':')[1].strip()
+
+    def get_key_value_unitless(self, data, key):
+        """Return value based off of key with unit (if any) stripped off."""
+        value = self.get_key_value(data, key)
+        if value:
+            return value.split()[0]
 
     def list_machines(self):
         """Lists all VMs by name."""
@@ -336,7 +342,7 @@
         if output is None:
             maaslog.error("Failed to get pod CPU speed")
             return None
-        return int(self.get_key_value(output, "CPU frequency"))
+        return int(self.get_key_value_unitless(output, "CPU frequency"))
 
     def get_pod_memory(self):
         """Gets the total memory of the pod."""
@@ -344,7 +350,7 @@
         if output is None:
             maaslog.error("Failed to get pod memory")
             return None
-        KiB = int(self.get_key_value(output, "Memory size"))
+        KiB = int(self.get_key_value_unitless(output, "Memory size"))
         # Memory in MiB.
         return int(KiB / 1024)
 
@@ -354,10 +360,34 @@
         if output is None:
             maaslog.error("%s: Failed to get machine memory", machine)
             return None
-        KiB = int(self.get_key_value(output, "Max memory"))
+        KiB = int(self.get_key_value_unitless(output, "Max memory"))
         # Memory in MiB.
         return int(KiB / 1024)
 
+    def _convert_to_bytes(self, value):
+        """Converts values with KiB, MiB, GiB, TiB units to bytes."""
+        capacity_def = value.split(' ', 1)
+        if len(capacity_def) == 1:
+            # No unit specified, default to bytes.
+            capacity_value = int(capacity_def[0])
+            multiplier = 1
+        else:
+            capacity_value, capacity_unit = capacity_def
+            capacity_value = float(capacity_value)
+            if capacity_unit == 'TiB':
+                multiplier = 2**40
+            elif capacity_unit == 'GiB':
+                multiplier = 2**30
+            elif capacity_unit == 'MiB':
+                multiplier = 2**20
+            elif capacity_unit == 'KiB':
+                multiplier = 2**10
+            else:
+                maaslog.warn("Unknown capacity unit %s", capacity_unit)
+                multiplier = 1
+        # Convert value to bytes.
+        return int(capacity_value * multiplier)
+
     def get_pod_pool_size_map(self, key):
         """Return the mapping for a size calculation based on key."""
         pools = {}
@@ -366,10 +396,8 @@
             if output is None:
                 # Skip if cannot get more information.
                 continue
-            capacity = float(
-                self.get_key_value(output, key))
-            # Convert GiB to bytes.
-            pools[pool] = int(capacity * 2**30)
+            pools[pool] = self._convert_to_bytes(
+                self.get_key_value(output, "Capacity"))
         return pools
 
     def get_pod_local_storage(self):
@@ -390,10 +418,10 @@
                 maaslog.error(
                     "Failed to get available pod local storage")
                 return None
-            local_storage += float(self.get_key_value(
-                output, "Available"))
-        # Local storage in bytes. GiB to bytes.
-        return int(local_storage * 2**30)
+            local_storage += self._convert_to_bytes(
+                self.get_key_value(output, "Available"))
+        # Local storage in bytes.
+        return local_storage
 
     def get_machine_local_storage(self, machine, device):
         """Gets the VM local storage for device."""