[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