[pve-devel] [PATCH guest-common 1/6] Cleanup fix for stateless jobs.

[Wolfgang Bumiller]

On Thu, Dec 14, 2017 at 07:34:32AM +0100, Thomas Lamprecht wrote:
> On 12/13/2017 03:46 PM, Wolfgang Link wrote:
> > If a VM config was stolen or migrated through HA there is no state on that node.
> > It must be possible to cleanup the remote-side without state.

Functionality-wise this patch filters a list of storage IDs. The commit
message should IMO explain how/why that helps, since the name of the
used helpers (remote_prepare_local_job & friends) don't say much about
what they do. (That one in particular just calls out to 'pvesr', so you
need to look at another package (or the pvesr manpage) to figure out
what's going on.)

> > ---
> >  PVE/Replication.pm | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/PVE/Replication.pm b/PVE/Replication.pm
> > index c25ed44..ce2109e 100644
> > --- a/PVE/Replication.pm
> > +++ b/PVE/Replication.pm
> > @@ -200,8 +200,14 @@ sub replicate {
> >  
> >  	if ($remove_job eq 'full' && $jobcfg->{target} ne $local_node) {
> >  	    # remove all remote volumes
> > +	    my $store_list = [];
> > +	    foreach my $volid (@$sorted_volids) {
> > +		my ($storeid) = PVE::Storage::parse_volume_id($volid);
> > +		push @$store_list, $storeid;
> > +	    }
> 
> FYI, this five lines could be written as:
> 
> my @store_list = map { PVE::Storage::parse_volume_id($_) } @$sorted_volids;

Almost. parse_volume_id() returns a list but we only want the first
element.
    my @store_list = map { (PVE::Storage::parse_volume_id($_))[0] } @$sorted_volids;

> 
> # then below:
> remote_prepare_local_job($ssh_info, $jobid, $vmid, [], \@store_list, 0, undef, 1, $logfunc);

Or put [] around the map{} above:
    my $store_list = [ map { (PVE::Storage::parse_volume_id($_))[0] } @$sorted_volids ];
(Since we rarely use \ ).

> 
> map[1] can be used for array to array mappings quite well, it goes
> through the whole array and applies the code in its { BLOCK },
> the return value (which is the last statement if no return is there)
> is the element of a new array. Also array to hash mapping can be done
> quite nicely, as hashes in perl can be seens as fancy arrays with even
> number of elements.
> 
> It's, IMO, expressive and makes the code easier/quicker to read.
> But, just a note, your way works naturally too :)
> 
> > +
> >  	    my $ssh_info = PVE::Cluster::get_ssh_info($jobcfg->{target});
> > -	    remote_prepare_local_job($ssh_info, $jobid, $vmid, [], $state->{storeid_list}, 0, undef, 1, $logfunc);
> > +	    remote_prepare_local_job($ssh_info, $jobid, $vmid, [], $store_list, 0, undef, 1, $logfunc);
> >  
> >  	}
> >  	# remove all local replication snapshots (lastsync => 0)
> >