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.gruenbich...@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@pve.proxmox.com > https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel > _______________________________________________ pve-devel mailing list pve-devel@pve.proxmox.com https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel