[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