From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 7D9206212C for ; Tue, 29 Sep 2020 10:37:19 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7B960D5CA for ; Tue, 29 Sep 2020 10:37:19 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 3844FD5C1 for ; Tue, 29 Sep 2020 10:37:18 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 04448440FC for ; Tue, 29 Sep 2020 10:37:18 +0200 (CEST) From: Fabian Ebner To: pve-devel@lists.proxmox.com Date: Tue, 29 Sep 2020 10:37:04 +0200 Message-Id: <20200929083705.20124-2-f.ebner@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200929083705.20124-1-f.ebner@proxmox.com> References: <20200929083705.20124-1-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.050 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [vzdump.pm] Subject: [pve-devel] [PATCH v5 manager 1/2] Allow prune-backups as an alternative to maxfiles X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 29 Sep 2020 08:37:19 -0000 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 --- Changes from v4: * add newline to 'cannot have ... at the same time' error message * fix typo and correctly assign to $opts->{'prune-backups'} instead of $opts->{'prune_backups'}. Because of this, the mapping of maxfiles to keep-last had no effect in v4 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 6e0d3dbf..1fe4c4ee 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.20.1