MAAS

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

virsh-storage-units
Merge into 2.2

Proposed by Данило Шеган on 2017-06-19

Status:

Rejected

Rejected by:

MAAS Lander on 2017-06-22

Proposed branch:

lp://staging/~danilo/maas/virsh-storage-units

Merge into:

lp://staging/maas/2.2

Diff against target:

141 lines (+63/-12)

2 files modified

src/provisioningserver/drivers/pod/tests/test_virsh.py (+23/-0)
src/provisioningserver/drivers/pod/virsh.py (+40/-12)

To merge this branch:

bzr merge lp://staging/~danilo/maas/virsh-storage-units

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mike Pontillo (community)		2017-06-19	Needs Fixing on 2017-06-19
Review via email: mp+325913@code.staging.launchpad.net

Commit message

Consider size units for storage when evaluating virsh pod storage.

Description of the change

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

Revision history for this message

Mike Pontillo (mpontillo) wrote on 2017-06-19:

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

Revision history for this message

Mike Pontillo (mpontillo) wrote on 2017-06-19:

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

Revision history for this message

Mike Pontillo (mpontillo) wrote on 2017-06-19:

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,
}