From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Fabian Ebner <f.ebner@proxmox.com>, pve-devel@lists.proxmox.com
Cc: Thomas Lamprecht <t.lamprecht@proxmox.com>
Subject: Re: [pve-devel] [PATCH storage 6/6] Add API and pvesm calls for prune_backups
Date: Wed, 08 Jul 2020 10:17:08 +0200 [thread overview]
Message-ID: <1594195681.jjvsw8qb4o.astroid@nora.none> (raw)
In-Reply-To: <ae57fdf5-c43a-f828-0d47-8e7aceb5ea62@proxmox.com>
On July 8, 2020 9:48 am, Fabian Ebner wrote:
> 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.
well, there could be backups for a container with ID 100 that no longer
exists, and a VM with ID 100 (that might or might not still exist).
since those are pruned/grouped separately, we might offer this as filter
here as well. also, I might only want to prune all VM backups right now ;)
>
>>> + 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?
yes, for PBS it's not really possible since the actual removal is
delegated to GC anyway.
>
>>> + };
>>> +
>>> + 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.
we could have
pvesm prune-backups <storage> [filters]
=> prune without asking
pvesm prune-backups <storage> [filters] --dry-run
=> show what would be pruned
pvesm prune-backups <storage> [filters] --interactive
=> show, ask, (potentially) prune
I am not a big fan of --yes, and --force implies other things to me
(like attempting to prune as much as possible even in the face of
errors).
maybe '--confirm' would also be an option instead of '--interactive'?
we could also make it magic based on whether we can prompt on STDIN,
than script users can just </dev/null
>
>>
>> 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
>>
>
prev parent reply other threads:[~2020-07-08 8:26 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20200604090838.24203-1-f.ebner@proxmox.com>
[not found] ` <20200604090838.24203-5-f.ebner@proxmox.com>
[not found] ` <1594035985.yxpjph40wm.astroid@nora.none>
2020-07-08 7:20 ` [pve-devel] [PATCH storage 4/6] Add prune_backups to storage API Fabian Ebner
2020-07-08 7:26 ` Fabian Ebner
[not found] ` <20200604090838.24203-7-f.ebner@proxmox.com>
[not found] ` <1594040467.ikb3n0sela.astroid@nora.none>
2020-07-08 7:48 ` [pve-devel] [PATCH storage 6/6] Add API and pvesm calls for prune_backups Fabian Ebner
2020-07-08 8:17 ` Fabian Grünbichler [this message]
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=1594195681.jjvsw8qb4o.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=f.ebner@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
--cc=t.lamprecht@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.