[pve-devel] [PATCH guest-common 1/3] snapshot_rollback: flock earlier
Fabian Ebner
f.ebner at proxmox.com
Mon Apr 27 13:08:26 CEST 2020
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.
> + }
> +
> + $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};
>
More information about the pve-devel
mailing list