From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.ebner@proxmox.com>
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 E96F35BBD7
for <pve-devel@lists.proxmox.com>; Wed, 8 Jul 2020 09:49:19 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id DC1A12DE68
for <pve-devel@lists.proxmox.com>; Wed, 8 Jul 2020 09:48:49 +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 D56C62DE5E
for <pve-devel@lists.proxmox.com>; Wed, 8 Jul 2020 09:48:47 +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 90F4A4231E
for <pve-devel@lists.proxmox.com>; Wed, 8 Jul 2020 09:48:47 +0200 (CEST)
To: pve-devel@lists.proxmox.com
References: <20200604090838.24203-1-f.ebner@proxmox.com>
<20200604090838.24203-7-f.ebner@proxmox.com>
<1594040467.ikb3n0sela.astroid@nora.none>
From: Fabian Ebner <f.ebner@proxmox.com>
Cc: =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= <f.gruenbichler@proxmox.com>,
Thomas Lamprecht <t.lamprecht@proxmox.com>
Message-ID: <ae57fdf5-c43a-f828-0d47-8e7aceb5ea62@proxmox.com>
Date: Wed, 8 Jul 2020 09:48:45 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <1594040467.ikb3n0sela.astroid@nora.none>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.001 Adjusted score from AWL reputation of From: address
KAM_ASCII_DIVIDERS 0.8 Spam that uses ascii formatting tricks
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. [status.pm, config.pm, content.pm, plugin.pm, pbsplugin.pm,
proxmox.com, pvesm.pm, prunebackups.pm]
Subject: Re: [pve-devel] [PATCH storage 6/6] Add API and pvesm calls for
prune_backups
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 08 Jul 2020 07:49:20 -0000
Am 07.07.20 um 08:46 schrieb Fabian Grünbichler:
> s/VM/guest in most descriptions - this is not in qemu-server ;)
>
> On June 4, 2020 11:08 am, Fabian Ebner wrote:
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Not sure if this is the best place for the new API endpoints.
>>
>> I decided to opt for two distinct calls rather than just using a
>> --dry-run option and use a worker for actually pruning, because
>> removing many backups over the network might take a while.
>
> see comments below
>
>>
>> PVE/API2/Storage/Makefile | 2 +-
>> PVE/API2/Storage/PruneBackups.pm | 153 +++++++++++++++++++++++++++++++
>> PVE/API2/Storage/Status.pm | 7 ++
>> PVE/CLI/pvesm.pm | 27 ++++++
>> 4 files changed, 188 insertions(+), 1 deletion(-)
>> create mode 100644 PVE/API2/Storage/PruneBackups.pm
>>
>> diff --git a/PVE/API2/Storage/Makefile b/PVE/API2/Storage/Makefile
>> index a33525b..3f667e8 100644
>> --- a/PVE/API2/Storage/Makefile
>> +++ b/PVE/API2/Storage/Makefile
>> @@ -1,5 +1,5 @@
>>
>> -SOURCES= Content.pm Status.pm Config.pm
>> +SOURCES= Content.pm Status.pm Config.pm PruneBackups.pm
>>
>> .PHONY: install
>> install:
>> diff --git a/PVE/API2/Storage/PruneBackups.pm b/PVE/API2/Storage/PruneBackups.pm
>> new file mode 100644
>> index 0000000..7968730
>> --- /dev/null
>> +++ b/PVE/API2/Storage/PruneBackups.pm
>> @@ -0,0 +1,153 @@
>> +package PVE::API2::Storage::PruneBackups;
>> +
>> +use strict;
>> +use warnings;
>> +
>> +use PVE::Cluster;
>> +use PVE::JSONSchema qw(get_standard_option);
>> +use PVE::RESTHandler;
>> +use PVE::RPCEnvironment;
>> +use PVE::Storage;
>> +use PVE::Tools qw(extract_param);
>> +
>> +use base qw(PVE::RESTHandler);
>> +
>> +__PACKAGE__->register_method ({
>> + name => 'index',
>
> IMHO this is not an index, but just a regular 'GET' API path.
>
Ok
>> + path => '',
>> + method => 'GET',
>> + description => "Get prune information for backups.",
>> + permissions => {
>> + check => ['perm', '/storage/{storage}', ['Datastore.Audit', 'Datastore.AllocateSpace'], any => 1],
>> + },
>> + protected => 1,
>> + proxyto => 'node',
>> + parameters => {
>> + additionalProperties => 0,
>> + properties => {
>> + node => get_standard_option('pve-node'),
>> + storage => get_standard_option('pve-storage-id', {
>> + completion => \&PVE::Storage::complete_storage_enabled,
>> + }),
>> + 'prune-backups' => get_standard_option('prune-backups', {
>> + description => "Use these retention options instead of those from the storage configuration.",
>> + optional => 1,
>> + }),
>> + vmid => get_standard_option('pve-vmid', {
>> + description => "Only consider backups for this VM.",
>
> of this guest
>
> would it make sense to allow specification of guest-type as well, so
> that it's possible to indirectly specify the 'backup-group' ?
>
We don't really support having backups of two kinds of guest with the
same ID currently, do we? If this is really needed at some point, we can
still extend this, but for now it should be enough if the such backups
get grouped correctly as to not interfere with one another.
>> + optional => 1,
>> + completion => \&PVE::Cluster::complete_vmid,
>> + }),
>> + },
>> + },
>> + returns => {
>> + type => 'array',
>> + items => {
>> + type => 'object',
>> + properties => {
>> + volid => {
>> + description => "Backup volume ID.",
>> + type => 'string',
>> + },
>> + type => {
>> + description => "One of 'qemu', 'lxc', 'openvz' or 'unknown'.",
>> + type => 'string',
>> + },
>> + 'ctime' => {
>> + description => "Creation time of the backup (seconds since the UNIX epoch).",
>> + type => 'integer',
>> + },
>> + 'mark' => {
>> + description => "Whether the backup would be kept or removed. For backups that don't " .
>> + "use the standard naming scheme, it's 'protected'.",
>> + type => 'string',
>> + },
>> + 'vmid' => {
>> + description => "The VM the backup belongs to.",
>> + type => 'integer',
>> + optional => 1,
>> + },
>> + },
>> + },
>> + },
>> + code => sub {
>> + my ($param) = @_;
>> +
>> + my $cfg = PVE::Storage::config();
>> +
>> + my $vmid = extract_param($param, 'vmid');
>> + my $storeid = extract_param($param, 'storage');
>> + my $prune_options = extract_param($param, 'prune-backups');
>> +
>> + my $opts_override;
>> + if (defined($prune_options)) {
>> + $opts_override = PVE::JSONSchema::parse_property_string('prune-backups', $prune_options);
>> + }
>> +
>> + my @res = PVE::Storage::prune_backups($cfg, $storeid, $opts_override, $vmid, 1);
>> + return \@res;
>
> one more reason to make the return value a reference ;)
>
>> + }});
>> +
>> +__PACKAGE__->register_method ({
>> + name => 'delete',
>> + path => '',
>> + method => 'DELETE',
>> + description => "Prune backups. Only those using the standard naming scheme are considered.",
>> + permissions => {
>> + description => "You need the 'Datastore.Allocate' privilege on the storage " .
>> + "(or if a VM ID is specified, 'Datastore.AllocateSpace' and 'VM.Backup' for the VM).",
>> + user => 'all',
>> + },
>> + protected => 1,
>> + proxyto => 'node',
>> + parameters => {
>> + additionalProperties => 0,
>> + properties => {
>> + node => get_standard_option('pve-node'),
>> + storage => get_standard_option('pve-storage-id', {
>> + completion => \&PVE::Storage::complete_storage,
>> + }),
>> + 'prune-backups' => get_standard_option('prune-backups', {
>> + description => "Use these retention options instead of those from the storage configuration.",
>> + }),
>> + vmid => get_standard_option('pve-vmid', {
>> + description => "Only prune backups for this VM.",
>> + completion => \&PVE::Cluster::complete_vmid,
>> + optional => 1,
>> + }),
>> + },
>> + },
>> + returns => { type => 'string' },
>> + code => sub {
>> + my ($param) = @_;
>> +
>> + my $rpcenv = PVE::RPCEnvironment::get();
>> + my $authuser = $rpcenv->get_user();
>> +
>> + my $cfg = PVE::Storage::config();
>> +
>> + my $vmid = extract_param($param, 'vmid');
>> + my $storeid = extract_param($param, 'storage');
>> + my $prune_options = extract_param($param, 'prune-backups');
>> +
>> + my $opts_override;
>> + if (defined($prune_options)) {
>> + $opts_override = PVE::JSONSchema::parse_property_string('prune-backups', $prune_options);
>> + }
>> +
>> + if (defined($vmid)) {
>> + $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.AllocateSpace']);
>> + $rpcenv->check($authuser, "/vms/$vmid", ['VM.Backup']);
>> + } else {
>> + $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Allocate']);
>> + }
>> +
>> + my $id = (defined($vmid) ? "$vmid@" : '') . $storeid;
>> + my $worker = sub {
>> + PVE::Storage::prune_backups($cfg, $storeid, $opts_override, $vmid, 0);
>
> it would be nice to print progress in $PLUGIN::prune_backups (e.g. when
> iterating over the entries before deletion, otherwise neither this API
> call nor the CLI wrapper give any indication which backup archives got
> pruned?
>
For Plugin.pm one can print something while iterating, but for
PBSPlugin.pm that's not really possible is it? So maybe just output
something like "executing proxmox-backup-client prune ..." there?
>> + };
>> +
>> + return $rpcenv->fork_worker('prunebackups', $id, $authuser, $worker);
>> + }});
>> +
>> +1;
>> diff --git a/PVE/API2/Storage/Status.pm b/PVE/API2/Storage/Status.pm
>> index d9d9b36..d12643f 100644
>> --- a/PVE/API2/Storage/Status.pm
>> +++ b/PVE/API2/Storage/Status.pm
>> @@ -11,6 +11,7 @@ use PVE::Cluster;
>> use PVE::RRD;
>> use PVE::Storage;
>> use PVE::API2::Storage::Content;
>> +use PVE::API2::Storage::PruneBackups;
>> use PVE::RESTHandler;
>> use PVE::RPCEnvironment;
>> use PVE::JSONSchema qw(get_standard_option);
>> @@ -18,6 +19,11 @@ use PVE::Exception qw(raise_param_exc);
>>
>> use base qw(PVE::RESTHandler);
>>
>> +__PACKAGE__->register_method ({
>> + subclass => "PVE::API2::Storage::PruneBackups",
>> + path => '{storage}/prunebackups',
>> +});
>> +
>> __PACKAGE__->register_method ({
>> subclass => "PVE::API2::Storage::Content",
>> # set fragment delimiter (no subdirs) - we need that, because volume
>> @@ -214,6 +220,7 @@ __PACKAGE__->register_method ({
>> { subdir => 'upload' },
>> { subdir => 'rrd' },
>> { subdir => 'rrddata' },
>> + { subdir => 'prunebackups' },
>> ];
>>
>> return $res;
>> diff --git a/PVE/CLI/pvesm.pm b/PVE/CLI/pvesm.pm
>> index 30bdcf6..478842f 100755
>> --- a/PVE/CLI/pvesm.pm
>> +++ b/PVE/CLI/pvesm.pm
>> @@ -12,8 +12,10 @@ use PVE::Cluster;
>> use PVE::INotify;
>> use PVE::RPCEnvironment;
>> use PVE::Storage;
>> +use PVE::Tools;
>> use PVE::API2::Storage::Config;
>> use PVE::API2::Storage::Content;
>> +use PVE::API2::Storage::PruneBackups;
>> use PVE::API2::Storage::Status;
>> use PVE::JSONSchema qw(get_standard_option);
>> use PVE::PTY;
>> @@ -818,6 +820,31 @@ our $cmddef = {
>> print "APIVER $res->{apiver}\n";
>> print "APIAGE $res->{apiage}\n";
>> }],
>> + 'prune-backups-list' => [ "PVE::API2::Storage::PruneBackups", 'index', ['storage'], { node => $nodename }, sub {
>
> I know this is easier since it's a one-to-one mapping to the API
> endpoints, but IMHO this would really make more sense to have as an
> option to 'pvesm prune-backups' from a usability POV..
>
Ok
>> + my $res = shift;
>> +
>> + my @sorted = sort {
>> + my $vmcmp = PVE::Tools::safe_compare($a->{vmid}, $b->{vmid}, sub { $_[0] <=> $_[1] });
>> + return $vmcmp if $vmcmp ne 0;
>> + return $a->{ctime} <=> $b->{ctime};
>
> should sort by type first in case IDs are re-used between containers and
> VMs
>
Ok
>> + } @{$res};
>> +
>> + my $maxlen = 0;
>> + foreach my $backup (@sorted) {
>> + my $volid = $backup->{volid};
>> + $maxlen = length($volid) if length($volid) > $maxlen;
>> + }
>> + $maxlen+=1;
>> +
>> + printf("%-${maxlen}s %15s %10s\n", 'Backup', 'Backup-ID', 'Prune-Mark');
>> + foreach my $backup (@sorted) {
>> + my $type = $backup->{type};
>> + my $vmid = $backup->{vmid};
>> + my $backup_id = defined($vmid) ? "$type/$vmid" : "$type";
>> + printf("%-${maxlen}s %15s %10s\n", $backup->{volid}, $backup_id, $backup->{mark});
>> + }
>> + }],
>> + 'prune-backups' => [ "PVE::API2::Storage::PruneBackups", 'delete', ['storage'], { node => $nodename }, ],
>> };
>
> for the CLI it would probably be nice to have an interactive mode?
>
> pvesm prune-backups <storage> [vmid] [--interactive]
> ...
> list of backups and their prune status
>
> do you want to prune these backups? [y|N]
> ...
>
Should this be the default behavior for the CLI command? Then we can
have something like --yes or --force to skip the question.
>
> in general it would be nice also for API users if we had a way to
> - get list of to-be-pruned backups (client <-> API)
> - ask user for confirmation (client <-> user)
> - prune exactly those confirmed archives (client <-> API)
>
> otherwise the dry-run mode is a bit dangerous as it offers a false sense
> of security. the last step maybe even only if all the ones marked as
> keep still exist? I don't know, just some food for thought..
>
>>
>> 1;
>> --
>> 2.20.1
>>
>>
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel@pve.proxmox.com
>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>
>>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>