[pve-devel] [PATCH-SERIES v2 manager/guest-common] replication: improve removal of stale snapshots/volumes

[pve-devel] [PATCH-SERIES v2 manager/guest-common] replication: improve removal of stale snapshots/volumes

[Fabian Ebner]

In certain scenarios, see manager 2/2 and guest-common 3/4, replicated
volumes could end up orphaned. This series attempts to fix those, as
well as making removal of stale replication snaphsots a bit more
robust. Also includes some slight documentation improvements regarding
prepare().

v1 can be found here (but there was no discussion):
https://lists.proxmox.com/pipermail/pve-devel/2020-October/045388.html

Changes from v1:
    * Adapt to changed behavior of prepare(), so we still only catch
      volumes that had replication snapshots belonging to the job.
    * Drop simplification that would only rely on replication state
      to get storages for full removal.
    * Add safe-guard to only remove other replication snaphsots if the
      last_sync snapshot is present.


manager:

Fabian Ebner (2):
  pvesr: rename last_snapshots to local_snapshots
  pvesr: prepare local job: remove stale replicated volumes immediately

 PVE/CLI/pvesr.pm | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)


guest-common:

Fabian Ebner (4):
  replication: prepare: adapt/expand function comment
  replication: rename last_snapshots to local_snapshots
  replication: also consider storages from replication state upon
    removal
  replication: prepare: safeguard against removal if expected snapshot
    is missing

 src/PVE/Replication.pm | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH v2 manager 1/2] pvesr: rename last_snapshots to local_snapshots

[Fabian Ebner]

while dropping the instance where the local variable was unused.
prepare() was changed a while ago to return all local snapshots.

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

New in v2.

 PVE/CLI/pvesr.pm | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/PVE/CLI/pvesr.pm b/PVE/CLI/pvesr.pm
index cb79e2bf..a1be88af 100644
--- a/PVE/CLI/pvesr.pm
+++ b/PVE/CLI/pvesr.pm
@@ -136,16 +136,16 @@ __PACKAGE__->register_method ({
 	    my $images = $plugin->list_images($storeid, $scfg, $vmid, undef, $cache);
 	    push @$volids, map { $_->{volid} } @$images;
 	}
-	my ($last_snapshots, $cleaned_replicated_volumes) = PVE::Replication::prepare($storecfg, $volids, $jobid, $last_sync, $parent_snapname, $logfunc);
+	my ($local_snapshots, $cleaned_replicated_volumes) = PVE::Replication::prepare($storecfg, $volids, $jobid, $last_sync, $parent_snapname, $logfunc);
 	foreach my $volid (keys %$cleaned_replicated_volumes) {
 	    if (!$wanted_volids->{$volid}) {
 		$logfunc->("$jobid: delete stale volume '$volid'");
 		PVE::Storage::vdisk_free($storecfg, $volid);
-		delete $last_snapshots->{$volid};
+		delete $local_snapshots->{$volid};
 	    }
 	}
 
-	print to_json($last_snapshots) . "\n";
+	print to_json($local_snapshots) . "\n";
 
 	return undef;
     }});
@@ -200,8 +200,7 @@ __PACKAGE__->register_method ({
 	    print STDERR "$msg\n";
 	};
 
-	my $last_snapshots = PVE::Replication::prepare(
-	    $storecfg, $volids, $jobid, $last_sync, undef, $logfunc);
+	PVE::Replication::prepare($storecfg, $volids, $jobid, $last_sync, undef, $logfunc);
 
 	return undef;
     }});
-- 
2.30.2

[pve-devel] [PATCH v2 manager 2/2] pvesr: prepare local job: remove stale replicated volumes immediately

[Fabian Ebner]

Commit 0433b86df6dfdf1d64ee09322719a02a91690707 introduced a
regression where only stale replicated volumes with an older
timestamp would be cleaned up. This meant that after removing a volume
from the guest config, it would only be cleaned up the second time the
replication ran afterwards. And the volume could become completely
orphaned in case the relevant storage wasn't used by the job anymore.

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

Dependency bump for libpve-guest-common >= 4.0-3 needed for
is_replication_snapshot().

Changes from v1:
    * Adapt to changed behavior of prepare() so we still only catch
      volumes that had replication snapshots belonging to the job.

 PVE/CLI/pvesr.pm | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/PVE/CLI/pvesr.pm b/PVE/CLI/pvesr.pm
index a1be88af..95dad64e 100644
--- a/PVE/CLI/pvesr.pm
+++ b/PVE/CLI/pvesr.pm
@@ -137,8 +137,18 @@ __PACKAGE__->register_method ({
 	    push @$volids, map { $_->{volid} } @$images;
 	}
 	my ($local_snapshots, $cleaned_replicated_volumes) = PVE::Replication::prepare($storecfg, $volids, $jobid, $last_sync, $parent_snapname, $logfunc);
-	foreach my $volid (keys %$cleaned_replicated_volumes) {
-	    if (!$wanted_volids->{$volid}) {
+	for my $volid ($volids->@*) {
+	    next if $wanted_volids->{$volid};
+
+	    my $stale = $cleaned_replicated_volumes->{$volid};
+	    # prepare() will not remove the last_sync snapshot, but if the volume was used by the
+	    # job and is not wanted anymore, it is stale too. And not removing it now might cause
+	    # it to be missed later, because the relevant storage might not get scanned anymore.
+	    $stale ||= grep {
+		PVE::Replication::is_replication_snapshot($_, $jobid)
+	    } keys %{$local_snapshots->{$volid} // {}};
+
+	    if ($stale) {
 		$logfunc->("$jobid: delete stale volume '$volid'");
 		PVE::Storage::vdisk_free($storecfg, $volid);
 		delete $local_snapshots->{$volid};
-- 
2.30.2

[pve-devel] [PATCH v2 guest-common 1/4] replication: prepare: adapt/expand function comment

[Fabian Ebner]

Commit 8d1cd44 ("partially fix #3111: replication: be less picky when
selecting incremental base") changed prepare() to return all local
snapshots.

Special behavior regarding last_sync is also better mentioned
explicitly.

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

New in v2.

 src/PVE/Replication.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 31cabec..fcd59bb 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -156,8 +156,9 @@ sub remote_finalize_local_job {
     PVE::Tools::run_command($cmd, outfunc => $logger, errfunc => $logger);
 }
 
-# finds local replication snapshots from $last_sync
-# and removes all replication snapshots with other time stamps
+# Finds all local snapshots and removes replication snapshots not matching $last_sync. Use
+# last_sync=0 (or undef) to prevent removal (useful if VM was stolen). Use last_sync=1 to remove all
+# replication snapshots (limited to job if specified).
 sub prepare {
     my ($storecfg, $volids, $jobid, $last_sync, $parent_snapname, $logfunc) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH v2 guest-common 2/4] replication: rename last_snapshots to local_snapshots

[Fabian Ebner]

because prepare() was changed in 8d1cd44 ("partially fix #3111:
replication: be less picky when selecting incremental base") to return
all local snapshots.

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

New in v2.

 src/PVE/Replication.pm | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index fcd59bb..d84f469 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -36,10 +36,7 @@ sub find_common_replication_snapshot {
     my $last_sync_snapname =
 	PVE::ReplicationState::replication_snapshot_name($jobid, $last_sync);
 
-    # test if we have a replication_ snapshot from last sync
-    # and remove all other/stale replication snapshots
-
-    my $last_snapshots =
+    my $local_snapshots =
 	prepare($storecfg, $volumes, $jobid, $last_sync, $parent_snapname, $logfunc);
 
     # prepare remote side
@@ -58,7 +55,7 @@ sub find_common_replication_snapshot {
     my $base_snapshots = {};
 
     foreach my $volid (@$volumes) {
-	my $local_info = $last_snapshots->{$volid};
+	my $local_info = $local_snapshots->{$volid};
 	my $remote_info = $remote_snapshots->{$volid};
 
 	if (defined($local_info) && defined($remote_info)) {
@@ -103,7 +100,7 @@ sub find_common_replication_snapshot {
 	}
     }
 
-    return ($base_snapshots, $last_snapshots, $last_sync_snapname);
+    return ($base_snapshots, $local_snapshots, $last_sync_snapname);
 }
 
 sub remote_prepare_local_job {
@@ -172,7 +169,7 @@ sub prepare {
 	$prefix = '__replicate_';
     }
 
-    my $last_snapshots = {};
+    my $local_snapshots = {};
     my $cleaned_replicated_volumes = {};
     foreach my $volid (@$volids) {
 	my $info = PVE::Storage::volume_snapshot_info($storecfg, $volid);
@@ -200,12 +197,12 @@ sub prepare {
 		    $logfunc->("delete stale replication snapshot error: $err");
 		}
 	    } else {
-		$last_snapshots->{$volid}->{$snap} = $info->{$snap};
+		$local_snapshots->{$volid}->{$snap} = $info->{$snap};
 	    }
 	}
     }
 
-    return wantarray ? ($last_snapshots, $cleaned_replicated_volumes) : $last_snapshots;
+    return wantarray ? ($local_snapshots, $cleaned_replicated_volumes) : $local_snapshots;
 }
 
 sub replicate_volume {
@@ -291,7 +288,7 @@ sub replicate {
 
     my $ssh_info = PVE::SSHInfo::get_ssh_info($jobcfg->{target}, $migration_network);
 
-    my ($base_snapshots, $last_snapshots, $last_sync_snapname) = find_common_replication_snapshot(
+    my ($base_snapshots, $local_snapshots, $last_sync_snapname) = find_common_replication_snapshot(
 	$ssh_info, $jobid, $vmid, $storecfg, $sorted_volids, $state->{storeid_list}, $last_sync, $conf, $logfunc);
 
     my $storeid_hash = {};
@@ -373,7 +370,7 @@ sub replicate {
     PVE::ReplicationState::record_sync_end($jobcfg, $state, $start_time);
 
     # remove old snapshots because they are no longer needed
-    $cleanup_local_snapshots->($last_snapshots, $last_sync_snapname);
+    $cleanup_local_snapshots->($local_snapshots, $last_sync_snapname);
 
     eval {
 	remote_finalize_local_job($ssh_info, $jobid, $vmid, $sorted_volids, $start_time, $logfunc);
-- 
2.30.2

[pve-devel] [PATCH v2 guest-common 3/4] replication: also consider storages from replication state upon removal

[Fabian Ebner]

This prevents left-over volume(s) in the following situation:
1. replication with volumes on different storages A and B
2. remove all volumes on storage B from the guest configuration
3. schedule full removal before the next normal replication runs

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

No changes from v1.

 src/PVE/Replication.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index d84f469..33fcc48 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -270,6 +270,7 @@ sub replicate {
 	if ($remove_job eq 'full' && $jobcfg->{target} ne $local_node) {
 	    # remove all remote volumes
 	    my @store_list = map { (PVE::Storage::parse_volume_id($_))[0] } @$sorted_volids;
+	    push @store_list, @{$state->{storeid_list}};
 
 	    my %hash = map { $_ => 1 } @store_list;
 
-- 
2.30.2

[pve-devel] [RFC v2 guest-common 4/4] replication: prepare: safeguard against removal if expected snapshot is missing

[Fabian Ebner]

Such a check would also have prevented the issue in 1aa4d84
("ReplicationState: purge state from non local vms") and other
scenarios where state and disk state are inconsistent with regard to
the last_sync snapshot.

AFAICT, all existing callers intending to remove all snapshots use
last_sync=1 so chaning the behavior for other (non-zero) values should
be fine.

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

New in v2.

 src/PVE/Replication.pm | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 33fcc48..8591d0e 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -153,9 +153,9 @@ sub remote_finalize_local_job {
     PVE::Tools::run_command($cmd, outfunc => $logger, errfunc => $logger);
 }
 
-# Finds all local snapshots and removes replication snapshots not matching $last_sync. Use
-# last_sync=0 (or undef) to prevent removal (useful if VM was stolen). Use last_sync=1 to remove all
-# replication snapshots (limited to job if specified).
+# Finds all local snapshots and removes replication snapshots not matching $last_sync after checking
+# that it is present. Use last_sync=0 (or undef) to prevent removal (useful if VM was stolen). Use
+# last_sync=1 to remove all replication snapshots (limited to job if specified).
 sub prepare {
     my ($storecfg, $volids, $jobid, $last_sync, $parent_snapname, $logfunc) = @_;
 
@@ -173,12 +173,19 @@ sub prepare {
     my $cleaned_replicated_volumes = {};
     foreach my $volid (@$volids) {
 	my $info = PVE::Storage::volume_snapshot_info($storecfg, $volid);
+
+	my $removal_ok = !defined($snapname) || $info->{$snapname};
+	$removal_ok = 0 if $last_sync == 0; # last_sync=0 if the VM was stolen, don't remove!
+	$removal_ok = 1 if $last_sync == 1; # last_sync=1 is a special value used to remove all
+	$logfunc->("expected snapshot $snapname not present for $volid, not removing others")
+	    if !$removal_ok && $last_sync > 1;
+
 	for my $snap (keys $info->%*) {
 	    if ( # check if it's a stale replication snapshot
 		!(defined($snapname) && $snap eq $snapname) &&
 		!(defined($parent_snapname) && $snap eq $parent_snapname) &&
 		$snap =~ m/^\Q$prefix\E/ &&
-		$last_sync != 0 # last_sync is 0 if the VM was stolen
+		$removal_ok
 	    ) {
 		$logfunc->("delete stale replication snapshot '$snap' on $volid");
 		eval {
-- 
2.30.2

Re: [pve-devel] [PATCH-SERIES v2 manager/guest-common] replication: improve removal of stale snapshots/volumes

[Fiona Ebner]

Am 13.06.22 um 12:29 schrieb Fabian Ebner:
> In certain scenarios, see manager 2/2 and guest-common 3/4, replicated
> volumes could end up orphaned. This series attempts to fix those, as
> well as making removal of stale replication snaphsots a bit more
> robust. Also includes some slight documentation improvements regarding
> prepare().
> 

Ping

> v1 can be found here (but there was no discussion):
> https://lists.proxmox.com/pipermail/pve-devel/2020-October/045388.html
> 
> Changes from v1:
>     * Adapt to changed behavior of prepare(), so we still only catch
>       volumes that had replication snapshots belonging to the job.
>     * Drop simplification that would only rely on replication state
>       to get storages for full removal.
>     * Add safe-guard to only remove other replication snaphsots if the
>       last_sync snapshot is present.
> 
> 

[pve-devel] applied-series: [PATCH-SERIES v2 manager/guest-common] replication: improve removal of stale snapshots/volumes

[Fabian Grünbichler]

including RFC patch, which looked sensible to me ;)

thanks!

On July 27, 2022 1:22 pm, Fiona Ebner wrote:
> Am 13.06.22 um 12:29 schrieb Fabian Ebner:
>> In certain scenarios, see manager 2/2 and guest-common 3/4, replicated
>> volumes could end up orphaned. This series attempts to fix those, as
>> well as making removal of stale replication snaphsots a bit more
>> robust. Also includes some slight documentation improvements regarding
>> prepare().
>> 
> 
> Ping
> 
>> v1 can be found here (but there was no discussion):
>> https://lists.proxmox.com/pipermail/pve-devel/2020-October/045388.html
>> 
>> Changes from v1:
>>     * Adapt to changed behavior of prepare(), so we still only catch
>>       volumes that had replication snapshots belonging to the job.
>>     * Drop simplification that would only rely on replication state
>>       to get storages for full removal.
>>     * Add safe-guard to only remove other replication snaphsots if the
>>       last_sync snapshot is present.
>> 
>> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>