[pve-devel] [PATCH guest-common] replication: snapshot cleanup: only attempt to remove snapshots that exist

[pve-devel] [PATCH guest-common] replication: snapshot cleanup: only attempt to remove snapshots that exist

[Fiona Ebner]

Since commit a6f5b35 ("replication: prepare: include volumes without
snapshots in the result"), attempts would be made to remove previous
replication snapshots from volumes on which they didn't exist. This
was noticed by Thomas since the output of a replication test in
pve-manager changed.

The issue is not completely new, i.e. there was no check that the
(previous) replication snapshot acutally exists before attempting
removal during the cleanup phase. Fix the issue by adding such a
check.

The $replicate_snapshots hash is only used for this, so the change
there is fine.

Fixes: a6f5b35 ("replication: prepare: include volumes without snapshots in the result")
Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/Replication.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 984ea34..20c3485 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -336,7 +336,7 @@ sub replicate {
 	foreach my $volid (@$sorted_volids) {
 	    $logfunc->("create snapshot '${sync_snapname}' on $volid");
 	    PVE::Storage::volume_snapshot($storecfg, $volid, $sync_snapname);
-	    $replicate_snapshots->{$volid} = 1;
+	    $replicate_snapshots->{$volid}->{$sync_snapname} = 1;
 	}
     };
     my $err = $@;
@@ -350,6 +350,7 @@ sub replicate {
     my $cleanup_local_snapshots = sub {
 	my ($volid_hash, $snapname) = @_;
 	foreach my $volid (sort keys %$volid_hash) {
+	    next if !$volid_hash->{$volid}->{$snapname};
 	    $logfunc->("delete previous replication snapshot '$snapname' on $volid");
 	    eval { PVE::Storage::volume_snapshot_delete($storecfg, $volid, $snapname); };
 	    warn $@ if $@;
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager] Revert "tests: update expected replication log output"

[Fiona Ebner]

This reverts commit 3a259c22e64ff22049856256a1dad643439c79ef.

There was an oversight with recent replication fixes that led to
attempting to remove snapshots that do not exist (in more scenarios).
While not an issue with real consequences, it's confusing to users.
This has since been fixed by pve-guest-common commit "replication:
snapshot cleanup: only attempt to remove snapshots that exist".

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

Build-depends on new pve-guest-common.

 test/replication_test5.log | 2 --
 1 file changed, 2 deletions(-)

diff --git a/test/replication_test5.log b/test/replication_test5.log
index 196d0fd0..928feca3 100644
--- a/test/replication_test5.log
+++ b/test/replication_test5.log
@@ -5,7 +5,6 @@
 1000 job_900_to_node2: create snapshot '__replicate_job_900_to_node2_1000__' on local-zfs:vm-900-disk-1
 1000 job_900_to_node2: using secure transmission, rate limit: none
 1000 job_900_to_node2: full sync 'local-zfs:vm-900-disk-1' (__replicate_job_900_to_node2_1000__)
-1000 job_900_to_node2: delete previous replication snapshot '__replicate_job_900_to_node2_0__' on local-zfs:vm-900-disk-1
 1000 job_900_to_node2: end replication job
 1000 job_900_to_node2: changed config next_sync => 1800
 1000 job_900_to_node2: changed state last_node => node1, last_try => 1000, last_sync => 1000
@@ -38,7 +37,6 @@
 3040 job_900_to_node2: incremental sync 'local-zfs:vm-900-disk-1' (__replicate_job_900_to_node2_1840__ => __replicate_job_900_to_node2_3040__)
 3040 job_900_to_node2: full sync 'local-zfs:vm-900-disk-2' (__replicate_job_900_to_node2_3040__)
 3040 job_900_to_node2: delete previous replication snapshot '__replicate_job_900_to_node2_1840__' on local-zfs:vm-900-disk-1
-3040 job_900_to_node2: delete previous replication snapshot '__replicate_job_900_to_node2_1840__' on local-zfs:vm-900-disk-2
 3040 job_900_to_node2: end replication job
 3040 job_900_to_node2: changed config next_sync => 3600
 3040 job_900_to_node2: changed state last_try => 3040, last_sync => 3040, fail_count => 0, error => 
-- 
2.39.2



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH guest-common] replication: snapshot cleanup: only attempt to remove snapshots that exist

[Thomas Lamprecht]

Am 18/04/2024 um 09:06 schrieb Fiona Ebner:
> Since commit a6f5b35 ("replication: prepare: include volumes without
> snapshots in the result"), attempts would be made to remove previous
> replication snapshots from volumes on which they didn't exist. This
> was noticed by Thomas since the output of a replication test in
> pve-manager changed.
> 
> The issue is not completely new, i.e. there was no check that the
> (previous) replication snapshot acutally exists before attempting
> removal during the cleanup phase. Fix the issue by adding such a
> check.
> 
> The $replicate_snapshots hash is only used for this, so the change
> there is fine.
> 
> Fixes: a6f5b35 ("replication: prepare: include volumes without snapshots in the result")
> Reported-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  src/PVE/Replication.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
>

applied both patches, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel