From: Fabian Ebner <f.ebner@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH v5 manager 1/2] Allow prune-backups as an alternative to maxfiles
Date: Tue, 29 Sep 2020 10:37:04 +0200 [thread overview]
Message-ID: <20200929083705.20124-2-f.ebner@proxmox.com> (raw)
In-Reply-To: <20200929083705.20124-1-f.ebner@proxmox.com>
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 <f.ebner@proxmox.com>
---
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
next prev parent reply other threads:[~2020-09-29 8:37 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-29 8:37 [pve-devel] [PATCH-SERIES v5] fix #2649: introduce prune-backups property for storages supporting backups Fabian Ebner
2020-09-29 8:37 ` Fabian Ebner [this message]
2020-09-29 8:37 ` [pve-devel] [PATCH v5 manager 2/2] Always use prune-backups instead of maxfiles internally Fabian Ebner
2020-10-01 14:36 ` [pve-devel] applied: [PATCH-SERIES v5] fix #2649: introduce prune-backups property for storages supporting backups Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200929083705.20124-2-f.ebner@proxmox.com \
--to=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal