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 581B981885 for ; Wed, 24 Nov 2021 18:46:37 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 45AFD231F1 for ; Wed, 24 Nov 2021 18:46:07 +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)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 57BDE231E3 for ; Wed, 24 Nov 2021 18:46:06 +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 25BE4447E2 for ; Wed, 24 Nov 2021 18:46:06 +0100 (CET) Message-ID: <1e36e64e-e8bc-940e-df9e-1079a353463c@proxmox.com> Date: Wed, 24 Nov 2021 18:46:04 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:95.0) Gecko/20100101 Thunderbird/95.0 Content-Language: en-US To: Proxmox VE development discussion , Stoiko Ivanov , Dominik Csapak References: <20211124144748.68687-1-d.csapak@proxmox.com> <20211124144748.68687-4-d.csapak@proxmox.com> <20211124183237.6e6fc40b@rosa.proxmox.com> From: Thomas Lamprecht In-Reply-To: <20211124183237.6e6fc40b@rosa.proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 1.863 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 -3.515 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 Subject: Re: [pve-devel] [PATCH manager 1/1] api: journal: stream the journal data to the client 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, 24 Nov 2021 17:46:37 -0000 On 24.11.21 18:32, Stoiko Ivanov wrote: > Huge thanks for addressing this so quickly and elegantly! > > One question/nit: > > On Wed, 24 Nov 2021 15:47:48 +0100 > Dominik Csapak wrote: > >> instead of accumulating the whole output of 'mini-journalreader' in >> the api call (this can be quite big), use the download mechanic of the >> http-server to stream the output to the client. >> >> we lose some error handling possibilities, but we do not have >> to allocate anything here, and since perl does not free memory after >> allocating[0] this is our desired behaviour. >> >> to keep api compatiblitiy, we need to give the journalreader the '-j' >> flag to let it output json. >> >> also tell the http server that the encoding is gzip and pipe >> the output through it. >> >> 0: https://perldoc.perl.org/perlfaq3#How-can-I-free-an-array-or-hash-so-my-program-shrinks? >> >> Signed-off-by: Dominik Csapak >> --- >> PVE/API2/Nodes.pm | 22 ++++++++++++++-------- >> 1 file changed, 14 insertions(+), 8 deletions(-) >> >> diff --git a/PVE/API2/Nodes.pm b/PVE/API2/Nodes.pm >> index 565cbccc..d57a1937 100644 >> --- a/PVE/API2/Nodes.pm >> +++ b/PVE/API2/Nodes.pm >> @@ -819,19 +819,25 @@ __PACKAGE__->register_method({ >> my $rpcenv = PVE::RPCEnvironment::get(); >> my $user = $rpcenv->get_user(); >> >> - my $cmd = ["/usr/bin/mini-journalreader"]; >> + my $cmd = ["/usr/bin/mini-journalreader", "-j"]; >> push @$cmd, '-n', $param->{lastentries} if $param->{lastentries}; >> push @$cmd, '-b', $param->{since} if $param->{since}; >> push @$cmd, '-e', $param->{until} if $param->{until}; >> - push @$cmd, '-f', $param->{startcursor} if $param->{startcursor}; >> - push @$cmd, '-t', $param->{endcursor} if $param->{endcursor}; >> + push @$cmd, '-f', PVE::Tools::shellquote($param->{startcursor}) if $param->{startcursor}; >> + push @$cmd, '-t', PVE::Tools::shellquote($param->{endcursor}) if $param->{endcursor}; >> + push @$cmd, ' | gzip '; > Not sure which would be more efficient - but the http-server does support > gzipping the result from the API It has no api result in that case, it just returns the open file handle and anyevent can then stream that directly, see the response_stream method in PVE::APIServer::AnyEvent. > might it make sense to let the worker do the gzip-encoding vs. forking > gzip here? contrary to our rust infra from PBS we do not have a streaming encoder directly in the server that we can just layer in between, pve-http-server can currently only compress if it has the whole response, and that's, which is what we want to avoid here. The response method explicitly guards for the stream-an-fh case, AnyEvent.pm line 325: if ($content && !$stream_fh) { # ... my $comp = Compress::Zlib::memGzip($content); What we can do though is compress directly in the mini-journal reader, but that'll be way easier once we rewrite in in rust (heh), as then we can reuse our existing infrastructure to just layer that in-between.