From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 82F0A8D8AB; Wed, 9 Nov 2022 08:07:40 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 64E5618EB2; Wed, 9 Nov 2022 08:07:40 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS; Wed, 9 Nov 2022 08:07:39 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 1147D435CF; Wed, 9 Nov 2022 08:07:39 +0100 (CET) Message-ID: <3e9833c7-d901-5a96-e17f-d254afb61a70@proxmox.com> Date: Wed, 9 Nov 2022 08:07:37 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.4.2 Content-Language: en-US To: Thomas Lamprecht , Proxmox VE development discussion , pbs-devel@lists.proxmox.com References: <20220527082203.1653182-1-d.csapak@proxmox.com> <20220527082203.1653182-2-d.csapak@proxmox.com> <15393855-d2d4-8a6f-6916-dd895e7b6cef@proxmox.com> <8dd1d4cb-509f-e000-9c19-4165c9a74b6f@proxmox.com> From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.066 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox.com] Subject: Re: [pve-devel] [PATCH common v3 1/1] PBSClient: file_restore_list: add timeout parameter X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Nov 2022 07:07:40 -0000 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