[pve-devel] [PATCH v2 manager 1/3] task index: allow selection of task source(s)

[Thomas Lamprecht]

On 1/9/19 2:04 PM, Fabian Grünbichler wrote:
> otherwise there is no way to find out about (all) active tasks over the
> API if their UPIDs were not recorded when the initial API calls happened.
> 
> Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> ---
> changed from v1:
> - moved from bool to tri-state as suggested by W.Bumiller
> - previously called 'task index: optionally include active tasks'

When starting a backup task and doing:

# pvenode task list --source active

I get:
> Use of uninitialized value in string ne at /usr/share/perl5/PVE/CLI/pvenode.pm line 168.

We access $task->{status} in the CLI output parser, and this is not defined there
yet for (still) running task. Besides the uninitialized value warning this then shows
even "ERROR" in the Status column for all running tasks. You may want to adapt the
output formatter to print RUNNING if no status is defined in this patch? And then
maybe it's warranted to split the refactor $filter_task out and introduce "source"
parameter into two patches out, but no strong feelings here..

> 
> this is a nice addition for external monitoring/dashboard/UI
> applications, e.g. see #1997
> 
>  PVE/API2/Tasks.pm | 68 ++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 49 insertions(+), 19 deletions(-)
> 
> diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
> index a5acb118..da601ed9 100644
> --- a/PVE/API2/Tasks.pm
> +++ b/PVE/API2/Tasks.pm
> @@ -58,6 +58,13 @@ __PACKAGE__->register_method({
>  		default => 0,
>  		optional => 1,
>  	    },
> +	    source => {
> +		type => 'string',
> +		enum => ['archive', 'active', 'all'],
> +		default => 'archive',
> +		optional => 1,
> +		description => 'List archived, active or all tasks.',
> +	    },
>  	},
>      },
>      returns => {
> @@ -94,48 +101,71 @@ __PACKAGE__->register_method({
>  	my $limit = $param->{limit} // 50;
>  	my $userfilter = $param->{userfilter};
>  	my $errors = $param->{errors} // 0;
> +	my $source = $param->{source} // 'archive';
>  
>  	my $count = 0;
>  	my $line;
>  
>  	my $auditor = $rpcenv->check($user, "/nodes/$node", [ 'Sys.Audit' ], 1);
>  
> +	my $filter_task = sub {
> +	    my $task = shift;
> +
> +	    return 1 if $userfilter && $task->{user} !~ m/\Q$userfilter\E/i;
> +	    return 1 if !($auditor || $user eq $task->{user});
> +
> +	    return 1 if $errors && $task->{status} && $task->{status} eq 'OK';
> +	    return 1 if $param->{vmid} && (!$task->{id} || $task->{id} ne $param->{vmid});
> +
> +	    return 1 if $count++ < $start;
> +	    return 1 if $limit <= 0;
> +
> +	    return 0;
> +	};
> +
>  	my $parse_line = sub {
>  	    if ($line =~ m/^(\S+)(\s([0-9A-Za-z]{8})(\s(\S.*))?)?$/) {
>  		my $upid = $1;
>  		my $endtime = $3;
>  		my $status = $5;
>  		if ((my $task = PVE::Tools::upid_decode($upid, 1))) {
> -		    return if $userfilter && $task->{user} !~ m/\Q$userfilter\E/i;
> -		    return if !($auditor || $user eq $task->{user});
> -
> -		    return if $errors && $status && $status eq 'OK';
> -
> -		    return if $param->{vmid} && (!$task->{id} || $task->{id} ne $param->{vmid});
> -
> -		    return if $count++ < $start;
> -		    return if $limit <= 0;
>  
>  		    $task->{upid} = $upid;
>  		    $task->{endtime} = hex($endtime) if $endtime;
>  		    $task->{status} = $status if $status;
> -		    push @$res, $task;
> -		    $limit--;
> +
> +		    if (!$filter_task->($task)) {
> +			push @$res, $task;
> +			$limit--;
> +		    }
>  		}
>  	    }
>  	};
>  
> -	if (my $bw = File::ReadBackwards->new($filename)) {
> -	    while (defined ($line = $bw->readline)) {
> -		&$parse_line();
> +	if ($source eq 'active' || $source eq 'all') {
> +	    my $recent_tasks = PVE::INotify::read_file('active');
> +	    for my $task (@$recent_tasks) {
> +		next if delete $task->{saved}; # archived task, already in index(.1)
> +		if (!$filter_task->($task)) {
> +		    push @$res, $task;
> +		    $limit--;
> +		}
>  	    }
> -	    $bw->close();
>  	}
> -	if (my $bw = File::ReadBackwards->new("$filename.1")) {
> -	    while (defined ($line = $bw->readline)) {
> -		&$parse_line();
> +
> +	if ($source ne 'active') {

nit: I'd maybe use the same logic in both ifs, i.e., also here:

if ($source eq 'archive' || $source eq 'all') {


> +	    if (my $bw = File::ReadBackwards->new($filename)) {
> +		while (defined ($line = $bw->readline)) {
> +		    &$parse_line();
> +		}
> +		$bw->close();
> +	    }
> +	    if (my $bw = File::ReadBackwards->new("$filename.1")) {
> +		while (defined ($line = $bw->readline)) {
> +		    &$parse_line();
> +		}
> +		$bw->close();
>  	    }
> -	    $bw->close();
>  	}
>  
>  	$rpcenv->set_result_attrib('total', $count);
>