[pbs-devel] [PATCH widget-manager/backup/pmg v3] fix: #3971 Tasklog download button

[pbs-devel] [PATCH widget-manager/backup/pmg v3] 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 the tasklog more easily.
(The tasklog-viewer only displays 50 lines at a time)
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)

This patch series revision did not see any changes to the two patches
to be applied in the proxmox-widget-toolkit. Therefore, they are
omitted here. To make this series fully functional, they have to be
applied as well.
However, the widget-toolkit frontend implementation needs all backend
patches to be working properly.


changes from v2
* replaced helper function in pve-commong with inline implementations
  in PMG and PVE backends
* improved API documentation for limit parameter
* minor refactorings


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

 src/api2/node/tasks.rs | 161 ++++++++++++++++++++++++++---------------
 1 file changed, 103 insertions(+), 58 deletions(-)

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

 src/PMG/API2/Tasks.pm | 44 ++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

Daniel Tschlatscher (1):
  make task log downloadable in the PVE manager backend

 PVE/API2/Tasks.pm | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

-- 
2.30.2

[pbs-devel] [PATCH backup v3] make tasklog downloadable in the backup server backend

[Daniel Tschlatscher]

The read tasklog API call now streams the log file if the limit
parameter is set to 0. If not, it should behave the same as before.
This is done in preparation for the task log download button to be
added in the TaskViewer.

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.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---

changes from v2
* improved the documentation for the limit parameter
* changed the api method name to "read_task_log"

 src/api2/node/tasks.rs | 161 ++++++++++++++++++++++++++---------------
 1 file changed, 103 insertions(+), 58 deletions(-)

diff --git a/src/api2/node/tasks.rs b/src/api2/node/tasks.rs
index cbd0894e..bc8e3ab5 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,22 @@ 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)
+        .default(0)
+        .schema();
+
+pub const LIMIT_PARAM_SCHEMA: Schema =
+    IntegerSchema::new("The amount of lines to read from the tasklog. Setting this parameter to 0 will stream the file directly")
+        .minimum(0)
+        .default(50)
+        .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 +290,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(&read_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 read_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_task_log_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_task_log_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 +396,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

[pbs-devel] [PATCH pmg-api v3] make tasklog downloadable in the PMG backend

[Daniel Tschlatscher]

The read tasklog API call now streams the log file if the limit
parameter is set to 0. If not, it should behave the same as before.
This is done in preparation for the task log download button to be
added in the TaskViewer.

I saw an opportunity here to clear some redundant code for displaying
the tasklog and replaced it with a call to dump_logfile(), akin to how
this is handled in pve-manager.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---

changes from v2
* replaced helper call with inline implementation
* changed compression cutoff to 1024
* replaced stat($filename)→size call with "-s"

 src/PMG/API2/Tasks.pm | 43 ++++++++++++++++++++++++-------------------
 1 file changed, 24 insertions(+), 19 deletions(-)

diff --git a/src/PMG/API2/Tasks.pm b/src/PMG/API2/Tasks.pm
index 598fb4d..da53f48 100644
--- a/src/PMG/API2/Tasks.pm
+++ b/src/PMG/API2/Tasks.pm
@@ -243,6 +243,7 @@ __PACKAGE__->register_method({
 		type => 'integer',
 		minimum => 0,
 		optional => 1,
+		description => "The maximum amount of lines that should be printed. A limit of 0 streams the file directly, otherwise lines are json encoded.",
 	    },
 	},
     },
@@ -272,30 +273,34 @@ __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) {
+	    # 1024 is a practical cutoff for the size distribution of our log files.
+	    my $use_compression = ( -s $filename ) > 1024;
 
-	close($fh);
+	    my $fh;
+	    if ($use_compression) {
+		open($fh, "-|", "/usr/bin/gzip", "-c", "$filename")
+		    or die "Could not create compressed file stream for $filename - $!";
+	    } else {
+		open($fh, '<', $filename) or die "Could not open file $filename - $!";
+	    }
 
-	# 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 {
+		download => {
+		    fh => $fh,
+		    stream => 1,
+		    'content-encoding' => $use_compression ? 'gzip' : undef,
+		    'content-type' => "text/plain",
+		    'content-disposition' => "attachment; filename=\"".$param->{upid}."\"",
+		},
+	    },
 	}
 
+	my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
+
 	$restenv->set_result_attrib('total', $count);
 
 	return $lines;
-- 
2.30.2

[pbs-devel] [PATCH manager v3] make task log downloadable in the PVE manager backend

[Daniel Tschlatscher]

The read tasklog API call now streams the log file if the limit
parameter is set to 0. If not, it should behave the same as before.
This is done in preparation for the task log download button to be
added in the TaskViewer.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---

changes from v2
* replaced helper call with inline implementation
* changed compression cutoff to 1024
* replaced stat($filename)→size call with "-s"

 PVE/API2/Tasks.pm | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Tasks.pm b/PVE/API2/Tasks.pm
index 9cd1e56b..3bc00704 100644
--- a/PVE/API2/Tasks.pm
+++ b/PVE/API2/Tasks.pm
@@ -349,7 +349,7 @@ __PACKAGE__->register_method({
 		minimum => 0,
 		default => 50,
 		optional => 1,
-		description => "The maximum amount of lines that should be printed.",
+		description => "The maximum amount of lines that should be printed. A limit of 0 streams the file directly, otherwise lines are json encoded.",
 	    },
 	},
     },
@@ -387,6 +387,29 @@ __PACKAGE__->register_method({
 	    $rpcenv->check($user, "/nodes/$node", [ 'Sys.Audit' ]);
 	}
 
+	if ($limit == 0) {
+	    # 1024 is a practical cutoff for the size distribution of our log files.
+	    my $use_compression = ( -s $filename ) > 1024;
+
+	    my $fh;
+	    if ($use_compression) {
+		open($fh, "-|", "/usr/bin/gzip", "-c", "$filename")
+		    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' => $use_compression ? 'gzip' : undef,
+		    'content-type' => "text/plain",
+		    'content-disposition' => "attachment; filename=\"".$param->{upid}."\"",
+		},
+	    },
+	}
+
 	my ($count, $lines) = PVE::Tools::dump_logfile($filename, $start, $limit);
 
 	$rpcenv->set_result_attrib('total', $count);
-- 
2.30.2