[pve-devel] [PATCH pve-manager 1/2] Add/fix option descriptions in pvenode api docs

[pve-devel] [PATCH pve-manager 1/2] Add/fix option descriptions in pvenode api docs

[Dylan Whyte]

Adds some missing descriptions to the api/man page documentation, for
certain options of the `pvenode` command. Some minor language fix-ups
are also included

Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
---
 PVE/API2/Tasks.pm | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
index 056b698b..9cd1e56b 100644
--- a/PVE/API2/Tasks.pm
+++ b/PVE/API2/Tasks.pm
@@ -82,6 +82,7 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		default => 0,
 		optional => 1,
+		description => 'Only list tasks with a status of ERROR.',
 	    },
 	    source => {
 		type => 'string',
@@ -282,7 +283,7 @@ __PACKAGE__->register_method({
     method => 'DELETE',
     description => 'Stop a task.',
     permissions => {
-	description => "The user needs 'Sys.Modify' permissions on '/nodes/<node>' if the task does not belong to him.",
+	description => "The user needs 'Sys.Modify' permissions on '/nodes/<node>' if they aren't the owner of the task.",
 	user => 'all',
     },
     protected => 1,
@@ -322,7 +323,7 @@ __PACKAGE__->register_method({
     path => '{upid}/log',
     method => 'GET',
     permissions => {
-	description => "The user needs 'Sys.Audit' permissions on '/nodes/<node>' if the task does not belong to him.",
+	description => "The user needs 'Sys.Audit' permissions on '/nodes/<node>' if they aren't the owner of the task.",
 	user => 'all',
     },
     protected => 1,
@@ -332,18 +333,23 @@ __PACKAGE__->register_method({
     	additionalProperties => 0,
 	properties => {
 	    node => get_standard_option('pve-node'),
-	    upid => { type => 'string' },
+	    upid => {
+		type => 'string',
+		description => "The task's unique ID.",
+	    },
 	    start => {
 		type => 'integer',
 		minimum => 0,
 		default => 0,
 		optional => 1,
+		description => "The line number to start printing at.",
 	    },
 	    limit => {
 		type => 'integer',
 		minimum => 0,
 		default => 50,
 		optional => 1,
+		description => "The maximum amount of lines that should be printed.",
 	    },
 	},
     },
@@ -396,7 +402,7 @@ __PACKAGE__->register_method({
     path => '{upid}/status',
     method => 'GET',
     permissions => {
-	description => "The user needs 'Sys.Audit' permissions on '/nodes/<node>' if the task does not belong to him.",
+	description => "The user needs 'Sys.Audit' permissions on '/nodes/<node>' if they are not the owner of the task.",
 	user => 'all',
     },
     protected => 1,
@@ -406,7 +412,10 @@ __PACKAGE__->register_method({
     	additionalProperties => 0,
 	properties => {
 	    node => get_standard_option('pve-node'),
-	    upid => { type => 'string' },
+	    upid => {
+		type => 'string',
+		description => "The task's unique ID.",
+	    },
 	},
     },
     returns => {
-- 
2.30.2

[pve-devel] [PATCH pve-manager 2/2] pvenode task log: don't hardcode 'limit' in CLI

[Dylan Whyte]

Removes the hardcoded '--limit' option when setting up the
'pvenode task log' command. This allows a user to control this option
themselves.

Also changes the default limit from 50 to 500, as this is what we use in
the GUI, and should be plenty for the majority of tasks.

Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
---
 PVE/API2/Tasks.pm  | 4 ++--
 PVE/CLI/pvenode.pm | 3 +--
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
index 9cd1e56b..ce87cdac 100644
--- a/PVE/API2/Tasks.pm
+++ b/PVE/API2/Tasks.pm
@@ -347,7 +347,7 @@ __PACKAGE__->register_method({
 	    limit => {
 		type => 'integer',
 		minimum => 0,
-		default => 50,
+		default => 500,
 		optional => 1,
 		description => "The maximum amount of lines that should be printed.",
 	    },
@@ -379,7 +379,7 @@ __PACKAGE__->register_method({
 	my $user = $rpcenv->get_user();
 	my $node = $param->{node};
 	my $start = $param->{start} // 0;
-	my $limit = $param->{limit} // 50;
+	my $limit = $param->{limit} // 500;
 
 	$convert_token_task->($task);
 
diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
index acef6c3b..b45735e0 100644
--- a/PVE/CLI/pvenode.pm
+++ b/PVE/CLI/pvenode.pm
@@ -194,8 +194,7 @@ our $cmddef = {
 	    my ($data, $schema, $options) = @_;
 	    PVE::CLIFormatter::print_api_result($data, $schema, undef, $options);
 	}, $PVE::RESTHandler::standard_output_options],
-	# set limit to 1000000, so we see the whole log, not only the first 50 lines by default
-	log => [ 'PVE::API2::Tasks', 'read_task_log', [ 'upid' ], { node => $nodename, limit => 1000000 }, sub {
+	log => [ 'PVE::API2::Tasks', 'read_task_log', [ 'upid' ], { node => $nodename }, sub {
 	    my ($data, $resultprops) = @_;
 	    foreach my $line (@$data) {
 		print $line->{t} . "\n";
-- 
2.30.2

[pve-devel] applied: [PATCH pve-manager 1/2] Add/fix option descriptions in pvenode api docs

[Thomas Lamprecht]

On 31.08.21 16:42, Dylan Whyte wrote:
> Adds some missing descriptions to the api/man page documentation, for
> certain options of the `pvenode` command. Some minor language fix-ups
> are also included
> 
> Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
> ---
>  PVE/API2/Tasks.pm | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH pve-manager 2/2] pvenode task log: don't hardcode 'limit' in CLI

[Thomas Lamprecht]

On 31.08.21 16:42, Dylan Whyte wrote:
> Removes the hardcoded '--limit' option when setting up the
> 'pvenode task log' command. This allows a user to control this option
> themselves.
> 
> Also changes the default limit from 50 to 500, as this is what we use in
> the GUI, and should be plenty for the majority of tasks.

but certainly not all, and certainly a behavior change for something that is
possibly used in scripts.

> 
> Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
> ---
>  PVE/API2/Tasks.pm  | 4 ++--
>  PVE/CLI/pvenode.pm | 3 +--
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
> index 9cd1e56b..ce87cdac 100644
> --- a/PVE/API2/Tasks.pm
> +++ b/PVE/API2/Tasks.pm
> @@ -347,7 +347,7 @@ __PACKAGE__->register_method({
>  	    limit => {
>  		type => 'integer',
>  		minimum => 0,
> -		default => 50,
> +		default => 500,
>  		optional => 1,
>  		description => "The maximum amount of lines that should be printed.",
>  	    },
> @@ -379,7 +379,7 @@ __PACKAGE__->register_method({
>  	my $user = $rpcenv->get_user();
>  	my $node = $param->{node};
>  	my $start = $param->{start} // 0;
> -	my $limit = $param->{limit} // 50;
> +	my $limit = $param->{limit} // 500;

FWIW, we could also differ between the case for when the RPC env is of type CLI and
when not, falling back to different defaults then.

That'd keep the default of 50 lines (as that change could be counted as API change,
and while I'm not rejecting it I'd like at least another opinion on that) and also
the current 100k for the CLI one.

I.e., something like:

my $limit = $param->{limit} // ($rpcenv->{type} eq 'cli' ? 100000 : 50);


One could mayve document that in the param's description.

>  
>  	$convert_token_task->($task);
>  
> diff --git a/PVE/CLI/pvenode.pm b/PVE/CLI/pvenode.pm
> index acef6c3b..b45735e0 100644
> --- a/PVE/CLI/pvenode.pm
> +++ b/PVE/CLI/pvenode.pm
> @@ -194,8 +194,7 @@ our $cmddef = {
>  	    my ($data, $schema, $options) = @_;
>  	    PVE::CLIFormatter::print_api_result($data, $schema, undef, $options);
>  	}, $PVE::RESTHandler::standard_output_options],
> -	# set limit to 1000000, so we see the whole log, not only the first 50 lines by default
> -	log => [ 'PVE::API2::Tasks', 'read_task_log', [ 'upid' ], { node => $nodename, limit => 1000000 }, sub {
> +	log => [ 'PVE::API2::Tasks', 'read_task_log', [ 'upid' ], { node => $nodename }, sub {
>  	    my ($data, $resultprops) = @_;
>  	    foreach my $line (@$data) {
>  		print $line->{t} . "\n";
>