Re: [pve-devel] [PATCH storage] add Storage::get_bandwidth_limit helper
see inline
On Tue, Jan 30, 2018 at 04:34:42PM +0100, Wolfgang Bumiller wrote:
> Takes an operation, an optional requested bandwidth
> limit override, and a list of storages involved in the
> operation and lowers the requested bandwidth against global
> and storage-specific limits unless the user has permissions
> to change those.
> This means:
> * Global limits apply to all users without Sys.Modify on /
>(as they can change datacenter.cfg options via the API).
> * Storage specific limits apply to users without
>Datastore.Allocate access on /storage/X for any involved
>storage X.
>
> Signed-off-by: Wolfgang Bumiller
> ---
> PVE/Storage.pm | 84
> PVE/Storage/DRBDPlugin.pm| 1 +
> PVE/Storage/DirPlugin.pm | 2 +
> PVE/Storage/GlusterfsPlugin.pm | 1 +
> PVE/Storage/ISCSIDirectPlugin.pm | 1 +
> PVE/Storage/ISCSIPlugin.pm | 1 +
> PVE/Storage/LVMPlugin.pm | 1 +
> PVE/Storage/LvmThinPlugin.pm | 1 +
> PVE/Storage/NFSPlugin.pm | 1 +
> PVE/Storage/RBDPlugin.pm | 1 +
> PVE/Storage/SheepdogPlugin.pm| 1 +
> PVE/Storage/ZFSPlugin.pm | 1 +
> PVE/Storage/ZFSPoolPlugin.pm | 1 +
> test/Makefile| 5 +-
> test/run_bwlimit_tests.pl| 163
> +++
> 15 files changed, 264 insertions(+), 1 deletion(-)
> create mode 100755 test/run_bwlimit_tests.pl
>
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 73b21e1..7c07500 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1534,4 +1534,88 @@ sub complete_volume {
> return $res;
> }
>
> +# Various io-heavy operations require io/bandwidth limits which can be
> +# configured on multiple levels: The global defaults in datacenter.cfg, and
> +# per-storage overrides. When we want to do a restore from storage A to
> storage
> +# B, we should take the smaller limit defined for storages A and B, and if no
> +# such limit was specified, use the one from datacenter.cfg.
> +sub get_bandwidth_limit {
> +my ($operation, $override, $storage_list) = @_;
I'd switch the order here - no override is much more common than no
involved storages ;)
> +
> +# called for each limit (global, per-storage) with the 'default' and the
> +# $operation limit and should udpate $override for every limit affecting
s/udpate/update
> +# us.
> +my $was_limited = 0;
> +my $apply_limit = sub {
> + my ($max) = @_;
> + if (defined($max) && (!defined($override) || $max < $override)) {
> + $override = $max;
> + $was_limited = 1;
> + return 1;
> + }
> + return 0;
> +};
> +
> +my $rpcenv = PVE::RPCEnvironment->get();
> +my $authuser = $rpcenv->get_user();
> +
> +# Apply per-storage limits - if there are storages involved.
> +if (@$storage_list) {
> + my $config = config();
> +
> + # The Datastore.Allocate permission allows us to modify the per-storage
> + # limits, therefore it also allows us to override them.
> + # Since we have most likely multiple storages to check, do a quick
> check on
> + # the general '/storage' path to see if we can skip the checks entirely:
> + return $override if $rpcenv->check($authuser, '/storage',
> ['Datastore.Allocate'], 1);
this is kind of strange. if you ONLY have Datastore.Allocate on /storage
(without propagation), you can create a storage, but don't see it on the
GUI afterwards - likely because the GET path for the storage config needs
Datastore.Allocate on that specific storage? but the PUT and DELETE
don't need it for the specific storage..
> +
> + my %done;
> + my $enforce_global_limits = 0;
> + foreach my $storage (@$storage_list) {
> + # Avoid duplicate checks:
> + next if $done{$storage};
> + $done{$storage} = 1;
> +
> + # Otherwise we may still have individual /storage/$ID permissions:
> + if (!$rpcenv->check($authuser, "/storage/$storage",
> ['Datastore.Allocate'], 1)) {
> + # And if not: apply the limits.
> + my $storecfg = storage_config($config, $storage);
> + if (defined(my $bwlimit = $storecfg->{bwlimit})) {
> + my $limits =
> PVE::JSONSchema::parse_property_string('bwlimit', $bwlimit);
> + $apply_limit->($limits->{$operation})
> + or $apply_limit->($limits->{default});
isn't this wrong though?
$operation=move
$override=5
bwlimit: default=1,move=10
$apply_limit->(10) returns 0, so
$apply_limit->(1) gets called and limits to 1, even though the specific
limit was 10 and overriding with 5 is perfectly fine.
I think we want to do something like:
my $limit = $limits->{default};
$limit = $limits->{$operation} if defined($limits->{$operation});
$apply_limit($limit);
to preserve "specific limit beats general limit", and allow 0 for
unlimited. if neither is set, $limit is undef and $apply_limit re
[pve-devel] [PATCH storage] add Storage::get_bandwidth_limit helper
Takes an operation, an optional requested bandwidth
limit override, and a list of storages involved in the
operation and lowers the requested bandwidth against global
and storage-specific limits unless the user has permissions
to change those.
This means:
* Global limits apply to all users without Sys.Modify on /
(as they can change datacenter.cfg options via the API).
* Storage specific limits apply to users without
Datastore.Allocate access on /storage/X for any involved
storage X.
Signed-off-by: Wolfgang Bumiller
---
PVE/Storage.pm | 84
PVE/Storage/DRBDPlugin.pm| 1 +
PVE/Storage/DirPlugin.pm | 2 +
PVE/Storage/GlusterfsPlugin.pm | 1 +
PVE/Storage/ISCSIDirectPlugin.pm | 1 +
PVE/Storage/ISCSIPlugin.pm | 1 +
PVE/Storage/LVMPlugin.pm | 1 +
PVE/Storage/LvmThinPlugin.pm | 1 +
PVE/Storage/NFSPlugin.pm | 1 +
PVE/Storage/RBDPlugin.pm | 1 +
PVE/Storage/SheepdogPlugin.pm| 1 +
PVE/Storage/ZFSPlugin.pm | 1 +
PVE/Storage/ZFSPoolPlugin.pm | 1 +
test/Makefile| 5 +-
test/run_bwlimit_tests.pl| 163 +++
15 files changed, 264 insertions(+), 1 deletion(-)
create mode 100755 test/run_bwlimit_tests.pl
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 73b21e1..7c07500 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1534,4 +1534,88 @@ sub complete_volume {
return $res;
}
+# Various io-heavy operations require io/bandwidth limits which can be
+# configured on multiple levels: The global defaults in datacenter.cfg, and
+# per-storage overrides. When we want to do a restore from storage A to storage
+# B, we should take the smaller limit defined for storages A and B, and if no
+# such limit was specified, use the one from datacenter.cfg.
+sub get_bandwidth_limit {
+my ($operation, $override, $storage_list) = @_;
+
+# called for each limit (global, per-storage) with the 'default' and the
+# $operation limit and should udpate $override for every limit affecting
+# us.
+my $was_limited = 0;
+my $apply_limit = sub {
+ my ($max) = @_;
+ if (defined($max) && (!defined($override) || $max < $override)) {
+ $override = $max;
+ $was_limited = 1;
+ return 1;
+ }
+ return 0;
+};
+
+my $rpcenv = PVE::RPCEnvironment->get();
+my $authuser = $rpcenv->get_user();
+
+# Apply per-storage limits - if there are storages involved.
+if (@$storage_list) {
+ my $config = config();
+
+ # The Datastore.Allocate permission allows us to modify the per-storage
+ # limits, therefore it also allows us to override them.
+ # Since we have most likely multiple storages to check, do a quick
check on
+ # the general '/storage' path to see if we can skip the checks entirely:
+ return $override if $rpcenv->check($authuser, '/storage',
['Datastore.Allocate'], 1);
+
+ my %done;
+ my $enforce_global_limits = 0;
+ foreach my $storage (@$storage_list) {
+ # Avoid duplicate checks:
+ next if $done{$storage};
+ $done{$storage} = 1;
+
+ # Otherwise we may still have individual /storage/$ID permissions:
+ if (!$rpcenv->check($authuser, "/storage/$storage",
['Datastore.Allocate'], 1)) {
+ # And if not: apply the limits.
+ my $storecfg = storage_config($config, $storage);
+ if (defined(my $bwlimit = $storecfg->{bwlimit})) {
+ my $limits =
PVE::JSONSchema::parse_property_string('bwlimit', $bwlimit);
+ $apply_limit->($limits->{$operation})
+ or $apply_limit->($limits->{default});
+ } else {
+ # In case this storage has no limits, remember we *must*
+ # apply the global limits in its place
+ $enforce_global_limits = 1;
+ }
+ } else {
+ # we were specificially allowed to override the storage's limit
+ # and if no other storages are used we can skip global defaults
+ # unless $enforce_global_limits is set
+ $was_limited = 1;
+ }
+ }
+
+ # Storage limits take precedence over the datacenter defaults, so if
+ # a limit was applied:
+ return $override if $was_limited && !$enforce_global_limits;
+}
+
+# Sys.Modify on '/' means we can change datacenter.cfg which contains the
+# global default limits.
+if (!$rpcenv->check($authuser, '/', ['Sys.Modify'], 1)) {
+ # So if we cannot modify global limits, apply them to our currently
+ # requested override.
+ my $dc = cfs_read_file('datacenter.cfg');
+ if (defined(my $bwlimit = $dc->{bwlimit})) {
+ my $limits = PVE::JSONSchema::parse_property_string('bwlimit',
$bwlimit);
+ $ap
