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 F0B205BC2C for ; Wed, 8 Jul 2020 10:26:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E0FE32E33D for ; Wed, 8 Jul 2020 10:26:27 +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 238D72E331 for ; Wed, 8 Jul 2020 10:26:25 +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 E6F9440769 for ; Wed, 8 Jul 2020 10:17:19 +0200 (CEST) Date: Wed, 08 Jul 2020 10:17:08 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Fabian Ebner , pve-devel@lists.proxmox.com Cc: Thomas Lamprecht References: <20200604090838.24203-1-f.ebner@proxmox.com> <20200604090838.24203-7-f.ebner@proxmox.com> <1594040467.ikb3n0sela.astroid@nora.none> In-Reply-To: MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1594195681.jjvsw8qb4o.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL -0.400 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. [config.pm, status.pm, prunebackups.pm, pvesm.pm, proxmox.com, pbsplugin.pm, plugin.pm, content.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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jul 2020 08:26:28 -0000 On July 8, 2020 9:48 am, Fabian Ebner wrote: > Am 07.07.20 um 08:46 schrieb Fabian Gr=C3=BCnbichler: >> s/VM/guest in most descriptions - this is not in qemu-server ;) >>=20 >> On June 4, 2020 11:08 am, Fabian Ebner wrote: >>> Signed-off-by: Fabian Ebner >>> --- >>> >>> 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. >>=20 >> see comments below >>=20 >>> >>> 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 @@ >>> =20 >>> -SOURCES=3D Content.pm Status.pm Config.pm >>> +SOURCES=3D Content.pm Status.pm Config.pm PruneBackups.pm >>> =20 >>> .PHONY: install >>> install: >>> diff --git a/PVE/API2/Storage/PruneBackups.pm b/PVE/API2/Storage/PruneB= ackups.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 =3D> 'index', >>=20 >> IMHO this is not an index, but just a regular 'GET' API path. >>=20 >=20 > Ok >=20 >>> + path =3D> '', >>> + method =3D> 'GET', >>> + description =3D> "Get prune information for backups.", >>> + permissions =3D> { >>> + check =3D> ['perm', '/storage/{storage}', ['Datastore.Audit', 'Datast= ore.AllocateSpace'], any =3D> 1], >>> + }, >>> + protected =3D> 1, >>> + proxyto =3D> 'node', >>> + parameters =3D> { >>> + additionalProperties =3D> 0, >>> + properties =3D> { >>> + node =3D> get_standard_option('pve-node'), >>> + storage =3D> get_standard_option('pve-storage-id', { >>> + completion =3D> \&PVE::Storage::complete_storage_enabled, >>> + }), >>> + 'prune-backups' =3D> get_standard_option('prune-backups', { >>> + description =3D> "Use these retention options instead of those from = the storage configuration.", >>> + optional =3D> 1, >>> + }), >>> + vmid =3D> get_standard_option('pve-vmid', { >>> + description =3D> "Only consider backups for this VM.", >>=20 >> of this guest >>=20 >> would it make sense to allow specification of guest-type as well, so >> that it's possible to indirectly specify the 'backup-group' ? >>=20 >=20 > We don't really support having backups of two kinds of guest with the=20 > same ID currently, do we? If this is really needed at some point, we can=20 > still extend this, but for now it should be enough if the such backups=20 > get grouped correctly as to not interfere with one another. well, there could be backups for a container with ID 100 that no longer=20 exists, and a VM with ID 100 (that might or might not still exist).=20 since those are pruned/grouped separately, we might offer this as filter=20 here as well. also, I might only want to prune all VM backups right now ;) >=20 >>> + optional =3D> 1, >>> + completion =3D> \&PVE::Cluster::complete_vmid, >>> + }), >>> + }, >>> + }, >>> + returns =3D> { >>> + type =3D> 'array', >>> + items =3D> { >>> + type =3D> 'object', >>> + properties =3D> { >>> + volid =3D> { >>> + description =3D> "Backup volume ID.", >>> + type =3D> 'string', >>> + }, >>> + type =3D> { >>> + description =3D> "One of 'qemu', 'lxc', 'openvz' or 'unknown'.", >>> + type =3D> 'string', >>> + }, >>> + 'ctime' =3D> { >>> + description =3D> "Creation time of the backup (seconds since the= UNIX epoch).", >>> + type =3D> 'integer', >>> + }, >>> + 'mark' =3D> { >>> + description =3D> "Whether the backup would be kept or removed. F= or backups that don't " . >>> + "use the standard naming scheme, it's 'protected'.", >>> + type =3D> 'string', >>> + }, >>> + 'vmid' =3D> { >>> + description =3D> "The VM the backup belongs to.", >>> + type =3D> 'integer', >>> + optional =3D> 1, >>> + }, >>> + }, >>> + }, >>> + }, >>> + code =3D> sub { >>> + my ($param) =3D @_; >>> + >>> + my $cfg =3D PVE::Storage::config(); >>> + >>> + my $vmid =3D extract_param($param, 'vmid'); >>> + my $storeid =3D extract_param($param, 'storage'); >>> + my $prune_options =3D extract_param($param, 'prune-backups'); >>> + >>> + my $opts_override; >>> + if (defined($prune_options)) { >>> + $opts_override =3D PVE::JSONSchema::parse_property_string('prune-= backups', $prune_options); >>> + } >>> + >>> + my @res =3D PVE::Storage::prune_backups($cfg, $storeid, $opts_overrid= e, $vmid, 1); >>> + return \@res; >>=20 >> one more reason to make the return value a reference ;) >>=20 >>> + }}); >>> + >>> +__PACKAGE__->register_method ({ >>> + name =3D> 'delete', >>> + path =3D> '', >>> + method =3D> 'DELETE', >>> + description =3D> "Prune backups. Only those using the standard nam= ing scheme are considered.", >>> + permissions =3D> { >>> + description =3D> "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 =3D> 'all', >>> + }, >>> + protected =3D> 1, >>> + proxyto =3D> 'node', >>> + parameters =3D> { >>> + additionalProperties =3D> 0, >>> + properties =3D> { >>> + node =3D> get_standard_option('pve-node'), >>> + storage =3D> get_standard_option('pve-storage-id', { >>> + completion =3D> \&PVE::Storage::complete_storage, >>> + }), >>> + 'prune-backups' =3D> get_standard_option('prune-backups', { >>> + description =3D> "Use these retention options instead of those from = the storage configuration.", >>> + }), >>> + vmid =3D> get_standard_option('pve-vmid', { >>> + description =3D> "Only prune backups for this VM.", >>> + completion =3D> \&PVE::Cluster::complete_vmid, >>> + optional =3D> 1, >>> + }), >>> + }, >>> + }, >>> + returns =3D> { type =3D> 'string' }, >>> + code =3D> sub { >>> + my ($param) =3D @_; >>> + >>> + my $rpcenv =3D PVE::RPCEnvironment::get(); >>> + my $authuser =3D $rpcenv->get_user(); >>> + >>> + my $cfg =3D PVE::Storage::config(); >>> + >>> + my $vmid =3D extract_param($param, 'vmid'); >>> + my $storeid =3D extract_param($param, 'storage'); >>> + my $prune_options =3D extract_param($param, 'prune-backups'); >>> + >>> + my $opts_override; >>> + if (defined($prune_options)) { >>> + $opts_override =3D PVE::JSONSchema::parse_property_string('prune-= backups', $prune_options); >>> + } >>> + >>> + if (defined($vmid)) { >>> + $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Alloca= teSpace']); >>> + $rpcenv->check($authuser, "/vms/$vmid", ['VM.Backup']); >>> + } else { >>> + $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Alloca= te']); >>> + } >>> + >>> + my $id =3D (defined($vmid) ? "$vmid@" : '') . $storeid; >>> + my $worker =3D sub { >>> + PVE::Storage::prune_backups($cfg, $storeid, $opts_override, $vmid= , 0); >>=20 >> 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? >>=20 >=20 > For Plugin.pm one can print something while iterating, but for=20 > PBSPlugin.pm that's not really possible is it? So maybe just output=20 > something like "executing proxmox-backup-client prune ..." there? yes, for PBS it's not really possible since the actual removal is=20 delegated to GC anyway. >=20 >>> + }; >>> + >>> + 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); >>> =20 >>> use base qw(PVE::RESTHandler); >>> =20 >>> +__PACKAGE__->register_method ({ >>> + subclass =3D> "PVE::API2::Storage::PruneBackups", >>> + path =3D> '{storage}/prunebackups', >>> +}); >>> + >>> __PACKAGE__->register_method ({ >>> subclass =3D> "PVE::API2::Storage::Content", >>> # set fragment delimiter (no subdirs) - we need that, because vol= ume >>> @@ -214,6 +220,7 @@ __PACKAGE__->register_method ({ >>> { subdir =3D> 'upload' }, >>> { subdir =3D> 'rrd' }, >>> { subdir =3D> 'rrddata' }, >>> + { subdir =3D> 'prunebackups' }, >>> ]; >>> =20 >>> 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 =3D { >>> print "APIVER $res->{apiver}\n"; >>> print "APIAGE $res->{apiage}\n"; >>> }], >>> + 'prune-backups-list' =3D> [ "PVE::API2::Storage::PruneBackups", 'i= ndex', ['storage'], { node =3D> $nodename }, sub { >>=20 >> 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.. >>=20 >=20 > Ok >=20 >>> + my $res =3D shift; >>> + >>> + my @sorted =3D sort { >>> + my $vmcmp =3D PVE::Tools::safe_compare($a->{vmid}, $b->{vmid}, su= b { $_[0] <=3D> $_[1] }); >>> + return $vmcmp if $vmcmp ne 0; >>> + return $a->{ctime} <=3D> $b->{ctime}; >>=20 >> should sort by type first in case IDs are re-used between containers and >> VMs >>=20 >=20 > Ok >=20 >>> + } @{$res}; >>> + >>> + my $maxlen =3D 0; >>> + foreach my $backup (@sorted) { >>> + my $volid =3D $backup->{volid}; >>> + $maxlen =3D length($volid) if length($volid) > $maxlen; >>> + } >>> + $maxlen+=3D1; >>> + >>> + printf("%-${maxlen}s %15s %10s\n", 'Backup', 'Backup-ID', 'Prune-Mark= '); >>> + foreach my $backup (@sorted) { >>> + my $type =3D $backup->{type}; >>> + my $vmid =3D $backup->{vmid}; >>> + my $backup_id =3D defined($vmid) ? "$type/$vmid" : "$type"; >>> + printf("%-${maxlen}s %15s %10s\n", $backup->{volid}, $backup_id, = $backup->{mark}); >>> + } >>> + }], >>> + 'prune-backups' =3D> [ "PVE::API2::Storage::PruneBackups", 'delete= ', ['storage'], { node =3D> $nodename }, ], >>> }; >>=20 >> for the CLI it would probably be nice to have an interactive mode? >>=20 >> pvesm prune-backups [vmid] [--interactive] >> ... >> list of backups and their prune status >>=20 >> do you want to prune these backups? [y|N] >> ... >>=20 >=20 > Should this be the default behavior for the CLI command? Then we can=20 > have something like --yes or --force to skip the question. we could have pvesm prune-backups [filters] =3D> prune without asking pvesm prune-backups [filters] --dry-run =3D> show what would be pruned pvesm prune-backups [filters] --interactive =3D> show, ask, (potentially) prune I am not a big fan of --yes, and --force implies other things to me=20 (like attempting to prune as much as possible even in the face of=20 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,=20 than script users can just =20 >>=20 >> 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) >>=20 >> 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.. >>=20 >>> =20 >>> 1; >>> --=20 >>> 2.20.1 >>> >>> >>> _______________________________________________ >>> pve-devel mailing list >>> pve-devel@pve.proxmox.com >>> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>> >>> >>=20 >> _______________________________________________ >> pve-devel mailing list >> pve-devel@pve.proxmox.com >> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >>=20 >=20 =