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

[Dominik Csapak <d.csapak@proxmox.com>]

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,
>