[pve-devel] [PATCH-SERIES v2 guest-common] replication improvements

[pve-devel] [PATCH-SERIES v2 guest-common] replication improvements

[Fiona Ebner]

Improve error when finding a common base snapshot and fix the check if
a snapshot is needed by replication when there are volumes with
replicate setting turned off.

First version:
https://lists.proxmox.com/pipermail/pve-devel/2023-January/055513.html

Changes since v1:
    * add an additional fix

Fiona Ebner (3):
  replication: prepare: include volumes without snapshots in the result
  replication: find common base: improve error when no common base
    snapshot exists
  abstract config: fix snapshot needed by replication check

 src/PVE/AbstractConfig.pm |  2 +-
 src/PVE/Replication.pm    | 17 ++++++++++++-----
 2 files changed, 13 insertions(+), 6 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH v2 guest-common 1/3] replication: prepare: include volumes without snapshots in the result

[Fiona Ebner]

Note that PVE::Storage::volume_snapshot_info() will fail when a volume
does not exist, so no non-existing volume will end up in the result
(prepare() is only called with volumes that should exist).

This makes it possible to detect a volume without snapshots in the
result of prepare(), and as a consequence, replication will now also
fail early in a situation where source and remote volume both exist,
but (at least) one of them doesn't have any snapshots.

Such a situation can happen, for example, by deleting and re-creating
a volume with the same name on the source side without running
replication after deletion.

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

No changes in v2.

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

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index bd627e5..05c2632 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -172,6 +172,8 @@ sub prepare {
     my $local_snapshots = {};
     my $cleaned_replicated_volumes = {};
     foreach my $volid (@$volids) {
+	$local_snapshots->{$volid} = {};
+
 	my $info = PVE::Storage::volume_snapshot_info($storecfg, $volid);
 
 	my $removal_ok = !defined($snapname) || $info->{$snapname};
-- 
2.39.2

[pve-devel] [PATCH v2 guest-common 2/3] replication: find common base: improve error when no common base snapshot exists

[Fiona Ebner]

Suggest an alternative solution by removing the problematic volumes
from the replication target rather than the whole job.

This is helpful if there are multiple replicated volumes to avoid the
need to fully re-sync all volumes in many cases.

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

No changes in v2.

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

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 05c2632..984ea34 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -53,6 +53,7 @@ sub find_common_replication_snapshot {
     );
 
     my $base_snapshots = {};
+    my @missing_snapshots = ();
 
     foreach my $volid (@$volumes) {
 	my $local_info = $local_snapshots->{$volid};
@@ -91,15 +92,19 @@ sub find_common_replication_snapshot {
 		    next;
 		}
 
-		# The volume exists on the remote side, so trying a full sync won't work.
-		# Die early with a clean error.
-		die "No common base to restore the job state\n".
-		    "please delete jobid: $jobid and create the job again\n"
-		    if !defined($base_snapshots->{$volid});
+		push @missing_snapshots, $volid if !defined($base_snapshots->{$volid});
 	    }
 	}
     }
 
+    if (scalar(@missing_snapshots) > 0) {
+	# There exist volumes without common base snapshot on the remote side.
+	# Trying to (do a full) sync won't work, so die early with a clean error.
+	my $volume_string = join(',', @missing_snapshots);
+	die "No common base snapshot on volume(s) $volume_string\nPlease remove the problematic " .
+	    "volume(s) from the replication target or delete and re-create the whole job $jobid\n";
+    }
+
     return ($base_snapshots, $local_snapshots, $last_sync_snapname);
 }
 
-- 
2.39.2

[pve-devel] [PATCH v2 guest-common 3/3] abstract config: fix snapshot needed by replication check

[Fiona Ebner]

Do not pass the cleanup flag to get_replicatable_volumes() which leads
to replicatable volumes that have the replicate setting turned off to
be part of the result.

Instead pass the noerr flag, because things like missing the
storage-level replicate feature should not lead to an error here.

Reported in the community forum:
https://forum.proxmox.com/threads/120910/post-605574

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

New in v2.

 src/PVE/AbstractConfig.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index a286b13..5d5f9b4 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -862,7 +862,7 @@ my $snapshot_delete_assert_not_needed_by_replication = sub {
     my $storecfg = PVE::Storage::config();
 
     # Current config's volumes are relevant for replication.
-    my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $conf, 1);
+    my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $conf, 0, 1);
 
     my $replication_jobs = $repl_conf->list_guests_local_replication_jobs($vmid);
 
-- 
2.39.2

Re: [pve-devel] [PATCH-SERIES v2 guest-common] replication improvements

[Fiona Ebner]

Am 13.12.23 um 15:17 schrieb Fiona Ebner:
> Improve error when finding a common base snapshot and fix the check if
> a snapshot is needed by replication when there are volumes with
> replicate setting turned off.
> 
> First version:
> https://lists.proxmox.com/pipermail/pve-devel/2023-January/055513.html
> 
> Changes since v1:
>     * add an additional fix
> 
> Fiona Ebner (3):
>   replication: prepare: include volumes without snapshots in the result
>   replication: find common base: improve error when no common base
>     snapshot exists
>   abstract config: fix snapshot needed by replication check
> 
>  src/PVE/AbstractConfig.pm |  2 +-
>  src/PVE/Replication.pm    | 17 ++++++++++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 

Ping

[pve-devel] applied-series: [PATCH-SERIES v2 guest-common] replication improvements

[Thomas Lamprecht]

On 13/12/2023 15:17, Fiona Ebner wrote:
> Improve error when finding a common base snapshot and fix the check if
> a snapshot is needed by replication when there are volumes with
> replicate setting turned off.
> 
> First version:
> https://lists.proxmox.com/pipermail/pve-devel/2023-January/055513.html
> 
> Changes since v1:
>     * add an additional fix
> 
> Fiona Ebner (3):
>   replication: prepare: include volumes without snapshots in the result
>   replication: find common base: improve error when no common base
>     snapshot exists
>   abstract config: fix snapshot needed by replication check
> 
>  src/PVE/AbstractConfig.pm |  2 +-
>  src/PVE/Replication.pm    | 17 ++++++++++++-----
>  2 files changed, 13 insertions(+), 6 deletions(-)
> 


applied, thanks!

Had some slight reservations about recommending deletion of volumes to
user, but maybe I'm to paranoid about them not reading the "target" part
and disregarding any basic sense..