[pve-devel] [PATCH http/common/manager/wt/backup/pmg v2] fix: #3971 Tasklog download button

[pve-devel] [PATCH http/common/manager/wt/backup/pmg v2] fix: #3971 Tasklog download button

[Daniel Tschlatscher]

This patch series' aim is to add a download button in the tasklog-
viewer GUI so that users may access all parts of the tasklog more
easily (The tasklog-viewer only displays 50 lines at a time).

For this change in the GUI (proxmox-widget-toolkit) there were revised
implementations for all backends (PVE, PMG and PBS) needed.
Now, when the parameter 'limit' for the /log API call is set to '0',
the backends will stream the file rather than returning a JSON array
with the corresponding lines. (Beforehand, it would return 0 lines)

I initially included a number of patches aimed at refactoring
redundant implementations of downloading a file via a button click.
However, after talking with Sterzy, Dominik and Aaron a bit about
it, I decided to only send the "core" task log download patch series
now, because the refactoring depends on the changes made in widget-
toolkit, but is done in the same repositories as the backend patches.
This would make both reviewing and applying them more confusing, and
the refactoring patches are "a step removed" anyway from this patch
series. Therefore, I am going to send a separate patch series for them
later instead.


During the implementation of the file stream download I found a bug
concerning chromium browsers because of which downloads "randomly"
fail.
The problem was, when implementing the download through an html a tag
and setting its property "download" to anything, (even undefined)
with a not imported self-signed certificate, the download will fail
once the SSL connection is reset.
In my testing this was the case after about 5 seconds. This problem is
especially pronounced in the PMG as there are less background calls in
contrast to the PVE. The PBS, however, does not seem to suffer this
error at all, because the SSL handshake timeout seems to not run out.
 
Without the 'download' attribute you can't set the filename though.
The file will only have a proper name if the server adds the correct
'content-disposition' header. A few calls to the 'downloadAsFile'
function set the filename anyway, because in most cases the browser
frontend is making enough polling calls so that the SSL handshake
does not have to be executed for the download. (That is also why it
probably did not come up before)
This problem does not apply in Firefox as far as I could tell.

See https://bugs.chromium.org/p/chromium/issues/detail?id=993362 for
more information on this.

-- Changes from v1 --
* Rewrote commit messages and cover letter
* Got rid of some refactoring which were initially included
  (going to send as own patch series later)
* Rebased to current repository versions


Daniel Tschlatscher (1):
  make tasklog downloadable in the backup server backend

 src/api2/node/tasks.rs             | 159 ++++++++++++++++++-----------
 4 files changed, 107 insertions(+), 79 deletions(-)

Daniel Tschlatscher (1):
  stream file method for PMG and PVE

 src/PVE/Tools.pm | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
 
Daniel Tschlatscher (1):
  acknowledge content-disposition header

 src/PVE/APIServer/AnyEvent.pm | 3 +++
 1 file changed, 3 insertions(+)
 
Daniel Tschlatscher (1):
  Revised task log API call when parameter 'limit' is 0

 PVE/API2/Tasks.pm                 | 13 ++++++++++---
 3 files changed, 15 insertions(+), 24 deletions(-)
 
Daniel Tschlatscher (1):
  revised task log download function in the PMG backend

 src/PMG/API2/Tasks.pm | 34 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

Daniel Tschlatscher (3):
  Source file download call in central function
  add task log download button in TaskViewer

 src/Utils.js              | 13 +++++++++++++
 src/window/TaskViewer.js  | 17 +++++++++++++++--
 src/window/FileBrowser.js | 11 +++++------
 3 files changed, 38 insertions(+), 8 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH proxmox-backup v2 1/7] make tasklog downloadable in the backup server backend

[Daniel Tschlatscher]

The API call for getting the tasklog now returns a file stream when
the URL parameter 'limit' is set to 0, in accordance with the changes
in the PMG and PVE backends.
To make this work I had to use one of the lower level apimethod types
from the proxmox-router. Therefore I also had to change declarations
for the API routing and the corresponding Object Schemas.
If the parameter 'limit' is not set to 0 the API should behave the
same as before.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/api2/node/tasks.rs | 159 ++++++++++++++++++++++++++---------------
 1 file changed, 101 insertions(+), 58 deletions(-)

diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index cbd0894e..99600bf4 100644
--- a/src/api2/node/tasks.rs
+++ b/src/api2/node/tasks.rs
@@ -1,11 +1,17 @@
 use std::fs::File;
 use std::io::{BufRead, BufReader};
+use std::path::PathBuf;
 
 use anyhow::{bail, Error};
+use futures::FutureExt;
+use http::{Response, StatusCode, header};
+use http::request::Parts;
+use hyper::Body;
+use proxmox_async::stream::AsyncReaderStream;
 use serde_json::{json, Value};
 
-use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap};
-use proxmox_schema::api;
+use proxmox_router::{list_subdirs_api_method, Permission, Router, RpcEnvironment, SubdirMap, ApiMethod, ApiHandler, ApiResponseFuture};
+use proxmox_schema::{api, Schema, IntegerSchema, BooleanSchema, ObjectSchema};
 use proxmox_sys::sortable;
 
 use pbs_api_types::{
@@ -19,6 +25,20 @@ use crate::api2::pull::check_pull_privs;
 use pbs_config::CachedUserInfo;
 use proxmox_rest_server::{upid_log_path, upid_read_status, TaskListInfoIterator, TaskState};
 
+pub const START_PARAM_SCHEMA: Schema =
+    IntegerSchema::new("Start at this line when reading the tasklog")
+        .minimum(0)
+        .schema();
+
+pub const LIMIT_PARAM_SCHEMA: Schema =
+    IntegerSchema::new("The amount of lines to read from the tasklog")
+        .minimum(0)
+        .schema();
+
+pub const TEST_STATUS_PARAM_SCHEMA: Schema =
+    BooleanSchema::new("Test task status, and set result attribute \"active\" accordingly.")
+        .schema();
+
 // matches respective job execution privileges
 fn check_job_privs(auth_id: &Authid, user_info: &CachedUserInfo, upid: &UPID) -> Result<(), Error> {
     match (upid.worker_type.as_str(), &upid.worker_id) {
@@ -268,58 +288,88 @@ fn extract_upid(param: &Value) -> Result<UPID, Error> {
     upid_str.parse::<UPID>()
 }
 
-#[api(
-    input: {
-        properties: {
-            node: {
-                schema: NODE_SCHEMA,
-            },
-            upid: {
-                schema: UPID_SCHEMA,
-            },
-            "test-status": {
-                type: bool,
-                optional: true,
-                description: "Test task status, and set result attribute \"active\" accordingly.",
-            },
-            start: {
-                type: u64,
-                optional: true,
-                description: "Start at this line.",
-                default: 0,
-            },
-            limit: {
-                type: u64,
-                optional: true,
-                description: "Only list this amount of lines.",
-                default: 50,
-            },
-        },
-    },
-    access: {
-        description: "Users can access their own tasks, or need Sys.Audit on /system/tasks.",
-        permission: &Permission::Anybody,
-    },
-)]
-/// Read task log.
-async fn read_task_log(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
-    let upid = extract_upid(&param)?;
-
-    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
-
-    check_task_access(&auth_id, &upid)?;
-
-    let test_status = param["test-status"].as_bool().unwrap_or(false);
-
-    let start = param["start"].as_u64().unwrap_or(0);
-    let mut limit = param["limit"].as_u64().unwrap_or(50);
-
-    let mut count: u64 = 0;
+#[sortable]
+pub const API_METHOD_READ_TASK_LOG: ApiMethod = ApiMethod::new(
+    &ApiHandler::AsyncHttp(&download_task_log),
+    &ObjectSchema::new(
+        "Read the task log",
+        &sorted!([
+            ("node", false, &NODE_SCHEMA),
+            ("upid", false, &UPID_SCHEMA),
+            ("start", true, &START_PARAM_SCHEMA),
+            ("limit", true, &LIMIT_PARAM_SCHEMA),
+            ("test-status", true, &TEST_STATUS_PARAM_SCHEMA)
+        ]),
+    ),
+)
+.access(
+    Some("Users can access their own tasks, or need Sys.Audit on /system/tasks."),
+    &Permission::Anybody,
+);
+fn download_task_log(
+    _parts: Parts,
+    _req_body: Body,
+    param: Value,
+    _info: &ApiMethod,
+    rpcenv: Box<dyn RpcEnvironment>,
+) -> ApiResponseFuture {
+    async move {
+        let upid: UPID = extract_upid(&param)?;
+        let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+        check_task_access(&auth_id, &upid)?;
+
+        let test_status = param["test-status"].as_bool().unwrap_or(false);
+        let start = param["start"].as_u64().unwrap_or(0);
+        let limit = param["limit"].as_u64().unwrap_or(50);
+
+        let path = upid_log_path(&upid)?;
+
+        if limit == 0 {
+            let file = tokio::fs::File::open(path).await?;
+
+            let header_disp = format!("attachment; filename={}", &upid.to_string());
+
+            let stream = AsyncReaderStream::new(file);
+
+            Ok(Response::builder()
+                .status(StatusCode::OK)
+                .header(header::CONTENT_TYPE, "text/plain")
+                .header(header::CONTENT_DISPOSITION, &header_disp)
+                .body(Body::wrap_stream(stream))
+                .unwrap())
+        } else {
+            let (count, lines) = read_tasklog_lines(path, start, limit).unwrap();
+
+            let mut json = json!({
+                "data": lines,
+                "total": count,
+                "success": 1,
+            });
+
+            if test_status {
+                let active = proxmox_rest_server::worker_is_active(&upid).await?;
+
+                json["test-status"] = Value::from(active);
+            }
 
-    let path = upid_log_path(&upid)?;
+            Ok(Response::builder()
+                .status(StatusCode::OK)
+                .header(header::CONTENT_TYPE, "application/json")
+                .body(Body::from(json.to_string()))
+                .unwrap())
+        }
+    }
+    .boxed()
+}
 
+fn read_tasklog_lines(
+    path: PathBuf,
+    start: u64,
+    mut limit: u64,
+) -> Result<(u64, Vec<Value>), Error> {
     let file = File::open(path)?;
 
+    let mut count: u64 = 0;
     let mut lines: Vec<Value> = vec![];
 
     for line in BufReader::new(file).lines() {
@@ -344,14 +394,7 @@ async fn read_task_log(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<
         }
     }
 
-    rpcenv["total"] = Value::from(count);
-
-    if test_status {
-        let active = proxmox_rest_server::worker_is_active(&upid).await?;
-        rpcenv["active"] = Value::from(active);
-    }
-
-    Ok(json!(lines))
+    Ok((count, lines))
 }
 
 #[api(
-- 
2.30.2

[pve-devel] [PATCH http-server v2 2/7] acknowledge content-disposition header

[Daniel Tschlatscher]

Acknowledging the Content-Disposition header makes it possible for the
backend to tell the browser whether a file should be downloaded,
rather than displayed inline, and what it's default name should be.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@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 877a4e6..130727b 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -426,6 +426,7 @@ sub send_file_start {
 	    if (ref($download) eq 'HASH') {
 		$mime = $download->{'content-type'};
 		my $encoding = $download->{'content-encoding'};
+		my $disposition = $download->{'content-disposition'};
 
 		if ($download->{path} && $download->{stream} &&
 		    $reqstate->{request}->header('PVEDisableProxy'))
@@ -438,6 +439,7 @@ sub send_file_start {
 			Content_Type => $mime,
 		    );
 		    $header->header('Content-Encoding' => $encoding) if defined($encoding);
+		    $header->header('Content-Disposition' => $disposition) if defined($disposition);
 		    # 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");
@@ -456,6 +458,7 @@ sub send_file_start {
 		if ($download->{stream}) {
 		    my $header = HTTP::Headers->new(Content_Type => $mime);
 		    $header->header('Content-Encoding' => $encoding) if defined($encoding);
+		    $header->header('Content-Disposition' => $disposition) if defined($disposition);
 		    my $resp = HTTP::Response->new(200, "OK", $header);
 		    $self->response($reqstate, $resp, undef, 1, 0, $fh);
 		    return;
-- 
2.30.2

[pve-devel] [PATCH common v2 3/7] stream file method for PMG and PVE

[Daniel Tschlatscher]

Creating the filestream for the tasklog download is sourced in its own
function to avoid redundant implementations in pmg-api and pve-manager

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/PVE/Tools.pm | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/src/PVE/Tools.pm b/src/PVE/Tools.pm
index eb81b96..5639c32 100644
--- a/src/PVE/Tools.pm
+++ b/src/PVE/Tools.pm
@@ -2056,4 +2056,32 @@ sub get_file_hash {
     return lc($digest);
 }
 
+sub stream_file {
+    my ($filename, $suggested_name, $compress, $content_type) = @_;
+
+    my $fh;
+    $content_type = $content_type // "text/plain";
+    $suggested_name = $suggested_name // $filename;
+
+    if ($compress) {
+	my $cmd = ["/usr/bin/gzip", "-c", "$filename"];
+
+	open($fh, "-|", join(' ', @$cmd))
+	    or die "Could not create compressed file stream for $filename - $!";
+    } else {
+	open($fh, '<', $filename)
+	    or die "Could not open file $filename - $!";
+    }
+
+    return {
+	download => {
+	    fh => $fh,
+	    stream => 1,
+	    'content-encoding' => $compress ? 'gzip' : undef,
+	    'content-type' => $content_type,
+	    'content-disposition' => "attachment; filename=\"$suggested_name\"",
+	},
+    },
+}
+
 1;
-- 
2.30.2

[pve-devel] [PATCH manager v2 4/7] revised task log API call for PVE

[Daniel Tschlatscher]

The API call for fetching a tasklog with limit=0 now returns the whole
log as a file stream rather than reading all lines in memory and then
transfering them in JSON format. The behaviour when the url parameter
limit is undefined or not 0 is the same as before, in accordance
with the API specification.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 PVE/API2/Tasks.pm | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
index 9cd1e56b..88762f2f 100644
--- a/PVE/API2/Tasks.pm
+++ b/PVE/API2/Tasks.pm
@@ -5,6 +5,7 @@ use warnings;
 use POSIX;
 use IO::File;
 use File::ReadBackwards;
+use File::stat;
 use PVE::Tools;
 use PVE::SafeSyslog;
 use PVE::RESTHandler;
@@ -387,11 +388,17 @@ __PACKAGE__->register_method({
 	    $rpcenv->check($user, "/nodes/$node", [ 'Sys.Audit' ]);
 	}
 
-	my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
+	if ($limit == 0) {
+	    # TCP Max Transfer Unit size is 1500, compression for lower numbers has no effect
+	    my $use_compression = stat($filename)->size > 1500;
+	    return PVE::Tools::stream_file($filename, $param->{upid}, $use_compression);
+	} else {
+	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
 
-	$rpcenv->set_result_attrib('total', $count);
+	    $rpcenv->set_result_attrib('total', $count);
 
-	return $lines;
+	    return $lines;
+	}
     }});
 
 
-- 
2.30.2

[pve-devel] [PATCH pmg-api v2 5/7] revised task log download function in the PMG backend

[Daniel Tschlatscher]

With the newly added button in the tasklog the implementation for the
PMG server needs to be adapted. I saw an opportunity here to clear
some redundant code for displaying the tasklog and replace it with a
call to dump_logfile(), akin to how this is handled in pve-manager.

The tasklog download functionality now streams the file by invoking
the newly created function in pve-common.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/PMG/API2/Tasks.pm | 34 +++++++++++-----------------------
 1 file changed, 11 insertions(+), 23 deletions(-)

diff --git a/src/PMG/API2/Tasks.pm b/src/PMG/API2/Tasks.pm
index 598fb4d..0fd3780 100644
--- a/src/PMG/API2/Tasks.pm
+++ b/src/PMG/API2/Tasks.pm
@@ -5,6 +5,7 @@ use warnings;
 use POSIX;
 use IO::File;
 use File::ReadBackwards;
+use File::stat;
 use PVE::Tools;
 use PVE::SafeSyslog;
 use PVE::RESTHandler;
@@ -272,33 +273,20 @@ __PACKAGE__->register_method({
 
 	my $restenv = PMG::RESTEnvironment->get();
 
-	my $fh = IO::File->new($filename, "r");
-	raise_param_exc({ upid => "no such task - unable to open file - $!" }) if !$fh;
+	my $start = $param->{start} // 0;
+	my $limit = $param->{limit} // 50;
 
-	my $start = $param->{start} || 0;
-	my $limit = $param->{limit} || 50;
-	my $count = 0;
-	my $line;
-	while (defined ($line = <$fh>)) {
-	    next if $count++ < $start;
-	    next if $limit <= 0;
-	    chomp $line;
-	    push @$lines, { n => $count, t => $line};
-	    $limit--;
-	}
+	if ($limit == 0) {
+	    # TCP Max Transfer Unit size is 1500, compression for lower numbers has no effect
+	    my $use_compression = stat($filename)->size > 1500;
+	    return PVE::Tools::stream_logfile($filename, $param->{upid}, $use_compression);
+	} else {
+	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
 
-	close($fh);
+	    $restenv->set_result_attrib('total', $count);
 
-	# HACK: ExtJS store.guaranteeRange() does not like empty array
-	# so we add a line
-	if (!$count) {
-	    $count++;
-	    push @$lines, { n => $count, t => "no content"};
+	    return $lines;
 	}
-
-	$restenv->set_result_attrib('total', $count);
-
-	return $lines;
     }});
 
 
-- 
2.30.2

[pve-devel] [PATCH widget-toolkit v2 6/7] Source file download call in central function

[Daniel Tschlatscher]

Adds a function for downloading a file from a remote URL in the Utils
class and uses it to revise one similar usage in FileBrowser.js

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/Utils.js              | 13 +++++++++++++
 src/window/FileBrowser.js | 11 +++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/Utils.js b/src/Utils.js
index 6a03057..bb68937 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -1272,6 +1272,19 @@ utilities: {
 	    .map(val => val.charCodeAt(0)),
 	);
     },
+
+    // Setting filename here when downloading from a remote url sometimes fails in chromium browsers
+    // because of a bug when using attribute download in conjunction with a self signed certificate.
+    // For more info see https://bugs.chromium.org/p/chromium/issues/detail?id=993362
+    downloadAsFile: function(source, fileName) {
+	let hiddenElement = document.createElement('a');
+	hiddenElement.href = source;
+	hiddenElement.target = '_blank';
+	if (fileName) {
+	    hiddenElement.download = fileName;
+	}
+	hiddenElement.click();
+    },
 },
 
     singleton: true,
diff --git a/src/window/FileBrowser.js b/src/window/FileBrowser.js
index a519d6b..4e4c639 100644
--- a/src/window/FileBrowser.js
+++ b/src/window/FileBrowser.js
@@ -101,18 +101,17 @@ Ext.define("Proxmox.window.FileBrowser", {
 	    let params = { ...view.extraParams };
 	    params.filepath = data.filepath;
 
-	    let atag = document.createElement('a');
-	    atag.download = view.downloadPrefix + data.text;
+	    let filename = view.downloadPrefix + data.text;
 	    if (data.type === 'd') {
 		if (tar) {
 		    params.tar = 1;
-		    atag.download += ".tar.zst";
+		    filename += ".tar.zst";
 		} else {
-		    atag.download += ".zip";
+		    filename += ".zip";
 		}
 	    }
-	    atag.href = me.buildUrl(view.downloadURL, params);
-	    atag.click();
+
+	    Proxmox.Utils.downloadAsFile(me.buildUrl(view.downloadURL, params), filename);
 	},
 
 	fileChanged: function() {
-- 
2.30.2

[pve-devel] [PATCH widget-toolkit v2 7/7] add task log download button in TaskViewer

[Daniel Tschlatscher]

Adds a download button in the TaskViewer. Uses the newly created
downloadAsFile() method in the Utils class.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 src/window/TaskViewer.js | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/src/window/TaskViewer.js b/src/window/TaskViewer.js
index 9293d95..f1de188 100644
--- a/src/window/TaskViewer.js
+++ b/src/window/TaskViewer.js
@@ -230,11 +230,23 @@ Ext.define('Proxmox.window.TaskViewer', {
 	    border: false,
 	});
 
+	let downloadBtn = new Ext.Button({
+	    text: gettext('Download'),
+	    iconCls: 'fa fa-download',
+	    handler: function() {
+		let url = '/api2/extjs/nodes/' +
+		    `${task.node}/tasks/${encodeURIComponent(me.upid)}/log?limit=0`;
+
+		Proxmox.Utils.downloadAsFile(url);
+	    },
+	});
+
+
 	let logView = Ext.create('Proxmox.panel.LogView', {
 	    title: gettext('Output'),
-	    tbar: [stop_btn2],
+	    tbar: [stop_btn2, '->', downloadBtn],
 	    border: false,
-	    url: "/api2/extjs/nodes/" + task.node + "/tasks/" + encodeURIComponent(me.upid) + "/log",
+	    url: `/api2/extjs/nodes/${task.node}/tasks/${encodeURIComponent(me.upid)}/log`,
 	});
 
 	me.mon(statstore, 'load', function() {
@@ -249,6 +261,7 @@ Ext.define('Proxmox.window.TaskViewer', {
 
 	    stop_btn1.setDisabled(status !== 'running');
 	    stop_btn2.setDisabled(status !== 'running');
+	    downloadBtn.setDisabled(status === 'running');
 	});
 
 	statstore.startUpdate();
-- 
2.30.2

Re: [pve-devel] [pmg-devel] [PATCH pmg-api v2 5/7] revised task log download function in the PMG backend

[Stefan Sterz]

On 9/7/22 10:56, Daniel Tschlatscher wrote:
> With the newly added button in the tasklog the implementation for the
> PMG server needs to be adapted. I saw an opportunity here to clear
> some redundant code for displaying the tasklog and replace it with a
> call to dump_logfile(), akin to how this is handled in pve-manager.
> 
> The tasklog download functionality now streams the file by invoking
> the newly created function in pve-common.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
>  src/PMG/API2/Tasks.pm | 34 +++++++++++-----------------------
>  1 file changed, 11 insertions(+), 23 deletions(-)
> 
> diff --git a/src/PMG/API2/Tasks.pm b/src/PMG/API2/Tasks.pm
> index 598fb4d..0fd3780 100644
> --- a/src/PMG/API2/Tasks.pm
> +++ b/src/PMG/API2/Tasks.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use POSIX;
>  use IO::File;
>  use File::ReadBackwards;
> +use File::stat;
>  use PVE::Tools;
>  use PVE::SafeSyslog;
>  use PVE::RESTHandler;
> @@ -272,33 +273,20 @@ __PACKAGE__->register_method({
>  
>  	my $restenv = PMG::RESTEnvironment->get();
>  
> -	my $fh = IO::File->new($filename, "r");
> -	raise_param_exc({ upid => "no such task - unable to open file - $!" }) if !$fh;
> +	my $start = $param->{start} // 0;
> +	my $limit = $param->{limit} // 50;
>  
> -	my $start = $param->{start} || 0;
> -	my $limit = $param->{limit} || 50;
> -	my $count = 0;
> -	my $line;
> -	while (defined ($line = <$fh>)) {
> -	    next if $count++ < $start;
> -	    next if $limit <= 0;
> -	    chomp $line;
> -	    push @$lines, { n => $count, t => $line};
> -	    $limit--;
> -	}
> +	if ($limit == 0) {
> +	    # TCP Max Transfer Unit size is 1500, compression for lower numbers has no effect
> +	    my $use_compression = stat($filename)->size > 1500;
> +	    return PVE::Tools::stream_logfile($filename, $param->{upid}, $use_compression);

I am pretty sure that should be `stream_file` and not `stream_logfile`.
We've already discussed this off list, but I wanted to document it here too.

> +	} else {
> +	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
>  
> -	close($fh);
> +	    $restenv->set_result_attrib('total', $count);
>  
> -	# HACK: ExtJS store.guaranteeRange() does not like empty array
> -	# so we add a line
> -	if (!$count) {
> -	    $count++;
> -	    push @$lines, { n => $count, t => "no content"};
> +	    return $lines;
>  	}
> -
> -	$restenv->set_result_attrib('total', $count);
> -
> -	return $lines;
>      }});
>  
>  

Re: [pve-devel] [pmg-devel] [PATCH http/common/manager/wt/backup/pmg v2] fix: #3971 Tasklog download button

[Stefan Sterz]

On 9/7/22 10:56, Daniel Tschlatscher wrote:
> This patch series' aim is to add a download button in the tasklog-
> viewer GUI so that users may access all parts of the tasklog more
> easily (The tasklog-viewer only displays 50 lines at a time).
> 
> For this change in the GUI (proxmox-widget-toolkit) there were revised
> implementations for all backends (PVE, PMG and PBS) needed.
> Now, when the parameter 'limit' for the /log API call is set to '0',
> the backends will stream the file rather than returning a JSON array
> with the corresponding lines. (Beforehand, it would return 0 lines)
> 
> I initially included a number of patches aimed at refactoring
> redundant implementations of downloading a file via a button click.
> However, after talking with Sterzy, Dominik and Aaron a bit about
> it, I decided to only send the "core" task log download patch series
> now, because the refactoring depends on the changes made in widget-
> toolkit, but is done in the same repositories as the backend patches.
> This would make both reviewing and applying them more confusing, and
> the refactoring patches are "a step removed" anyway from this patch
> series. Therefore, I am going to send a separate patch series for them
> later instead.
> 
> 
> During the implementation of the file stream download I found a bug
> concerning chromium browsers because of which downloads "randomly"
> fail.
> The problem was, when implementing the download through an html a tag
> and setting its property "download" to anything, (even undefined)
> with a not imported self-signed certificate, the download will fail
> once the SSL connection is reset.
> In my testing this was the case after about 5 seconds. This problem is
> especially pronounced in the PMG as there are less background calls in
> contrast to the PVE. The PBS, however, does not seem to suffer this
> error at all, because the SSL handshake timeout seems to not run out.
>  
> Without the 'download' attribute you can't set the filename though.
> The file will only have a proper name if the server adds the correct
> 'content-disposition' header. A few calls to the 'downloadAsFile'
> function set the filename anyway, because in most cases the browser
> frontend is making enough polling calls so that the SSL handshake
> does not have to be executed for the download. (That is also why it
> probably did not come up before)
> This problem does not apply in Firefox as far as I could tell.
> 
> See https://bugs.chromium.org/p/chromium/issues/detail?id=993362 for
> more information on this.
> 
> -- Changes from v1 --
> * Rewrote commit messages and cover letter
> * Got rid of some refactoring which were initially included
>   (going to send as own patch series later)
> * Rebased to current repository versions
> 
> 
> Daniel Tschlatscher (1):
>   make tasklog downloadable in the backup server backend
> 
>  src/api2/node/tasks.rs             | 159 ++++++++++++++++++-----------
>  4 files changed, 107 insertions(+), 79 deletions(-)
> 
> Daniel Tschlatscher (1):
>   stream file method for PMG and PVE
> 
>  src/PVE/Tools.pm | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  
> Daniel Tschlatscher (1):
>   acknowledge content-disposition header
> 
>  src/PVE/APIServer/AnyEvent.pm | 3 +++
>  1 file changed, 3 insertions(+)
>  
> Daniel Tschlatscher (1):
>   Revised task log API call when parameter 'limit' is 0
> 
>  PVE/API2/Tasks.pm                 | 13 ++++++++++---
>  3 files changed, 15 insertions(+), 24 deletions(-)
>  
> Daniel Tschlatscher (1):
>   revised task log download function in the PMG backend
> 
>  src/PMG/API2/Tasks.pm | 34 +++++++++++-----------------------
>  1 file changed, 11 insertions(+), 23 deletions(-)
> 
> Daniel Tschlatscher (3):
>   Source file download call in central function
>   add task log download button in TaskViewer
> 
>  src/Utils.js              | 13 +++++++++++++
>  src/window/TaskViewer.js  | 17 +++++++++++++++--
>  src/window/FileBrowser.js | 11 +++++------
>  3 files changed, 38 insertions(+), 8 deletions(-)
> 

Other than one bug in `pmg-api` (see my other email) this series seems
to work as intended. The bug in `pmg-api` is fixed fairly easily by just
calling the right function and I tested that locally already. So unless
there are other considerations:

Tested-by: Stefan Sterz <s.sterz@proxmox.com>

Re: [pve-devel] [pmg-devel] [PATCH pmg-api v2 5/7] revised task log download function in the PMG backend

[Daniel Tschlatscher]

On 9/8/22 11:36, Stefan Sterz wrote:
> On 9/7/22 10:56, Daniel Tschlatscher wrote:
>> With the newly added button in the tasklog the implementation for the
>> PMG server needs to be adapted. I saw an opportunity here to clear
>> some redundant code for displaying the tasklog and replace it with a
>> call to dump_logfile(), akin to how this is handled in pve-manager.
>>
>> The tasklog download functionality now streams the file by invoking
>> the newly created function in pve-common.
>>
>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>> ---
>>  src/PMG/API2/Tasks.pm | 34 +++++++++++-----------------------
>>  1 file changed, 11 insertions(+), 23 deletions(-)
>>
>> diff --git a/src/PMG/API2/Tasks.pm b/src/PMG/API2/Tasks.pm
>> index 598fb4d..0fd3780 100644
>> --- a/src/PMG/API2/Tasks.pm
>> +++ b/src/PMG/API2/Tasks.pm
>> @@ -5,6 +5,7 @@ use warnings;
>>  use POSIX;
>>  use IO::File;
>>  use File::ReadBackwards;
>> +use File::stat;
>>  use PVE::Tools;
>>  use PVE::SafeSyslog;
>>  use PVE::RESTHandler;
>> @@ -272,33 +273,20 @@ __PACKAGE__->register_method({
>>  
>>  	my $restenv = PMG::RESTEnvironment->get();
>>  
>> -	my $fh = IO::File->new($filename, "r");
>> -	raise_param_exc({ upid => "no such task - unable to open file - $!" }) if !$fh;
>> +	my $start = $param->{start} // 0;
>> +	my $limit = $param->{limit} // 50;
>>  
>> -	my $start = $param->{start} || 0;
>> -	my $limit = $param->{limit} || 50;
>> -	my $count = 0;
>> -	my $line;
>> -	while (defined ($line = <$fh>)) {
>> -	    next if $count++ < $start;
>> -	    next if $limit <= 0;
>> -	    chomp $line;
>> -	    push @$lines, { n => $count, t => $line};
>> -	    $limit--;
>> -	}
>> +	if ($limit == 0) {
>> +	    # TCP Max Transfer Unit size is 1500, compression for lower numbers has no effect
>> +	    my $use_compression = stat($filename)->size > 1500;
>> +	    return PVE::Tools::stream_logfile($filename, $param->{upid}, $use_compression);
> 
> I am pretty sure that should be `stream_file` and not `stream_logfile`.
> We've already discussed this off list, but I wanted to document it here too.
> 
Thanks for catching this!
Yes, I switched something up here when preparing the series. The
function call here should be "stream_file".
>> +	} else {
>> +	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
>>  
>> -	close($fh);
>> +	    $restenv->set_result_attrib('total', $count);
>>  
>> -	# HACK: ExtJS store.guaranteeRange() does not like empty array
>> -	# so we add a line
>> -	if (!$count) {
>> -	    $count++;
>> -	    push @$lines, { n => $count, t => "no content"};
>> +	    return $lines;
>>  	}
>> -
>> -	$restenv->set_result_attrib('total', $count);
>> -
>> -	return $lines;
>>      }});
>>  
>>  
> 

[pve-devel] applied: [PATCH http-server v2 2/7] acknowledge content-disposition header

[Thomas Lamprecht]

Am 07/09/2022 um 10:56 schrieb Daniel Tschlatscher:
> Acknowledging the Content-Disposition header makes it possible for the
> backend to tell the browser whether a file should be downloaded,
> rather than displayed inline, and what it's default name should be.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
>  src/PVE/APIServer/AnyEvent.pm | 3 +++
>  1 file changed, 3 insertions(+)
> 
>

applied, thanks!

Re: [pve-devel] [PATCH manager v2 4/7] revised task log API call for PVE

[Thomas Lamprecht]

Am 07/09/2022 um 10:56 schrieb Daniel Tschlatscher:
> The API call for fetching a tasklog with limit=0 now returns the whole
> log as a file stream rather than reading all lines in memory and then
> transfering them in JSON format. The behaviour when the url parameter
> limit is undefined or not 0 is the same as before, in accordance
> with the API specification.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
>  PVE/API2/Tasks.pm | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
> index 9cd1e56b..88762f2f 100644
> --- a/PVE/API2/Tasks.pm
> +++ b/PVE/API2/Tasks.pm
> @@ -5,6 +5,7 @@ use warnings;
>  use POSIX;
>  use IO::File;
>  use File::ReadBackwards;
> +use File::stat;
>  use PVE::Tools;
>  use PVE::SafeSyslog;
>  use PVE::RESTHandler;
> @@ -387,11 +388,17 @@ __PACKAGE__->register_method({
>  	    $rpcenv->check($user, "/nodes/$node", [ 'Sys.Audit' ]);
>  	}
>  
> -	my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
> +	if ($limit == 0) {
> +	    # TCP Max Transfer Unit size is 1500, compression for lower numbers has no effect

What do you mean here? The MTU of the Ethernet data layer? As that would make
more sense, there the most common (but not guaranteed) MTU is indeed 1500. TCP
can send more data per packet just fine if the MTU of the underlying (ethernet)
network is bigger, like the often used MTU of 9000 in LANs.

Note also that the actual file size that can be transmitted in one packet would
need to factor in IP, TCP and HTTP overhead though.

For IPv4 you'd use 16 byte for the IP and 24 bytes for the TCP part of the
packet, so it'd be 1460 bytes, the HTTP header isn't as deterministic and for
IPv6 it's more overhead eating away the possible payload size.

That alls said, we could still use the 1500 as cut-off, nothing inherently against
that per se, but the comment should not refer to the confusing TCP MTU and mention
that the boundary depends.

For finding a cutoff we could look at file distribution size of task log in existing
(non-test) instances, e.g. with:

find /var/log/pve/tasks/ -mindepth 2 -type f -print0 | xargs -0 ls -l | awk '{size[int(log($5)/log(2))]++}END{for (i in size) printf("%10d %3d\n", 2^i, size[i])}' | sort -n


For three relatively active and long lived host this gives:
    Size       A      B     C
                       1
         8    567    669   121
        16      3     28
        32     40    106     6
        64     60     23
       128     63     28
       256     22     25     8
       512      8     12     4
      1024      8     32    40
      2048      2     12    17
      4096      5      1
      8192     14      1   695
     16384     18            1
     32768     24


I then compressed all files with gzip and reran:

        32   1
        64 596
       128 154
       256  14
       512  13
      1024   4
      2048  14
      4096  18
      8192  24

        32   1
        64 784
       128  74
       256  38
       512  41
      1024   2
      4096   1
      8192   1

So, I'd just use 1024 as cut-off, that isn't bound to such dynamic limits like TCP
actual payload per single package size, but will still fit in most and even captures
most of the previously selected files in practice anyway. Also gzip is normally
able to compress text quite well so doing that with log (i.e., not random data) text
files bigger than 1KiB will almost never over the uncompressed limit. If you rather
like a higher size you can also use 2 or 4 KiB, both fine too IMO.

> +	    my $use_compression = stat($filename)->size > 1500;

should be able to use: -s $filename > X

> +	    return PVE::Tools::stream_file($filename, $param->{upid}, $use_compression);

IMO the helper isn't to useful, I'd just do this inline.

> +	} else {

no else required, other branch returns.

> +	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
>  
> -	$rpcenv->set_result_attrib('total', $count);
> +	    $rpcenv->set_result_attrib('total', $count);
>  
> -	return $lines;
> +	    return $lines;
> +	}
>      }});
>  
>  

Re: [pve-devel] [PATCH manager v2 4/7] revised task log API call for PVE

[Daniel Tschlatscher]

Thanks for the review!

On 10/5/22 14:30, Thomas Lamprecht wrote:
> Am 07/09/2022 um 10:56 schrieb Daniel Tschlatscher:
>> The API call for fetching a tasklog with limit=0 now returns the whole
>> log as a file stream rather than reading all lines in memory and then
>> transfering them in JSON format. The behaviour when the url parameter
>> limit is undefined or not 0 is the same as before, in accordance
>> with the API specification.
>>
>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>> ---
>>  PVE/API2/Tasks.pm | 13 ++++++++++---
>>  1 file changed, 10 insertions(+), 3 deletions(-)
>>
>> diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
>> index 9cd1e56b..88762f2f 100644
>> --- a/PVE/API2/Tasks.pm
>> +++ b/PVE/API2/Tasks.pm
>> @@ -5,6 +5,7 @@ use warnings;
>>  use POSIX;
>>  use IO::File;
>>  use File::ReadBackwards;
>> +use File::stat;
>>  use PVE::Tools;
>>  use PVE::SafeSyslog;
>>  use PVE::RESTHandler;
>> @@ -387,11 +388,17 @@ __PACKAGE__->register_method({
>>  	    $rpcenv->check($user, "/nodes/$node", [ 'Sys.Audit' ]);
>>  	}
>>  
>> -	my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
>> +	if ($limit == 0) {
>> +	    # TCP Max Transfer Unit size is 1500, compression for lower numbers has no effect
> 
> What do you mean here? The MTU of the Ethernet data layer? As that would make
> more sense, there the most common (but not guaranteed) MTU is indeed 1500. TCP
> can send more data per packet just fine if the MTU of the underlying (ethernet)
> network is bigger, like the often used MTU of 9000 in LANs.
> 
> Note also that the actual file size that can be transmitted in one packet would
> need to factor in IP, TCP and HTTP overhead though.
> 
> For IPv4 you'd use 16 byte for the IP and 24 bytes for the TCP part of the
> packet, so it'd be 1460 bytes, the HTTP header isn't as deterministic and for
> IPv6 it's more overhead eating away the possible payload size.
> 
> That alls said, we could still use the 1500 as cut-off, nothing inherently against
> that per se, but the comment should not refer to the confusing TCP MTU and mention
> that the boundary depends.
> 
> For finding a cutoff we could look at file distribution size of task log in existing
> (non-test) instances, e.g. with:
> 
> find /var/log/pve/tasks/ -mindepth 2 -type f -print0 | xargs -0 ls -l | awk '{size[int(log($5)/log(2))]++}END{for (i in size) printf("%10d %3d\n", 2^i, size[i])}' | sort -n
> 
> 
> For three relatively active and long lived host this gives:
>     Size       A      B     C
>                        1
>          8    567    669   121
>         16      3     28
>         32     40    106     6
>         64     60     23
>        128     63     28
>        256     22     25     8
>        512      8     12     4
>       1024      8     32    40
>       2048      2     12    17
>       4096      5      1
>       8192     14      1   695
>      16384     18            1
>      32768     24
> 
> 
> I then compressed all files with gzip and reran:
> 
>         32   1
>         64 596
>        128 154
>        256  14
>        512  13
>       1024   4
>       2048  14
>       4096  18
>       8192  24
> 
>         32   1
>         64 784
>        128  74
>        256  38
>        512  41
>       1024   2
>       4096   1
>       8192   1
> 
> So, I'd just use 1024 as cut-off, that isn't bound to such dynamic limits like TCP
> actual payload per single package size, but will still fit in most and even captures
> most of the previously selected files in practice anyway. Also gzip is normally
> able to compress text quite well so doing that with log (i.e., not random data) text
> files bigger than 1KiB will almost never over the uncompressed limit. If you rather
> like a higher size you can also use 2 or 4 KiB, both fine too IMO.
> 

Sorry for the confusion, my train of thought here was that compression
would be most useful for connections over the internet. Of which the big
majority use an MTU size of 1500, as far as I understand.
Thank you for the inquisitive input here, I think 1024 might be the most
useful number to use for this then.

>> +	    my $use_compression = stat($filename)->size > 1500;
> 
> should be able to use: -s $filename > X
> 
>> +	    return PVE::Tools::stream_file($filename, $param->{upid}, $use_compression);
> 
> IMO the helper isn't to useful, I'd just do this inline.
> 

The stream_file helper?
It does save about 20 lines of very redundant code in both pmg and pve
each and should make it easy to implement potential other download
calls. Though, that hinges on the question on how likely it is that
there will be such a need.

Or are you talking about something else here?

>> +	} else {
> 
> no else required, other branch returns.
> 
>> +	    my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
>>  
>> -	$rpcenv->set_result_attrib('total', $count);
>> +	    $rpcenv->set_result_attrib('total', $count);
>>  
>> -	return $lines;
>> +	    return $lines;
>> +	}
>>      }});
>>  
>>  
> 

Re: [pve-devel] [PATCH manager v2 4/7] revised task log API call for PVE

[Thomas Lamprecht]

Am 10/10/2022 um 13:40 schrieb Daniel Tschlatscher:
> It does save about 20 lines of very redundant code in both pmg and pve
> each and should make it easy to implement potential other download
> calls. Though, that hinges on the question on how likely it is that
> there will be such a need.


20 lines is:
1) really not _that_ much
2) especially not dramatic as its just plain boilerplate info that won't
   change
3) only your version needs that much ;-P Can be easily cut down of 16 lines

    my $fh;
    if ($compress) {
        open($fh, '-|', "/usr/bin/gzip", "-c", "$file") or die "could not open file $file - $!";
    } else {
        open($fh, '<', $file) or die "could not open file $file- $!";
    }

    return {
        download => {
            fh => $fh,
            stream => 1,
            'content-encoding' => $compress ? 'gzip' : undef,
            'content-type' => $content_type // 'text/plain',
            'content-disposition' => 'attachment; filename="'. ($suggested_name // $file) .'"',
        },
    };



And independent of that, pve-common would be the wrong place for that helper, as it has no
control over how the http server takes the streaming hint, i.e., this is not a general
stream middleware but only prepare it in the specific format that our perl http server
expects it. So _iff_ it should go into pve-http-server, as otherwise any changes would need
and extra level of coordination on upgrade and possibly even make pve-common depend on
pve-http-server, introducing a circular dependency.