[pve-devel] [PATCH v5 manager 1/2] vzdump: move remaining guest include logic to single method

[Aaron Lauterer]

On 6/5/20 8:39 PM, Thomas Lamprecht wrote:
> On 5/6/20 11:57 AM, Aaron Lauterer wrote:
>>
[...]
>> 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->%* ];

Thanks for that idea :)
> 
> 
> (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.

yeah sounds good.

> 
>> +	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.

hehe, I just read our coding guidelines a few days ago and that 
particular thing was one of my personal take aways ;)
> 
>>       }
>>   
>>       # 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.

I agree on the early sorting, but I am not sure about that oneliner. 
It's not as easy to comprehend what's going on for someone who isn't 
used to this kind of syntax already.

> 
>> +	    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;
>>
>