On 5/4/20 4:08 PM, Aaron Lauterer wrote:
> As a first step to make the whole guest include logic more testable the
> part from the API endpoint has been moved to its own method with as
> little changes as possible.
>
> Everything concerning `all` and `exclude` logic is still in the
> PVE::VZDump->exec_backup() method.
>
> Signed-off-by: Aaron Lauterer <a.laute...@proxmox.com>
> ---
>
> v1 -> v2:
> * fixed return value. Array refs inside an array lead to nested
> arrays not working with `my ($foo, $bar) = method();`
>
>
> As talked with thomas on[0] and off list, this patch series is meant to
> have more confidence in the ongoing changes.
>
> My other ongoing patch series [1] will move the all the logic, even the
> one in the `exec_backup()` method into one single method.
>
> [0] https://pve.proxmox.com/pipermail/pve-devel/2020-April/042795.html
> [1] https://pve.proxmox.com/pipermail/pve-devel/2020-April/042753.html
>
> PVE/API2/VZDump.pm | 36 ++++++------------------------------
> PVE/VZDump.pm | 36 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 42 insertions(+), 30 deletions(-)
>
> diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm
> index f01e4de0..68a3de89 100644
> --- a/PVE/API2/VZDump.pm
> +++ b/PVE/API2/VZDump.pm
> @@ -69,39 +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'));
> - }
> + my ($vmids, $skiplist) = PVE::VZDump->get_included_guests($param);
>
> if($param->{stop}){
> PVE::VZDump::stop_running_backups();
> - return 'OK' if !scalar(@vmids);
> + return 'OK' if !scalar(@{$vmids});
> }
>
> - 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);
> - }
> -
> - $param->{vmids} = PVE::VZDump::check_vmids(@vmids)
> - }
> + # silent exit if specified VMs 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);
> @@ -118,7 +94,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};
> @@ -167,7 +143,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..73ad9088 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);
>
> @@ -1156,4 +1157,39 @@ sub stop_running_backups {
> }
> }
>
> +sub get_included_guests {
> + my ($self, $job) = @_;
do we need $self here? Why not call it like:
PVE::VZDump::get_included_guests($params) ?
> +
> + my $nodename = PVE::INotify::nodename();
> + my $vmids = [];
> +
> + # convert string lists to arrays
> + if ($job->{pool}) {
> + $vmids = PVE::API2Tools::get_resource_pool_guest_members($job->{pool});
You use API2Tools here but do not add it as module use statement.
> + } else {
> + $vmids = [ PVE::Tools::split_list(extract_param($job, 'vmid')) ];
> + }
> +
> + 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;
> + }
> +
> + $job->{vmids} = PVE::VZDump::check_vmids(@{$vmids})
> + }
> +
> + return ($vmids, $skiplist);
> +}
> +
> 1;
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
