[virt-tools-list] [PATCH] Show disk allocation & size on Details tab in VMM

[Cole Robinson]

On 08/05/2009 06:10 AM, Michal Novotny wrote:
> Hi Haydn,
> you were right. It was not working right in some circumstances - the values of allocation & size were not reset when type was not 'file' and this is the corrected version of this patch. Thanks once again Haydn...
> 
> Thanks,
> Michal
> 

Hi Michal,

Sorry for the very belated review.

First off, I don't think we should show disk 'allocation' just yet.
Reason being it doesn't necessarily mean what people think it means. A
user will see allocation and likely think 'how much disk space the guest
is using' when it actually means 'how much of the media has been
allocated'. A non-sparse file will appear to be fully allocated when the
guest has only used up a fraction of the available space.

If we want to show 'allocation', it should be called 'Size on disk', but
I don't think that is anything that needs to be exposed in the
Details->Disk section, that's more of a 'Host Details->Storage' piece.

Using something like libguestfs we could determine the actual disk usage
inside the VM, but that can be a separate task.

Comments inline.

> # HG changeset patch
> # User Michal Novotny <minovotn at redhat.com>
> # Date 1249466832 -7200
> # Node ID ec2ddfac511f7d78cf715e7375c54aa22ac4b01c
> # Parent  18e673ca4e148a3e023f56353412d259aea487c2
> Show disk size and allocation bits in Details tab
> 
> When user selected a disk, it was never showing disk allocation and
> size. The allocation itself is (according to my testing) available
> only for managed storages so user can check the allocation of
> managed storages and real capacity (disk image size) bit for both
> of types of storages.
> 
> diff -r 18e673ca4e14 -r ec2ddfac511f src/virtManager/details.py
> --- a/src/virtManager/details.py	Tue Jul 28 22:25:13 2009 -0400
> +++ b/src/virtManager/details.py	Tue Aug 04 17:34:37 2009 +0200
> @@ -1050,6 +1050,14 @@
>          self.window.get_widget("disk-source-path").set_text(diskinfo[3])
>          self.window.get_widget("disk-target-type").set_text(diskinfo[4])
>          self.window.get_widget("disk-target-device").set_text(diskinfo[2])
> +
> +        if diskinfo[9] is None:
> +            diskinfo[9] = "N/A"
> +        if diskinfo[10] is None:
> +            diskinfo[10] = "N/A"
> +

I'd say 'Unknown' instead of N/A here.

> +        self.window.get_widget("disk-size-capacity").set_text(diskinfo[9])
> +        self.window.get_widget("disk-size-allocation").set_text(diskinfo[10])
>          if diskinfo[6] == True:
>              perms = "Readonly"
>          else:
> diff -r 18e673ca4e14 -r ec2ddfac511f src/virtManager/domain.py
> --- a/src/virtManager/domain.py	Tue Jul 28 22:25:13 2009 -0400
> +++ b/src/virtManager/domain.py	Tue Aug 04 17:34:37 2009 +0200
> @@ -25,7 +25,7 @@
>  import time
>  import difflib
>  
> -from virtManager import util
> +from virtManager import util, storagevol
>  import virtinst.util as vutil
>  import virtinst
>  
> @@ -796,6 +796,17 @@
>  
>  
>      # ----------------
> +    # prettyify_disk_size formats the size in pretty string
> +    # val have to be passed in GB (as it's a result of VirtualDisk' size)
> +    # ----------------
> +

No need for the big comment here, the function is pretty straightforward.

> +    def prettyify_disk_size(self, val):
> +        if val > 1:
> +            return "%2.2f GB" % val
> +        else:
> +            return "%2.2f MB" % (val * 1024)
> +
> +    # ----------------
>      # get_X_devices functions: return a list of lists. Each sublist represents
>      # a device, of the format:
>      # [ device_type, unique_attribute(s), hw column label, attr1, attr2, ... ]
> @@ -840,11 +851,30 @@
>                  if devdst == None:
>                      raise RuntimeError("missing destination device")
>  
> +                capacity = None
> +                allocation = None
> +                selinux_label = None
> +                if typ == 'file':

This code would work for block devices as well, so you can drop this check.

> +                    if virtinst._util.is_storage_capable( self.connection.vmm ):
> +                        try:
> +                            vol = self.connection.vmm.storageVolLookupByPath( srcpath )
> +                            volobj = storagevol.vmmStorageVolume( self.config, self.connection.vmm, vol, vol )
> +                            capacity = volobj.get_pretty_capacity()
> +                            allocation = volobj.get_pretty_allocation()
> +                        except libvirt.libvirtError:
> +                            pass
> +

There is a helper function that does this in vmmConnection called
get_vol_by_path.

> +                    vd = virtinst.VirtualDisk( path = srcpath )

Your also going to want to pass conn=self.connection.vmm and probably
readOnly=True to avoid any validation errors here. Also wrap this in
'try ... except' since it can easily complain.

> +                    if capacity is None:
> +                        capacity = self.prettyify_disk_size( vd.size )
> +
> +                    #selinux_label = vd.selinux_label or "None"
> +

Why no selinux label info?

>                  # [ devicetype, unique, device target, source path,
>                  #   disk device type, disk type, readonly?, sharable?,
> -                #   bus type ]
> +                #   bus type, capacity, allocation ]
>                  disks.append(["disk", devdst, devdst, srcpath, devtype, typ,
> -                              readonly, sharable, bus])
> +                              readonly, sharable, bus, capacity, allocation])
>  
>              return disks
>  

UI changes look fine.

Thanks,
Cole