[pve-devel] [PATCH-SERIES zsync] fix #3351: allow keeping a different number of snapshots on source and destination

[pve-devel] [PATCH-SERIES zsync] fix #3351: allow keeping a different number of snapshots on source and destination

[Fabian Ebner]

by introducing a new dest-maxsnap parameter which can be used to override
maxsnap for the destination side.

This is useful for backups, as one can potentially save a lot of space on the
source side by keeping fewer snapshots around.


Fabian Ebner (6):
  param_to_job: handle --maxsnap 0 on creation
  usage: improve maxsnap description
  remove all old snapshots belonging to a job
  snapshot_get: make interface agnostic to source/dest
  snapshot_destroy: make interface agnostic to source/dest
  fix #3351: allow keeping a different number of snapshots on source and
    destination

 pve-zsync | 94 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 58 insertions(+), 36 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH zsync 1/6] param_to_job: handle --maxsnap 0 on creation

[Fabian Ebner]

format_job expects the value to be set, so creating a job with '--maxsnap 0' led
to '--maxsnap' being written without an argument, and thus a (for zsync)
unparsable cron file.

However, issuing sync jobs via CLI with '--maxsnap 0' works with 0 being treated
as unlimited. There is a default value, so no need to worry about definedness,
simply pass along the value.

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 82e85bc..088b7f2 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -335,7 +335,7 @@ sub param_to_job {
     $job->{method} = "local" if !$dest->{ip} && !$source->{ip};
     $job->{method} = "ssh" if !$job->{method};
     $job->{limit} = $param->{limit};
-    $job->{maxsnap} = $param->{maxsnap} if $param->{maxsnap};
+    $job->{maxsnap} = $param->{maxsnap};
     $job->{source} = $param->{source};
     $job->{source_user} = $param->{source_user};
     $job->{dest_user} = $param->{dest_user};
-- 
2.20.1

[pve-devel] [PATCH zsync 2/6] usage: improve maxsnap description

[Fabian Ebner]

and also mention that 0 means unlimited.

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

diff --git a/pve-zsync b/pve-zsync
index 088b7f2..6ca2da1 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -1155,7 +1155,8 @@ $PROGNAME create --dest <string> --source <string> [OPTIONS]
 		Maximal sync speed in kBytes/s, default is unlimited
 
 	--maxsnap   integer
-		How much snapshots will be kept before get erased, default 1
+		The number of snapshots to keep until older ones are erased.
+		The default is 1, use 0 for unlimited.
 
 	--name      string
 		The name of the sync job, if not set it is default
@@ -1194,7 +1195,8 @@ $PROGNAME sync --dest <string> --source <string> [OPTIONS]\n
 		The maximal sync speed in kBytes/s, default is unlimited
 
 	--maxsnap   integer
-		Configure how many snapshots will be kept before get erased, default 1
+		The number of snapshots to keep until older ones are erased.
+		The default is 1, use 0 for unlimited.
 
 	--name      string
 		The name of the sync job, if not set it is 'default'.
-- 
2.20.1

[pve-devel] [PATCH zsync 3/6] remove all old snapshots belonging to a job

[Fabian Ebner]

Changing maxsnap to something smaller can lead to left-over snaphsots otherwise,
as previously at most one snapshot would be removed, even if there are multiple
old snapshots according to the new setting.

Hopefully nobody relied on the fact that pve-zsync didn't clean up after itself
in such cases...

Negative values and 0 for 'maxsnap' should still be interpreted as infinity to
match the previous behavior.

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

diff --git a/pve-zsync b/pve-zsync
index 6ca2da1..ff05f46 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -685,8 +685,9 @@ sub sync {
 
 	    send_image($source, $dest, $param);
 
-	    snapshot_destroy($source, $dest, $param->{method}, $dest->{old_snap}, $param->{source_user}, $param->{dest_user}) if ($source->{destroy} && $dest->{old_snap});
-
+	    for my $old_snap (@{$dest->{old_snap}}) {
+		snapshot_destroy($source, $dest, $param->{method}, $old_snap, $param->{source_user}, $param->{dest_user});
+	    }
 	};
 
 	eval{
@@ -763,7 +764,7 @@ sub snapshot_get{
     my $index = 0;
     my $line = "";
     my $last_snap = undef;
-    my $old_snap;
+    my $old_snap = [];
 
     while ($raw && $raw =~ s/^(.*?)(\n|$)//) {
 	$line = $1;
@@ -771,12 +772,15 @@ sub snapshot_get{
 	    $last_snap = $1 if (!$last_snap);
 	}
 	if ($line =~ m/(rep_\Q${name}\E_\d{4}-\d{2}-\d{2}_\d{2}:\d{2}:\d{2})$/) {
-	    $old_snap = $1;
+	    # interpreted as infinity
+	    last if $max_snap <= 0;
+
+	    my $snap = $1;
 	    $index++;
-	    if ($index == $max_snap) {
-		$source->{destroy} = 1;
-		last;
-	    };
+
+	    if ($index >= $max_snap) {
+		push @{$old_snap}, $snap;
+	    }
 	}
     }
 
@@ -1067,8 +1071,8 @@ sub send_config{
 	    run_cmd(['scp', '--', "$source_user\@[$source->{ip}]:$source_target", $dest_target_new]);
 	}
 
-	if ($source->{destroy}){
-	    my $dest_target_old ="${config_dir}/$source->{vmid}.conf.$source->{vm_type}.$dest->{old_snap}";
+	for my $old_snap (@{$dest->{old_snap}}) {
+	    my $dest_target_old ="${config_dir}/$source->{vmid}.conf.$source->{vm_type}.${old_snap}";
 	    if($dest->{ip}){
 		run_cmd(['ssh', "$dest_user\@$dest->{ip}", '--', 'rm', '-f', '--', $dest_target_old]);
 	    } else {
-- 
2.20.1

[pve-devel] [PATCH zsync 4/6] snapshot_get: make interface agnostic to source/dest

[Fabian Ebner]

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

diff --git a/pve-zsync b/pve-zsync
index ff05f46..c162230 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -677,7 +677,15 @@ sub sync {
 	my $sync_path = sub {
 	    my ($source, $dest, $job, $param, $date) = @_;
 
-	    ($dest->{old_snap}, $dest->{last_snap}) = snapshot_get($source, $dest, $param->{maxsnap}, $param->{name}, $param->{dest_user});
+	    my $dest_dataset = target_dataset($source, $dest);
+
+	    ($dest->{old_snap}, $dest->{last_snap}) = snapshot_get(
+		$dest_dataset,
+		$param->{maxsnap},
+		$param->{name},
+		$dest->{ip},
+		$param->{dest_user},
+	    );
 
 	    prepare_prepended_target($source, $dest, $param->{dest_user}) if defined($dest->{prepend});
 
@@ -746,14 +754,12 @@ sub sync {
 }
 
 sub snapshot_get{
-    my ($source, $dest, $max_snap, $name, $dest_user) = @_;
+    my ($dataset, $max_snap, $name, $ip, $user) = @_;
 
     my $cmd = [];
-    push @$cmd, 'ssh', "$dest_user\@$dest->{ip}", '--', if $dest->{ip};
+    push @$cmd, 'ssh', "$user\@$ip", '--', if $ip;
     push @$cmd, 'zfs', 'list', '-r', '-t', 'snapshot', '-Ho', 'name', '-S', 'creation';
-
-    my $path = target_dataset($source, $dest);
-    push @$cmd, $path;
+    push @$cmd, $dataset;
 
     my $raw;
     eval {$raw = run_cmd($cmd)};
-- 
2.20.1

[pve-devel] [PATCH zsync 5/6] snapshot_destroy: make interface agnostic to source/dest

[Fabian Ebner]

Also drop the 'method' parameter which is not used consistently (e.g. ignored
in the later half of the very same function, in snapshot_exist, vm_exists,...),
and not documented. Simply rely on the presence of the IP address as is done in
many other places already.

In snapshot_add, there is no need to try and destroy the snapshot on the
destination (did happen previously, because $dest was passed along), because we
just failed to create the snapshot on the source side.

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

diff --git a/pve-zsync b/pve-zsync
index c162230..1213361 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -694,7 +694,8 @@ sub sync {
 	    send_image($source, $dest, $param);
 
 	    for my $old_snap (@{$dest->{old_snap}}) {
-		snapshot_destroy($source, $dest, $param->{method}, $old_snap, $param->{source_user}, $param->{dest_user});
+		snapshot_destroy($source->{all}, $old_snap, $source->{ip}, $param->{source_user});
+		snapshot_destroy($dest_dataset, $old_snap, $dest->{ip}, $param->{dest_user});
 	    }
 	};
 
@@ -812,7 +813,7 @@ sub snapshot_add {
     };
 
     if (my $err = $@) {
-	snapshot_destroy($source, $dest, 'ssh', $snap_name, $source_user, $dest_user);
+	snapshot_destroy($source->{all}, $snap_name, $source->{ip}, $source_user);
 	die "$err\n";
     }
 }
@@ -967,14 +968,14 @@ sub prepare_prepended_target {
 }
 
 sub snapshot_destroy {
-    my ($source, $dest, $method, $snap, $source_user, $dest_user) = @_;
+    my ($dataset, $snap, $ip, $user) = @_;
 
     my @zfscmd = ('zfs', 'destroy');
-    my $snapshot = "$source->{all}\@$snap";
+    my $snapshot = "$dataset\@$snap";
 
     eval {
-	if($source->{ip} && $method eq 'ssh'){
-	    run_cmd(['ssh', "$source_user\@$source->{ip}", '--', @zfscmd, $snapshot]);
+	if ($ip) {
+	    run_cmd(['ssh', "$user\@$ip", '--', @zfscmd, $snapshot]);
 	} else {
 	    run_cmd([@zfscmd, $snapshot]);
 	}
@@ -982,18 +983,6 @@ sub snapshot_destroy {
     if (my $erro = $@) {
 	warn "WARN: $erro";
     }
-    if ($dest) {
-	my @ssh = $dest->{ip} ? ('ssh', "$dest_user\@$dest->{ip}", '--') : ();
-
-	my $path = target_dataset($source, $dest);
-
-	eval {
-	    run_cmd([@ssh, @zfscmd, "$path\@$snap"]);
-	};
-	if (my $erro = $@) {
-	    warn "WARN: $erro";
-	}
-    }
 }
 
 # check if snapshot for incremental sync exist on source side
@@ -1048,7 +1037,7 @@ sub send_image {
     };
 
     if (my $erro = $@) {
-	snapshot_destroy($source, undef, $param->{method}, $source->{new_snap}, $param->{source_user}, $param->{dest_user});
+	snapshot_destroy($source->{all}, $source->{new_snap}, $source->{ip}, $param->{source_user});
 	die $erro;
     };
 }
-- 
2.20.1

[pve-devel] [PATCH zsync 6/6] fix #3351: allow keeping a different number of snapshots on source and destination

[Fabian Ebner]

by introducing a new dest-maxsnap parameter which can be used to override
maxsnap for the destination side.

This is useful for backups, as one can potentially save a lot of space on the
source side (or the destination side if one can come up with a use case for
that) by keeping fewer snapshots around.

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

diff --git a/pve-zsync b/pve-zsync
index 1213361..39ead0d 100755
--- a/pve-zsync
+++ b/pve-zsync
@@ -244,6 +244,7 @@ sub parse_argv {
 	verbose => undef,
 	limit => undef,
 	maxsnap => undef,
+	dest_maxsnap => undef,
 	name => undef,
 	skip => undef,
 	method => undef,
@@ -261,6 +262,7 @@ sub parse_argv {
 	'verbose' => \$param->{verbose},
 	'limit=i' => \$param->{limit},
 	'maxsnap=i' => \$param->{maxsnap},
+	'dest-maxsnap=i' => \$param->{dest_maxsnap},
 	'name=s' => \$param->{name},
 	'skip' => \$param->{skip},
 	'method=s' => \$param->{method},
@@ -336,6 +338,7 @@ sub param_to_job {
     $job->{method} = "ssh" if !$job->{method};
     $job->{limit} = $param->{limit};
     $job->{maxsnap} = $param->{maxsnap};
+    $job->{dest_maxsnap} = $param->{dest_maxsnap};
     $job->{source} = $param->{source};
     $job->{source_user} = $param->{source_user};
     $job->{dest_user} = $param->{dest_user};
@@ -460,6 +463,7 @@ sub format_job {
     $text .= " root";
     $text .= " $PROGNAME sync --source $job->{source} --dest $job->{dest}";
     $text .= " --name $job->{name} --maxsnap $job->{maxsnap}";
+    $text .= " --dest-maxsnap $job->{dest_maxsnap}" if defined($job->{dest_maxsnap});
     $text .= " --limit $job->{limit}" if $job->{limit};
     $text .= " --method $job->{method}";
     $text .= " --verbose" if $job->{verbose};
@@ -681,20 +685,31 @@ sub sync {
 
 	    ($dest->{old_snap}, $dest->{last_snap}) = snapshot_get(
 		$dest_dataset,
-		$param->{maxsnap},
+		$param->{dest_maxsnap} // $param->{maxsnap},
 		$param->{name},
 		$dest->{ip},
 		$param->{dest_user},
 	    );
 
+	    ($source->{old_snap}) = snapshot_get(
+		$source->{all},
+		$param->{maxsnap},
+		$param->{name},
+		$source->{ip},
+		$param->{source_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);
 
-	    for my $old_snap (@{$dest->{old_snap}}) {
+	    for my $old_snap (@{$source->{old_snap}}) {
 		snapshot_destroy($source->{all}, $old_snap, $source->{ip}, $param->{source_user});
+	    }
+
+	    for my $old_snap (@{$dest->{old_snap}}) {
 		snapshot_destroy($dest_dataset, $old_snap, $dest->{ip}, $param->{dest_user});
 	    }
 	};
@@ -1157,6 +1172,9 @@ $PROGNAME create --dest <string> --source <string> [OPTIONS]
 		The number of snapshots to keep until older ones are erased.
 		The default is 1, use 0 for unlimited.
 
+	--dest-maxsnap   integer
+		Override maxsnap for the destination dataset.
+
 	--name      string
 		The name of the sync job, if not set it is default
 
@@ -1197,6 +1215,9 @@ $PROGNAME sync --dest <string> --source <string> [OPTIONS]\n
 		The number of snapshots to keep until older ones are erased.
 		The default is 1, use 0 for unlimited.
 
+	--dest-maxsnap   integer
+		Override maxsnap for the destination dataset.
+
 	--name      string
 		The name of the sync job, if not set it is 'default'.
 		It is only necessary if scheduler allready contains this source.
-- 
2.20.1

Re: [pve-devel] [PATCH zsync 6/6] fix #3351: allow keeping a different number of snapshots on source and destination

[Bruce Wainer]

Hello Fabian,
Since this is a series of patches, could you provide the full pve-zsync
file with all the patches? It would be easier for me to test it this way.
Thank you,
Bruce

On Tue, May 11, 2021 at 9:00 AM Fabian Ebner <f.ebner@proxmox.com> wrote:

> by introducing a new dest-maxsnap parameter which can be used to override
> maxsnap for the destination side.
>
> This is useful for backups, as one can potentially save a lot of space on
> the
> source side (or the destination side if one can come up with a use case for
> that) by keeping fewer snapshots around.
>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  pve-zsync | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
>
> diff --git a/pve-zsync b/pve-zsync
> index 1213361..39ead0d 100755
> --- a/pve-zsync
> +++ b/pve-zsync
> @@ -244,6 +244,7 @@ sub parse_argv {
>         verbose => undef,
>         limit => undef,
>         maxsnap => undef,
> +       dest_maxsnap => undef,
>         name => undef,
>         skip => undef,
>         method => undef,
> @@ -261,6 +262,7 @@ sub parse_argv {
>         'verbose' => \$param->{verbose},
>         'limit=i' => \$param->{limit},
>         'maxsnap=i' => \$param->{maxsnap},
> +       'dest-maxsnap=i' => \$param->{dest_maxsnap},
>         'name=s' => \$param->{name},
>         'skip' => \$param->{skip},
>         'method=s' => \$param->{method},
> @@ -336,6 +338,7 @@ sub param_to_job {
>      $job->{method} = "ssh" if !$job->{method};
>      $job->{limit} = $param->{limit};
>      $job->{maxsnap} = $param->{maxsnap};
> +    $job->{dest_maxsnap} = $param->{dest_maxsnap};
>      $job->{source} = $param->{source};
>      $job->{source_user} = $param->{source_user};
>      $job->{dest_user} = $param->{dest_user};
> @@ -460,6 +463,7 @@ sub format_job {
>      $text .= " root";
>      $text .= " $PROGNAME sync --source $job->{source} --dest
> $job->{dest}";
>      $text .= " --name $job->{name} --maxsnap $job->{maxsnap}";
> +    $text .= " --dest-maxsnap $job->{dest_maxsnap}" if
> defined($job->{dest_maxsnap});
>      $text .= " --limit $job->{limit}" if $job->{limit};
>      $text .= " --method $job->{method}";
>      $text .= " --verbose" if $job->{verbose};
> @@ -681,20 +685,31 @@ sub sync {
>
>             ($dest->{old_snap}, $dest->{last_snap}) = snapshot_get(
>                 $dest_dataset,
> -               $param->{maxsnap},
> +               $param->{dest_maxsnap} // $param->{maxsnap},
>                 $param->{name},
>                 $dest->{ip},
>                 $param->{dest_user},
>             );
>
> +           ($source->{old_snap}) = snapshot_get(
> +               $source->{all},
> +               $param->{maxsnap},
> +               $param->{name},
> +               $source->{ip},
> +               $param->{source_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);
>
> -           for my $old_snap (@{$dest->{old_snap}}) {
> +           for my $old_snap (@{$source->{old_snap}}) {
>                 snapshot_destroy($source->{all}, $old_snap, $source->{ip},
> $param->{source_user});
> +           }
> +
> +           for my $old_snap (@{$dest->{old_snap}}) {
>                 snapshot_destroy($dest_dataset, $old_snap, $dest->{ip},
> $param->{dest_user});
>             }
>         };
> @@ -1157,6 +1172,9 @@ $PROGNAME create --dest <string> --source <string>
> [OPTIONS]
>                 The number of snapshots to keep until older ones are
> erased.
>                 The default is 1, use 0 for unlimited.
>
> +       --dest-maxsnap   integer
> +               Override maxsnap for the destination dataset.
> +
>         --name      string
>                 The name of the sync job, if not set it is default
>
> @@ -1197,6 +1215,9 @@ $PROGNAME sync --dest <string> --source <string>
> [OPTIONS]\n
>                 The number of snapshots to keep until older ones are
> erased.
>                 The default is 1, use 0 for unlimited.
>
> +       --dest-maxsnap   integer
> +               Override maxsnap for the destination dataset.
> +
>         --name      string
>                 The name of the sync job, if not set it is 'default'.
>                 It is only necessary if scheduler allready contains this
> source.
> --
> 2.20.1
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>

Re: [pve-devel] [PATCH zsync 6/6] fix #3351: allow keeping a different number of snapshots on source and destination

[Thomas Lamprecht]

On 24.05.21 18:00, Bruce Wainer wrote:
> Hello Fabian,
> Since this is a series of patches, could you provide the full pve-zsync
> file with all the patches? It would be easier for me to test it this way.

FYI: If you have git ready (`apt install git` else) it wouldn't be to hard to
apply multiple patches from a mail:

1. git clone git://git.proxmox.com/git/pve-zsync.git

2. save all patch-mails from 1/6 to 6/6 in a folder, e.g. /tmp/patches

3. git am /tmp/patches/*

[pve-devel] applied-series: [PATCH-SERIES zsync] fix #3351: allow keeping a different number of snapshots on source and destination

[Thomas Lamprecht]

On 11.05.21 14:59, Fabian Ebner wrote:
> by introducing a new dest-maxsnap parameter which can be used to override
> maxsnap for the destination side.
> 
> This is useful for backups, as one can potentially save a lot of space on the
> source side by keeping fewer snapshots around.
> 
> 
> Fabian Ebner (6):
>   param_to_job: handle --maxsnap 0 on creation
>   usage: improve maxsnap description
>   remove all old snapshots belonging to a job
>   snapshot_get: make interface agnostic to source/dest
>   snapshot_destroy: make interface agnostic to source/dest
>   fix #3351: allow keeping a different number of snapshots on source and
>     destination
> 
>  pve-zsync | 94 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 58 insertions(+), 36 deletions(-)
> 


applied series, thanks!