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

Wolfgang Bumiller w.bumiller at proxmox.com
Thu Feb 1 10:46:10 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>
---
Changes to v1:
  - Paremter order switched
  - A limit of 0 means 'explicitly-unlimited' (while undef still means
    'no override was specified')
  - Fixed previously wrong cases and added a couple more testcases for
    for them.
  - Better factorization of $apply_limit->() should also make it easier
    in the future if we ever want to add another layer (like
    host-specific per-storage limits).

pve-common & cluster are still the same, so I'm not resending those
patches for now.

 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 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', ['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 at 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 at 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 at 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 at 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.11.0





More information about the pve-devel mailing list