[pve-devel] [PATCH guest-common 1/3] snapshot_rollback: flock earlier

[Fabian Grünbichler]

On April 27, 2020 1:08 pm, Fabian Ebner wrote:
> One not-patch-related observation inline.
> 
> On 27.04.20 10:24, Fabian Grünbichler wrote:
>> to protect checks against concurrent modifications
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
>> ---
>> 
>> Notes:
>>      bested viewed with --patience -w
>> 
>>   PVE/AbstractConfig.pm | 45 +++++++++++++++++++++----------------------
>>   1 file changed, 22 insertions(+), 23 deletions(-)
>> 
>> diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
>> index 15368de..70311df 100644
>> --- a/PVE/AbstractConfig.pm
>> +++ b/PVE/AbstractConfig.pm
>> @@ -919,9 +919,10 @@ sub snapshot_rollback {
>>   
>>       my $storecfg = PVE::Storage::config();
>>   
>> -    my $conf = $class->load_config($vmid);
>> +    my $data = {};
>>   
>>       my $get_snapshot_config = sub {
>> +	my ($conf) = @_;
>>   
>>   	die "you can't rollback if vm is a template\n" if $class->is_template($conf);
>>   
>> @@ -932,32 +933,30 @@ sub snapshot_rollback {
>>   	return $res;
>>       };
>>   
>> -    my $repl_conf = PVE::ReplicationConfig->new();
>> -    if ($repl_conf->check_for_existing_jobs($vmid, 1)) {
>> -	# remove all replication snapshots
>> -	my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $conf, 1);
>> -	my $sorted_volids = [ sort keys %$volumes ];
>> -
>> -	# remove all local replication snapshots (jobid => undef)
>> -	my $logfunc = sub { my $line = shift; chomp $line; print "$line\n"; };
>> -	PVE::Replication::prepare($storecfg, $sorted_volids, undef, 0, undef, $logfunc);
>> -    }
>> -
>> -    my $snap = &$get_snapshot_config();
>> -
>> -    $class->foreach_volume($snap, sub {
>> -	my ($vs, $volume) = @_;
>> -
>> -	$class->__snapshot_rollback_vol_possible($volume, $snapname);
>> -    });
>> -
>> -    my $data = {};
>> +    my $snap;
>>   
>>       my $updatefn = sub {
>> +	my $conf = $class->load_config($vmid);
>> +	$snap = $get_snapshot_config->($conf);
>>   
>> -	$conf = $class->load_config($vmid);
>> +	if ($prepare) {
>> +	    my $repl_conf = PVE::ReplicationConfig->new();
>> +	    if ($repl_conf->check_for_existing_jobs($vmid, 1)) {
>> +		# remove all replication snapshots
>> +		my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $conf, 1);
>> +		my $sorted_volids = [ sort keys %$volumes ];
>> +
>> +		# remove all local replication snapshots (jobid => undef)
>> +		my $logfunc = sub { my $line = shift; chomp $line; print "$line\n"; };
>> +		PVE::Replication::prepare($storecfg, $sorted_volids, undef, 0, undef, $logfunc);
> 
> This has nothing to do with the locking/this patch, but here we'd need 
> to call prepare with last_sync=1 instead of last_sync=0, no? This is 
> because of commit a1dfeff3a8502544123245ea61ad62cbe97803b7 which made 
> last_sync=0 a special case and changed the behavior. I can send a patch 
> once this is applied.

thanks for noticing - yes, that seems to be correct.

> 
>> +	    }
>> +
>> +	    $class->foreach_volume($snap, sub {
>> +	       my ($vs, $volume) = @_;
>>   
>> -	$snap = &$get_snapshot_config();
>> +	       $class->__snapshot_rollback_vol_possible($volume, $snapname);
>> +	    });
>> +        }
>>   
>>   	die "unable to rollback to incomplete snapshot (snapstate = $snap->{snapstate})\n"
>>   	    if $snap->{snapstate};
>> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>