[pve-devel] [PATCH storage 1/3] volume export/import: allow uppercase letters

[pve-devel] [PATCH storage 1/3] volume export/import: allow uppercase letters

[Fabian Ebner]

Bug reported in the community forum[0].

Currently, it's possible to break replication by:
1. have an existing snapshot whose name contains an uppercase letter
2. set up a replication job and run it
3. rollback to the existing snapshot
4. replicate again -> fails

The failure occurs, because after step 3, the most recent common snapshot is the
previously existing one and currently no uppercase letters are allowed for
export/import.

The pve-snapshot-name option uses the CONFIGID_RE
    qr/[a-z][a-z0-9_-]+/i
so it cannot be used here, because it would not allow for e.g. '__migrate__'.
Simply allow uppercase letters, to be backwards compatible and allow all
possible pve-snapshot-name values.

There is still an issue if there also was state volume, but that's a different
bug[1].

[0]: https://forum.proxmox.com/threads/solved-migration-error-base-value-does-not-match-the-regex-pattern.85946/
[1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3111

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/CLI/pvesm.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
index 3594774..7b46897 100755
--- a/PVE/CLI/pvesm.pm
+++ b/PVE/CLI/pvesm.pm
@@ -244,14 +244,14 @@ __PACKAGE__->register_method ({
 	    base => {
 		description => "Snapshot to start an incremental stream from",
 		type => 'string',
-		pattern => qr/[a-z0-9_\-]{1,40}/,
+		pattern => qr/[a-z0-9_\-]{1,40}/i,
 		maxLength => 40,
 		optional => 1,
 	    },
 	    snapshot => {
 		description => "Snapshot to export",
 		type => 'string',
-		pattern => qr/[a-z0-9_\-]{1,40}/,
+		pattern => qr/[a-z0-9_\-]{1,40}/i,
 		maxLength => 40,
 		optional => 1,
 	    },
@@ -321,7 +321,7 @@ __PACKAGE__->register_method ({
 	    base => {
 		description => "Base snapshot of an incremental stream",
 		type => 'string',
-		pattern => qr/[a-z0-9_\-]{1,40}/,
+		pattern => qr/[a-z0-9_\-]{1,40}/i,
 		maxLength => 40,
 		optional => 1,
 	    },
@@ -335,7 +335,7 @@ __PACKAGE__->register_method ({
 	    'delete-snapshot' => {
 		description => "A snapshot to delete on success",
 		type => 'string',
-		pattern => qr/[a-z0-9_\-]{1,80}/,
+		pattern => qr/[a-z0-9_\-]{1,80}/i,
 		maxLength => 80,
 		optional => 1,
 	    },
-- 
2.20.1

[pve-devel] [PATCH guest-common 2/3] partially fix 3111: snapshot rollback: fix removing replication snapshots

[Fabian Ebner]

Get the replicatable volumes from the snapshot config rather than the current
config. And filter those volumes further to those that will actually be rolled
back.

Previously, a volume that only had replication snapshots (e.g. because it was
added after the non-replication snapshot was taken, or the vmstate volume) would
end up without any snapshots whatsoever. Then, on the next replication run, such
a volume would lead to an error, because replication tried to do a full sync,
but the target volume still existed.

Should be enough for many real-world scenarios, but not a complete fix:
It is still possible to run into the problem by removing the last
(non-replication) snapshot after a rollback before replication can run once.

The list of volids is not required to be sorted by volid for prepare().
(It is now sorted by how foreach_volume() iterates.)

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/AbstractConfig.pm | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
index 3348d8a..c4d1d6c 100644
--- a/PVE/AbstractConfig.pm
+++ b/PVE/AbstractConfig.pm
@@ -974,13 +974,22 @@ sub snapshot_rollback {
 	if ($prepare) {
 	    my $repl_conf = PVE::ReplicationConfig->new();
 	    if ($repl_conf->check_for_existing_jobs($vmid, 1)) {
-		# remove all replication snapshots
-		my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $conf, 1);
-		my $sorted_volids = [ sort keys %$volumes ];
+		# remove replication snapshots on volumes affected by rollback *only*!
+		my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $snap, 1);
+
+		my $volids = [];
+		$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};
+		});
 
 		# remove all local replication snapshots (jobid => undef)
 		my $logfunc = sub { my $line = shift; chomp $line; print "$line\n"; };
-		PVE::Replication::prepare($storecfg, $sorted_volids, undef, 1, undef, $logfunc);
+		PVE::Replication::prepare($storecfg, $volids, undef, 1, undef, $logfunc);
 	    }
 
 	    $class->foreach_volume($snap, sub {
-- 
2.20.1

[pve-devel] [RFC guest-common 3/3] fix 3111: replicate guest on rollback if there are replication jobs for it

[Fabian Ebner]

so that there will be a valid replication snapshot again.

Otherwise, replication will be broken after a rollback if the last
(non-replication) snapshot is removed before replication can run once.

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

Not a huge fan of this, but the alternatives I could come up with don't seem
much better IMHO:

1. Invalidate/remove replicated volumes after a rollback altogether and require
a full sync on the next replication job afterwards.

2. Another one is to disallow removing the last non-replication snapshot if:
    * there is a replication job configured
    * no replication snapshot for that job currently exists (which likely
      means it was removed by a previous rollback operation, but can also happen
      for a new job that didn't run yet).

3. Hope not very many people immediately delete their snapshots after rollback.

Pick a favorite or suggest your own ;)

 PVE/AbstractConfig.pm    | 19 +++++++++++++++++--
 PVE/ReplicationConfig.pm | 14 ++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/PVE/AbstractConfig.pm b/PVE/AbstractConfig.pm
index c4d1d6c..b169b8b 100644
--- a/PVE/AbstractConfig.pm
+++ b/PVE/AbstractConfig.pm
@@ -951,6 +951,9 @@ sub snapshot_rollback {
 
     my $storecfg = PVE::Storage::config();
 
+    my $repl_conf = PVE::ReplicationConfig->new();
+    my $logfunc = sub { my $line = shift; chomp $line; print "$line\n"; };
+
     my $data = {};
 
     my $get_snapshot_config = sub {
@@ -972,7 +975,6 @@ sub snapshot_rollback {
 	$snap = $get_snapshot_config->($conf);
 
 	if ($prepare) {
-	    my $repl_conf = PVE::ReplicationConfig->new();
 	    if ($repl_conf->check_for_existing_jobs($vmid, 1)) {
 		# remove replication snapshots on volumes affected by rollback *only*!
 		my $volumes = $class->get_replicatable_volumes($storecfg, $vmid, $snap, 1);
@@ -988,7 +990,6 @@ sub snapshot_rollback {
 		});
 
 		# 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);
 	    }
 
@@ -1046,6 +1047,20 @@ sub snapshot_rollback {
 
     $prepare = 0;
     $class->lock_config($vmid, $updatefn);
+
+    my $replication_jobs = $repl_conf->list_guests_replication_jobs($vmid);
+    for my $job (@{$replication_jobs}) {
+	my $target = $job->{target};
+	$logfunc->("replicating rolled back guest to node '$target'");
+
+	my $start_time = time();
+	eval {
+	    PVE::Replication::run_replication($class, $job, $start_time, $start_time, $logfunc);
+	};
+	if (my $err = $@) {
+	    warn "unable to replicate rolled back guest to node '$target' - $err";
+	}
+    }
 }
 
 # bash completion helper
diff --git a/PVE/ReplicationConfig.pm b/PVE/ReplicationConfig.pm
index fd856a0..84a718f 100644
--- a/PVE/ReplicationConfig.pm
+++ b/PVE/ReplicationConfig.pm
@@ -228,6 +228,20 @@ sub find_local_replication_job {
     return undef;
 }
 
+sub list_guests_replication_jobs {
+    my ($cfg, $vmid) = @_;
+
+    my $jobs = [];
+
+    for my $job (values %{$cfg->{ids}}) {
+	next if $job->{type} ne 'local' || $job->{guest} != $vmid;
+
+	push @{$jobs}, $job;
+    }
+
+    return $jobs;
+}
+
 # makes old_target the new source for all local jobs of this guest
 # makes new_target the target for the single local job with target old_target
 sub switch_replication_job_target_nolock {
-- 
2.20.1

[pve-devel] applied: [PATCH storage 1/3] volume export/import: allow uppercase letters

[Thomas Lamprecht]

On 12.04.21 13:37, Fabian Ebner wrote:
> Bug reported in the community forum[0].
> 
> Currently, it's possible to break replication by:
> 1. have an existing snapshot whose name contains an uppercase letter
> 2. set up a replication job and run it
> 3. rollback to the existing snapshot
> 4. replicate again -> fails
> 
> The failure occurs, because after step 3, the most recent common snapshot is the
> previously existing one and currently no uppercase letters are allowed for
> export/import.
> 
> The pve-snapshot-name option uses the CONFIGID_RE
>     qr/[a-z][a-z0-9_-]+/i
> so it cannot be used here, because it would not allow for e.g. '__migrate__'.
> Simply allow uppercase letters, to be backwards compatible and allow all
> possible pve-snapshot-name values.
> 
> There is still an issue if there also was state volume, but that's a different
> bug[1].
> 
> [0]: https://forum.proxmox.com/threads/solved-migration-error-base-value-does-not-match-the-regex-pattern.85946/
> [1]: https://bugzilla.proxmox.com/show_bug.cgi?id=3111
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/CLI/pvesm.pm | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
>

applied, thanks!

albeit with perl's extensice regex and the subtleties of unicode casing I'm not
always sure using the case-insensitive flag is the way to go, adding A-Z to the
allowed character set would be actually the smaller semantic change.

But it should not matter that much here, so I kept it as is for now...