[pbs-devel] [PATCH proxmox-backup] fix #4646: Set BackupReader debug to current loglevel

[pbs-devel] [PATCH proxmox-backup] fix #4646: Set BackupReader debug to current loglevel

[Gabriel Goller]

Previously the BackupReader debug parameter was always hardcoded. Now we
check the current loglevel with the `log_enabled!()` macro and set debug
to true if we are on Trace or Debug. This allow the user to set the loglevel
using `PBS_LOG`.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 proxmox-backup-client/src/catalog.rs | 5 +++--
 proxmox-backup-client/src/main.rs    | 3 ++-
 proxmox-backup-client/src/mount.rs   | 3 ++-
 proxmox-file-restore/src/main.rs     | 3 ++-
 src/bin/proxmox_backup_debug/diff.rs | 3 ++-
 src/server/pull.rs                   | 3 ++-
 6 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/proxmox-backup-client/src/catalog.rs b/proxmox-backup-client/src/catalog.rs
index 8c8c1458..c9cf1b5c 100644
--- a/proxmox-backup-client/src/catalog.rs
+++ b/proxmox-backup-client/src/catalog.rs
@@ -3,6 +3,7 @@ use std::os::unix::fs::OpenOptionsExt;
 use std::sync::Arc;
 
 use anyhow::{bail, format_err, Error};
+use log::{log_enabled, Level};
 use serde_json::Value;
 
 use proxmox_router::cli::*;
@@ -80,7 +81,7 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
         repo.store(),
         &backup_ns,
         &snapshot,
-        true,
+        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
     )
     .await?;
 
@@ -192,7 +193,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
         repo.store(),
         &backup_ns,
         &backup_dir,
-        true,
+        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
     )
     .await?;
 
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index d9e7b899..f89823c2 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -7,6 +7,7 @@ use std::task::Context;
 
 use anyhow::{bail, format_err, Error};
 use futures::stream::{StreamExt, TryStreamExt};
+use log::{log_enabled, Level};
 use serde::Deserialize;
 use serde_json::{json, Value};
 use tokio::sync::mpsc;
@@ -1300,7 +1301,7 @@ async fn restore(
         repo.store(),
         &ns,
         &backup_dir,
-        true,
+        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
     )
     .await?;
 
diff --git a/proxmox-backup-client/src/mount.rs b/proxmox-backup-client/src/mount.rs
index 242556d0..aacd4362 100644
--- a/proxmox-backup-client/src/mount.rs
+++ b/proxmox-backup-client/src/mount.rs
@@ -9,6 +9,7 @@ use anyhow::{bail, format_err, Error};
 use futures::future::FutureExt;
 use futures::select;
 use futures::stream::{StreamExt, TryStreamExt};
+use log::{log_enabled, Level};
 use nix::unistd::{fork, ForkResult};
 use serde_json::Value;
 use tokio::signal::unix::{signal, SignalKind};
@@ -239,7 +240,7 @@ async fn mount_do(param: Value, pipe: Option<OwnedFd>) -> Result<Value, Error> {
         repo.store(),
         &backup_ns,
         &backup_dir,
-        true,
+        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
     )
     .await?;
 
diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
index 9c74a476..96c2a802 100644
--- a/proxmox-file-restore/src/main.rs
+++ b/proxmox-file-restore/src/main.rs
@@ -5,6 +5,7 @@ use std::sync::Arc;
 
 use anyhow::{bail, format_err, Error};
 use futures::StreamExt;
+use log::{log_enabled, Level};
 use serde_json::{json, Value};
 use tokio::io::AsyncWriteExt;
 
@@ -112,7 +113,7 @@ async fn list_files(
         repo.store(),
         &namespace,
         &snapshot,
-        true,
+        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
     )
     .await?;
 
diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
index 9924fb7b..c4cf70ba 100644
--- a/src/bin/proxmox_backup_debug/diff.rs
+++ b/src/bin/proxmox_backup_debug/diff.rs
@@ -9,6 +9,7 @@ use anyhow::{bail, Context as AnyhowContext, Error};
 use futures::future::BoxFuture;
 use futures::FutureExt;
 
+use log::{log_enabled, Level};
 use proxmox_human_byte::HumanByte;
 use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
 use proxmox_schema::api;
@@ -299,7 +300,7 @@ async fn create_backup_reader(
         params.repo.store(),
         &params.namespace,
         &backup_dir,
-        false,
+        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
     )
     .await?;
     Ok(backup_reader)
diff --git a/src/server/pull.rs b/src/server/pull.rs
index a973a10e..bbaa9681 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -8,6 +8,7 @@ use std::time::SystemTime;
 
 use anyhow::{bail, format_err, Error};
 use http::StatusCode;
+use log::{log_enabled, Level};
 use pbs_config::CachedUserInfo;
 use serde_json::json;
 
@@ -743,7 +744,7 @@ async fn pull_group(
             params.source.store(),
             &remote_ns,
             &snapshot,
-            true,
+            log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
         )
         .await?;
 
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] fix #4646: Set BackupReader debug to current loglevel

[Wolfgang Bumiller]

Oof. This controls the debug parameter for the remote side.

IMO it should be a separate parameter passed through from the CLI,
rather than the current log level.
I mean, AFAICT this controls task logs which you can view eg. via the
UI, and not the actual console output locally? Or do we display the
remote task output in all of the affected commands?
If I set a log level and don't see anything because it just happens
remotely, that would be weird :-)

On Mon, Aug 07, 2023 at 02:57:37PM +0200, Gabriel Goller wrote:
> Previously the BackupReader debug parameter was always hardcoded. Now we
> check the current loglevel with the `log_enabled!()` macro and set debug
> to true if we are on Trace or Debug. This allow the user to set the loglevel
> using `PBS_LOG`.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  proxmox-backup-client/src/catalog.rs | 5 +++--
>  proxmox-backup-client/src/main.rs    | 3 ++-
>  proxmox-backup-client/src/mount.rs   | 3 ++-
>  proxmox-file-restore/src/main.rs     | 3 ++-
>  src/bin/proxmox_backup_debug/diff.rs | 3 ++-
>  src/server/pull.rs                   | 3 ++-
>  6 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/proxmox-backup-client/src/catalog.rs b/proxmox-backup-client/src/catalog.rs
> index 8c8c1458..c9cf1b5c 100644
> --- a/proxmox-backup-client/src/catalog.rs
> +++ b/proxmox-backup-client/src/catalog.rs
> @@ -3,6 +3,7 @@ use std::os::unix::fs::OpenOptionsExt;
>  use std::sync::Arc;
>  
>  use anyhow::{bail, format_err, Error};
> +use log::{log_enabled, Level};
>  use serde_json::Value;
>  
>  use proxmox_router::cli::*;
> @@ -80,7 +81,7 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
>          repo.store(),
>          &backup_ns,
>          &snapshot,
> -        true,
> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>      )
>      .await?;
>  
> @@ -192,7 +193,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
>          repo.store(),
>          &backup_ns,
>          &backup_dir,
> -        true,
> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>      )
>      .await?;
>  
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index d9e7b899..f89823c2 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -7,6 +7,7 @@ use std::task::Context;
>  
>  use anyhow::{bail, format_err, Error};
>  use futures::stream::{StreamExt, TryStreamExt};
> +use log::{log_enabled, Level};
>  use serde::Deserialize;
>  use serde_json::{json, Value};
>  use tokio::sync::mpsc;
> @@ -1300,7 +1301,7 @@ async fn restore(
>          repo.store(),
>          &ns,
>          &backup_dir,
> -        true,
> +        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
>      )
>      .await?;
>  
> diff --git a/proxmox-backup-client/src/mount.rs b/proxmox-backup-client/src/mount.rs
> index 242556d0..aacd4362 100644
> --- a/proxmox-backup-client/src/mount.rs
> +++ b/proxmox-backup-client/src/mount.rs
> @@ -9,6 +9,7 @@ use anyhow::{bail, format_err, Error};
>  use futures::future::FutureExt;
>  use futures::select;
>  use futures::stream::{StreamExt, TryStreamExt};
> +use log::{log_enabled, Level};
>  use nix::unistd::{fork, ForkResult};
>  use serde_json::Value;
>  use tokio::signal::unix::{signal, SignalKind};
> @@ -239,7 +240,7 @@ async fn mount_do(param: Value, pipe: Option<OwnedFd>) -> Result<Value, Error> {
>          repo.store(),
>          &backup_ns,
>          &backup_dir,
> -        true,
> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>      )
>      .await?;
>  
> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
> index 9c74a476..96c2a802 100644
> --- a/proxmox-file-restore/src/main.rs
> +++ b/proxmox-file-restore/src/main.rs
> @@ -5,6 +5,7 @@ use std::sync::Arc;
>  
>  use anyhow::{bail, format_err, Error};
>  use futures::StreamExt;
> +use log::{log_enabled, Level};
>  use serde_json::{json, Value};
>  use tokio::io::AsyncWriteExt;
>  
> @@ -112,7 +113,7 @@ async fn list_files(
>          repo.store(),
>          &namespace,
>          &snapshot,
> -        true,
> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>      )
>      .await?;
>  
> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
> index 9924fb7b..c4cf70ba 100644
> --- a/src/bin/proxmox_backup_debug/diff.rs
> +++ b/src/bin/proxmox_backup_debug/diff.rs
> @@ -9,6 +9,7 @@ use anyhow::{bail, Context as AnyhowContext, Error};
>  use futures::future::BoxFuture;
>  use futures::FutureExt;
>  
> +use log::{log_enabled, Level};
>  use proxmox_human_byte::HumanByte;
>  use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
>  use proxmox_schema::api;
> @@ -299,7 +300,7 @@ async fn create_backup_reader(
>          params.repo.store(),
>          &params.namespace,
>          &backup_dir,
> -        false,
> +        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
>      )
>      .await?;
>      Ok(backup_reader)
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index a973a10e..bbaa9681 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -8,6 +8,7 @@ use std::time::SystemTime;
>  
>  use anyhow::{bail, format_err, Error};
>  use http::StatusCode;
> +use log::{log_enabled, Level};
>  use pbs_config::CachedUserInfo;
>  use serde_json::json;
>  
> @@ -743,7 +744,7 @@ async fn pull_group(
>              params.source.store(),
>              &remote_ns,
>              &snapshot,
> -            true,
> +            log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>          )
>          .await?;
>  
> -- 
> 2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] fix #4646: Set BackupReader debug to current loglevel

[Gabriel Goller]

Yes, this controls the log level on the remote side, which is a bit 
weird (as discussed here [0]).
The CLI output locally won't change (we use `env_logger` there and get 
the loglevel from `PBS_LOG`).

[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=4646

On 8/8/23 12:38, Wolfgang Bumiller wrote:
> Oof. This controls the debug parameter for the remote side.
>
> IMO it should be a separate parameter passed through from the CLI,
> rather than the current log level.
> I mean, AFAICT this controls task logs which you can view eg. via the
> UI, and not the actual console output locally? Or do we display the
> remote task output in all of the affected commands?
> If I set a log level and don't see anything because it just happens
> remotely, that would be weird :-)
>
> On Mon, Aug 07, 2023 at 02:57:37PM +0200, Gabriel Goller wrote:
>> Previously the BackupReader debug parameter was always hardcoded. Now we
>> check the current loglevel with the `log_enabled!()` macro and set debug
>> to true if we are on Trace or Debug. This allow the user to set the loglevel
>> using `PBS_LOG`.
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>   proxmox-backup-client/src/catalog.rs | 5 +++--
>>   proxmox-backup-client/src/main.rs    | 3 ++-
>>   proxmox-backup-client/src/mount.rs   | 3 ++-
>>   proxmox-file-restore/src/main.rs     | 3 ++-
>>   src/bin/proxmox_backup_debug/diff.rs | 3 ++-
>>   src/server/pull.rs                   | 3 ++-
>>   6 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/proxmox-backup-client/src/catalog.rs b/proxmox-backup-client/src/catalog.rs
>> index 8c8c1458..c9cf1b5c 100644
>> --- a/proxmox-backup-client/src/catalog.rs
>> +++ b/proxmox-backup-client/src/catalog.rs
>> @@ -3,6 +3,7 @@ use std::os::unix::fs::OpenOptionsExt;
>>   use std::sync::Arc;
>>   
>>   use anyhow::{bail, format_err, Error};
>> +use log::{log_enabled, Level};
>>   use serde_json::Value;
>>   
>>   use proxmox_router::cli::*;
>> @@ -80,7 +81,7 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
>>           repo.store(),
>>           &backup_ns,
>>           &snapshot,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> @@ -192,7 +193,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
>>           repo.store(),
>>           &backup_ns,
>>           &backup_dir,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
>> index d9e7b899..f89823c2 100644
>> --- a/proxmox-backup-client/src/main.rs
>> +++ b/proxmox-backup-client/src/main.rs
>> @@ -7,6 +7,7 @@ use std::task::Context;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use futures::stream::{StreamExt, TryStreamExt};
>> +use log::{log_enabled, Level};
>>   use serde::Deserialize;
>>   use serde_json::{json, Value};
>>   use tokio::sync::mpsc;
>> @@ -1300,7 +1301,7 @@ async fn restore(
>>           repo.store(),
>>           &ns,
>>           &backup_dir,
>> -        true,
>> +        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
>>       )
>>       .await?;
>>   
>> diff --git a/proxmox-backup-client/src/mount.rs b/proxmox-backup-client/src/mount.rs
>> index 242556d0..aacd4362 100644
>> --- a/proxmox-backup-client/src/mount.rs
>> +++ b/proxmox-backup-client/src/mount.rs
>> @@ -9,6 +9,7 @@ use anyhow::{bail, format_err, Error};
>>   use futures::future::FutureExt;
>>   use futures::select;
>>   use futures::stream::{StreamExt, TryStreamExt};
>> +use log::{log_enabled, Level};
>>   use nix::unistd::{fork, ForkResult};
>>   use serde_json::Value;
>>   use tokio::signal::unix::{signal, SignalKind};
>> @@ -239,7 +240,7 @@ async fn mount_do(param: Value, pipe: Option<OwnedFd>) -> Result<Value, Error> {
>>           repo.store(),
>>           &backup_ns,
>>           &backup_dir,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
>> index 9c74a476..96c2a802 100644
>> --- a/proxmox-file-restore/src/main.rs
>> +++ b/proxmox-file-restore/src/main.rs
>> @@ -5,6 +5,7 @@ use std::sync::Arc;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use futures::StreamExt;
>> +use log::{log_enabled, Level};
>>   use serde_json::{json, Value};
>>   use tokio::io::AsyncWriteExt;
>>   
>> @@ -112,7 +113,7 @@ async fn list_files(
>>           repo.store(),
>>           &namespace,
>>           &snapshot,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
>> index 9924fb7b..c4cf70ba 100644
>> --- a/src/bin/proxmox_backup_debug/diff.rs
>> +++ b/src/bin/proxmox_backup_debug/diff.rs
>> @@ -9,6 +9,7 @@ use anyhow::{bail, Context as AnyhowContext, Error};
>>   use futures::future::BoxFuture;
>>   use futures::FutureExt;
>>   
>> +use log::{log_enabled, Level};
>>   use proxmox_human_byte::HumanByte;
>>   use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
>>   use proxmox_schema::api;
>> @@ -299,7 +300,7 @@ async fn create_backup_reader(
>>           params.repo.store(),
>>           &params.namespace,
>>           &backup_dir,
>> -        false,
>> +        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
>>       )
>>       .await?;
>>       Ok(backup_reader)
>> diff --git a/src/server/pull.rs b/src/server/pull.rs
>> index a973a10e..bbaa9681 100644
>> --- a/src/server/pull.rs
>> +++ b/src/server/pull.rs
>> @@ -8,6 +8,7 @@ use std::time::SystemTime;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use http::StatusCode;
>> +use log::{log_enabled, Level};
>>   use pbs_config::CachedUserInfo;
>>   use serde_json::json;
>>   
>> @@ -743,7 +744,7 @@ async fn pull_group(
>>               params.source.store(),
>>               &remote_ns,
>>               &snapshot,
>> -            true,
>> +            log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>           )
>>           .await?;
>>   
>> -- 
>> 2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] fix #4646: Set BackupReader debug to current loglevel

[Gabriel Goller]

On a second note, how about we also introduce more fine-grained logging 
settings to the api/proxy?
Currently on the api we hardcode the `log::max_level` to 
`LevelFilter::Info` and on the proxy we check
if the environment variable `PROXMOX_DEBUG` is set and use 
`LevelFilter::Debug` if it exists. We could
do the same as on the cli and allow `PROXMOX_DEBUG` (probably renaming 
it to `PROXMOX_LOG_LEVEL`
or something) to be set to `trace`, `debug`, `info`, `warning` or `error`.
Then we could also decide if we want to use the server `log::max_level` 
or the client-side `debug` flag in
the `download_file` and `download_chunk` functions (where the original 
issue #4646 comes from).

On 8/8/23 12:38, Wolfgang Bumiller wrote:
> Oof. This controls the debug parameter for the remote side.
>
> IMO it should be a separate parameter passed through from the CLI,
> rather than the current log level.
> I mean, AFAICT this controls task logs which you can view eg. via the
> UI, and not the actual console output locally? Or do we display the
> remote task output in all of the affected commands?
> If I set a log level and don't see anything because it just happens
> remotely, that would be weird :-)
>
> On Mon, Aug 07, 2023 at 02:57:37PM +0200, Gabriel Goller wrote:
>> Previously the BackupReader debug parameter was always hardcoded. Now we
>> check the current loglevel with the `log_enabled!()` macro and set debug
>> to true if we are on Trace or Debug. This allow the user to set the loglevel
>> using `PBS_LOG`.
>>
>> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
>> ---
>>   proxmox-backup-client/src/catalog.rs | 5 +++--
>>   proxmox-backup-client/src/main.rs    | 3 ++-
>>   proxmox-backup-client/src/mount.rs   | 3 ++-
>>   proxmox-file-restore/src/main.rs     | 3 ++-
>>   src/bin/proxmox_backup_debug/diff.rs | 3 ++-
>>   src/server/pull.rs                   | 3 ++-
>>   6 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/proxmox-backup-client/src/catalog.rs b/proxmox-backup-client/src/catalog.rs
>> index 8c8c1458..c9cf1b5c 100644
>> --- a/proxmox-backup-client/src/catalog.rs
>> +++ b/proxmox-backup-client/src/catalog.rs
>> @@ -3,6 +3,7 @@ use std::os::unix::fs::OpenOptionsExt;
>>   use std::sync::Arc;
>>   
>>   use anyhow::{bail, format_err, Error};
>> +use log::{log_enabled, Level};
>>   use serde_json::Value;
>>   
>>   use proxmox_router::cli::*;
>> @@ -80,7 +81,7 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
>>           repo.store(),
>>           &backup_ns,
>>           &snapshot,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> @@ -192,7 +193,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
>>           repo.store(),
>>           &backup_ns,
>>           &backup_dir,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
>> index d9e7b899..f89823c2 100644
>> --- a/proxmox-backup-client/src/main.rs
>> +++ b/proxmox-backup-client/src/main.rs
>> @@ -7,6 +7,7 @@ use std::task::Context;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use futures::stream::{StreamExt, TryStreamExt};
>> +use log::{log_enabled, Level};
>>   use serde::Deserialize;
>>   use serde_json::{json, Value};
>>   use tokio::sync::mpsc;
>> @@ -1300,7 +1301,7 @@ async fn restore(
>>           repo.store(),
>>           &ns,
>>           &backup_dir,
>> -        true,
>> +        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
>>       )
>>       .await?;
>>   
>> diff --git a/proxmox-backup-client/src/mount.rs b/proxmox-backup-client/src/mount.rs
>> index 242556d0..aacd4362 100644
>> --- a/proxmox-backup-client/src/mount.rs
>> +++ b/proxmox-backup-client/src/mount.rs
>> @@ -9,6 +9,7 @@ use anyhow::{bail, format_err, Error};
>>   use futures::future::FutureExt;
>>   use futures::select;
>>   use futures::stream::{StreamExt, TryStreamExt};
>> +use log::{log_enabled, Level};
>>   use nix::unistd::{fork, ForkResult};
>>   use serde_json::Value;
>>   use tokio::signal::unix::{signal, SignalKind};
>> @@ -239,7 +240,7 @@ async fn mount_do(param: Value, pipe: Option<OwnedFd>) -> Result<Value, Error> {
>>           repo.store(),
>>           &backup_ns,
>>           &backup_dir,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
>> index 9c74a476..96c2a802 100644
>> --- a/proxmox-file-restore/src/main.rs
>> +++ b/proxmox-file-restore/src/main.rs
>> @@ -5,6 +5,7 @@ use std::sync::Arc;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use futures::StreamExt;
>> +use log::{log_enabled, Level};
>>   use serde_json::{json, Value};
>>   use tokio::io::AsyncWriteExt;
>>   
>> @@ -112,7 +113,7 @@ async fn list_files(
>>           repo.store(),
>>           &namespace,
>>           &snapshot,
>> -        true,
>> +        log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>       )
>>       .await?;
>>   
>> diff --git a/src/bin/proxmox_backup_debug/diff.rs b/src/bin/proxmox_backup_debug/diff.rs
>> index 9924fb7b..c4cf70ba 100644
>> --- a/src/bin/proxmox_backup_debug/diff.rs
>> +++ b/src/bin/proxmox_backup_debug/diff.rs
>> @@ -9,6 +9,7 @@ use anyhow::{bail, Context as AnyhowContext, Error};
>>   use futures::future::BoxFuture;
>>   use futures::FutureExt;
>>   
>> +use log::{log_enabled, Level};
>>   use proxmox_human_byte::HumanByte;
>>   use proxmox_router::cli::{CliCommand, CliCommandMap, CommandLineInterface};
>>   use proxmox_schema::api;
>> @@ -299,7 +300,7 @@ async fn create_backup_reader(
>>           params.repo.store(),
>>           &params.namespace,
>>           &backup_dir,
>> -        false,
>> +        log_enabled!(Level::Trace) || log_enabled!(Level::Debug),
>>       )
>>       .await?;
>>       Ok(backup_reader)
>> diff --git a/src/server/pull.rs b/src/server/pull.rs
>> index a973a10e..bbaa9681 100644
>> --- a/src/server/pull.rs
>> +++ b/src/server/pull.rs
>> @@ -8,6 +8,7 @@ use std::time::SystemTime;
>>   
>>   use anyhow::{bail, format_err, Error};
>>   use http::StatusCode;
>> +use log::{log_enabled, Level};
>>   use pbs_config::CachedUserInfo;
>>   use serde_json::json;
>>   
>> @@ -743,7 +744,7 @@ async fn pull_group(
>>               params.source.store(),
>>               &remote_ns,
>>               &snapshot,
>> -            true,
>> +            log_enabled!(Level::Debug) || log_enabled!(Level::Trace),
>>           )
>>           .await?;
>>   
>> -- 
>> 2.39.2

Re: [pbs-devel] [PATCH proxmox-backup] fix #4646: Set BackupReader debug to current loglevel

[Wolfgang Bumiller]

On Fri, Aug 11, 2023 at 02:53:17PM +0200, Gabriel Goller wrote:
> On a second note, how about we also introduce more fine-grained logging
> settings to the api/proxy?
> Currently on the api we hardcode the `log::max_level` to `LevelFilter::Info`
> and on the proxy we check
> if the environment variable `PROXMOX_DEBUG` is set and use
> `LevelFilter::Debug` if it exists. We could
> do the same as on the cli and allow `PROXMOX_DEBUG` (probably renaming it to
> `PROXMOX_LOG_LEVEL`
> or something) to be set to `trace`, `debug`, `info`, `warning` or `error`.
> Then we could also decide if we want to use the server `log::max_level` or
> the client-side `debug` flag in
> the `download_file` and `download_chunk` functions (where the original issue
> #4646 comes from).

A while ago we Hannes added `init_cli_logger` to proxmox-router which
the CLI binaries call with the `$PBS_LOG` env var.
Having the same mechanism for the daemons' log levels would probably
make sense. We should probably also call it PBS_LOG then.
People could probably temporarily change it via `systemctl
set-environment` or more permanently via an `Environment=` systemd
service snippet file.

Re: [pbs-devel] [PATCH proxmox-backup] fix #4646: Set BackupReader debug to current loglevel

[Gabriel Goller]

Submitted a patch with the proposed change.

On 8/17/23 14:42, Wolfgang Bumiller wrote:
> On Fri, Aug 11, 2023 at 02:53:17PM +0200, Gabriel Goller wrote:
>> On a second note, how about we also introduce more fine-grained logging
>> settings to the api/proxy?
>> Currently on the api we hardcode the `log::max_level` to `LevelFilter::Info`
>> and on the proxy we check
>> if the environment variable `PROXMOX_DEBUG` is set and use
>> `LevelFilter::Debug` if it exists. We could
>> do the same as on the cli and allow `PROXMOX_DEBUG` (probably renaming it to
>> `PROXMOX_LOG_LEVEL`
>> or something) to be set to `trace`, `debug`, `info`, `warning` or `error`.
>> Then we could also decide if we want to use the server `log::max_level` or
>> the client-side `debug` flag in
>> the `download_file` and `download_chunk` functions (where the original issue
>> #4646 comes from).
> A while ago we Hannes added `init_cli_logger` to proxmox-router which
> the CLI binaries call with the `$PBS_LOG` env var.
> Having the same mechanism for the daemons' log levels would probably
> make sense. We should probably also call it PBS_LOG then.
> People could probably temporarily change it via `systemctl
> set-environment` or more permanently via an `Environment=` systemd
> service snippet file.