[pve-devel] [PATCH v3 manager 1/1] backup: move logic to include guests into method
Fabian Grünbichler
f.gruenbichler at proxmox.com
Tue Mar 17 15:33:56 CET 2020
On March 16, 2020 4:44 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 will return the VMIDs for the whole cluster. If a VMID is
> present on the local node and thus part of the local backup job is still
> determined in the VZDump::exec_backup method.
this is not true. previously, that was handled partly by the API
already, so behaviour changed in some cases.
>
> 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.
>
> The creation of the skiplist is moved to the VZDump::exec_backup method,
> close to the place where it is used.
but it was used already in the API, so it made sense to have it there.
see comments in-line. I am not saying we can't change this behaviour,
but from this description it does not sound like it was (fully)
intentional.
>
> Signed-off-by: Aaron Lauterer <a.lauterer at proxmox.com>
> ---
> changes: rebased
>
> PVE/API2/VZDump.pm | 36 +++-------------------
> PVE/VZDump.pm | 74 ++++++++++++++++++++++++++++++++--------------
> 2 files changed, 55 insertions(+), 55 deletions(-)
>
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index f01e4de0..bc380099 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -69,43 +69,15 @@ __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'));
> - }
> +
> + $param->{vmids} = PVE::VZDump->get_included_guests($param, $nodename);
> + my @vmids = @{$param->{vmids}};
so this is just to not change a few call sites down below ;)
>
> if($param->{stop}){
> PVE::VZDump::stop_running_backups();
> return 'OK' if !scalar(@vmids);
> }
>
> - my $skiplist = [];
> - if (!$param->{all}) {
> - if (!$param->{node} || $param->{node} eq $nodename) {
so here skip list was only done when not including all VMs (since when
doing --all, we'd usually skip lots of VMs and don't want to be too
noisy I guess?)
> - 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;
and here the actual skipping happened (well, see below ;)). which means
that the API from this point on already had the filtered list, for
decisions such as
> - # silent exit if specified VMs run on other nodes
> - return "OK" if !scalar(@vmids);
this ^ , but also whether to associate the forked worker with a single
VMID or not, etc.pp.
> - }
> -
> - $param->{vmids} = PVE::VZDump::check_vmids(@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'} || '');
> @@ -130,7 +102,7 @@ __PACKAGE__->register_method ({
> die "interrupted by signal\n";
> };
>
> - my $vzdump = PVE::VZDump->new($cmdline, $param, $skiplist);
> + my $vzdump = PVE::VZDump->new($cmdline, $param);
>
> eval {
> $vzdump->getlock($upid); # only one process allowed
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index f3274196..9368d439 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);
>
> @@ -379,7 +380,7 @@ sub sendmail {
> };
>
> sub new {
> - my ($class, $cmdline, $opts, $skiplist) = @_;
> + my ($class, $cmdline, $opts) = @_;
>
> mkpath $logdir;
>
> @@ -418,8 +419,7 @@ sub new {
> $opts->{dumpdir} =~ s|/+$|| if ($opts->{dumpdir});
> $opts->{tmpdir} =~ s|/+$|| if ($opts->{tmpdir});
>
> - $skiplist = [] if !$skiplist;
> - my $self = bless { cmdline => $cmdline, opts => $opts, skiplist => $skiplist };
> + my $self = bless { cmdline => $cmdline, opts => $opts };
>
> my $findexcl = $self->{findexcl} = [];
> if ($defaults->{'exclude-path'}) {
> @@ -1026,33 +1026,33 @@ sub exec_backup {
>
> my $opts = $self->{opts};
>
> + my $nodename = PVE::INotify::nodename();
> + my $skiplist = [];
> +
> + if ($nodename && (!$opts->{node} ||$opts->{node} eq $nodename)) {
> + my $vmlist = PVE::Cluster::get_vmlist();
> + foreach my $vmid (@{$opts->{vmids}}) {
> + my $d = $vmlist->{ids}->{$vmid};
> + if ($d && ($d->{node} ne $nodename)) {
> + push @$skiplist, $vmid;
> + }
> + }
> + }
now skiplist is done always, but there is no actual skipping? (well,
there is, but only implicitly below because $plugin->vmlist() also
filters by node!)
> +
> debugmsg ('info', "starting new backup job: $self->{cmdline}", undef, 1);
> - debugmsg ('info', "skip external VMs: " . join(', ', @{$self->{skiplist}}))
> - if scalar(@{$self->{skiplist}});
> + debugmsg ('info', "skip external VMs: " . join(', ', @{$skiplist}))
> + if scalar(@{$skiplist});
>
> my $tasklist = [];
>
> - if ($opts->{all}) {
> + foreach my $vmid (sort @{$opts->{vmids}}) {
> 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;
> - }
> + if (grep { $_ eq $vmid } @$vmlist) {
> + next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ], $opts->{all});
> + push @$tasklist, { vmid => $vmid, state => 'todo', plugin => $plugin, mode => $opts->{mode} };
> + last;
> }
> - $rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Backup' ]);
> - push @$tasklist, { vmid => $vmid, state => 'todo', plugin => $plugin, mode => $opts->{mode} };
> }
> }
>
> @@ -1156,4 +1156,32 @@ sub stop_running_backups {
> }
> }
>
> +sub get_included_guests {
> + my ($self, $job, $nodename) = @_;
unused parameter $nodename?
> +
> + my @vmids;
> + my $vmlist = PVE::Cluster::get_vmlist();
> +
> + if ($job->{pool}) {
> + @vmids = @{PVE::API2Tools::get_resource_pool_guest_members($job->{pool})};
> + } elsif ($job->{vmid}) {
> + # selected VMIDs
> + @vmids = PVE::Tools::split_list(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};
but here we basically replicate the skiplist stuff (without logging), but
only for the 'no pool and no vmid list' case
> + }
> + }
> + @vmids = sort {$a <=> $b} @vmids;
> +
> + return PVE::VZDump::check_vmids(@vmids);
so why not return ($included, $skipped) here ?
otherwise we have one place where we do the skipping, and another place
where we do the log what we are skipping. now the code is the same, but
in XX weeks/months/years?
> +}
> +
> 1;
> --
> 2.20.1
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
More information about the pve-devel
mailing list