public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH proxmox v2] worker_task: write result message manually, bypassing tracing
@ 2025-02-18 14:52 Gabriel Goller
  2025-02-18 14:57 ` Gabriel Goller
  2025-02-18 15:05 ` Wolfgang Bumiller
  0 siblings, 2 replies; 4+ messages in thread
From: Gabriel Goller @ 2025-02-18 14:52 UTC (permalink / raw)
  To: pve-devel

The workertasks currently get their status from parsing the log
messages in the task-log file. The problem is that if these messages are
filtered – which is now possible using the PBS_LOG env variable – some
workertasks will end up with a "stopped: unknown" status. This is not
desirable so write the message manually to the workertask file and
bypass tracing.

This way we are guaranteed that, regardless of the max logging level the
user sets, the final message (and status) is written.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

Changelog:
v2:
 * remove jank (detect workertask failure based on return value instead
   of message)

 proxmox-log/src/lib.rs                 | 17 +++++++++++++++++
 proxmox-rest-server/src/worker_task.rs | 16 +++++++++++++---
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
index 8c74e42b618d..755d1b4a850c 100644
--- a/proxmox-log/src/lib.rs
+++ b/proxmox-log/src/lib.rs
@@ -191,3 +191,20 @@ pub fn init_cli_logger(
     LogTracer::init_with_filter(log_level.as_log())?;
     Ok(())
 }
+
+/// Write manually to the current tasklog bypassing the whole tracing infrastructure. Note that this
+/// will also bypass all the filtering and writing to journald or elsewhere. If has_failed is true,
+/// print to stderr as well.
+pub fn log_manually_to_tasklog(msg: &str, has_failed: bool) -> Result<(), anyhow::Error> {
+    if let Some(ctx) = LogContext::current() {
+        let mut logger = ctx.logger.lock().unwrap();
+        // If the message is an error, print to stderr (which will end up in journald) as well.
+        if has_failed {
+            eprintln!("{msg}");
+        }
+        logger.logger.log(msg);
+        Ok(())
+    } else {
+        anyhow::bail!("Unable to manually write message to workertask log file: No workertask exists in this task.");
+    }
+}
diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
index 24e2676e69c7..06d986f41dbb 100644
--- a/proxmox-rest-server/src/worker_task.rs
+++ b/proxmox-rest-server/src/worker_task.rs
@@ -7,14 +7,14 @@ use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
 use std::sync::{Arc, LazyLock, Mutex, OnceLock};
 use std::time::{Duration, SystemTime};
 
-use anyhow::{bail, format_err, Error};
+use anyhow::{bail, format_err, Context, Error};
 use futures::*;
 use nix::fcntl::OFlag;
 use serde::{Deserialize, Serialize};
 use serde_json::{json, Value};
 use tokio::signal::unix::SignalKind;
 use tokio::sync::{oneshot, watch};
-use tracing::{info, warn};
+use tracing::{error, info, warn};
 
 use proxmox_daemon::command_socket::CommandSocket;
 use proxmox_lang::try_block;
@@ -1025,7 +1025,17 @@ impl WorkerTask {
     /// Log task result, remove task from running list
     pub fn log_result(&self, result: &Result<(), Error>) {
         let state = self.create_state(result);
-        self.log_message(state.result_text());
+
+        // Write the result manually to the workertask file. We don't want to filter or process
+        // this message by the logging system. This also guarantees the result message will be in
+        // the file, regardless of the logging level.
+        proxmox_log::log_manually_to_tasklog(&state.result_text(), result.is_err())
+            .context("Error writing final result message")
+            .unwrap_or_else(|err| {
+                // Note: this will probably also fail to be written to the tasklog, but at least it
+                // ends up in the journal.
+                error!("{err:#}");
+            });
 
         WORKER_TASK_LIST.lock().unwrap().remove(&self.upid.task_id);
         // this wants to access WORKER_TASK_LIST, so we need to drop the lock above
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH proxmox v2] worker_task: write result message manually, bypassing tracing
  2025-02-18 14:52 [pve-devel] [PATCH proxmox v2] worker_task: write result message manually, bypassing tracing Gabriel Goller
@ 2025-02-18 14:57 ` Gabriel Goller
  2025-02-18 15:05 ` Wolfgang Bumiller
  1 sibling, 0 replies; 4+ messages in thread
From: Gabriel Goller @ 2025-02-18 14:57 UTC (permalink / raw)
  To: pve-devel

Wrong mailing list, please ignore :(


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH proxmox v2] worker_task: write result message manually, bypassing tracing
  2025-02-18 14:52 [pve-devel] [PATCH proxmox v2] worker_task: write result message manually, bypassing tracing Gabriel Goller
  2025-02-18 14:57 ` Gabriel Goller
@ 2025-02-18 15:05 ` Wolfgang Bumiller
  2025-02-18 16:36   ` Gabriel Goller
  1 sibling, 1 reply; 4+ messages in thread
From: Wolfgang Bumiller @ 2025-02-18 15:05 UTC (permalink / raw)
  To: Gabriel Goller; +Cc: pve-devel

On Tue, Feb 18, 2025 at 03:52:26PM +0100, Gabriel Goller wrote:
> The workertasks currently get their status from parsing the log
> messages in the task-log file. The problem is that if these messages are
> filtered – which is now possible using the PBS_LOG env variable – some
> workertasks will end up with a "stopped: unknown" status. This is not
> desirable so write the message manually to the workertask file and
> bypass tracing.
> 
> This way we are guaranteed that, regardless of the max logging level the
> user sets, the final message (and status) is written.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> Changelog:
> v2:
>  * remove jank (detect workertask failure based on return value instead
>    of message)
> 
>  proxmox-log/src/lib.rs                 | 17 +++++++++++++++++
>  proxmox-rest-server/src/worker_task.rs | 16 +++++++++++++---
>  2 files changed, 30 insertions(+), 3 deletions(-)
> 
> diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
> index 8c74e42b618d..755d1b4a850c 100644
> --- a/proxmox-log/src/lib.rs
> +++ b/proxmox-log/src/lib.rs
> @@ -191,3 +191,20 @@ pub fn init_cli_logger(
>      LogTracer::init_with_filter(log_level.as_log())?;
>      Ok(())
>  }
> +
> +/// Write manually to the current tasklog bypassing the whole tracing infrastructure. Note that this
> +/// will also bypass all the filtering and writing to journald or elsewhere. If has_failed is true,
> +/// print to stderr as well.
> +pub fn log_manually_to_tasklog(msg: &str, has_failed: bool) -> Result<(), anyhow::Error> {

What's "manual" about using a provided helper function? :-P
Besides, the name kind of conflicts with the stderr write, which seems
even more specific to the rest-server case.

Maybe a `LogContext::log_unfiltered(&str)` and rest-server just calls
this+eprintln!()? Then the `error!()` invocation could be in a `None`
match arm on the `LogContext::current()` match which IMO makes for much
nicer control flow.

Do we even anticipate any other use case than the one in rest-server?

> +    if let Some(ctx) = LogContext::current() {
> +        let mut logger = ctx.logger.lock().unwrap();
> +        // If the message is an error, print to stderr (which will end up in journald) as well.
> +        if has_failed {
> +            eprintln!("{msg}");
> +        }
> +        logger.logger.log(msg);
> +        Ok(())
> +    } else {
> +        anyhow::bail!("Unable to manually write message to workertask log file: No workertask exists in this task.");
> +    }
> +}
> diff --git a/proxmox-rest-server/src/worker_task.rs b/proxmox-rest-server/src/worker_task.rs
> index 24e2676e69c7..06d986f41dbb 100644
> --- a/proxmox-rest-server/src/worker_task.rs
> +++ b/proxmox-rest-server/src/worker_task.rs
> @@ -7,14 +7,14 @@ use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering};
>  use std::sync::{Arc, LazyLock, Mutex, OnceLock};
>  use std::time::{Duration, SystemTime};
>  
> -use anyhow::{bail, format_err, Error};
> +use anyhow::{bail, format_err, Context, Error};
>  use futures::*;
>  use nix::fcntl::OFlag;
>  use serde::{Deserialize, Serialize};
>  use serde_json::{json, Value};
>  use tokio::signal::unix::SignalKind;
>  use tokio::sync::{oneshot, watch};
> -use tracing::{info, warn};
> +use tracing::{error, info, warn};
>  
>  use proxmox_daemon::command_socket::CommandSocket;
>  use proxmox_lang::try_block;
> @@ -1025,7 +1025,17 @@ impl WorkerTask {
>      /// Log task result, remove task from running list
>      pub fn log_result(&self, result: &Result<(), Error>) {
>          let state = self.create_state(result);
> -        self.log_message(state.result_text());
> +
> +        // Write the result manually to the workertask file. We don't want to filter or process
> +        // this message by the logging system. This also guarantees the result message will be in
> +        // the file, regardless of the logging level.
> +        proxmox_log::log_manually_to_tasklog(&state.result_text(), result.is_err())
> +            .context("Error writing final result message")
> +            .unwrap_or_else(|err| {
> +                // Note: this will probably also fail to be written to the tasklog, but at least it
> +                // ends up in the journal.
> +                error!("{err:#}");
> +            });
>  
>          WORKER_TASK_LIST.lock().unwrap().remove(&self.upid.task_id);
>          // this wants to access WORKER_TASK_LIST, so we need to drop the lock above
> -- 
> 2.39.5
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [pve-devel] [PATCH proxmox v2] worker_task: write result message manually, bypassing tracing
  2025-02-18 15:05 ` Wolfgang Bumiller
@ 2025-02-18 16:36   ` Gabriel Goller
  0 siblings, 0 replies; 4+ messages in thread
From: Gabriel Goller @ 2025-02-18 16:36 UTC (permalink / raw)
  To: Wolfgang Bumiller; +Cc: pve-devel

On 18.02.2025 16:05, Wolfgang Bumiller wrote:
>On Tue, Feb 18, 2025 at 03:52:26PM +0100, Gabriel Goller wrote:
>> diff --git a/proxmox-log/src/lib.rs b/proxmox-log/src/lib.rs
>> index 8c74e42b618d..755d1b4a850c 100644
>> --- a/proxmox-log/src/lib.rs
>> +++ b/proxmox-log/src/lib.rs
>> @@ -191,3 +191,20 @@ pub fn init_cli_logger(
>>      LogTracer::init_with_filter(log_level.as_log())?;
>>      Ok(())
>>  }
>> +
>> +/// Write manually to the current tasklog bypassing the whole tracing infrastructure. Note that this
>> +/// will also bypass all the filtering and writing to journald or elsewhere. If has_failed is true,
>> +/// print to stderr as well.
>> +pub fn log_manually_to_tasklog(msg: &str, has_failed: bool) -> Result<(), anyhow::Error> {
>
>What's "manual" about using a provided helper function? :-P

I thought of "manual" = "without tracing".

>Besides, the name kind of conflicts with the stderr write, which seems
>even more specific to the rest-server case.

True.

>Maybe a `LogContext::log_unfiltered(&str)` and rest-server just calls
>this+eprintln!()? Then the `error!()` invocation could be in a `None`
>match arm on the `LogContext::current()` match which IMO makes for much
>nicer control flow.

Damn, that's neat!

>Do we even anticipate any other use case than the one in rest-server?

Nope, not really, just this one.

Will submit a new patch with the changes soon!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2025-02-18 16:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2025-02-18 14:52 [pve-devel] [PATCH proxmox v2] worker_task: write result message manually, bypassing tracing Gabriel Goller
2025-02-18 14:57 ` Gabriel Goller
2025-02-18 15:05 ` Wolfgang Bumiller
2025-02-18 16:36   ` Gabriel Goller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal