[pve-devel] [PATCH-SERIES zsync] fix #1669: allow prepending storage ID

[pve-devel] [PATCH-SERIES zsync] fix #1669: allow prepending storage ID

[Fabian Ebner]

which makes it possible to use pve-zsync for guests with disks with the same
name on different storages, i.e.
    storageA:vm-123-disk-0
    storageB:vm-123-disk-0

Currently they would collide to
    target/vm-123-disk-0

When the new prepend-storage-id flag is specified, they will be synced to
    target/storageA/vm-123-disk-0$
    target/storageB/vm-123-disk-0$
instead.


The first four patches are cleanups/usage description fixes.

The next five patches are preparations for the last patch, which is the one
actually implementing the new feature.


Fabian Ebner (10):
  whitespace fix
  copyright: update year
  usage: describe flag parameters correctly
  usage: fix type for maxsnap
  add target_dataset function
  add check_dataset_exists function
  add create_file_system function
  parse disks: don't include colon in storage name variable
  parse disks: also include storage ID information
  fix #1669: add prepend-storage-id flag

 pve-zsync | 118 ++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 89 insertions(+), 29 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH zsync 01/10] whitespace fix

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pve-zsync b/pve-zsync
index 5c95955..71cb6dd 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -862,7 +862,7 @@ sub parse_disks {
 	my $path = run_cmd($cmd);
 
 	die "Get no path from pvesm path $stor$disk\n" if !$path;
- 
+
 	if ($vm_type eq 'qemu' && $path =~ m/^\/dev\/zvol\/(\w+.*)(\/$disk)$/) {
 
 	    my @array = split('/', $1);
-- 
2.20.1

[pve-devel] [PATCH zsync 02/10] copyright: update year

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pve-zsync b/pve-zsync
index 71cb6dd..deecfed 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -1365,7 +1365,7 @@ The VM config get copied on the destination machine to  /var/lib/pve-zsync/
 
 =head1 COPYRIGHT AND DISCLAIMER
 
-Copyright (C) 2007-2015 Proxmox Server Solutions GmbH
+Copyright (C) 2007-2021 Proxmox Server Solutions GmbH
 
 This program is free software: you can redistribute it and/or modify it
 under the terms of the GNU Affero General Public License as published
-- 
2.20.1

[pve-devel] [PATCH zsync 03/10] usage: describe flag parameters correctly

[Fabian Ebner]

It's not possible to provide a false value. From the documentation for
GetOptionsFromArray:
    The argument specification is optional. If omitted, the option is considered
    boolean, a value of 1 will be assigned when the option is used on the
    command line.

Make it clear that specifying the flag alone already enables it.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/pve-zsync b/pve-zsync
index deecfed..2c10a4d 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -1114,9 +1114,9 @@ $PROGNAME create -dest <string> -source <string> [OPTIONS]
 
 		name of the sync job, if not set it is default
 
-        -skip      boolean
+        -skip
 
-		if this flag is set it will skip the first sync
+		If specified, skip the first sync.
 
         -source    string
 
@@ -1126,9 +1126,9 @@ $PROGNAME create -dest <string> -source <string> [OPTIONS]
 
 		name of the user on the source target, root by default
 
-	-properties	boolean
+	-properties
 
-		Include the dataset's properties in the stream.
+		If specified, include the dataset's properties in the stream.
 
 	-dest-config-path    string
 
@@ -1168,13 +1168,13 @@ $PROGNAME sync -dest <string> -source <string> [OPTIONS]\n
 
 		name of the user on the source target, root by default
 
-	-verbose   boolean
+	-verbose
 
-		print out the sync progress.
+		If specified, print out the sync progress.
 
-	-properties	boolean
+	-properties
 
-		Include the dataset's properties in the stream.
+		If specified, include the dataset's properties in the stream.
 
 	-dest-config-path    string
 
@@ -1199,7 +1199,7 @@ $PROGNAME help <cmd> [OPTIONS]
 
 		Command name
 
-	-verbose   boolean
+	-verbose
 
 		Verbose output format.
     },
-- 
2.20.1

[pve-devel] [PATCH zsync 04/10] usage: fix type for maxsnap

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pve-zsync b/pve-zsync
index 2c10a4d..a21da49 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -1106,7 +1106,7 @@ $PROGNAME create -dest <string> -source <string> [OPTIONS]
 
 		max sync speed in kBytes/s, default unlimited
 
-        -maxsnap   string
+        -maxsnap   integer
 
 		how much snapshots will be kept before get erased, default 1
 
-- 
2.20.1

[pve-devel] [PATCH zsync 05/10] add target_dataset function

[Fabian Ebner]

so the logic can be re-used and extended in a single place.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/pve-zsync b/pve-zsync
index a21da49..93b0d0d 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -725,8 +725,7 @@ sub snapshot_get{
     push @$cmd, 'ssh', "$dest_user\@$dest->{ip}", '--', if $dest->{ip};
     push @$cmd, 'zfs', 'list', '-r', '-t', 'snapshot', '-Ho', 'name', '-S', 'creation';
 
-    my $path = $dest->{all};
-    $path .= "/$source->{last_part}" if $source->{last_part};
+    my $path = target_dataset($source, $dest);
     push @$cmd, $path;
 
     my $raw;
@@ -900,6 +899,17 @@ sub parse_disks {
     return $disks;
 }
 
+# how the corresponding dataset is named on the target
+sub target_dataset {
+    my ($source, $dest) = @_;
+
+    my $target = "$dest->{all}";
+    $target .= "/$source->{last_part}" if $source->{last_part};
+    $target =~ s!/+!/!g;
+
+    return $target;
+}
+
 sub snapshot_destroy {
     my ($source, $dest, $method, $snap, $source_user, $dest_user) = @_;
 
@@ -919,8 +929,7 @@ sub snapshot_destroy {
     if ($dest) {
 	my @ssh = $dest->{ip} ? ('ssh', "$dest_user\@$dest->{ip}", '--') : ();
 
-	my $path = "$dest->{all}";
-	$path .= "/$source->{last_part}" if $source->{last_part};
+	my $path = target_dataset($source, $dest);
 
 	eval {
 	    run_cmd([@ssh, @zfscmd, "$path\@$snap"]);
@@ -971,9 +980,7 @@ sub send_image {
 	my $bwl = $param->{limit}*1024;
 	push @$cmd, \'|', 'cstream', '-t', $bwl;
     }
-    my $target = "$dest->{all}";
-    $target .= "/$source->{last_part}" if $source->{last_part};
-    $target =~ s!/+!/!g;
+    my $target = target_dataset($source, $dest);
 
     push @$cmd, \'|';
     push @$cmd, 'ssh', '-o', 'BatchMode=yes', "$param->{dest_user}\@$dest->{ip}", '--' if $dest->{ip};
-- 
2.20.1

[pve-devel] [PATCH zsync 06/10] add check_dataset_exists function

[Fabian Ebner]

as a generalization and replacement of check_pool_exists.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/pve-zsync b/pve-zsync
index 93b0d0d..a327d42 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -152,15 +152,15 @@ sub get_status {
     return undef;
 }
 
-sub check_pool_exists {
-    my ($target, $user) = @_;
+sub check_dataset_exists {
+    my ($dataset, $ip, $user) = @_;
 
     my $cmd = [];
 
-    if ($target->{ip}) {
-	push @$cmd, 'ssh', "$user\@$target->{ip}", '--';
+    if ($ip) {
+	push @$cmd, 'ssh', "$user\@$ip", '--';
     }
-    push @$cmd, 'zfs', 'list', '-H', '--', $target->{all};
+    push @$cmd, 'zfs', 'list', '-H', '--', $dataset;
     eval {
 	run_cmd($cmd);
     };
@@ -517,10 +517,12 @@ sub init {
 	    run_cmd(['ssh-copy-id', '-i', '/root/.ssh/id_rsa.pub', "$param->{source_user}\@$ip"]);
 	}
 
-	die "Pool $dest->{all} does not exists\n" if !check_pool_exists($dest, $param->{dest_user});
+	die "Pool $dest->{all} does not exist\n"
+	    if !check_dataset_exists($dest->{all}, $dest->{ip}, $param->{dest_user});
 
 	if (!defined($source->{vmid})) {
-	    die "Pool $source->{all} does not exists\n" if !check_pool_exists($source, $param->{source_user});
+	    die "Pool $source->{all} does not exist\n"
+		if !check_dataset_exists($source->{all}, $source->{ip}, $param->{source_user});
 	}
 
 	my $vm_type = vm_exists($source, $param->{source_user});
-- 
2.20.1

[pve-devel] [PATCH zsync 07/10] add create_file_system function

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/pve-zsync b/pve-zsync
index a327d42..101dc03 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -171,6 +171,19 @@ sub check_dataset_exists {
     return 1;
 }
 
+sub create_file_system {
+    my ($file_system, $ip, $user) = @_;
+
+    my $cmd = [];
+
+    if ($ip) {
+	push @$cmd, 'ssh', "$user\@$ip", '--';
+    }
+    push @$cmd, 'zfs', 'create', $file_system;
+
+    run_cmd($cmd);
+}
+
 sub parse_target {
     my ($text) = @_;
 
-- 
2.20.1

[pve-devel] [PATCH zsync 08/10] parse disks: don't include colon in storage name variable

[Fabian Ebner]

in preparation to re-use it elsewhere.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/pve-zsync b/pve-zsync
index 101dc03..e4f927e 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -858,7 +858,7 @@ sub parse_disks {
 	    my @parameter = split(/,/,$1);
 
 	    foreach my $opt (@parameter) {
-		if ($opt =~ m/^(?:file=|volume=)?([^:]+:)([A-Za-z0-9\-]+)$/){
+		if ($opt =~ m/^(?:file=|volume=)?([^:]+):([A-Za-z0-9\-]+)$/){
 		    $disk = $2;
 		    $stor = $1;
 		    last;
@@ -872,10 +872,10 @@ sub parse_disks {
 
 	my $cmd = [];
 	push @$cmd, 'ssh', "$user\@$ip", '--' if $ip;
-	push @$cmd, 'pvesm', 'path', "$stor$disk";
+	push @$cmd, 'pvesm', 'path', "$stor:$disk";
 	my $path = run_cmd($cmd);
 
-	die "Get no path from pvesm path $stor$disk\n" if !$path;
+	die "Get no path from pvesm path $stor:$disk\n" if !$path;
 
 	if ($vm_type eq 'qemu' && $path =~ m/^\/dev\/zvol\/(\w+.*)(\/$disk)$/) {
 
-- 
2.20.1

[pve-devel] [PATCH zsync 09/10] parse disks: also include storage ID information

[Fabian Ebner]

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

diff --git a/pve-zsync b/pve-zsync
index e4f927e..75d4d7c 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -877,6 +877,8 @@ sub parse_disks {
 
 	die "Get no path from pvesm path $stor:$disk\n" if !$path;
 
+	$disks->{$num}->{storage_id} = $stor;
+
 	if ($vm_type eq 'qemu' && $path =~ m/^\/dev\/zvol\/(\w+.*)(\/$disk)$/) {
 
 	    my @array = split('/', $1);
-- 
2.20.1

[pve-devel] [PATCH zsync 10/10] fix #1669: add prepend-storage-id flag

[Fabian Ebner]

so one can set up jobs for guests with disks with the same name on different
storages. For example,
    storageA:vm-123-disk-0
    storageB:vm-123-disk-0
will be synced to
    target/storageA/vm-123-disk-0
    target/storageB/vm-123-disk-0
when the flag is specified.

The necessary parent file systems (one per storage ID) are created as needed.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 pve-zsync | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/pve-zsync b/pve-zsync
index 75d4d7c..521abf0 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -249,6 +249,7 @@ sub parse_argv {
 	method => undef,
 	source_user => undef,
 	dest_user => undef,
+	prepend_storage_id => undef,
 	properties => undef,
 	dest_config_path => undef,
     };
@@ -265,6 +266,7 @@ sub parse_argv {
 	'method=s' => \$param->{method},
 	'source-user=s' => \$param->{source_user},
 	'dest-user=s' => \$param->{dest_user},
+	'prepend-storage-id' => \$param->{prepend_storage_id},
 	'properties' => \$param->{properties},
 	'dest-config-path=s' => \$param->{dest_config_path},
     );
@@ -336,6 +338,7 @@ sub param_to_job {
     $job->{source} = $param->{source};
     $job->{source_user} = $param->{source_user};
     $job->{dest_user} = $param->{dest_user};
+    $job->{prepend_storage_id} = !!$param->{prepend_storage_id};
     $job->{properties} = !!$param->{properties};
     $job->{dest_config_path} = $param->{dest_config_path} if $param->{dest_config_path};
 
@@ -461,6 +464,7 @@ sub format_job {
     $text .= " --verbose" if $job->{verbose};
     $text .= " --source-user $job->{source_user}";
     $text .= " --dest-user $job->{dest_user}";
+    $text .= " --prepend-storage-id" if $job->{prepend_storage_id};
     $text .= " --properties" if $job->{properties};
     $text .= " --dest-config-path $job->{dest_config_path}" if $job->{dest_config_path};
     $text .= "\n";
@@ -674,6 +678,8 @@ sub sync {
 
 	    ($dest->{old_snap}, $dest->{last_snap}) = snapshot_get($source, $dest, $param->{maxsnap}, $param->{name}, $param->{dest_user});
 
+	    prepare_prepended_target($source, $dest, $param->{dest_user}) if defined($dest->{prepend});
+
 	    snapshot_add($source, $dest, $param->{name}, $date, $param->{source_user}, $param->{dest_user});
 
 	    send_image($source, $dest, $param);
@@ -693,6 +699,10 @@ sub sync {
 		    $source->{pool} = $disks->{$disk}->{pool};
 		    $source->{path} = $disks->{$disk}->{path} if $disks->{$disk}->{path};
 		    $source->{last_part} = $disks->{$disk}->{last_part};
+
+		    $dest->{prepend} = $disks->{$disk}->{storage_id}
+			if $param->{prepend_storage_id};
+
 		    &$sync_path($source, $dest, $job, $param, $date);
 		}
 		if ($param->{method} eq "ssh" && ($source->{ip} || $dest->{ip})) {
@@ -921,12 +931,30 @@ sub target_dataset {
     my ($source, $dest) = @_;
 
     my $target = "$dest->{all}";
+    $target .= "/$dest->{prepend}" if defined($dest->{prepend});
     $target .= "/$source->{last_part}" if $source->{last_part};
     $target =~ s!/+!/!g;
 
     return $target;
 }
 
+# create the parent dataset for the actual target
+sub prepare_prepended_target {
+    my ($source, $dest, $dest_user) = @_;
+
+    die "internal error - not a prepended target\n" if !defined($dest->{prepend});
+
+    # The parent dataset shouldn't be the actual target.
+    die "internal error - no last_part for source\n" if !$source->{last_part};
+
+    my $target = "$dest->{all}/$dest->{prepend}";
+    $target =~ s!/+!/!g;
+
+    return if check_dataset_exists($target, $dest->{ip}, $dest_user);
+
+    create_file_system($target, $dest->{ip}, $dest_user);
+}
+
 sub snapshot_destroy {
     my ($source, $dest, $method, $snap, $source_user, $dest_user) = @_;
 
@@ -1138,6 +1166,10 @@ $PROGNAME create -dest <string> -source <string> [OPTIONS]
 
 		name of the sync job, if not set it is default
 
+        -prepend-storage-id
+
+		If specified, prepend the storage ID to the destination's path(s).
+
         -skip
 
 		If specified, skip the first sync.
@@ -1184,6 +1216,10 @@ $PROGNAME sync -dest <string> -source <string> [OPTIONS]\n
 		name of the sync job, if not set it is default.
 		It is only necessary if scheduler allready contains this source.
 
+        -prepend-storage-id
+
+		If specified, prepend the storage ID to the destination's path(s).
+
         -source    string
 
 		the source can be an <VMID> or [IP:]<ZFSPool>[/Path]
-- 
2.20.1

[pve-devel] applied-series: [PATCH-SERIES zsync] fix #1669: allow prepending storage ID

[Thomas Lamprecht]

On 04.05.21 10:09, Fabian Ebner wrote:
> which makes it possible to use pve-zsync for guests with disks with the same
> name on different storages, i.e.
>     storageA:vm-123-disk-0
>     storageB:vm-123-disk-0
> 
> Currently they would collide to
>     target/vm-123-disk-0
> 
> When the new prepend-storage-id flag is specified, they will be synced to
>     target/storageA/vm-123-disk-0$
>     target/storageB/vm-123-disk-0$
> instead.
> 
> 
> The first four patches are cleanups/usage description fixes.
> 
> The next five patches are preparations for the last patch, which is the one
> actually implementing the new feature.
> 
> 
> Fabian Ebner (10):
>   whitespace fix
>   copyright: update year
>   usage: describe flag parameters correctly
>   usage: fix type for maxsnap
>   add target_dataset function
>   add check_dataset_exists function
>   add create_file_system function
>   parse disks: don't include colon in storage name variable
>   parse disks: also include storage ID information
>   fix #1669: add prepend-storage-id flag
> 
>  pve-zsync | 118 ++++++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 89 insertions(+), 29 deletions(-)
> 

applied series, thanks!