[pve-devel] [PATCH v6 manager 1/2] vzdump: move remaining guest include logic to single method
Aaron Lauterer
a.lauterer at proxmox.com
Wed Jun 17 14:13:38 CEST 2020
The `guest include` logic handling `all` and `exclude` parameters was in
the `PVE::VZDump->exec_backup()` method. Moving this logic into the
`get_included_guests` method allows us to simplify and generalize it.
This helps to make the overall logic easier to test and develop other
features around vzdump backup jobs.
The method now returns a hash with node names as keys mapped to arrays
of VMIDs on these nodes that are included in the vzdump job.
The VZDump API call to create a new backup is adapted to use the new
method to create the list of local VMIDs and the skiplist.
Permission checks are kept where they are to be able to handle missing
permissions according to the current context. The old behavior to die
on a backup job when the user is missing the permission to a guest and
the job is not an 'all' or 'exclude' job is kept.
Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
---
v5 -> v6:
* incorporate suggested changes
* did not move sorting in PVE::VZDump::get_included_guests into the `if`
clause because that would sort them only for `all` and/or `exclude`
jobs but the VMIDs can be provided in random order for other job types
like manually selected VMIDs as well.
v4 -> v5:
* move handling of `all` and `exlcude` cases from
`PVE::VZDump->exec_backup` to `PVE::VZDump->get_included_guests`
* change return value to hash of node names with arrays of included
VMIDs to be more general
* adapt API call to start a VZDump job accordingly. Creating the list of
local VMIDs and the skiplist is handled right there again as so far
this is the only place where that kind of separation is needed
v3 -> v4:
* reworked the whole logic according to the discussion with
fabian [1].
* re-added missing early exit
[1] https://pve.proxmox.com/pipermail/pve-devel/2020-March/042451.html
PVE/API2/VZDump.pm | 19 ++++++-----
PVE/VZDump.pm | 80 ++++++++++++++++++++++------------------------
2 files changed, 50 insertions(+), 49 deletions(-)
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index 9a55dc22..2eda973e 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -69,18 +69,20 @@ __PACKAGE__->register_method ({
return 'OK' if $param->{node} && $param->{node} ne $nodename;
my $cmdline = PVE::VZDump::Common::command_line($param);
- my ($vmids, $skiplist) = PVE::VZDump::get_included_guests($param);
+
+ my $vmids_per_node = PVE::VZDump::get_included_guests($param);
+
+ my $local_vmids = delete $vmids_per_node->{$nodename} // [];
+
+ my $skiplist = [ map { @$_ } values $vmids_per_node->%* ];
if($param->{stop}){
PVE::VZDump::stop_running_backups();
- return 'OK' if !scalar(@{$vmids});
+ return 'OK' if !scalar(@{$local_vmids});
}
# silent exit if specified VMs run on other nodes
- return "OK" if !scalar(@{$vmids}) && !$param->{all};
-
- my @exclude = PVE::Tools::split_list(extract_param($param, 'exclude'));
- $param->{exclude} = PVE::VZDump::check_vmids(@exclude);
+ return "OK" if !scalar(@{$local_vmids}) && !$param->{all};
# exclude-path list need to be 0 separated
if (defined($param->{'exclude-path'})) {
@@ -94,7 +96,7 @@ __PACKAGE__->register_method ({
}
die "you can only backup a single VM with option --stdout\n"
- if $param->{stdout} && scalar(@{$vmids}) != 1;
+ if $param->{stdout} && scalar(@{$local_vmids}) != 1;
$rpcenv->check($user, "/storage/$param->{storage}", [ 'Datastore.AllocateSpace' ])
if $param->{storage};
@@ -106,6 +108,7 @@ __PACKAGE__->register_method ({
die "interrupted by signal\n";
};
+ $param->{vmids} = $local_vmids;
my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
eval {
@@ -143,7 +146,7 @@ __PACKAGE__->register_method ({
}
my $taskid;
- $taskid = $vmids->[0] if scalar(@{$vmids}) == 1;
+ $taskid = $local_vmids->[0] if scalar(@{$local_vmids}) == 1;
return $rpcenv->fork_worker('vzdump', $taskid, $user, $worker);
}});
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index bdbf641e..2f477606 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -1044,29 +1044,23 @@ sub exec_backup {
if scalar(@{$self->{skiplist}});
my $tasklist = [];
+ my $vzdump_plugins = {};
+ foreach my $plugin (@{$self->{plugins}}) {
+ my $type = $plugin->type();
+ next if exists $vzdump_plugins->{$type};
+ $vzdump_plugins->{$type} = $plugin;
+ }
- if ($opts->{all}) {
- foreach my $plugin (@{$self->{plugins}}) {
- my $vmlist = $plugin->vmlist();
- foreach my $vmid (sort @$vmlist) {
- next if grep { $_ eq $vmid } @{$opts->{exclude}};
- next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], 1);
- push @$tasklist, { vmid => $vmid, state => 'todo', plugin => $plugin, mode => $opts->{mode} };
- }
- }
- } else {
- foreach my $vmid (sort @{$opts->{vmids}}) {
- my $plugin;
- foreach my $pg (@{$self->{plugins}}) {
- my $vmlist = $pg->vmlist();
- if (grep { $_ eq $vmid } @$vmlist) {
- $plugin = $pg;
- last;
- }
- }
- $rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ]);
- push @$tasklist, { vmid => $vmid, state => 'todo', plugin => $plugin, mode => $opts->{mode} };
- }
+ my $vmlist = PVE::Cluster::get_vmlist();
+ foreach my $vmid (sort @{$opts->{vmids}}) {
+ my $guest_type = $vmlist->{ids}->{$vmid}->{type};
+ my $plugin = $vzdump_plugins->{$guest_type};
+ next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], $opts->{all});
+ push @$tasklist, {
+ vmid => $vmid,
+ state => 'todo',
+ plugin => $plugin,
+ mode => $opts->{mode} };
}
# Use in-memory files for the outer hook logs to pass them to sendmail.
@@ -1174,34 +1168,38 @@ sub get_included_guests {
my $nodename = PVE::INotify::nodename();
my $vmids = [];
+ my $vmids_per_node = {};
+
+ my $vmlist = PVE::Cluster::get_vmlist();
- # convert string lists to arrays
if ($job->{pool}) {
$vmids = PVE::API2Tools::get_resource_pool_guest_members($job->{pool});
+ } elsif ($job->{vmid}) {
+ $vmids = [ PVE::Tools::split_list($job->{vmid}) ];
} else {
- $vmids = [ PVE::Tools::split_list(extract_param($job, 'vmid')) ];
+ # all or exclude
+ my @exclude = PVE::Tools::split_list($job->{exclude});
+ @exclude = @{PVE::VZDump::check_vmids(@exclude)};
+ my $excludehash = { map { $_ => 1 } @exclude };
+
+ for my $id (keys %{ $vmlist->{ids} }) {
+ next if $excludehash->{$id};
+ push @$vmids, $id;
+ }
}
+ $vmids = [ sort {$a <=> $b} @$vmids];
- my $skiplist = [];
- if (!$job->{all}) {
- if (!$job->{node} || $job->{node} eq $nodename) {
- my $vmlist = PVE::Cluster::get_vmlist();
- my $localvmids = [];
- foreach my $vmid (@{$vmids}) {
- my $d = $vmlist->{ids}->{$vmid};
- if ($d && ($d->{node} ne $nodename)) {
- push @{$skiplist}, $vmid;
- } else {
- push @{$localvmids}, $vmid;
- }
- }
- $vmids = $localvmids;
- }
+ $vmids = PVE::VZDump::check_vmids(@$vmids);
+
+ foreach my $vmid (@$vmids) {
+ my $vmid_data = $vmlist->{ids}->{$vmid};
+ my $node = $vmid_data->{node};
- $job->{vmids} = PVE::VZDump::check_vmids(@{$vmids})
+ next if (defined $job->{node} && $job->{node} ne $node);
+ push @{$vmids_per_node->{$node}}, $vmid;
}
- return ($vmids, $skiplist);
+ return $vmids_per_node;
}
1;
--
2.20.1
More information about the pve-devel
mailing list