MAAS

src/provisioningserver/drivers/pod/tests/test_virsh.py (+23/-0)
src/provisioningserver/drivers/pod/virsh.py (+17/-12)
src/provisioningserver/utils/__init__.py (+45/-0)
src/provisioningserver/utils/tests/test_utils.py (+38/-0)

To merge this branch:

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

High

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Mike Pontillo (community)		2017-06-19	Approve on 2017-06-20
Review via email: mp+325920@code.qastaging.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:

My mistake; I commented on the 2.2 branch instead of this one.

See comments here.

https://code.launchpad.net/~danilo/maas/virsh-storage-units/+merge/325913

(Usually we land a branch on trunk first and then self-approve backports.)

Revision history for this message

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

review: Needs Fixing

Revision history for this message

Данило Шеган (danilo) wrote on 2017-06-20:

I'll work on moving the helper and adding the unit tests as you suggested later: marking as WIP for now.

FWIW, Dmitrii has confirmed in the bug report that this solves the problem for him.

Revision history for this message

Данило Шеган (danilo) wrote on 2017-06-20:

I've addressed your review comments: moved the function into provisioningserver.utils.convert_size_to_bytes and added unit tests (thanks for raising this, I was going to add them then I got side-tracked filing other bugs as offshoots of the original one, and it slipped!).