From e6946086e3588b9c33bf7a0c572ae8f5d7b4e101 Mon Sep 17 00:00:00 2001 From: Fabian Ebner Date: Tue, 29 Sep 2020 10:37:04 +0200 Subject: [PATCH] Allow prune-backups as an alternative to maxfiles and make the two options mutally exclusive as long as they are specified on the same level (e.g. both from the storage configuration). Otherwise prefer option > storage config > default (only maxfiles has a default currently). Defines the backup limit for prune-backups as the sum of all keep-values. There is no perfect way to determine whether a new backup would trigger a removal with prune later: 1. we would need a way to include the not yet existing backup in a 'prune --dry-run' check. 2. even if we had that check, if it's executed right before a full hour, and the actual backup happens after the full hour, the information from the check is not correct. So in some cases, we allow backup jobs with remove=0, that will lead to a removal when the next prune is executed. Still, the job with remove=0 does not execute a prune, so: 1. There is a well-defined limit. 2. A job with remove=0 never removes an old backup. Signed-off-by: Fabian Ebner --- PVE/API2/VZDump.pm | 4 +-- PVE/VZDump.pm | 88 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 64 insertions(+), 28 deletions(-) diff --git a/PVE/API2/VZDump.pm b/PVE/API2/VZDump.pm index 2eda973e..19fa1e3b 100644 --- a/PVE/API2/VZDump.pm +++ b/PVE/API2/VZDump.pm @@ -25,7 +25,7 @@ __PACKAGE__->register_method ({ method => 'POST', description => "Create backup.", permissions => { - description => "The user needs 'VM.Backup' permissions on any VM, and 'Datastore.AllocateSpace' on the backup storage. The 'maxfiles', 'tmpdir', 'dumpdir', 'script', 'bwlimit' and 'ionice' parameters are restricted to the 'root\@pam' user.", + description => "The user needs 'VM.Backup' permissions on any VM, and 'Datastore.AllocateSpace' on the backup storage. The 'maxfiles', 'prune-backups', 'tmpdir', 'dumpdir', 'script', 'bwlimit' and 'ionice' parameters are restricted to the 'root\@pam' user.", user => 'all', }, protected => 1, @@ -58,7 +58,7 @@ __PACKAGE__->register_method ({ if $param->{stdout}; } - foreach my $key (qw(maxfiles tmpdir dumpdir script bwlimit ionice)) { + foreach my $key (qw(maxfiles prune-backups tmpdir dumpdir script bwlimit ionice)) { raise_param_exc({ $key => "Only root may set this option."}) if defined($param->{$key}) && ($user ne 'root@pam'); } diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm index aea7389b..ecf0b7de 100644 --- a/PVE/VZDump.pm +++ b/PVE/VZDump.pm @@ -89,6 +89,12 @@ sub storage_info { maxfiles => $scfg->{maxfiles}, }; + $info->{'prune-backups'} = PVE::JSONSchema::parse_property_string('prune-backups', $scfg->{'prune-backups'}) + if defined($scfg->{'prune-backups'}); + + die "cannot have 'maxfiles' and 'prune-backups' configured at the same time\n" + if defined($info->{'prune-backups'}) && defined($info->{maxfiles}); + if ($type eq 'pbs') { $info->{pbs} = 1; } else { @@ -459,12 +465,18 @@ sub new { if ($opts->{storage}) { my $info = eval { storage_info ($opts->{storage}) }; - $errors .= "could not get storage information for '$opts->{storage}': $@" - if ($@); - $opts->{dumpdir} = $info->{dumpdir}; - $opts->{scfg} = $info->{scfg}; - $opts->{pbs} = $info->{pbs}; - $opts->{maxfiles} //= $info->{maxfiles}; + if (my $err = $@) { + $errors .= "could not get storage information for '$opts->{storage}': $err"; + } else { + $opts->{dumpdir} = $info->{dumpdir}; + $opts->{scfg} = $info->{scfg}; + $opts->{pbs} = $info->{pbs}; + + if (!defined($opts->{'prune-backups'}) && !defined($opts->{maxfiles})) { + $opts->{'prune-backups'} = $info->{'prune-backups'}; + $opts->{maxfiles} = $info->{maxfiles}; + } + } } elsif ($opts->{dumpdir}) { $errors .= "dumpdir '$opts->{dumpdir}' does not exist" if ! -d $opts->{dumpdir}; @@ -472,7 +484,9 @@ sub new { die "internal error"; } - $opts->{maxfiles} //= $defaults->{maxfiles}; + if (!defined($opts->{'prune-backups'}) && !defined($opts->{maxfiles})) { + $opts->{maxfiles} = $defaults->{maxfiles}; + } if ($opts->{tmpdir} && ! -d $opts->{tmpdir}) { $errors .= "\n" if $errors; @@ -653,6 +667,7 @@ sub exec_backup_task { my $opts = $self->{opts}; + my $cfg = PVE::Storage::config(); my $vmid = $task->{vmid}; my $plugin = $task->{plugin}; my $vmtype = $plugin->type(); @@ -706,8 +721,18 @@ sub exec_backup_task { my $basename = $bkname . strftime("-%Y_%m_%d-%H_%M_%S", localtime($task->{backup_time})); my $maxfiles = $opts->{maxfiles}; + my $prune_options = $opts->{'prune-backups'}; + + my $backup_limit = 0; + if (defined($maxfiles)) { + $backup_limit = $maxfiles; + } elsif (defined($prune_options)) { + foreach my $keep (values %{$prune_options}) { + $backup_limit += $keep; + } + } - if ($maxfiles && !$opts->{remove}) { + if ($backup_limit && !$opts->{remove}) { my $count; if ($self->{opts}->{pbs}) { my $res = PVE::Storage::PBSPlugin::run_client_cmd($opts->{scfg}, $opts->{storage}, 'snapshots', $pbs_group_name); @@ -716,10 +741,10 @@ sub exec_backup_task { my $bklist = get_backup_file_list($opts->{dumpdir}, $bkname); $count = scalar(@$bklist); } - die "There is a max backup limit of ($maxfiles) enforced by the". + die "There is a max backup limit of $backup_limit enforced by the". " target storage or the vzdump parameters.". " Either increase the limit or delete old backup(s).\n" - if $count >= $maxfiles; + if $count >= $backup_limit; } if (!$self->{opts}->{pbs}) { @@ -926,23 +951,28 @@ sub exec_backup_task { } # purge older backup - if ($maxfiles && $opts->{remove}) { - - if ($self->{opts}->{pbs}) { - my $args = [$pbs_group_name, '--quiet', '1', '--keep-last', $maxfiles]; - my $logfunc = sub { my $line = shift; debugmsg ('info', $line, $logfd); }; - PVE::Storage::PBSPlugin::run_raw_client_cmd( - $opts->{scfg}, $opts->{storage}, 'prune', $args, logfunc => $logfunc); - } else { - my $bklist = get_backup_file_list($opts->{dumpdir}, $bkname, $task->{target}); - $bklist = [ sort { $b->{ctime} <=> $a->{ctime} } @$bklist ]; - - while (scalar (@$bklist) >= $maxfiles) { - my $d = pop @$bklist; - my $archive_path = $d->{path}; - debugmsg ('info', "delete old backup '$archive_path'", $logfd); - PVE::Storage::archive_remove($archive_path); + if ($opts->{remove}) { + if ($maxfiles) { + + if ($self->{opts}->{pbs}) { + my $args = [$pbs_group_name, '--quiet', '1', '--keep-last', $maxfiles]; + my $logfunc = sub { my $line = shift; debugmsg ('info', $line, $logfd); }; + PVE::Storage::PBSPlugin::run_raw_client_cmd( + $opts->{scfg}, $opts->{storage}, 'prune', $args, logfunc => $logfunc); + } else { + my $bklist = get_backup_file_list($opts->{dumpdir}, $bkname, $task->{target}); + $bklist = [ sort { $b->{ctime} <=> $a->{ctime} } @$bklist ]; + + while (scalar (@$bklist) >= $maxfiles) { + my $d = pop @$bklist; + my $archive_path = $d->{path}; + debugmsg ('info', "delete old backup '$archive_path'", $logfd); + PVE::Storage::archive_remove($archive_path); + } } + } elsif (defined($prune_options)) { + my $logfunc = sub { debugmsg($_[0], $_[1], $logfd) }; + PVE::Storage::prune_backups($cfg, $opts->{storage}, $prune_options, $vmid, $vmtype, 0, $logfunc); } } @@ -1129,6 +1159,12 @@ sub verify_vzdump_parameters { raise_param_exc({ pool => "option conflicts with option 'vmid'"}) if $param->{pool} && $param->{vmid}; + raise_param_exc({ 'prune-backups' => "option conflicts with option 'maxfiles'"}) + if defined($param->{'prune-backups'}) && defined($param->{maxfiles}); + + $param->{'prune-backups'} = PVE::JSONSchema::parse_property_string('prune-backups', $param->{'prune-backups'}) + if defined($param->{'prune-backups'}); + $param->{all} = 1 if (defined($param->{exclude}) && !$param->{pool}); warn "option 'size' is deprecated and will be removed in a future " . -- 2.39.2