[pve-devel] [PATCH storage] add Storage::get_bandwidth_limit helper

Wolfgang Bumiller w.bumiller at proxmox.com
Tue Jan 30 16:34:42 CET 2018


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) = @_;
+
+    # 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);
+	    $apply_limit->($limits->{$operation})
+	    or $apply_limit->($limits->{default});
+	}
+    }
+
+    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..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





More information about the pve-devel mailing list