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. > [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; > _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
