* [pve-devel] [PATCH v2 storage] api: fix get content call for volumes
@ 2023-03-06 13:07 Christian Ebner
2023-03-07 8:31 ` Fiona Ebner
0 siblings, 1 reply; 5+ messages in thread
From: Christian Ebner @ 2023-03-06 13:07 UTC (permalink / raw)
To: pve-devel
`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
PVE/Storage/ISCSIDirectPlugin.pm | 2 +-
PVE/Storage/RBDPlugin.pm | 4 ++--
PVE/Storage/ZFSPoolPlugin.pm | 8 ++++++--
3 files changed, 9 insertions(+), 5 deletions(-)
diff --git a/PVE/Storage/ISCSIDirectPlugin.pm b/PVE/Storage/ISCSIDirectPlugin.pm
index 9777969..eb329d4 100644
--- a/PVE/Storage/ISCSIDirectPlugin.pm
+++ b/PVE/Storage/ISCSIDirectPlugin.pm
@@ -208,7 +208,7 @@ sub volume_size_info {
my $vollist = iscsi_ls($scfg,$storeid);
my $info = $vollist->{$storeid}->{$volname};
- return $info->{size};
+ return wantarray ? ($info->{size}, 'raw', 0, undef) : $info->{size};
}
sub volume_resize {
diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
index 9047504..e7a2137 100644
--- a/PVE/Storage/RBDPlugin.pm
+++ b/PVE/Storage/RBDPlugin.pm
@@ -729,8 +729,8 @@ sub volume_size_info {
my ($class, $scfg, $storeid, $volname, $timeout) = @_;
my ($vtype, $name, $vmid) = $class->parse_volname($volname);
- my ($size, undef) = rbd_volume_info($scfg, $storeid, $name);
- return $size;
+ my ($size, $parent) = rbd_volume_info($scfg, $storeid, $name);
+ return wantarray ? ($size, 'raw', 0, $parent) : $size;
}
sub volume_resize {
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 = ($used =~ /^(\d+)$/) ? $1 : 0;
+
if ($value =~ /^(\d+)$/) {
- return $1;
+ return wantarray ? ($1, $format, $used, $parent) : $1;
}
die "Could not get zfs volume size\n";
--
2.30.2
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH v2 storage] api: fix get content call for volumes
2023-03-06 13:07 [pve-devel] [PATCH v2 storage] api: fix get content call for volumes Christian Ebner
@ 2023-03-07 8:31 ` Fiona Ebner
2023-03-07 8:51 ` Fabian Grünbichler
0 siblings, 1 reply; 5+ messages in thread
From: Fiona Ebner @ 2023-03-07 8:31 UTC (permalink / raw)
To: Proxmox VE development discussion, Christian Ebner
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
>
> PVE/Storage/ISCSIDirectPlugin.pm | 2 +-
> PVE/Storage/RBDPlugin.pm | 4 ++--
> PVE/Storage/ZFSPoolPlugin.pm | 8 ++++++--
> 3 files changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/PVE/Storage/ISCSIDirectPlugin.pm b/PVE/Storage/ISCSIDirectPlugin.pm
> index 9777969..eb329d4 100644
> --- a/PVE/Storage/ISCSIDirectPlugin.pm
> +++ b/PVE/Storage/ISCSIDirectPlugin.pm
> @@ -208,7 +208,7 @@ sub volume_size_info {
> my $vollist = iscsi_ls($scfg,$storeid);
> my $info = $vollist->{$storeid}->{$volname};
>
> - return $info->{size};
> + return wantarray ? ($info->{size}, 'raw', 0, undef) : $info->{size};
Hmm, maybe we can call Plugin's file_size_info, passing in the path with
iscsi://... to get $used (rather than always 0)? But would need to be
tested if the information is correct. The call could be made
conditionally, only if wantarray.
> }
>
> sub volume_resize {
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index 9047504..e7a2137 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -729,8 +729,8 @@ sub volume_size_info {
> my ($class, $scfg, $storeid, $volname, $timeout) = @_;
>
> my ($vtype, $name, $vmid) = $class->parse_volname($volname);
> - my ($size, undef) = rbd_volume_info($scfg, $storeid, $name);
> - return $size;
> + my ($size, $parent) = rbd_volume_info($scfg, $storeid, $name);
> + return wantarray ? ($size, 'raw', 0, $parent) : $size;
Can't we extend rbd_volume_info to also return $used or is that not
included in the rbd info result? If not, we could again consider
re-using file_size_info.
> }
>
> sub volume_resize {
> 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.
> +
> + $used = ($used =~ /^(\d+)$/) ? $1 : 0;
> +
> if ($value =~ /^(\d+)$/) {
> - return $1;
> + return wantarray ? ($1, $format, $used, $parent) : $1;
> }
>
> die "Could not get zfs volume size\n";
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH v2 storage] api: fix get content call for volumes
2023-03-07 8:31 ` Fiona Ebner
@ 2023-03-07 8:51 ` Fabian Grünbichler
2023-03-07 9:15 ` Fiona Ebner
2023-03-07 9:17 ` Christian Ebner
0 siblings, 2 replies; 5+ messages in thread
From: Fabian Grünbichler @ 2023-03-07 8:51 UTC (permalink / raw)
To: Christian Ebner, Proxmox VE development discussion
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..
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [pve-devel] [PATCH v2 storage] api: fix get content call for volumes
2023-03-07 8:51 ` Fabian Grünbichler
@ 2023-03-07 9:15 ` Fiona Ebner
2023-03-07 9:17 ` Christian Ebner
1 sibling, 0 replies; 5+ messages in thread
From: Fiona Ebner @ 2023-03-07 9:15 UTC (permalink / raw)
To: Fabian Grünbichler, Christian Ebner,
Proxmox VE development discussion
Am 07.03.23 um 09:51 schrieb Fabian Grünbichler:
> On March 7, 2023 9:31 am, Fiona Ebner wrote:
>
> 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..
Oh, right. Well, I'd say usedbydataset is still more consistent with the
current values for $used and I guess it's technically true even if $used
> $size.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pve-devel] [PATCH v2 storage] api: fix get content call for volumes
2023-03-07 8:51 ` Fabian Grünbichler
2023-03-07 9:15 ` Fiona Ebner
@ 2023-03-07 9:17 ` Christian Ebner
1 sibling, 0 replies; 5+ messages in thread
From: Christian Ebner @ 2023-03-07 9:17 UTC (permalink / raw)
To: Fabian Grünbichler, Proxmox VE development discussion
> 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?
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-03-07 9:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-06 13:07 [pve-devel] [PATCH v2 storage] api: fix get content call for volumes Christian Ebner
2023-03-07 8:31 ` Fiona Ebner
2023-03-07 8:51 ` Fabian Grünbichler
2023-03-07 9:15 ` Fiona Ebner
2023-03-07 9:17 ` Christian Ebner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal