From 9edb99a5a763f03e031ffdce151c739d6ffaca0c Mon Sep 17 00:00:00 2001 From: Wolfgang Bumiller Date: Tue, 30 Jan 2018 11:46:19 +0100 Subject: [PATCH] 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 | 72 ++++++++++++ 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 | 182 +++++++++++++++++++++++++++++++ 15 files changed, 271 insertions(+), 1 deletion(-) create mode 100755 test/run_bwlimit_tests.pl diff --git a/PVE/Storage.pm b/PVE/Storage.pm index 73b21e1..995ebd3 100755 --- a/PVE/Storage.pm +++ b/PVE/Storage.pm @@ -1534,4 +1534,76 @@ 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, $storage_list, $override) = @_; + + # called for each limit (global, per-storage) with the 'default' and the + # $operation limit and should udpate $override for every limit affecting + # us. + my $use_global_limits = 0; + my $apply_limit = sub { + my ($bwlimit) = @_; + if (defined($bwlimit)) { + my $limits = PVE::JSONSchema::parse_property_string('bwlimit', $bwlimit); + my $limit = $limits->{$operation} // $limits->{default}; + if (defined($limit)) { + if (!$override || $limit < $override) { + $override = $limit; + } + return; + } + } + # If there was no applicable limit, try to apply the global ones. + $use_global_limits = 1; + }; + + 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; + 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); + $apply_limit->($storecfg->{bwlimit}); + } + } + + # Storage limits take precedence over the datacenter defaults, so if + # a limit was applied: + return $override if !$use_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'); + $apply_limit->($dc->{bwlimit}); + } + + return $override; +} + 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..e4f723f --- /dev/null +++ b/test/run_bwlimit_tests.pl @@ -0,0 +1,182 @@ +#!/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 + +dir: m50 + path /dir/e + bwlimit move=50 + +dir: d200 + path /dir/f + bwlimit default=200 + +EOF + +my $permissions = { + 'user1@test' => {}, + 'user2@test' => { '/' => ['Sys.Modify'], }, + 'user3@test' => { '/storage' => ['Datastore.Allocate'], }, + 'user4@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@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@pam' ], + [ ['unknown', ['nolimit'], undef], undef, 'root / generic default limit' ], + [ ['move', ['nolimit'], undef], undef, 'root / specific default limit (move)' ], + [ ['restore', ['nolimit'], undef], undef, 'root / specific default limit (restore)' ], + [ ['unknown', ['d50m40r30'], undef], undef, 'root / storage default limit' ], + [ ['move', ['d50m40r30'], undef], undef, 'root / specific storage limit (move)' ], + [ ['restore', ['d50m40r30'], undef], undef, 'root / specific storage limit (restore)' ], + + [ user => 'user1@test' ], + [ ['unknown', ['nolimit'], undef], 100, 'generic default limit' ], + [ ['move', ['nolimit'], undef], 80, 'specific default limit (move)' ], + [ ['restore', ['nolimit'], undef], 60, 'specific default limit (restore)' ], + [ ['unknown', ['d50m40r30'], undef], 50, 'storage default limit' ], + [ ['move', ['d50m40r30'], undef], 40, 'specific storage limit (move)' ], + [ ['restore', ['d50m40r30'], undef], 30, 'specific storage limit (restore)' ], + [ ['unknown', ['d200m400r300'], undef], 200, 'storage default limit above datacenter limits' ], + [ ['move', ['d200m400r300'], undef], 400, 'specific storage limit above datacenter limits (move)' ], + [ ['restore', ['d200m400r300'], undef], 300, 'specific storage limit above datacenter limits (restore)' ], + [ ['unknown', ['d50'], undef], 50, 'storage default limit' ], + [ ['move', ['d50'], undef], 50, 'storage default limit (move)' ], + [ ['restore', ['d50'], undef], 50, 'storage default limit (restore)' ], + + [ user => 'user2@test' ], + [ ['unknown', ['nolimit'], 0], 0, 'generic default limit with Sys.Modify, passing unlimited' ], + [ ['unknown', ['nolimit'], undef], undef, 'generic default limit with Sys.Modify' ], + [ ['restore', ['nolimit'], undef], undef, 'specific default limit with Sys.Modify (restore)' ], + [ ['move', ['nolimit'], undef], undef, 'specific default limit with Sys.Modify (move)' ], + [ ['unknown', ['d50m40r30'], undef], 50, 'storage default limit with Sys.Modify' ], + [ ['move', ['d50m40r30'], undef], 40, 'specific storage limit with Sys.Modify (move)' ], + [ ['restore', ['d50m40r30'], undef], 30, 'specific storage limit with Sys.Modify (restore)' ], + + [ user => 'user3@test' ], + [ ['unknown', ['nolimit'], 80], 80, 'generic default limit with privileges on /, passing an override value' ], + [ ['unknown', ['nolimit'], 0], 0, 'generic default limit with privileges on /, passing unlimited' ], + [ ['unknown', ['nolimit'], undef], undef, 'generic default limit with privileges on /' ], + [ ['move', ['nolimit'], undef], undef, 'specific default limit with privileges on / (move)' ], + [ ['restore', ['nolimit'], undef], undef, 'specific default limit with privileges on / (restore)' ], + [ ['unknown', ['d50m20r20'], 0], 0, 'storage default limit with privileges on /, passing unlimited' ], + [ ['unknown', ['d50m20r20'], undef], undef, 'storage default limit with privileges on /' ], + [ ['move', ['d50m20r20'], undef], undef, 'specific storage limit with privileges on / (move)' ], + [ ['restore', ['d50m20r20'], undef], undef, 'specific storage limit with privileges on / (restore)' ], + + [ user => 'user4@test' ], + [ ['unknown', ['nolimit'], 10], 10, 'generic default limit with privileges on a different storage, passing lower override' ], + [ ['unknown', ['nolimit'], undef], 100, 'generic default limit with privileges on a different storage' ], + [ ['unknown', ['nolimit'], 0], 100, 'generic default limit with privileges on a different storage, passing unlimited' ], + [ ['move', ['nolimit'], undef], 80, 'specific default limit with privileges on a different storage (move)' ], + [ ['restore', ['nolimit'], undef], 60, 'specific default limit with privileges on a different storage (restore)' ], + [ ['unknown', ['d50m40r30'], undef], 50, 'storage default limit with privileges on a different storage' ], + [ ['move', ['d50m40r30'], undef], 40, 'specific storage limit with privileges on a different storage (move)' ], + [ ['restore', ['d50m40r30'], undef], 30, 'specific storage limit with privileges on a different storage (restore)' ], + [ ['unknown', ['d20m40r30'], undef], undef, 'storage default limit with privileges on that storage' ], + [ ['unknown', ['d20m40r30'], 0], 0, 'storage default limit with privileges on that storage, passing unlimited' ], + [ ['move', ['d20m40r30'], undef], undef, 'specific storage limit with privileges on that storage (move)' ], + [ ['restore', ['d20m40r30'], undef], undef, 'specific storage limit with privileges on that storage (restore)' ], + [ ['unknown', ['d50m40r30', 'd20m40r30'], undef], 50, 'multiple storages default limit with privileges on one of them' ], + [ ['move', ['d50m40r30', 'd20m40r30'], undef], 40, 'multiple storages specific limit with privileges on one of them (move)' ], + [ ['restore', ['d50m40r30', 'd20m40r30'], undef], 30, 'multiple storages specific limit with privileges on one of them (restore)' ], + [ ['unknown', ['d10', 'd20m40r30'], undef], 10, 'multiple storages default limit with privileges on one of them (storage limited)' ], + [ ['move', ['d10', 'd20m40r30'], undef], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (move)' ], + [ ['restore', ['d10', 'd20m40r30'], undef], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (restore)' ], + [ ['restore', ['d10', 'd20m40r30'], 5], 5, 'multiple storages specific limit (storage limited) (restore), passing lower override' ], + [ ['restore', ['d200', 'd200m400r300'], 65], 65, 'multiple storages specific limit (storage limited) (restore), passing lower override' ], + [ ['restore', ['d200', 'd200m400r300'], 400], 200, 'multiple storages specific limit (storage limited) (restore), passing higher override' ], + [ ['restore', ['d200', 'd200m400r300'], 0], 200, 'multiple storages specific limit (storage limited) (restore), passing unlimited' ], + [ ['restore', ['d200', 'd200m400r300'], 1], 1, 'multiple storages specific limit (storage limited) (restore), passing 1' ], + [ ['restore', ['d10', 'd20m40r30'], 500], 10, 'multiple storages specific limit with privileges on one of them (storage limited) (restore), passing higher override' ], + [ ['unknown', ['nolimit', 'd20m40r30'], undef], 100, 'multiple storages default limit with privileges on one of them (default limited)' ], + [ ['move', ['nolimit', 'd20m40r30'], undef], 80, 'multiple storages specific limit with privileges on one of them (default limited) (move)' ], + [ ['restore', ['nolimit', 'd20m40r30'], undef], 60, 'multiple storages specific limit with privileges on one of them (default limited) (restore)' ], + [ ['restore', ['d20m40r30', 'm50'], 200], 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.39.2