[pve-devel] [PATCH common/storage/proxmxo-backup v3] improve file-restore timeout behaviour

[pve-devel] [PATCH common/storage/proxmxo-backup v3] improve file-restore timeout behaviour

[Dominik Csapak]

this series improves the behaviour of the file-restore when some mount
operations take longer than the 30 second pveproxy timeout

some of the previous version was already applied (pbs + gui) so only
the pve part is still missing

i changed the way we return the error a bit from my previous (and
already applied) attempt, namely i couple how we return the error
on the 'output-format' instead of a seperate 'json-error'

while this is technically a change in behaviour, this is imho ok because:
* setting 'output-format' to json(-pretty) did not format the error,
  so a tool would have to cope with the pure text
* AFAIK we don't have the same guarantees for the cli as for the api
* it's a much better interface than before

but since this changed, and the 'json-error' was already applied,
the dependencies go as follows:

pve-storage depends on the new proxmox-backup-file-restore/pve-common
to work properly, but it's not broken with the current versions
proxmox-backup breaks the current pve-storage (so that we must bump and
add a 'breaks <<' probably)

the gui should already handle the new code, so that is fine either way

changes from v2:
* couple the error format to 'ouput-format' instead of 'json-error'
* remove 'error' property from 'error json object' (was redundant)
* always expect an error when we get an object and always treat
  it as ok when we get a list

changes from v1:
* rebased on master
* moved the json-error and timeout directly into pve-common (hardcoded)
  since there is only one usage of that function


pve-common:

Dominik Csapak (1):
  PBSClient: file_restore_list: add timeout parameter

 src/PVE/PBSClient.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

pve-storage:

Dominik Csapak (1):
  api: FileRestore: decode and return proper error with new file-restore
    params

 PVE/API2/Storage/FileRestore.pm | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

proxmox-backup:

Dominik Csapak (1):
  file-restore: remove 'json-error' parameter  from list_files

 proxmox-file-restore/src/main.rs | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH common v3 1/1] PBSClient: file_restore_list: add timeout parameter

[Dominik Csapak]

we always want the restore_list to use a timeout here. Set it to 25 seconds
so there is a little headroom between this and pveproxys 30s one.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/PBSClient.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
index 37385d7..7eaace3 100644
--- a/src/PVE/PBSClient.pm
+++ b/src/PVE/PBSClient.pm
@@ -378,7 +378,7 @@ sub file_restore_list {
     return run_client_cmd(
 	$self,
 	"list",
-	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ],
+	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0, '--timeout', 25],
 	0,
 	"proxmox-file-restore",
 	$namespace,
-- 
2.30.2

Re: [pve-devel] [PATCH common v3 1/1] PBSClient: file_restore_list: add timeout parameter

[Thomas Lamprecht]

subject is not wrong but worded rather confusingly, as of now it rather
implies that this adds a new parameter allowing callers to control the
timeout, but actually it sets the timeout hard-coded to 25s.

Am 27/05/2022 um 10:22 schrieb Dominik Csapak:
> we always want the restore_list to use a timeout here. Set it to 25 seconds

Such statements could be a bit more useful with some actual reasoning
(e.g., short sentence about ill effects of lacking timeout)

> so there is a little headroom between this and pveproxys 30s one.

what if we'd add a call site outside the sync API response context
(e.g., task worker or CLI rpcenv)? could be an artificial limitation
in that case.

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/PBSClient.pm | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
> index 37385d7..7eaace3 100644
> --- a/src/PVE/PBSClient.pm
> +++ b/src/PVE/PBSClient.pm
> @@ -378,7 +378,7 @@ sub file_restore_list {
>      return run_client_cmd(
>  	$self,
>  	"list",
> -	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ],
> +	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0, '--timeout', 25],
>  	0,
>  	"proxmox-file-restore",
>  	$namespace,

Re: [pve-devel] [PATCH common v3 1/1] PBSClient: file_restore_list: add timeout parameter

[Dominik Csapak]

On 11/7/22 15:17, Thomas Lamprecht wrote:
> subject is not wrong but worded rather confusingly, as of now it rather
> implies that this adds a new parameter allowing callers to control the
> timeout, but actually it sets the timeout hard-coded to 25s.
> 
> Am 27/05/2022 um 10:22 schrieb Dominik Csapak:
>> we always want the restore_list to use a timeout here. Set it to 25 seconds
> 
> Such statements could be a bit more useful with some actual reasoning
> (e.g., short sentence about ill effects of lacking timeout)

ok i thought the sentence below would be enough reasoning

> 
>> so there is a little headroom between this and pveproxys 30s one.
> 
> what if we'd add a call site outside the sync API response context
> (e.g., task worker or CLI rpcenv)? could be an artificial limitation
> in that case.

i followed your suggestion from the v1 version by hardcoding the options
and you applied the pbs ones from v2 partially without
complaining about this ;)

in any case, since i have to touch this again when doing the
user dependent memory increase for the file restore,
i'd rather use the other approach wolfang mentioned
by having a %param hash with the 'timeout' (and
dynamic memory) option.

i'd send these two things together in one (pve) series.
is that ok for you?

> 
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/PVE/PBSClient.pm | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/PVE/PBSClient.pm b/src/PVE/PBSClient.pm
>> index 37385d7..7eaace3 100644
>> --- a/src/PVE/PBSClient.pm
>> +++ b/src/PVE/PBSClient.pm
>> @@ -378,7 +378,7 @@ sub file_restore_list {
>>       return run_client_cmd(
>>   	$self,
>>   	"list",
>> -	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0 ],
>> +	[ $snapshot, $filepath, "--base64", $base64 ? 1 : 0, '--timeout', 25],
>>   	0,
>>   	"proxmox-file-restore",
>>   	$namespace,
> 

Re: [pve-devel] [PATCH common v3 1/1] PBSClient: file_restore_list: add timeout parameter

[Thomas Lamprecht]

Am 08/11/2022 um 12:20 schrieb Dominik Csapak:
> On 11/7/22 15:17, Thomas Lamprecht wrote:
>> subject is not wrong but worded rather confusingly, as of now it rather
>> implies that this adds a new parameter allowing callers to control the
>> timeout, but actually it sets the timeout hard-coded to 25s.
>>
>> Am 27/05/2022 um 10:22 schrieb Dominik Csapak:
>>> we always want the restore_list to use a timeout here. Set it to 25 seconds
>>
>> Such statements could be a bit more useful with some actual reasoning
>> (e.g., short sentence about ill effects of lacking timeout)
> 
> ok i thought the sentence below would be enough reasoning
> 

not really as it doesn't explains much for **why** headroom would be
required, if the clients gets the response cut short anyway at 30s
what benefits do we gain here, just another error message or otherwise
improved behavior? Why not just alarm($foo) on the call site.

main point is, it really doesn't hurts to have that relevant information
here too, not just in the pbs side of the commit..

>>
>>> so there is a little headroom between this and pveproxys 30s one.
>>
>> what if we'd add a call site outside the sync API response context
>> (e.g., task worker or CLI rpcenv)? could be an artificial limitation
>> in that case.
> 
> i followed your suggestion from the v1 version by hardcoding the options
> and you applied the pbs ones from v2 partially without
> complaining about this ;)

You mean
https://lists.proxmox.com/pipermail/pve-devel/2022-February/051664.html
? That was a comment about a horrid general "pass anything" interface
Wolfgang agreed too, not specific to the timeout param and it's implications,
that's why I asked here about if you thought about that (which you did not
replied at all...)

> 
> in any case, since i have to touch this again when doing the
> user dependent memory increase for the file restore,
> i'd rather use the other approach wolfang mentioned
> by having a %param hash with the 'timeout' (and
> dynamic memory) option.
> 
> i'd send these two things together in one (pve) series.
> is that ok for you?
> 

not sure, my question about what happens if I call this in CLI and if,
whatever does, should happen is not really answered.

Re: [pve-devel] [PATCH common v3 1/1] PBSClient: file_restore_list: add timeout parameter

[Dominik Csapak]

On 11/8/22 16:53, Thomas Lamprecht wrote:
> Am 08/11/2022 um 12:20 schrieb Dominik Csapak:
>> On 11/7/22 15:17, Thomas Lamprecht wrote:
>>> subject is not wrong but worded rather confusingly, as of now it rather
>>> implies that this adds a new parameter allowing callers to control the
>>> timeout, but actually it sets the timeout hard-coded to 25s.
>>>
>>> Am 27/05/2022 um 10:22 schrieb Dominik Csapak:
>>>> we always want the restore_list to use a timeout here. Set it to 25 seconds
>>>
>>> Such statements could be a bit more useful with some actual reasoning
>>> (e.g., short sentence about ill effects of lacking timeout)
>>
>> ok i thought the sentence below would be enough reasoning
>>
> 
> not really as it doesn't explains much for **why** headroom would be
> required, if the clients gets the response cut short anyway at 30s
> what benefits do we gain here, just another error message or otherwise
> improved behavior? Why not just alarm($foo) on the call site.

thanks, you're right, i should have explained it better.
having no timeout is imho not a good option because when we let an api
call run into the pveproxy 30s limit, the forked 'run_command' will not
immediately terminate, but run in the background and do unnecessary work

also we don't want the api call to run into the pveproxy limit since
we want the correct error to return (a 503) so that the gui
can detect that specific file-restore timeout so it can retry
(we could also detect the pveproxy timeout, but we couldn't
differentiate between "planned" timeouts, and timeouts
that happened for different reasons where we should abort)

so while we could have the wanted effect with 'alarm', we'd then
have to clean up the process somehow and alarm handling is imo not great
in general. putting that logic inside the file-restore binary makes it
easier.

as for the headroom, the api call does have some overhead, and i
estimated 5s should be enough for that processing
(fork, json decode, etc)

does that make sense for you?

> 
> main point is, it really doesn't hurts to have that relevant information
> here too, not just in the pbs side of the commit..

understood

> 
>>>
>>>> so there is a little headroom between this and pveproxys 30s one.
>>>
>>> what if we'd add a call site outside the sync API response context
>>> (e.g., task worker or CLI rpcenv)? could be an artificial limitation
>>> in that case.
>>
>> i followed your suggestion from the v1 version by hardcoding the options
>> and you applied the pbs ones from v2 partially without
>> complaining about this ;)
> 
> You mean
> https://lists.proxmox.com/pipermail/pve-devel/2022-February/051664.html
> ? That was a comment about a horrid general "pass anything" interface
> Wolfgang agreed too, not specific to the timeout param and it's implications,
> that's why I asked here about if you thought about that (which you did not
> replied at all...)
> 
>>
>> in any case, since i have to touch this again when doing the
>> user dependent memory increase for the file restore,
>> i'd rather use the other approach wolfang mentioned
>> by having a %param hash with the 'timeout' (and
>> dynamic memory) option.
>>
>> i'd send these two things together in one (pve) series.
>> is that ok for you?
>>
> 
> not sure, my question about what happens if I call this in CLI and if,
> whatever does, should happen is not really answered.
> 

yes, if we decide to call it from cli, we probably don't want to have 
the same (then unnecessary) timeout to limit us.

so i'd put it into an extra options hash (together with the dynamic 
memory option, but as seperate patch of course) then we can decide
on every call site if we need that timeout

[pve-devel] [PATCH storage v3 1/1] api: FileRestore: decode and return proper error with new file-restore params

[Dominik Csapak]

pbsclient now uses a timeout of 25 seconds do decode and return the error
if one is returned in json form.

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 PVE/API2/Storage/FileRestore.pm | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Storage/FileRestore.pm b/PVE/API2/Storage/FileRestore.pm
index 5630f52..7495e77 100644
--- a/PVE/API2/Storage/FileRestore.pm
+++ b/PVE/API2/Storage/FileRestore.pm
@@ -121,13 +121,24 @@ __PACKAGE__->register_method ({
 	my $client = PVE::PBSClient->new($scfg, $storeid);
 	my $ret = $client->file_restore_list([$scfg->{namespace}, $snap], $path, $base64);
 
-	# 'leaf' is a proper JSON boolean, map to perl-y bool
-	# TODO: make PBSClient decode all bools always as 1/0?
-	foreach my $item (@$ret) {
-	    $item->{leaf} = $item->{leaf} ? 1 : 0;
+	if (ref($ret) eq "HASH") {
+	    my $msg = $ret->{message};
+	    if (my $code = $ret->{code}) {
+		die PVE::Exception->new("$msg\n", code => $code);
+	    } else {
+		die "$msg\n";
+	    }
+	} elsif (ref($ret) eq "ARRAY") {
+	    # 'leaf' is a proper JSON boolean, map to perl-y bool
+	    # TODO: make PBSClient decode all bools always as 1/0?
+	    foreach my $item (@$ret) {
+		$item->{leaf} = $item->{leaf} ? 1 : 0;
+	    }
+
+	    return $ret;
 	}
 
-	return $ret;
+	die "invalid proxmox-file-restore output";
     }});
 
 __PACKAGE__->register_method ({
-- 
2.30.2

[pve-devel] [PATCH proxmox-backup v3 1/1] file-restore: remove 'json-error' parameter from list_files

[Dominik Csapak]

we can reuse the 'output_format' here
also remove the 'error: true' here. we can determine it was an error,
by checking if it's an object with a 'message' property

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-file-restore/src/main.rs | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
index 3420ea8e..ee5b5b9c 100644
--- a/proxmox-file-restore/src/main.rs
+++ b/proxmox-file-restore/src/main.rs
@@ -217,12 +217,6 @@ async fn list_files(
                 schema: OUTPUT_FORMAT,
                 optional: true,
             },
-            "json-error": {
-                type: Boolean,
-                description: "If set, errors are returned as json instead of writing to stderr",
-                optional: true,
-                default: false,
-            },
             "timeout": {
                 type: Integer,
                 description: "Defines the maximum time the call can should take.",
@@ -245,7 +239,6 @@ async fn list(
     snapshot: String,
     path: String,
     base64: bool,
-    json_error: bool,
     timeout: Option<u64>,
     param: Value,
 ) -> Result<(), Error> {
@@ -290,7 +283,7 @@ async fn list(
     let output_format = get_output_format(&param);
 
     if let Err(err) = result {
-        if !json_error {
+        if &output_format == "text" {
             return Err(err);
         }
         let (msg, code) = match err.downcast_ref::<HttpError>() {
@@ -298,7 +291,6 @@ async fn list(
             None => (err.to_string(), None),
         };
         let mut json_err = json!({
-            "error": true,
             "message": msg,
         });
         if let Some(code) = code {
-- 
2.30.2

[pve-devel] applied: [pbs-devel] [PATCH proxmox-backup v3 1/1] file-restore: remove 'json-error' parameter from list_files

[Thomas Lamprecht]

Am 27/05/2022 um 10:22 schrieb Dominik Csapak:
> we can reuse the 'output_format' here
> also remove the 'error: true' here. we can determine it was an error,
> by checking if it's an object with a 'message' property
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  proxmox-file-restore/src/main.rs | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH common/storage/proxmxo-backup v3] improve file-restore timeout behaviour

[Dominik Csapak]

ping, the proxmox-backup patch was applied, but the common/storage
ones were not. without them it won't work should we bump file-restore