[pve-devel] [PATCH-SERIES storage/guest-common/manager] Follow-up for fixing replication/rollback interaction

[pve-devel] [PATCH-SERIES storage/guest-common/manager] Follow-up for fixing replication/rollback interaction

[Fabian Ebner]

Returning more information about snapshots allows for better decisions
when picking the incremental base snapshot. Namely, to distinguish
between different snapshots with the same name, and to choose a more
recent snapshot in some cases, reducing the send delta. On top of
that, the code in find_common_replication_snapshot becomes simpler.

Mostly as discussed off-list with Fabian G., but instead of just
changing the return type of volume_snapshot_list, it has been replaced
with a new function. I felt like that would allow for a cleaner break
and the old name was not fully accurate anymore.


Applies on top of the original series:
https://lists.proxmox.com/pipermail/pve-devel/2021-August/049699.html


Dependencies/breaks:
1. pve-guest-common (except patch #1) depends on pve-storage patch #1.
2. pve-storage patch #2 breaks old pve-guest-common.
3. pve-guest-common patch #2 build-breaks old pve-manager.
4. pve-manager patch #3 build-depends on pve-guest-common patch #2.


pve-storage:

Fabian Ebner (3):
  plugin: add volume_snapshot_info function
  plugin: remove volume_snapshot_list
  bump APIVER and APIAGE

 ApiChangeLog                 | 16 ++++++++++++++++
 PVE/Storage.pm               | 21 +++++++--------------
 PVE/Storage/Plugin.pm        | 11 ++++++-----
 PVE/Storage/ZFSPlugin.pm     |  6 ------
 PVE/Storage/ZFSPoolPlugin.pm | 23 ++++++++++++++++++-----
 5 files changed, 47 insertions(+), 30 deletions(-)


pve-guest-common:

Fabian Ebner (4):
  replication: refactor finding most recent common replication snapshot
  replication: prepare: return additional information about snapshots
  replication: find common snapshot: use additional information
  config: snapshot delete check: use volume_snapshot_info

 src/PVE/AbstractConfig.pm |  4 +--
 src/PVE/Replication.pm    | 73 ++++++++++++++++++---------------------
 2 files changed, 36 insertions(+), 41 deletions(-)


pve-manager:

Fabian Ebner (3):
  test: replication: avoid implicit return for volume_snapshot
  test: replication: mock volume_snapshot_info
  test: replication: remove mocking for obsolete volume_snapshot_list

 test/ReplicationTestEnv.pm | 40 +++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 16 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH storage 1/3] plugin: add volume_snapshot_info function

[Fabian Ebner]

which allows for better choices of common replication snapshots.

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

If this is applied without the following patches, it still needs an
APIVER+APIAGE bump and API changelog entry.

 PVE/Storage.pm               |  9 +++++++++
 PVE/Storage/Plugin.pm        | 10 ++++++++++
 PVE/Storage/ZFSPoolPlugin.pm | 22 ++++++++++++++++++++++
 3 files changed, 41 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index e314bfc..f9582d0 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -371,6 +371,15 @@ sub volume_has_feature {
     }
 }
 
+sub volume_snapshot_info {
+    my ($cfg, $volid) = @_;
+
+    my ($storeid, $volname) = parse_volume_id($volid);
+    my $scfg = storage_config($cfg, $storeid);
+    my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
+    return $plugin->volume_snapshot_info($scfg, $storeid, $volname);
+}
+
 sub volume_snapshot_list {
     my ($cfg, $volid) = @_;
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index e1f9335..d4b3615 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1212,6 +1212,16 @@ sub status {
     return ($res->{total}, $res->{avail}, $res->{used}, 1);
 }
 
+# Returns a hash with the snapshot names as keys and the following data:
+# id        - Unique id to distinguish different snapshots even if the have the same name.
+# timestamp - Creation time of the snapshot (seconds since epoch).
+# Returns an empty hash if the volume does not exist.
+sub volume_snapshot_info {
+    my ($class, $scfg, $storeid, $volname) = @_;
+
+    die "volume_snapshot_info is not implemented for $class";
+}
+
 sub volume_snapshot_list {
     my ($class, $scfg, $storeid, $volname) = @_;
 
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index b73d895..01d0743 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -510,6 +510,28 @@ sub volume_rollback_is_possible {
     return 1;
 }
 
+sub volume_snapshot_info {
+    my ($class, $scfg, $storeid, $volname) = @_;
+
+    my $vname = ($class->parse_volname($volname))[1];
+
+    my @params = ('-Hp', '-t', 'snapshot', '-o', 'name,guid,creation', "$scfg->{pool}\/$vname");
+    my $text = $class->zfs_request($scfg, undef, 'list', @params);
+    my @lines = split(/\n/, $text);
+
+    my $info = {};
+    for my $line (@lines) {
+	my ($snapshot, $guid, $creation) = split(/\s+/, $line);
+	(my $snap_name = $snapshot) =~ s/^.*@//;
+
+	$info->{$snap_name} = {
+	    id => $guid,
+	    timestamp => $creation,
+	};
+    }
+    return $info;
+}
+
 sub volume_snapshot_list {
     my ($class, $scfg, $storeid, $volname) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH storage 2/3] plugin: remove volume_snapshot_list

[Fabian Ebner]

which was only used by replication, but now replication uses
volume_snapshot_info instead.

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

Breaks old pve-guest-common.

Requires APIVER+APIAGE bump (it's in the next patch).

 PVE/Storage.pm               | 16 ----------------
 PVE/Storage/Plugin.pm        |  9 ---------
 PVE/Storage/ZFSPlugin.pm     |  6 ------
 PVE/Storage/ZFSPoolPlugin.pm |  9 ---------
 4 files changed, 40 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index f9582d0..5d389be 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -380,22 +380,6 @@ sub volume_snapshot_info {
     return $plugin->volume_snapshot_info($scfg, $storeid, $volname);
 }
 
-sub volume_snapshot_list {
-    my ($cfg, $volid) = @_;
-
-    my ($storeid, $volname) = parse_volume_id($volid, 1);
-    if ($storeid) {
-	my $scfg = storage_config($cfg, $storeid);
-	my $plugin = PVE::Storage::Plugin->lookup($scfg->{type});
-	return $plugin->volume_snapshot_list($scfg, $storeid, $volname);
-    } elsif ($volid =~ m|^(/.+)$| && -e $volid) {
-	die "send file/device '$volid' is not possible\n";
-    } else {
-	die "unable to parse volume ID '$volid'\n";
-    }
-    # return an empty array if dataset does not exist.
-}
-
 sub get_image_dir {
     my ($cfg, $storeid, $vmid) = @_;
 
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index d4b3615..42eabdf 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -1222,15 +1222,6 @@ sub volume_snapshot_info {
     die "volume_snapshot_info is not implemented for $class";
 }
 
-sub volume_snapshot_list {
-    my ($class, $scfg, $storeid, $volname) = @_;
-
-    # implement in subclass
-    die "Volume_snapshot_list is not implemented for $class";
-
-    # return an empty array if dataset does not exist.
-}
-
 sub activate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
diff --git a/PVE/Storage/ZFSPlugin.pm b/PVE/Storage/ZFSPlugin.pm
index 5b84d85..d4dc2a4 100644
--- a/PVE/Storage/ZFSPlugin.pm
+++ b/PVE/Storage/ZFSPlugin.pm
@@ -391,12 +391,6 @@ sub volume_has_feature {
     return undef;
 }
 
-sub volume_snapshot_list {
-    my ($class, $scfg, $storeid, $volname) = @_;
-    # return an empty array if dataset does not exist.
-    die "Volume_snapshot_list is not implemented for ZFS over iSCSI.\n";
-}
-
 sub activate_storage {
     my ($class, $storeid, $scfg, $cache) = @_;
 
diff --git a/PVE/Storage/ZFSPoolPlugin.pm b/PVE/Storage/ZFSPoolPlugin.pm
index 01d0743..278438b 100644
--- a/PVE/Storage/ZFSPoolPlugin.pm
+++ b/PVE/Storage/ZFSPoolPlugin.pm
@@ -532,15 +532,6 @@ sub volume_snapshot_info {
     return $info;
 }
 
-sub volume_snapshot_list {
-    my ($class, $scfg, $storeid, $volname) = @_;
-
-    my $snaps = [];
-    # return an empty array if dataset does not exist.
-    eval { $snaps = $class->zfs_get_sorted_snapshot_list($scfg, $volname, ['-S', 'name']); };
-    return $snaps;
-}
-
 my sub dataset_mounted_heuristic {
     my ($dataset) = @_;
 
-- 
2.30.2

[pve-devel] [PATCH storage 3/3] bump APIVER and APIAGE

[Fabian Ebner]

Added blockers parameter to volume_rollback_is_possible.
Replaced volume_snapshot_list with volume_snapshot_info.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 ApiChangeLog   | 16 ++++++++++++++++
 PVE/Storage.pm |  4 ++--
 2 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/ApiChangeLog b/ApiChangeLog
index 8c119c5..4094ca2 100644
--- a/ApiChangeLog
+++ b/ApiChangeLog
@@ -6,6 +6,21 @@ without breaking anything unaware of it.)
 
 Future changes should be documented in here.
 
+##  Version 10:
+
+* Replace `volume_snapshot_list` with `volume_snapshot_info`:
+
+  `volume_snapshot_list` was used exclusively by replication and currently, replication is only
+  allowed for the storage type `zfspool`. Thus, no external plugins should be affected by this
+  change and `APIAGE` is *not* reset.
+
+  `volume_snapshot_info` returns a hash with snapshot names as keys and `id` and `timestamp` data
+  for each snapshot, rather than just an array of snaphsot names like `volume_snapshot_list` did.
+
+* Add `blockers` parameter to `volume_rollback_is_possible`:
+
+  The parameter *can* be used to return a list of snapshots that is currently preventing rollback.
+
 ##  Version 9: (AGE resets to 0):
 
 * volume_import_formats gets a new parameter *inserted*:
@@ -23,3 +38,4 @@ Future changes should be documented in here.
 * $with_snapshots *may* now be an array reference containing an ordered list of
   snapshots, but *may* also just be a boolean, and the contained list *may* be
   ignored, so it can still be treated as a boolean.
+
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 5d389be..bc477c1 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -41,11 +41,11 @@ use PVE::Storage::PBSPlugin;
 use PVE::Storage::BTRFSPlugin;
 
 # Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 9;
+use constant APIVER => 10;
 # Age is the number of versions we're backward compatible with.
 # This is like having 'current=APIVER' and age='APIAGE' in libtool,
 # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 0;
+use constant APIAGE => 1;
 
 # load standard plugins
 PVE::Storage::DirPlugin->register();
-- 
2.30.2

[pve-devel] [PATCH guest-common 1/4] replication: refactor finding most recent common replication snapshot

[Fabian Ebner]

By using a single loop instead. Should make the code more readable,
but also more efficient.

Suggested-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/Replication.pm | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 098ac00..32395ed 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -67,17 +67,19 @@ sub find_common_replication_snapshot {
 		$base_snapshots->{$volid} = $parent_snapname;
 	    } else {
 		# First, try all replication snapshots.
-		my @desc_sorted_snap =
-		    map { $_->[1] } sort { $b->[0] <=> $a->[0] }
-		    grep { $_->[0] != 0 } # only consider replication snapshots
-		    map { [ ($_ =~ /__replicate_\Q$vmid\E-(?:\d+)_(\d+)_/)[0] || 0, $_ ] }
-		    keys %{$remote_snapshots->{$volid}};
-
-		foreach my $remote_snap (@desc_sorted_snap) {
-		    if (defined($last_snapshots->{$volid}->{$remote_snap})) {
-			$base_snapshots->{$volid} = $remote_snap;
-			last;
-		    }
+		my $most_recent = [0, undef];
+		for my $remote_snap (keys $remote_snapshots->{$volid}->%*) {
+		    next if !defined($last_snapshots->{$volid}->{$remote_snap});
+
+		    my $timestamp = ($remote_snap =~ /__replicate_\Q$vmid\E-(?:\d+)_(\d+)_/)[0];
+		    next if !$timestamp;
+
+		    $most_recent = [$timestamp, $remote_snap] if $timestamp > $most_recent->[0];
+		}
+
+		if ($most_recent->[1]) {
+		    $base_snapshots->{$volid} = $most_recent->[1];
+		    next;
 		}
 
 		# Then, try config snapshots ($parent_snapname was already tested for above).
-- 
2.30.2

[pve-devel] [PATCH guest-common 2/4] replication: prepare: return additional information about snapshots

[Fabian Ebner]

This is backwards compatible, because existing users of prepare() only
rely on the elements to evaluate to true or be defined.

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

Depends on pve-storage for the new volume_snapshot_info function.

Build-breaks old pve-manager, because the test there assumes
volume_snapshot_list is used.

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

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 32395ed..4d4f62f 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -181,11 +181,11 @@ sub prepare {
     my $last_snapshots = {};
     my $cleaned_replicated_volumes = {};
     foreach my $volid (@$volids) {
-	my $list = PVE::Storage::volume_snapshot_list($storecfg, $volid);
-	foreach my $snap (@$list) {
+	my $info = PVE::Storage::volume_snapshot_info($storecfg, $volid);
+	for my $snap (keys $info->%*) {
 	    if ((defined($snapname) && ($snap eq $snapname)) ||
 		(defined($parent_snapname) && ($snap eq $parent_snapname))) {
-		$last_snapshots->{$volid}->{$snap} = 1;
+		$last_snapshots->{$volid}->{$snap} = $info->{$snap};
 	    } elsif ($snap =~ m/^\Q$prefix\E/) {
 		if ($last_sync != 0) {
 		    $logfunc->("delete stale replication snapshot '$snap' on $volid");
@@ -206,11 +206,11 @@ sub prepare {
 		    }		
 		# Last_sync=0 and a replication snapshot only occur, if the VM was stolen
 		} else {
-		    $last_snapshots->{$volid}->{$snap} = 1;
+		    $last_snapshots->{$volid}->{$snap} = $info->{$snap};
 		}
 	    # Other snapshots might need to serve as replication base after rollback
 	    } else {
-		$last_snapshots->{$volid}->{$snap} = 1;
+		$last_snapshots->{$volid}->{$snap} = $info->{$snap};
 	    }
 	}
     }
-- 
2.30.2

[pve-devel] [PATCH guest-common 3/4] replication: find common snapshot: use additional information

[Fabian Ebner]

which is now available from the storage back-end.

The benefits are:

1. Ability to detect different snapshots even if they have the same
name. Rather hard to reach, but for example with:
Snapshots A -> B -> C -> __replicationXYZ
Remove B, rollback to C (causes __replicationXYZ to be removed),
create a new snapshot B. Previously, B was selected as replication
base, but it didn't match on source and target. Now, C is correctly
selected.
2. Smaller delta in some cases by not prefering replication snapshots
over config snapshots, but using the most recent possible one from
both types of snapshots.
3. Less code complexity for snapshot selection.

If the remote side is old, it's not possible to detect mismatch of
distinct snapshots with the same name, but the timestamps from the
local side can still be used.

Still limit to our snapshots (config and replication), because we
don't have guarantees for other snapshots (could be deleted in the
meantime, name might not fit import/export regex, etc.).

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PVE/Replication.pm | 55 ++++++++++++++++++------------------------
 1 file changed, 24 insertions(+), 31 deletions(-)

diff --git a/src/PVE/Replication.pm b/src/PVE/Replication.pm
index 4d4f62f..051dfd9 100644
--- a/src/PVE/Replication.pm
+++ b/src/PVE/Replication.pm
@@ -31,6 +31,7 @@ sub find_common_replication_snapshot {
     my ($ssh_info, $jobid, $vmid, $storecfg, $volumes, $storeid_list, $last_sync, $guest_conf, $logfunc) = @_;
 
     my $parent_snapname = $guest_conf->{parent};
+    my $conf_snapshots = $guest_conf->{snapshots};
 
     my $last_sync_snapname =
 	PVE::ReplicationState::replication_snapshot_name($jobid, $last_sync);
@@ -57,24 +58,35 @@ sub find_common_replication_snapshot {
     my $base_snapshots = {};
 
     foreach my $volid (@$volumes) {
-	if (defined($last_snapshots->{$volid}) && defined($remote_snapshots->{$volid})) {
-	    if ($last_snapshots->{$volid}->{$last_sync_snapname} &&
-		$remote_snapshots->{$volid}->{$last_sync_snapname}) {
+	my $local_info = $last_snapshots->{$volid};
+	my $remote_info = $remote_snapshots->{$volid};
+
+	if (defined($local_info) && defined($remote_info)) {
+	    my $common_snapshot = sub {
+		my ($snap) = @_;
+
+		return 0 if !$local_info->{$snap} || !$remote_info->{$snap};
+
+		# Check for ID if remote side supports it already.
+		return $local_info->{$snap}->{id} eq $remote_info->{$snap}->{id}
+		    if ref($remote_info->{$snap}) eq 'HASH';
+
+		return 1;
+	    };
+
+	    if ($common_snapshot->($last_sync_snapname)) {
 		$base_snapshots->{$volid} = $last_sync_snapname;
-	    } elsif (defined($parent_snapname) &&
-		     ($last_snapshots->{$volid}->{$parent_snapname} &&
-		      $remote_snapshots->{$volid}->{$parent_snapname})) {
+	    } elsif (defined($parent_snapname) && $common_snapshot->($parent_snapname)) {
 		$base_snapshots->{$volid} = $parent_snapname;
 	    } else {
-		# First, try all replication snapshots.
 		my $most_recent = [0, undef];
-		for my $remote_snap (keys $remote_snapshots->{$volid}->%*) {
-		    next if !defined($last_snapshots->{$volid}->{$remote_snap});
+		for my $snapshot (keys $local_info->%*) {
+		    next if !$common_snapshot->($snapshot);
+		    next if !$conf_snapshots->{$snapshot} && !is_replication_snapshot($snapshot);
 
-		    my $timestamp = ($remote_snap =~ /__replicate_\Q$vmid\E-(?:\d+)_(\d+)_/)[0];
-		    next if !$timestamp;
+		    my $timestamp = $local_info->{$snapshot}->{timestamp};
 
-		    $most_recent = [$timestamp, $remote_snap] if $timestamp > $most_recent->[0];
+		    $most_recent = [$timestamp, $snapshot] if $timestamp > $most_recent->[0];
 		}
 
 		if ($most_recent->[1]) {
@@ -82,25 +94,6 @@ sub find_common_replication_snapshot {
 		    next;
 		}
 
-		# Then, try config snapshots ($parent_snapname was already tested for above).
-		my $snapname = $parent_snapname // '';
-
-		# Be robust against loop, just in case.
-		# https://forum.proxmox.com/threads/snapshot-not-working.69511/post-312281
-		my $max_count = scalar(keys $guest_conf->{snapshots}->%*);
-		for (my $count = 0; $count < $max_count; $count++) {
-		    last if defined($base_snapshots->{$volid});
-
-		    my $snap_conf = $guest_conf->{snapshots}->{$snapname} || last;
-		    $snapname = $snap_conf->{parent} // last;
-
-		    if ($last_snapshots->{$volid}->{$snapname} &&
-			$remote_snapshots->{$volid}->{$snapname})
-		    {
-			$base_snapshots->{$volid} = $snapname;
-		    }
-		}
-
 		# 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".
-- 
2.30.2

[pve-devel] [RFC guest-common 4/4] config: snapshot delete check: use volume_snapshot_info

[Fabian Ebner]

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

To be applied if the RFC from the original series is:
https://lists.proxmox.com/pipermail/pve-devel/2021-August/049705.html

 src/PVE/AbstractConfig.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/PVE/AbstractConfig.pm b/src/PVE/AbstractConfig.pm
index 39f1cc8..e62f542 100644
--- a/src/PVE/AbstractConfig.pm
+++ b/src/PVE/AbstractConfig.pm
@@ -846,14 +846,14 @@ my $snapshot_delete_assert_not_needed_by_replication = sub {
 
 	return if !$volumes->{$volid};
 
-	my $snapshots = PVE::Storage::volume_snapshot_list($storecfg, $volid);
+	my $snapshots = PVE::Storage::volume_snapshot_info($storecfg, $volid);
 
 	for my $job ($replication_jobs->@*) {
 	    my $jobid = $job->{id};
 
 	    my @jobs_snapshots = grep {
 		PVE::Replication::is_replication_snapshot($_, $jobid)
-	    } $snapshots->@*;
+	    } keys $snapshots->%*;
 
 	    next if scalar(@jobs_snapshots) > 0;
 
-- 
2.30.2

[pve-devel] [PATCH manager 1/3] test: replication: avoid implicit return for volume_snapshot

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 test/ReplicationTestEnv.pm | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/test/ReplicationTestEnv.pm b/test/ReplicationTestEnv.pm
index 005e6d54..dea1921b 100755
--- a/test/ReplicationTestEnv.pm
+++ b/test/ReplicationTestEnv.pm
@@ -187,6 +187,8 @@ my $mocked_volume_snapshot = sub {
     my $d = $mocked_storage_content->{$storeid}->{$volname};
     die "no such volid '$volid'\n" if !$d;
     $d->{$snap} = 1;
+
+    return;
 };
 
 my $mocked_volume_snapshot_delete = sub {
-- 
2.30.2

[pve-devel] [PATCH manager 2/3] test: replication: mock volume_snapshot_info

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 test/ReplicationTestEnv.pm | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/test/ReplicationTestEnv.pm b/test/ReplicationTestEnv.pm
index dea1921b..35653e75 100755
--- a/test/ReplicationTestEnv.pm
+++ b/test/ReplicationTestEnv.pm
@@ -154,6 +154,17 @@ my $pve_storage_module = Test::MockModule->new('PVE::Storage');
 
 my $mocked_storage_content = {};
 
+my $timestamp_counter = 0;
+
+sub generate_snapshot_info {
+    $timestamp_counter++;
+
+    return {
+	id => $timestamp_counter,
+	timestamp => $timestamp_counter,
+    };
+}
+
 sub register_mocked_volid {
     my ($volid, $snapname) = @_;
 
@@ -163,7 +174,7 @@ sub register_mocked_volid {
 
     my $d = $mocked_storage_content->{$storeid}->{$volname} //= {};
 
-    $d->{$snapname} = 1 if $snapname;
+    $d->{$snapname} = generate_snapshot_info() if $snapname;
 }
 
 my $mocked_volume_snapshot_list = sub {
@@ -186,7 +197,7 @@ my $mocked_volume_snapshot = sub {
 
     my $d = $mocked_storage_content->{$storeid}->{$volname};
     die "no such volid '$volid'\n" if !$d;
-    $d->{$snap} = 1;
+    $d->{$snap} = generate_snapshot_info();
 
     return;
 };
@@ -200,6 +211,14 @@ my $mocked_volume_snapshot_delete = sub {
     delete $d->{$snap} || die "no such snapshot '$snap' on '$volid'\n";
 };
 
+my $mocked_volume_snapshot_info = sub {
+    my ($cfg, $volid) = @_;
+
+    my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
+
+    return $mocked_storage_content->{$storeid}->{$volname} // {};
+};
+
 my $pve_replication_module = Test::MockModule->new('PVE::Replication');
 
 my $mocked_job_logfile_name = sub {
@@ -253,6 +272,7 @@ sub setup {
     $pve_storage_module->mock(volume_snapshot_list => $mocked_volume_snapshot_list);
     $pve_storage_module->mock(volume_snapshot => $mocked_volume_snapshot);
     $pve_storage_module->mock(volume_snapshot_delete => $mocked_volume_snapshot_delete);
+    $pve_storage_module->mock(volume_snapshot_info => $mocked_volume_snapshot_info);
 
     $pve_replication_config_module->mock(
 	new => $mocked_replication_config_new,
-- 
2.30.2

[pve-devel] [PATCH manager 3/3] test: replication: remove mocking for obsolete volume_snapshot_list

[Fabian Ebner]

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

Build-depends on pve-guest-common already using volume_snapshot_info.

 test/ReplicationTestEnv.pm | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/test/ReplicationTestEnv.pm b/test/ReplicationTestEnv.pm
index 35653e75..883bebca 100755
--- a/test/ReplicationTestEnv.pm
+++ b/test/ReplicationTestEnv.pm
@@ -177,19 +177,6 @@ sub register_mocked_volid {
     $d->{$snapname} = generate_snapshot_info() if $snapname;
 }
 
-my $mocked_volume_snapshot_list = sub {
-    my ($cfg, $volid, $prefix) = @_;
-
-    my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid);
-    my $snaps = [];
-
-    if (my $d = $mocked_storage_content->{$storeid}->{$volname}) {
-	$snaps = [keys %$d];
-    }
-
-    return $snaps;
-};
-
 my $mocked_volume_snapshot = sub {
     my ($cfg, $volid, $snap) = @_;
 
@@ -269,7 +256,6 @@ sub setup {
     $pve_replication_module->mock(get_log_time => $mocked_get_log_time);
 
     $pve_storage_module->mock(config => sub { return $mocked_storage_config; });
-    $pve_storage_module->mock(volume_snapshot_list => $mocked_volume_snapshot_list);
     $pve_storage_module->mock(volume_snapshot => $mocked_volume_snapshot);
     $pve_storage_module->mock(volume_snapshot_delete => $mocked_volume_snapshot_delete);
     $pve_storage_module->mock(volume_snapshot_info => $mocked_volume_snapshot_info);
-- 
2.30.2

[pve-devel] applied-series: [PATCH-SERIES storage/guest-common/manager] Follow-up for fixing replication/rollback interaction

[Fabian Grünbichler]

On October 19, 2021 9:54 am, Fabian Ebner wrote:
> Returning more information about snapshots allows for better decisions
> when picking the incremental base snapshot. Namely, to distinguish
> between different snapshots with the same name, and to choose a more
> recent snapshot in some cases, reducing the send delta. On top of
> that, the code in find_common_replication_snapshot becomes simpler.
> 
> Mostly as discussed off-list with Fabian G., but instead of just
> changing the return type of volume_snapshot_list, it has been replaced
> with a new function. I felt like that would allow for a cleaner break
> and the old name was not fully accurate anymore.
> 
> 
> Applies on top of the original series:
> https://lists.proxmox.com/pipermail/pve-devel/2021-August/049699.html
> 
> 
> Dependencies/breaks:
> 1. pve-guest-common (except patch #1) depends on pve-storage patch #1.
> 2. pve-storage patch #2 breaks old pve-guest-common.
> 3. pve-guest-common patch #2 build-breaks old pve-manager.
> 4. pve-manager patch #3 build-depends on pve-guest-common patch #2.
> 
> 
> pve-storage:
> 
> Fabian Ebner (3):
>   plugin: add volume_snapshot_info function
>   plugin: remove volume_snapshot_list
>   bump APIVER and APIAGE
> 
>  ApiChangeLog                 | 16 ++++++++++++++++
>  PVE/Storage.pm               | 21 +++++++--------------
>  PVE/Storage/Plugin.pm        | 11 ++++++-----
>  PVE/Storage/ZFSPlugin.pm     |  6 ------
>  PVE/Storage/ZFSPoolPlugin.pm | 23 ++++++++++++++++++-----
>  5 files changed, 47 insertions(+), 30 deletions(-)
> 
> 
> pve-guest-common:
> 
> Fabian Ebner (4):
>   replication: refactor finding most recent common replication snapshot
>   replication: prepare: return additional information about snapshots
>   replication: find common snapshot: use additional information
>   config: snapshot delete check: use volume_snapshot_info
> 
>  src/PVE/AbstractConfig.pm |  4 +--
>  src/PVE/Replication.pm    | 73 ++++++++++++++++++---------------------
>  2 files changed, 36 insertions(+), 41 deletions(-)
> 
> 
> pve-manager:
> 
> Fabian Ebner (3):
>   test: replication: avoid implicit return for volume_snapshot
>   test: replication: mock volume_snapshot_info
>   test: replication: remove mocking for obsolete volume_snapshot_list
> 
>  test/ReplicationTestEnv.pm | 40 +++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 16 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>