[pve-devel] [PATCH storage v2] Fix #582: Make removing backups a background task

[Thomas Lamprecht]

On 5/10/19 12:42 PM, Dominic Jäger wrote:
> Previously, the web gui timed out when removing a
> backup took long. Doing the main part of the API
> DELETE call in a fork_worker solves this.
> 

FYI, this API entry point is for _all_ content removal, not just backups...
I.e., ISOs, disks, templates, backups, snippets..

> This relies on the patch that makes returning undef possible for API calls.
> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
>  PVE/API2/Storage/Content.pm | 44 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
> index cd4746b..3b62c60 100644
> --- a/PVE/API2/Storage/Content.pm
> +++ b/PVE/API2/Storage/Content.pm
> @@ -285,9 +285,16 @@ __PACKAGE__->register_method ({
>  		type => 'string',
>  		completion => \&PVE::Storage::complete_volume,
>  	    },
> +		background_delay => {
> +		    type => 'integer',
> +		    description => "Time to wait for the task to finish. We return 'null' if the task finish within that time.",
> +		    minimum => 1,
> +		    maximum => 30,
> +		    optional => 1,
> +		},

indentation error above

>  	},
>      },
> -    returns => { type => 'null' },
> +    returns => { type => 'string', optional => 1, },
>      code => sub {
>  	my ($param) = @_;
>  
> @@ -297,6 +304,7 @@ __PACKAGE__->register_method ({
>  	my $cfg = PVE::Storage::config();
>  
>  	my ($volid, $storeid) = &$real_volume_id($param->{storage}, $param->{volume});
> +	my $background_delay = $param->{background_delay};

declaration should be in the vicinity of use, if possible

>  
>  	my ($path, $ownervm, $vtype) = PVE::Storage::path($cfg, $volid);
>  	if ($vtype eq 'backup' && $ownervm) {
> @@ -306,16 +314,32 @@ __PACKAGE__->register_method ({
>  	    $rpcenv->check($authuser, "/storage/$storeid", ['Datastore.Allocate']);
>  	}
>  
> -	PVE::Storage::vdisk_free ($cfg, $volid);
> -
> -	if ($vtype eq 'backup'
> -	    && $path =~ /(.*\/vzdump-\w+-\d+-\d{4}_\d{2}_\d{2}-\d{2}_\d{2}_\d{2})[^\/]+$/) {
> -	    my $logpath = "$1.log";
> -	    # try to cleanup our backup log file too, if still exisiting, #318
> -	    unlink($logpath) if -e $logpath;
> +	my $worker = sub {
> +	    PVE::Storage::vdisk_free ($cfg, $volid);
> +	    if ($vtype eq 'backup'
> +		&& $path =~ /(.*\/vzdump-\w+-\d+-\d{4}_\d{2}_\d{2}-\d{2}_\d{2}_\d{2})[^\/]+$/) {
> +		my $logpath = "$1.log";
> +		# try to cleanup our backup log file too, if still exisiting, #318
> +		unlink($logpath) if -e $logpath;
> +	    }
> +	};
> +	my $upid = $rpcenv->fork_worker('imgdel', $ownervm, $authuser, $worker);

$ownervm can be undefined, e.g., when removing a ISO file...

I mean, it would be nice to be able to use the $volid as task, but even if
we just replace the ':' (which collides with task UUIDs separators) in the
volid we have still a few assumptions we can break with certain volids...

So, maybe use $storeid and add $ownervm if defined?
I mean, IMO, the $volid would be really best, as then one could have:
"Erase 'tom-nasi:iso/alpine-standard-3.5.0_rc5-x86.iso'" int the task log
description, which tells a user much more than just "Erase data"..

Also please add a 
> print "Removed volume '$volid'\n";
line after the free to actually reference what and that we removed something
in the task log..


Also the web gui part where the background_delay is passed misses still.
While we'd have this logic available in our proxmox edit window class the
parts where it needs to be used are not users of that class, so we may want
to add some functionallity to std remove button, possibly..

> +	if ($background_delay) {
> +	    my $end_time = time() + $background_delay;
> +	    my $task = PVE::Tools::upid_decode($upid);
> +	    my $currently_deleting; # not necessarily true, e.g. sequential api call from cli
> +	    do {
> +		$currently_deleting = PVE::ProcFSTools::check_process_running($task->{pid}, $task->{pstart});
> +		sleep 1 if $currently_deleting;
> +	    } while (time() < $end_time && $currently_deleting);
> +
> +	    if (!$currently_deleting) {
> +		my $status = PVE::Tools::upid_read_status($upid);
> +		return undef if $status eq 'OK';
> +		die $status;
> +	    }
>  	}
> -
> -	return undef;
> +	return $upid;
>      }});
>  
>  __PACKAGE__->register_method ({
>