On 4/6/20 1:23 PM, Thomas Lamprecht wrote:
On 4/3/20 4:11 PM, Aaron Lauterer wrote:
This extracts the logic which guests are to be included in a backup job
into its own method 'get_included_guests'. This makes it possible to
develop other features around backup jobs.
Logic which was spread out accross the API2/VZDump.pm file and the
VZDump.pm file is refactored into the new method, most notably the
logic that dealt with excluded guests.
The new method returns either just the included VMIDs on the local node
or an array with with the local VMIDs and the VMIDs present on other
nodes in the cluster. The latter is used for 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.laute...@proxmox.com>
---
v3 -> v4:
* reworked the whole logic according to the discussion with
fabian [0].
* re-added missing early exit
this seems to be a perfect fit for adding some light testing before doing
the change.
I'll look into it.
[0] https://pve.proxmox.com/pipermail/pve-devel/2020-March/042451.html
PVE/API2/VZDump.pm | 43 ++++++--------------------
PVE/VZDump.pm | 76 ++++++++++++++++++++++++++++++++--------------
2 files changed, 63 insertions(+), 56 deletions(-)
diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
index f01e4de0..88f53b82 100644
--- a/PVE/API2/VZDump.pm
+++ b/PVE/API2/VZDump.pm
@@ -69,43 +69,17 @@ __PACKAGE__->register_method ({
return 'OK' if $param->{node} && $param->{node} ne $nodename;
my $cmdline = PVE::VZDump::Common::command_line($param);
- my @vmids;
- # convert string lists to arrays
- if ($param->{pool}) {
- @vmids =
@{PVE::API2Tools::get_resource_pool_guest_members($param->{pool})};
- } else {
- @vmids = PVE::Tools::split_list(extract_param($param, 'vmid'));
- }
- if($param->{stop}){
- PVE::VZDump::stop_running_backups();
- return 'OK' if !scalar(@vmids);
- }
+ my ($vmids, $skiplist) = PVE::VZDump->get_included_guests($param);
- my $skiplist = [];
- if (!$param->{all}) {
- if (!$param->{node} || $param->{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;
- # silent exit if specified VMs run on other nodes
- return "OK" if !scalar(@vmids);
- }
+ # silent exit if specified guests run on other nodes
+ return "OK" if !scalar(@$vmids);
- $param->{vmids} = PVE::VZDump::check_vmids(@vmids)
+ if($param->{stop}){
+ PVE::VZDump::stop_running_backups();
+ return 'OK' if !scalar(@$vmids);
How can I even get to above, if we return 'OK' if @$vmids is empty before
this if already?
thx for catching this
}
- my @exclude = PVE::Tools::split_list(extract_param($param, 'exclude'));
- $param->{exclude} = PVE::VZDump::check_vmids(@exclude);
-
# exclude-path list need to be 0 separated
if (defined($param->{'exclude-path'})) {
my @expaths = split(/\0/, $param->{'exclude-path'} || '');
@@ -118,7 +92,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(@$vmids) != 1;
$rpcenv->check($user, "/storage/$param->{storage}", [ 'Datastore.AllocateSpace' ])
if $param->{storage};
@@ -130,6 +104,7 @@ __PACKAGE__->register_method ({
die "interrupted by signal\n";
};
+ $param->{vmids} = $vmids;
my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
eval {
@@ -167,7 +142,7 @@ __PACKAGE__->register_method ({
}
my $taskid;
- $taskid = $vmids[0] if scalar(@vmids) == 1;
+ $taskid = $$vmids[0] if scalar(@$vmids) == 1;
return $rpcenv->fork_worker('vzdump', $taskid, $user, $worker);
}});
diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index f3274196..3bcfd422 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -21,6 +21,7 @@ use PVE::RPCEnvironment;
use PVE::Storage;
use PVE::VZDump::Common;
use PVE::VZDump::Plugin;
+use PVE::Tools qw(extract_param);
my @posix_filesystems = qw(ext3 ext4 nfs nfs4 reiserfs xfs);
@@ -1026,34 +1027,23 @@ sub exec_backup {
my $opts = $self->{opts};
+ my $nodename = PVE::INotify::nodename();
+
debugmsg ('info', "starting new backup job: $self->{cmdline}", undef, 1);
debugmsg ('info', "skip external VMs: " . join(', ',
@{$self->{skiplist}}))
if scalar(@{$self->{skiplist}});
my $tasklist = [];
+ my $vzdump_plugins = {};
+ foreach my $p (@{$self->{plugins}}) {
I do not want such one character variables, even for short stuff
for my $plugin (@{$self->{plugins}}) {
Ok
+ $vzdump_plugins->{$p->type()} = $p;
my $type = $plugin->type();
vzdump_plugins->{$type} = $plugin;
ok
Also, this (+ related changes below) could well be it's own patch, it has
nothing
directly to do with pulling out the "which guest to include" logic out.
okay, will put this in a separate patch
+ }
- 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 $plugin = $vzdump_plugins->{$vmlist->{ids}->{$vmid}->{type}};
NAK, accessing other hashes with a hash key access is considered bad style,
avoid this.
same as above right?
+ 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.
@@ -1156,4 +1146,46 @@ sub stop_running_backups {
}
}
+sub get_included_guests {
+ my ($self, $job) = @_;
+
+ my $nodename = PVE::INotify::nodename();
+ my @vmids;
why not a array reference here? Would make most calls below a bit easier,
and cheaper (less copying).
okay, will do that
+ my $local_vmids = [];
+ my $foreign_vmids = [];
We never use "foreign" to describe that something is located on another
cluster node, use $cluster_vmids
Actually, we could just use a $hash with the nodes as key, as this seems a bit
a specific distinction to do in such a general method, e.g.:
{
node1 => [100, 200],
node1 => [3900, ...],
...
node1 => [...],
}
also a possible approach I agree. But then I would like to add more
wrapper methods. For now I definitely need one that returns all VMIDs
and one that separates between local and cluster ones. That would then
help the wantarray situation below.
Also, I like to avoid wantarray, at least if easily possible. If you'd need a
method for just the local vmids included in a job I'd add an additional method
which calls the more general one and filters plus explicitly tells the reader
the behaviour in it's name, e.g. "get_included_local_guests"
+
+ my $vmlist = PVE::Cluster::get_vmlist();
+
+ if ($job->{pool}) {
+ @vmids =
@{PVE::API2Tools::get_resource_pool_guest_members($job->{pool})};
+ } elsif ($job->{vmid}) {
+ # selected VMIDs
useless comment? What does it explain for a why or how question?
added it so one knows where the vmids in this step come from as it is
not as obvious, same goes for the comment in the next else clause
+ @vmids = PVE::Tools::split_list(extract_param($job, 'vmid'));
do we depend on the side effect that the $job hash ref has vmid deleted?
If so, a comment at the top of the method regarding this and exclude below
would be nice. Further,
AFAICT it's not really needed, the first elsif option should work as
well. Do I understand correctly that `extract_param()` should be avoided?
} elsif (my $vmids = $job->{vmid}) {
$vmids = [ PVE::Tools::split_list($vmid) ];
or
} elsif (my $vmids = delete $job->{vmid}) {
...
or if you really want to use extract_param
} elsif (my $vmids = $extract_param($job, 'vmid')) {
...
+ } else {
+ # all or exclude
+ my @exclude = PVE::Tools::split_list(extract_param($job, 'exclude'));
+ @exclude = @{PVE::VZDump::check_vmids(@exclude)};
+ my $excludehash = { map { $_ => 1 } @exclude };
+
+ foreach my $id (keys %{ $vmlist->{ids} }) {
+ next if $excludehash->{$id};
+ push @vmids, $id
+ if !$job->{node} || $vmlist->{ids}->{$id}->{node} eq
$job->{node};
+ }
+ }
+ @vmids = sort {$a <=> $b} @vmids;
+
+ @vmids = @{PVE::VZDump::check_vmids(@vmids)};
+
+ foreach my $vmid (@vmids) {
+ my $d = $vmlist->{ids}->{$vmid};
+ if ($d && ($d->{node} ne $nodename)) {
+ push @$foreign_vmids, $vmid;
+ } else {
+ push @$local_vmids, $vmid;
+ }
+ }
+ return wantarray ? ($local_vmids, $foreign_vmids) : $local_vmids;
+}
+
1;
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
