partially fix #3111: further improve removing replication snapshots

by using the new $blocker parameter. No longer remove all replication
snapshots from affected volumes unconditionally, but check first if
all blocking snapshots are replication snapshots. If they are, remove
them and proceed with rollback. If they are not, die without removing
any.

For backwards compatibility, it's still necessary to remove all
replication snapshots if $blockers is not available.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 0d1c7caf9d4f4a7fa1516455cda0302d1cef8b82..a5a15bf9ef7a6b4009424a4f14abb61b64c9f6af 100644 (file)
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -945,7 +945,7 @@ sub snapshot_delete {
 
 # Remove replication snapshots to make a rollback possible.
 my $rollback_remove_replication_snapshots = sub {
-    my ($class, $vmid, $snap) = @_;
+    my ($class, $vmid, $snap, $snapname) = @_;
 
     my $storecfg = PVE::Storage::config();
 
@@ -954,17 +954,56 @@ my $rollback_remove_replication_snapshots = sub {
 
     my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $snap, 1);
 
-    # filter by what we actually iterate over below (excludes vmstate!)
+    # For these, all replication snapshots need to be removed for backwards compatibility.
     my $volids = [];
+
+    # For these, we know more and can remove only the required replication snapshots.
+    my $blocking_snapshots = {};
+
+    # filter by what we actually iterate over below (excludes vmstate!)
     $class->foreach_volume($snap, sub {
        my ($vs, $volume) = @_;
 
        my $volid_key = $class->volid_key();
        my $volid = $volume->{$volid_key};
 
-       push @{$volids}, $volid if $volumes->{$volid};
+       return if !$volumes->{$volid};
+
+       my $blockers = [];
+       eval { $class->__snapshot_rollback_vol_possible($volume, $snapname, $blockers); };
+       if (my $err = $@) {
+           # FIXME die instead, once $blockers is required by the storage plugin API
+           # and the guest plugins are required to be new enough to support it too.
+           # Currently, it's not possible to distinguish between blockers being empty
+           # because the plugin is old versus because there is a different error.
+           if (scalar($blockers->@*) == 0) {
+               push @{$volids}, $volid;
+               return;
+           }
+
+           for my $blocker ($blockers->@*) {
+               die $err if !PVE::Replication::is_replication_snapshot($blocker);
+           }
+
+           $blocking_snapshots->{$volid} = $blockers;
+       }
     });
 
+    my $removed_repl_snapshot;
+    for my $volid (sort keys $blocking_snapshots->%*) {
+       my $blockers = $blocking_snapshots->{$volid};
+
+       for my $blocker ($blockers->@*) {
+           warn "WARN: removing replication snapshot '$volid\@$blocker'\n";
+           $removed_repl_snapshot = 1;
+           eval { PVE::Storage::volume_snapshot_delete($storecfg, $volid, $blocker); };
+           die $@ if $@;
+       }
+    }
+    warn "WARN: you shouldn't remove '$snapname' before running the next replication!\n"
+       if $removed_repl_snapshot;
+
+    # Need to keep using a hammer for backwards compatibility...
     # remove all local replication snapshots (jobid => undef)
     my $logfunc = sub { my $line = shift; chomp $line; print "$line\n"; };
     PVE::Replication::prepare($storecfg, $volids, undef, 1, undef, $logfunc);
@@ -997,7 +1036,7 @@ sub snapshot_rollback {
        $snap = $get_snapshot_config->($conf);
 
        if ($prepare) {
-           $rollback_remove_replication_snapshots->($class, $vmid, $snap);
+           $rollback_remove_replication_snapshots->($class, $vmid, $snap, $snapname);
 
            $class->foreach_volume($snap, sub {
               my ($vs, $volume) = @_;

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index ae0f145881a10bbf6ebd81a3c9405ef8b7c0bdf7..3da79be0254ea0475d38f03d73d3f5a892d6ef37 100644 (file)
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -435,4 +435,10 @@ sub run_replication {
     return $volumes;
 }
 
+sub is_replication_snapshot {
+    my ($snapshot_name) = @_;
+
+    return $snapshot_name =~ m/^__replicate_/ ? 1 : 0;
+}
+
 1;