Re: [pve-devel] [PATCH storage 6/6] Add API and pvesm calls for prune_backups

[Fabian Ebner <f.ebner@proxmox.com>]

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
>