On 5/6/20 11:57 AM, Aaron Lauterer wrote: > 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.laute...@proxmox.com> > ---
looks good in general, nice! some comments and nits, as you probably will have to send a next version due to the "avoid $self in get_included_guests" change anyway, still below. > > 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 | 18 ++++++++--- > PVE/VZDump.pm | 76 +++++++++++++++++++++------------------------- > 2 files changed, 48 insertions(+), 46 deletions(-) > > diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm > index 68a3de89..77dac1b1 100644 > --- a/PVE/API2/VZDump.pm > +++ b/PVE/API2/VZDump.pm > @@ -69,19 +69,26 @@ __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 $vmids = $vmids_per_node->{$nodename} // []; maybe it could make belows looping easier if you delete the local ones out? So, for above: my $local_vmids = delete $vmids_per_node->{$nodename} // []; > + > + my $skiplist = []; > + > + for my $current_node (keys %{$vmids_per_node}) { > + push @{$skiplist}, @{$vmids_per_node->{$current_node}} > + if $current_node ne $nodename; with the delete above you could drop the post-if here actually you should be able to just do: my $skiplist = [ map { @$_ } values $vmids_per_node->%* ]; (FYI: $vmids_per_node->%* is just alternate writing of %$vmids_per_node IIRC, Wolfgang prefers it) > + } > > if($param->{stop}){ > PVE::VZDump::stop_running_backups(); > return 'OK' if !scalar(@{$vmids}); > } > > - # silent exit if specified VMs run on other nodes > + # silent exit if specified guests run on other nodes > return "OK" if !scalar(@{$vmids}); > > - 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'} || ''); > @@ -106,6 +113,7 @@ __PACKAGE__->register_method ({ > die "interrupted by signal\n"; > }; > > + $param->{vmids} = $vmids; > my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist); > > eval { > diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm > index 68c08f47..cd278f4d 100644 > --- a/PVE/VZDump.pm > +++ b/PVE/VZDump.pm > @@ -1039,29 +1039,18 @@ sub exec_backup { > if scalar(@{$self->{skiplist}}); > > my $tasklist = []; > + my $vzdump_plugins = {}; > + foreach my $plugin (@{$self->{plugins}}) { maybe add: next if exists $vzdump_plugins->{$type}; as then we'd just overwrite the same type again. > + my $type = $plugin->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} }; I now it blows up lines a bit but I prefer having multiple hash elements on separate ones most of the time. push @$tasklist, { vmid => $vmid, ..., }; Easier to read and change. > } > > # Use in-memory files for the outer hook logs to pass them to sendmail. > @@ -1169,34 +1158,39 @@ 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} }) { you could sort it here already, avoiding the sort after the loop. Whole thing could also be written as: $vmids = [ grep { !$excludehash->{$_} } sort keys $vmlist->{ids}->%* ]; but no hard feeling here. > + 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}; > + > + next if (defined $job->{node} && $job->{node} ne $node);> > - $job->{vmids} = PVE::VZDump::check_vmids(@{$vmids}) > + push @{$vmids_per_node->{$node}}, $vmid; > } > > - return ($vmids, $skiplist); > + return $vmids_per_node; > } > > 1; > _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
