[pve-devel] [PATCH v4 manager 1/1] backup: move logic to include guests into method
Thomas Lamprecht
t.lamprecht at proxmox.com
Mon Apr 6 13:23:44 CEST 2020
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.lauterer at 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.
> [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?
> }
>
> - 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}}) {
> + $vzdump_plugins->{$p->type()} = $p;
my $type = $plugin->type();
vzdump_plugins->{$type} = $plugin;
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.
> + }
>
> - 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.
> + 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).
> + 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, 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?
> + @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,
} 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;
>
More information about the pve-devel
mailing list