Re: [pve-devel] [PATCH v2 storage] api: fix get content call for volumes

[Christian Ebner <c.ebner@proxmox.com>]

> On 07.03.2023 09:51 CET Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> On March 7, 2023 9:31 am, Fiona Ebner wrote:
> > Am 06.03.23 um 14:07 schrieb Christian Ebner:
> >> `pvesh get /nodes/{node}/storage/{storage}/content/{volume}` failed for
> >> several storage types, because the respective storage plugins returned
> >> only the volumes `size` on `volume_size_info` calls, while also the format
> >> is required.
> >> 
> >> This patch fixes the issue by returning also `format` and where possible `used`.
> >> 
> >> The issue was reported in the forum:
> >> https://forum.proxmox.com/threads/pvesh-get-nodes-node-storage-storage-content-volume-returns-error.123747/
> >> 
> >> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> >> ---
> >> 
> >> Changes since v1:
> >>  * Remove errous check for $used being set, rely on fallback to 0 if undef
> >>  * Return `parent` for RBD and ZFS
> >>  * Return `used` for ZFS
> >> 
> 
> [..]
> 
> >> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> >> index 9fbd149..acc50d9 100644
> >> --- a/PVE/Storage/ZFSPoolPlugin.pm
> >> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> >> @@ -446,13 +446,17 @@ sub status {
> >>  sub volume_size_info {
> >>      my ($class, $scfg, $storeid, $volname, $timeout) = @_;
> >>  
> >> -    my (undef, $vname, undef, undef, undef, undef, $format) =
> >> +    my (undef, $vname, undef, $parent, undef, undef, $format) =
> >>          $class->parse_volname($volname);
> >>  
> >>      my $attr = $format eq 'subvol' ? 'refquota' : 'volsize';
> >>      my $value = $class->zfs_get_properties($scfg, $attr, "$scfg->{pool}/$vname");
> >> +    my $used = $class->zfs_get_properties($scfg, 'used', "$scfg->{pool}/$vname");
> > 
> > used also includes usage of snapshots and children. I think
> > usedbydataset is better. Like that used <= size should be guaranteed.
> > See man zfsprops for more information.
> > 
> > Also, one call to zfs_get_properties should be enough by passing both
> > "$attr,used".
> > 
> > While you're at it, I'd also rename $value to $size then.
> 
> unfortunately not the case - a zvol can take up (significantly) more space than
> volsize:
> 
> $ zpool get ashift testpool
> NAME      PROPERTY  VALUE   SOURCE
> testpool  ashift    12      local
> $ zfs get volsize,volblocksize,space testpool/test
> NAME           PROPERTY              VALUE          SOURCE
> testpool/test  volsize               5G             local
> testpool/test  volblocksize          8K             default
> testpool/test  name                  testpool/test  -
> testpool/test  available             11.2G          -
> testpool/test  used                  7.43G          -
> testpool/test  usedbysnapshots       0B             -
> testpool/test  usedbydataset         7.32G          -
> testpool/test  usedbyrefreservation  111M           -
> testpool/test  usedbychildren        0B             -
> 
> this is the common "ashift=12 / volblocksize=8 / raidz2" pitfall..

Okay, but nevertheless it should make sense to use `usedbydataset` rather than `used`. The fact, that used > size is possible here is then an exercise in correct interpretation of the output anyway. Could this be documented in the API viewer?