[pve-devel] [PATCH storage] add Storage::get_bandwidth_limit helper
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Jan 31 10:14:38 CET 2018
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 <w.bumiller at proxmox.com>
> ---
> 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 returns 0
without touching $override.
> + } else {
> + # In case this storage has no limits, remember we *must*
> + # apply the global limits in its place
> + $enforce_global_limits = 1;
this is wrong/incomplete, unless we make default mandatory. the storage
could have a limit just for one specific operation, which we don't care
about here.
maybe we can re-use my previous comment, and set $enforce_global_limit
if $limit is undef in addition to this else here?
> + }
> + } 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;
but only if there actually is an override?
> + }
> + }
> +
> + # 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);
> + $apply_limit->($limits->{$operation})
> + or $apply_limit->($limits->{default});
same as above.
> + }
> + }
> +
> + return $override;
depending on how the limits are implemented it might make sense to
return an explicit '0' here if no limit is set? otherwise, you'd need to
check for both undef and 0 :P
> +}
> +
> 1;
> diff --git a/PVE/Storage/DRBDPlugin.pm b/PVE/Storage/DRBDPlugin.pm
> index 7536b42..75d53aa 100644
> --- a/PVE/Storage/DRBDPlugin.pm
> +++ b/PVE/Storage/DRBDPlugin.pm
> @@ -45,6 +45,7 @@ sub options {
> content => { optional => 1 },
> nodes => { optional => 1 },
> disable => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/DirPlugin.pm b/PVE/Storage/DirPlugin.pm
> index b3ddb5b..5224f4d 100644
> --- a/PVE/Storage/DirPlugin.pm
> +++ b/PVE/Storage/DirPlugin.pm
> @@ -42,6 +42,7 @@ sub properties {
> type => 'string',
> default => 'no',
> },
> + bwlimit => get_standard_option('bwlimit'),
> };
> }
>
> @@ -56,6 +57,7 @@ sub options {
> format => { optional => 1 },
> mkdir => { optional => 1 },
> is_mountpoint => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/GlusterfsPlugin.pm b/PVE/Storage/GlusterfsPlugin.pm
> index d8b7c8c..f3aa030 100644
> --- a/PVE/Storage/GlusterfsPlugin.pm
> +++ b/PVE/Storage/GlusterfsPlugin.pm
> @@ -136,6 +136,7 @@ sub options {
> content => { optional => 1 },
> format => { optional => 1 },
> mkdir => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/ISCSIDirectPlugin.pm b/PVE/Storage/ISCSIDirectPlugin.pm
> index 792d866..67bb40c 100644
> --- a/PVE/Storage/ISCSIDirectPlugin.pm
> +++ b/PVE/Storage/ISCSIDirectPlugin.pm
> @@ -68,6 +68,7 @@ sub options {
> nodes => { optional => 1},
> disable => { optional => 1},
> content => { optional => 1},
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/ISCSIPlugin.pm b/PVE/Storage/ISCSIPlugin.pm
> index 04b5172..131ffa0 100644
> --- a/PVE/Storage/ISCSIPlugin.pm
> +++ b/PVE/Storage/ISCSIPlugin.pm
> @@ -261,6 +261,7 @@ sub options {
> nodes => { optional => 1},
> disable => { optional => 1},
> content => { optional => 1},
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/LVMPlugin.pm b/PVE/Storage/LVMPlugin.pm
> index 9633e4c..94d3a33 100644
> --- a/PVE/Storage/LVMPlugin.pm
> +++ b/PVE/Storage/LVMPlugin.pm
> @@ -202,6 +202,7 @@ sub options {
> content => { optional => 1 },
> base => { fixed => 1, optional => 1 },
> tagged_only => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/LvmThinPlugin.pm b/PVE/Storage/LvmThinPlugin.pm
> index ccf5b7b..cb2c1a2 100644
> --- a/PVE/Storage/LvmThinPlugin.pm
> +++ b/PVE/Storage/LvmThinPlugin.pm
> @@ -49,6 +49,7 @@ sub options {
> nodes => { optional => 1 },
> disable => { optional => 1 },
> content => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/NFSPlugin.pm b/PVE/Storage/NFSPlugin.pm
> index 2f75eee..5862d82 100644
> --- a/PVE/Storage/NFSPlugin.pm
> +++ b/PVE/Storage/NFSPlugin.pm
> @@ -86,6 +86,7 @@ sub options {
> content => { optional => 1 },
> format => { optional => 1 },
> mkdir => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/RBDPlugin.pm b/PVE/Storage/RBDPlugin.pm
> index decfbf5..217434a 100644
> --- a/PVE/Storage/RBDPlugin.pm
> +++ b/PVE/Storage/RBDPlugin.pm
> @@ -278,6 +278,7 @@ sub options {
> username => { optional => 1 },
> content => { optional => 1 },
> krbd => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/SheepdogPlugin.pm b/PVE/Storage/SheepdogPlugin.pm
> index d9542a8..f10ef31 100644
> --- a/PVE/Storage/SheepdogPlugin.pm
> +++ b/PVE/Storage/SheepdogPlugin.pm
> @@ -115,6 +115,7 @@ sub options {
> disable => { optional => 1 },
> portal => { fixed => 1 },
> content => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/ZFSPlugin.pm b/PVE/Storage/ZFSPlugin.pm
> index d4cbb8d..f88fe94 100644
> --- a/PVE/Storage/ZFSPlugin.pm
> +++ b/PVE/Storage/ZFSPlugin.pm
> @@ -205,6 +205,7 @@ sub options {
> comstar_hg => { optional => 1 },
> comstar_tg => { optional => 1 },
> content => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
> index b971e3a..e864a58 100644
> --- a/PVE/Storage/ZFSPoolPlugin.pm
> +++ b/PVE/Storage/ZFSPoolPlugin.pm
> @@ -43,6 +43,7 @@ sub options {
> nodes => { optional => 1 },
> disable => { optional => 1 },
> content => { optional => 1 },
> + bwlimit => { optional => 1 },
> };
> }
>
> diff --git a/test/Makefile b/test/Makefile
> index b44d7be..833a597 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -1,9 +1,12 @@
> all: test
>
> -test: test_zfspoolplugin test_disklist
> +test: test_zfspoolplugin test_disklist test_bwlimit
>
> test_zfspoolplugin: run_test_zfspoolplugin.pl
> ./run_test_zfspoolplugin.pl
>
> test_disklist: run_disk_tests.pl
> ./run_disk_tests.pl
> +
> +test_bwlimit: run_bwlimit_tests.pl
> + ./run_bwlimit_tests.pl
> diff --git a/test/run_bwlimit_tests.pl b/test/run_bwlimit_tests.pl
> new file mode 100755
> index 0000000..08e96b1
> --- /dev/null
> +++ b/test/run_bwlimit_tests.pl
> @@ -0,0 +1,163 @@
> +#!/usr/bin/perl
> +
> +use strict;
> +use warnings;
> +
> +use Test::MockModule;
> +use Test::More;
> +
> +use lib ('.', '..');
> +use PVE::RPCEnvironment;
> +use PVE::Cluster;
> +use PVE::Storage;
> +
> +my $datacenter_cfg = <<'EOF';
> +bwlimit: default=100,move=80,restore=60
> +EOF
> +
> +my $storage_cfg = <<'EOF';
> +dir: nolimit
> + path /dir/a
> +
> +dir: d50
> + path /dir/b
> + bwlimit default=50
> +
> +dir: d50m40r30
> + path /dir/c
> + bwlimit default=50,move=40,restore=30
> +
> +dir: d20m40r30
> + path /dir/c
> + bwlimit default=20,move=40,restore=30
> +
> +dir: d200m400r300
> + path /dir/c
> + bwlimit default=200,move=400,restore=300
> +
> +dir: d10
> + path /dir/d
> + bwlimit default=10
> +EOF
> +
> +my $permissions = {
> + 'user1 at test' => {},
> + 'user2 at test' => { '/' => ['Sys.Modify'], },
> + 'user3 at test' => { '/storage' => ['Datastore.Allocate'], },
> + 'user4 at test' => { '/storage/d20m40r30' => ['Datastore.Allocate'], },
> +};
> +
> +my $pve_cluster_module;
> +$pve_cluster_module = Test::MockModule->new('PVE::Cluster');
> +$pve_cluster_module->mock(
> + cfs_update => sub {},
> + get_config => sub {
> + my ($file) = @_;
> + if ($file eq 'datacenter.cfg') {
> + return $datacenter_cfg;
> + } elsif ($file eq 'storage.cfg') {
> + return $storage_cfg;
> + }
> + die "TODO: mock get_config($file)\n";
> + },
> +);
> +
> +my $rpcenv_module;
> +$rpcenv_module = Test::MockModule->new('PVE::RPCEnvironment');
> +$rpcenv_module->mock(
> + check => sub {
> + my ($env, $user, $path, $perms, $noerr) = @_;
> + return 1 if $user eq 'root at pam';
> + my $userperms = $permissions->{$user}
> + or die "no permissions defined for user $user\n";
> + if (defined(my $pathperms = $userperms->{$path})) {
> + foreach my $pp (@$pathperms) {
> + foreach my $reqp (@$perms) {
> + return 1 if $pp eq $reqp;
> + }
> + }
> + }
> + die "permission denied\n" if !$noerr;
> + return 0;
> + },
> +);
> +
> +my $rpcenv = PVE::RPCEnvironment->init('pub');
> +
> +my @tests = (
> + [ user => 'root at pam' ],
> + [ ['unknown', undef, ['nolimit']], undef, 'root / generic default limit' ],
> + [ ['move', undef, ['nolimit']], undef, 'root / specific default limit (move)' ],
> + [ ['restore', undef, ['nolimit']], undef, 'root / specific default limit (restore)' ],
> + [ ['unknown', undef, ['d50m40r30']], undef, 'root / storage default limit' ],
> + [ ['move', undef, ['d50m40r30']], undef, 'root / specific storage limit (move)' ],
> + [ ['restore', undef, ['d50m40r30']], undef, 'root / specific storage limit (restore)' ],
> +
> + [ user => 'user1 at test' ],
> + [ ['unknown', undef, ['nolimit']], 100, 'generic default limit' ],
> + [ ['move', undef, ['nolimit']], 80, 'specific default limit (move)' ],
> + [ ['restore', undef, ['nolimit']], 60, 'specific default limit (restore)' ],
> + [ ['unknown', undef, ['d50m40r30']], 50, 'storage default limit' ],
> + [ ['move', undef, ['d50m40r30']], 40, 'specific storage limit (move)' ],
> + [ ['restore', undef, ['d50m40r30']], 30, 'specific storage limit (restore)' ],
> + [ ['unknown', undef, ['d200m400r300']], 200, 'storage default limit above datacenter limits' ],
> + [ ['move', undef, ['d200m400r300']], 400, 'specific storage limit above datacenter limits (move)' ],
> + [ ['restore', undef, ['d200m400r300']], 300, 'specific storage limit above datacenter limits (restore)' ],
> + [ ['unknown', undef, ['d50']], 50, 'storage default limit' ],
> + [ ['move', undef, ['d50']], 50, 'storage default limit (move)' ],
> + [ ['restore', undef, ['d50']], 50, 'storage default limit (restore)' ],
> +
> + [ user => 'user2 at test' ],
> + [ ['unknown', undef, ['nolimit']], undef, 'generic default limit with Sys.Modify' ],
> + [ ['restore', undef, ['nolimit']], undef, 'specific default limit with Sys.Modify (restore)' ],
> + [ ['move', undef, ['nolimit']], undef, 'specific default limit with Sys.Modify (move)' ],
> + [ ['unknown', undef, ['d50m40r30']], 50, 'storage default limit with Sys.Modify' ],
> + [ ['move', undef, ['d50m40r30']], 40, 'specific storage limit with Sys.Modify (move)' ],
> + [ ['restore', undef, ['d50m40r30']], 30, 'specific storage limit with Sys.Modify (restore)' ],
> +
> + [ user => 'user3 at test' ],
> + [ ['unknown', 80, ['nolimit']], 80, 'generic default limit with privileges on /, passing an override value' ],
> + [ ['unknown', undef, ['nolimit']], undef, 'generic default limit with privileges on /' ],
> + [ ['move', undef, ['nolimit']], undef, 'specific default limit with privileges on / (move)' ],
> + [ ['restore', undef, ['nolimit']], undef, 'specific default limit with privileges on / (restore)' ],
> + [ ['unknown', undef, ['d50m20r20']], undef, 'storage default limit with privileges on /' ],
> + [ ['move', undef, ['d50m20r20']], undef, 'specific storage limit with privileges on / (move)' ],
> + [ ['restore', undef, ['d50m20r20']], undef, 'specific storage limit with privileges on / (restore)' ],
> +
> + [ user => 'user4 at test' ],
> + [ ['unknown', 10, ['nolimit']], 10, 'generic default limit with privileges on a different storage, passing lower override' ],
> + [ ['unknown', undef, ['nolimit']], 100, 'generic default limit with privileges on a different storage' ],
> + [ ['move', undef, ['nolimit']], 80, 'specific default limit with privileges on a different storage (move)' ],
> + [ ['restore', undef, ['nolimit']], 60, 'specific default limit with privileges on a different storage (restore)' ],
> + [ ['unknown', undef, ['d50m40r30']], 50, 'storage default limit with privileges on a different storage' ],
> + [ ['move', undef, ['d50m40r30']], 40, 'specific storage limit with privileges on a different storage (move)' ],
> + [ ['restore', undef, ['d50m40r30']], 30, 'specific storage limit with privileges on a different storage (restore)' ],
> + [ ['unknown', undef, ['d20m40r30']], undef, 'storage default limit with privileges on that storage' ],
> + [ ['move', undef, ['d20m40r30']], undef, 'specific storage limit with privileges on that storage (move)' ],
> + [ ['restore', undef, ['d20m40r30']], undef, 'specific storage limit with privileges on that storage (restore)' ],
> + [ ['unknown', undef, ['d50m40r30', 'd20m40r30']], 50, 'multiple storages default limit with privileges on one of them' ],
> + [ ['move', undef, ['d50m40r30', 'd20m40r30']], 40, 'multiple storages specific limit with privileges on one of them (move)' ],
> + [ ['restore', undef, ['d50m40r30', 'd20m40r30']], 30, 'multiple storages specific limit with privileges on one of them (restore)' ],
> + [ ['unknown', undef, ['d10', 'd20m40r30']], 10, 'multiple storages default limit with privileges on one of them (storage limited)' ],
> + [ ['move', undef, ['d10', 'd20m40r30']], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (move)' ],
> + [ ['restore', undef, ['d10', 'd20m40r30']], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (restore)' ],
> + [ ['restore', 5, ['d10', 'd20m40r30']], 5, 'multiple storages specific limit with privileges on one of them (storage limited) (restore), passing lower override' ],
> + [ ['restore', 500, ['d10', 'd20m40r30']], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (restore), passing higher override' ],
> + [ ['unknown', undef, ['nolimit', 'd20m40r30']], 100, 'multiple storages default limit with privileges on one of them (default limited)' ],
> + [ ['move', undef, ['nolimit', 'd20m40r30']], 80, 'multiple storages specific limit with privileges on one of them (default limited) (move)' ],
> + [ ['restore', undef, ['nolimit', 'd20m40r30']], 60, 'multiple storages specific limit with privileges on one of them (default limited) (restore)' ],
> +);
> +
> +foreach my $t (@tests) {
> + my ($args, $expected, $description) = @$t;
> + if (!ref($args)) {
> + if ($args eq 'user') {
> + $rpcenv->set_user($expected);
> + } else {
> + die "not a test specification\n";
> + }
> + next;
> + }
> + is(PVE::Storage::get_bandwidth_limit(@$args), $expected, $description);
> +}
> +done_testing();
> --
> 2.11.0
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
More information about the pve-devel
mailing list