[pve-devel] [PATCH mini-journalreader/http-server/manager] optimize journal api cal

[pve-devel] [PATCH mini-journalreader/http-server/manager] optimize journal api cal

[Dominik Csapak]

this series changes the 'journal' api call to stream the journal to the
client, which reduces the memory footprint of the worker processes

for very big logs, the web ui may struggle (depending on browser/cpu/etc.).
if that turns out to be a common problem, we can further optimize
to pagination if necessary

proxmox-mini-journalreader:

Dominik Csapak (1):
  add '-j' flag to output json

 src/mini-journalreader.c | 66 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 3 deletions(-)

http-server:

Dominik Csapak (1):
  http-server: let the api call decide the content-encoding

 src/PVE/APIServer/AnyEvent.pm | 3 +++
 1 file changed, 3 insertions(+)

pve-manager:

Dominik Csapak (1):
  api: journal: stream the journal data to the client

 PVE/API2/Nodes.pm | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH mini-journalreader 1/1] add '-j' flag to output json

[Dominik Csapak]

in the format:
{"data":[... log lines ...],"success":1}

this is chosen so that we can achieve api compatibility when we stream
this output to an api client

strings are escaped by replacing '"', '\' and all values <= 0x1F by their
\uXXXX representation

invalid utf8 sequences will be returned as they are
(jq and the browser can handle that)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/mini-journalreader.c | 66 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 63 insertions(+), 3 deletions(-)

diff --git a/src/mini-journalreader.c b/src/mini-journalreader.c
index 92176ac..7ce7857 100644
--- a/src/mini-journalreader.c
+++ b/src/mini-journalreader.c
@@ -32,6 +32,8 @@
 #define BUFSIZE 4096
 
 static char BUF[BUFSIZE];
+bool json = false;
+bool first_line = true;
 
 static uint64_t get_timestamp(sd_journal *j) {
     uint64_t timestamp;
@@ -63,8 +65,19 @@ static void print_cursor(sd_journal *j) {
         fprintf(stderr, "Failed to get cursor: %s\n", strerror(-r));
         exit(1);
     }
+    if (json) {
+	if (!first_line) {
+	    print_to_buf(",\"", 2);
+	} else {
+	    print_to_buf("\"", 1);
+	}
+    }
     print_to_buf(cursor, strlen(cursor));
+    if (json) {
+	print_to_buf("\"", 1);
+    }
     print_to_buf("\n", 1);
+    first_line = false;
     free(cursor);
 }
 
@@ -93,7 +106,15 @@ static void print_reboot(sd_journal *j) {
     if (bootid[0] != '\0') { // we have some bootid
         if (memcmp(bootid, d, l)) { // a new bootid found
             memcpy(bootid, d, l);
-            print_to_buf("-- Reboot --\n", 13);
+            if (json) {
+                if (!first_line) {
+                    print_to_buf(",", 1);
+                }
+                print_to_buf("\"-- Reboot --\"\n", 15);
+                first_line = false;
+            } else {
+                print_to_buf("-- Reboot --\n", 13);
+            }
         }
     } else {
         memcpy(bootid, d, l);
@@ -149,13 +170,35 @@ static bool print_field(sd_journal *j, const char *field) {
     size_t fieldlen = strlen(field)+1;
     d += fieldlen;
     l -= fieldlen;
-    print_to_buf(d, l);
+
+    if (json) {
+	char tmp[7];
+	for (size_t i = 0; i < l;i++) {
+	    if (d[i] == '"' || d[i] == '\\' || (d[i] >= 0 && d[i] <= 0x1F)) {
+		sprintf(tmp, "\\u%04X", d[i]);
+		print_to_buf(tmp, 6);
+	    } else {
+		print_to_buf(d+i, 1);
+	    }
+	}
+    } else {
+	print_to_buf(d, l);
+    }
     return true;
 }
 
 
 static void print_line(sd_journal *j) {
     print_reboot(j);
+
+    if (json) {
+        if (!first_line) {
+            print_to_buf(",", 1);
+        }
+        print_to_buf("\"", 1);
+        first_line = false;
+    }
+
     print_timestamp(j);
     print_to_buf(" ", 1);
     print_field(j, "_HOSTNAME");
@@ -167,6 +210,11 @@ static void print_line(sd_journal *j) {
     print_pid(j);
     print_to_buf(": ", 2);
     print_field(j, "MESSAGE");
+
+    if (json) {
+        print_to_buf("\"", 1);
+    }
+
     print_to_buf("\n", 1);
 }
 
@@ -184,6 +232,7 @@ _Noreturn static void usage(char *error) {
         "  -n <integer>\t\tprint the last number entries logged\n"
         "  -f <cursor>\t\tprint from this cursor\n"
         "  -t <cursor>\t\tprint to this cursor\n"
+        "  -j \t\t\tprint as json"
         "  -h \t\t\tthis help\n"
         "\n"
         "Passing no range option will dump all the available journal\n"
@@ -217,7 +266,7 @@ int main(int argc, char *argv[]) {
 
     progname = argv[0];
 
-    while ((c = (char)getopt (argc, argv, "b:e:d:n:f:t:h")) != -1) {
+    while ((c = (char)getopt (argc, argv, "b:e:d:n:f:t:jh")) != -1) {
         switch (c) {
             case 'b':
                 begin = arg_to_uint64(optarg);
@@ -239,6 +288,9 @@ int main(int argc, char *argv[]) {
             case 't':
                 endcursor = optarg;
                 break;
+            case 'j':
+                json = true;
+                break;
             case 'h':
                 usage(NULL);
             case '?':
@@ -285,6 +337,10 @@ int main(int argc, char *argv[]) {
         return 1;
     }
 
+    if (json) {
+        print_to_buf("{\"data\":[", 9);
+    }
+
     // if we want to print the last x entries, seek to cursor or end,
     // then x entries back, print the cursor and finally print the
     // entries until end or cursor
@@ -350,6 +406,10 @@ int main(int argc, char *argv[]) {
     print_cursor(j);
     sd_journal_close(j);
 
+    if (json) {
+        print_to_buf("],\"success\":1}", 14);
+    }
+
     // print remaining buffer
     fflush_unlocked(stdout);
 
-- 
2.30.2

[pve-devel] applied: [PATCH mini-journalreader 1/1] add '-j' flag to output json

[Thomas Lamprecht]

On 24.11.21 15:47, Dominik Csapak wrote:
> in the format:
> {"data":[... log lines ...],"success":1}
> 
> this is chosen so that we can achieve api compatibility when we stream
> this output to an api client
> 
> strings are escaped by replacing '"', '\' and all values <= 0x1F by their
> \uXXXX representation
> 
> invalid utf8 sequences will be returned as they are
> (jq and the browser can handle that)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/mini-journalreader.c | 66 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 63 insertions(+), 3 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH http-server 1/1] http-server: let the api call decide the content-encoding

[Dominik Csapak]

this is useful if we want to pipe the output of a program e.g. through gzip

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/PVE/APIServer/AnyEvent.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index c159b8d..31556ce 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -413,6 +413,7 @@ sub send_file_start {
 
 	    if (ref($download) eq 'HASH') {
 		$mime = $download->{'content-type'};
+		my $encoding = $download->{'content-encoding'};
 
 		if ($download->{path} && $download->{stream} &&
 		    $reqstate->{request}->header('PVEDisableProxy'))
@@ -424,6 +425,7 @@ sub send_file_start {
 			pvestreamfile => $download->{path},
 			Content_Type => $mime,
 		    );
+		    $header->header('Content-Encoding' => $encoding) if defined($encoding);
 		    # we need some data so Content-Length gets set correctly and
 		    # the proxy doesn't wait for more data - place a canary
 		    my $resp = HTTP::Response->new(200, "OK", $header, "error canary");
@@ -441,6 +443,7 @@ sub send_file_start {
 
 		if ($download->{stream}) {
 		    my $header = HTTP::Headers->new(Content_Type => $mime);
+		    $header->header('Content-Encoding' => $encoding) if defined($encoding);
 		    my $resp = HTTP::Response->new(200, "OK", $header);
 		    $self->response($reqstate, $resp, undef, 1, 0, $fh);
 		    return;
-- 
2.30.2

[pve-devel] applied: [PATCH http-server 1/1] http-server: let the api call decide the content-encoding

[Thomas Lamprecht]

On 24.11.21 15:47, Dominik Csapak wrote:
> this is useful if we want to pipe the output of a program e.g. through gzip
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
>

applied, thanks! (and congrats to causing the 4.0-4 release ;)

[pve-devel] [PATCH manager 1/1] api: journal: stream the journal data to the client

[Dominik Csapak]

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 <d.csapak@proxmox.com>
---
 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 ';
 
-	my $lines = [];
-	my $parser = sub { push @$lines, shift };
+	open(my $fh, "-|", join(' ', @$cmd))
+	    or die "could not start mini-journalreader";
 
-	PVE::Tools::run_command($cmd, outfunc => $parser);
-
-	return $lines;
+	return {
+	    download => {
+		fh => $fh,
+		stream => 1,
+		'content-type' => 'application/json',
+		'content-encoding' => 'gzip',
+	    },
+	},
     }});
 
 my $sslcert;
-- 
2.30.2

Re: [pve-devel] [PATCH manager 1/1] api: journal: stream the journal data to the client

[Stoiko Ivanov]

Huge thanks for addressing this so quickly and elegantly!

One question/nit:

On Wed, 24 Nov 2021 15:47:48 +0100
Dominik Csapak <d.csapak@proxmox.com> 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 <d.csapak@proxmox.com>
> ---
>  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
might it make sense to let the worker do the gzip-encoding vs. forking
gzip here?

>  
> -	my $lines = [];
> -	my $parser = sub { push @$lines, shift };
> +	open(my $fh, "-|", join(' ', @$cmd))
> +	    or die "could not start mini-journalreader";
>  
> -	PVE::Tools::run_command($cmd, outfunc => $parser);
> -
> -	return $lines;
> +	return {
> +	    download => {
> +		fh => $fh,
> +		stream => 1,
> +		'content-type' => 'application/json',
> +		'content-encoding' => 'gzip',
if yes - I think it would only need to signal it here (maybe compress => 1)
and set the '$nocomp' parameter to the response sub in AnyEvent.pm based
on that (this last part without trying it out)

> +	    },
> +	},
>      }});
>  
>  my $sslcert;

Re: [pve-devel] [PATCH manager 1/1] api: journal: stream the journal data to the client

[Thomas Lamprecht]

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 <d.csapak@proxmox.com> 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 <d.csapak@proxmox.com>
>> ---
>>  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.

Re: [pve-devel] [PATCH manager 1/1] api: journal: stream the journal data to the client

[Stoiko Ivanov]

On Wed, 24 Nov 2021 18:46:04 +0100
Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:

> 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 <d.csapak@proxmox.com> 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 <d.csapak@proxmox.com>
> >> ---
> >>  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.
Thanks for the explanation! (and sorry for the noise)

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

[pve-devel] applied: [PATCH manager 1/1] api: journal: stream the journal data to the client

[Thomas Lamprecht]

On 24.11.21 15:47, 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 <d.csapak@proxmox.com>
> ---
>  PVE/API2/Nodes.pm | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
>

applied, thanks!