[pbs-devel] [PATCH proxmox-backup v1 1/1] close #3770: pbs-client: add quiet backup flag

[pbs-devel] [PATCH proxmox-backup v1 1/1] close #3770: pbs-client: add quiet backup flag

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
this feels 'chunky', having quiet and verbose(which is sometimes called
debug). Maybe some kind general verbosity enum might make sense, but then
it should not just be used here.

 pbs-client/src/backup_writer.rs        | 12 +++--
 proxmox-backup-client/src/benchmark.rs |  1 +
 proxmox-backup-client/src/main.rs      | 62 ++++++++++++++++----------
 3 files changed, 47 insertions(+), 28 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index b02798bd..1f53f52b 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -29,6 +29,7 @@ pub struct BackupWriter {
     h2: H2Client,
     abort: AbortHandle,
     verbose: bool,
+    quiet: bool,
     crypt_config: Option<Arc<CryptConfig>>,
 }
 
@@ -71,12 +72,14 @@ impl BackupWriter {
         abort: AbortHandle,
         crypt_config: Option<Arc<CryptConfig>>,
         verbose: bool,
+        quiet: bool,
     ) -> Arc<Self> {
         Arc::new(Self {
             h2,
             abort,
             crypt_config,
             verbose,
+            quiet,
         })
     }
 
@@ -90,6 +93,7 @@ impl BackupWriter {
         backup_id: &str,
         backup_time: i64,
         debug: bool,
+        quiet: bool,
         benchmark: bool,
     ) -> Result<Arc<BackupWriter>, Error> {
         let param = json!({
@@ -114,7 +118,7 @@ impl BackupWriter {
             .start_h2_connection(req, String::from(PROXMOX_BACKUP_PROTOCOL_ID_V1!()))
             .await?;
 
-        Ok(BackupWriter::new(h2, abort, crypt_config, debug))
+        Ok(BackupWriter::new(h2, abort, crypt_config, debug, quiet))
     }
 
     pub async fn get(&self, path: &str, param: Option<Value>) -> Result<Value, Error> {
@@ -340,7 +344,7 @@ impl BackupWriter {
         } else {
             pbs_tools::format::strip_server_file_extension(archive_name)
         };
-        if archive_name != CATALOG_NAME {
+        if !self.quiet && archive_name != CATALOG_NAME {
             let speed: HumanByte =
                 ((size_dirty * 1_000_000) / (upload_stats.duration.as_micros() as usize)).into();
             let size_dirty: HumanByte = size_dirty.into();
@@ -354,11 +358,11 @@ impl BackupWriter {
                 upload_stats.duration.as_secs_f64()
             );
             println!("{}: average backup speed: {}/s", archive, speed);
-        } else {
+        } else if !self.quiet {
             println!("Uploaded backup catalog ({})", size);
         }
 
-        if upload_stats.size_reused > 0 && upload_stats.size > 1024 * 1024 {
+        if !self.quiet && upload_stats.size_reused > 0 && upload_stats.size > 1024 * 1024 {
             let reused_percent = upload_stats.size_reused as f64 * 100. / upload_stats.size as f64;
             let reused: HumanByte = upload_stats.size_reused.into();
             println!(
diff --git a/proxmox-backup-client/src/benchmark.rs b/proxmox-backup-client/src/benchmark.rs
index 6a97b117..fdc4d989 100644
--- a/proxmox-backup-client/src/benchmark.rs
+++ b/proxmox-backup-client/src/benchmark.rs
@@ -235,6 +235,7 @@ async fn test_upload_speed(
         "benchmark",
         backup_time,
         false,
+        false,
         true
     ).await?;
 
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 7c022fad..94987ad1 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -611,6 +611,12 @@ fn spawn_catalog_upload(
                optional: true,
                default: false,
            },
+           "quiet": {
+               type: Boolean,
+               description: "Quiet output.",
+               optional: true,
+               default: false,
+           },
            "dry-run": {
                type: Boolean,
                description: "Just show what backup would do, but do not upload anything.",
@@ -627,6 +633,7 @@ async fn create_backup(
     skip_lost_and_found: bool,
     dry_run: bool,
     verbose: bool,
+    quiet: bool,
     _info: &ApiMethod,
     _rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
@@ -781,21 +788,21 @@ async fn create_backup(
     let client = connect_rate_limited(&repo, rate_limit)?;
     record_repository(&repo);
 
-    println!(
-        "Starting backup: {}/{}/{}",
-        backup_type,
-        backup_id,
-        BackupDir::backup_time_to_string(backup_time)?
-    );
-
-    println!("Client name: {}", proxmox_sys::nodename());
-
     let start_time = std::time::Instant::now();
 
-    println!(
-        "Starting backup protocol: {}",
-        strftime_local("%c", epoch_i64())?
-    );
+    if !quiet {
+        println!(
+            "Starting backup: {}/{}/{}",
+            backup_type,
+            backup_id,
+            BackupDir::backup_time_to_string(backup_time)?
+        );
+        println!("Client name: {}", proxmox_sys::nodename());
+        println!(
+            "Starting backup protocol: {}",
+            strftime_local("%c", epoch_i64())?
+        );
+    }
 
     let (crypt_config, rsa_encrypted_key) = match crypto.enc_key {
         None => (None, None),
@@ -837,20 +844,25 @@ async fn create_backup(
         backup_id,
         backup_time,
         verbose,
+        quiet,
         false,
     )
     .await?;
 
     let download_previous_manifest = match client.previous_backup_time().await {
         Ok(Some(backup_time)) => {
-            println!(
-                "Downloading previous manifest ({})",
-                strftime_local("%c", backup_time)?
-            );
+            if !quiet {
+                println!(
+                    "Downloading previous manifest ({})",
+                    strftime_local("%c", backup_time)?
+                );
+            }
             true
         }
         Ok(None) => {
-            println!("No previous manifest available.");
+            if !quiet {
+                println!("No previous manifest available.");
+            }
             false
         }
         Err(_) => {
@@ -887,7 +899,9 @@ async fn create_backup(
 
     let log_file = |desc: &str, file: &str, target: &str| {
         let what = if dry_run { "Would upload" } else { "Upload" };
-        println!("{} {} '{}' to '{}' as {}", what, desc, file, repo, target);
+        if !quiet {
+            println!("{} {} '{}' to '{}' as {}", what, desc, file, repo, target);
+        }
     };
 
     for (backup_type, filename, target, size) in upload_list {
@@ -1010,7 +1024,7 @@ async fn create_backup(
 
     if let Some(rsa_encrypted_key) = rsa_encrypted_key {
         let target = ENCRYPTED_KEY_BLOB_NAME;
-        println!("Upload RSA encoded key to '{:?}' as {}", repo, target);
+        if !quiet { println!("Upload RSA encoded key to '{:?}' as {}", repo, target); }
         let options = UploadOptions {
             compress: false,
             encrypt: false,
@@ -1043,10 +1057,10 @@ async fn create_backup(
 
     let end_time = std::time::Instant::now();
     let elapsed = end_time.duration_since(start_time);
-    println!("Duration: {:.2}s", elapsed.as_secs_f64());
-
-    println!("End Time: {}", strftime_local("%c", epoch_i64())?);
-
+    if !quiet {
+        println!("Duration: {:.2}s", elapsed.as_secs_f64());
+        println!("End Time: {}", strftime_local("%c", epoch_i64())?);
+    }
     Ok(Value::Null)
 }
 
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup v1 1/1] close #3770: pbs-client: add quiet backup flag

[Wolfgang Bumiller]

On Tue, Feb 22, 2022 at 12:27:29PM +0000, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
> this feels 'chunky', having quiet and verbose(which is sometimes called
> debug). Maybe some kind general verbosity enum might make sense, but then
> it should not just be used here.

Agreed!
A more general approach might be good.
I think we should be more strict about using the log crate's macros
(`log::info!(...)`, `log::error!(...)`, `log::debug!(...)`) and not use
`println!`/`eprintln!` at all, and have a way to control this (and
potentially the target output ) on a per-task basis. (Eg. via
thread-locals and wrapper Futures which set those before `poll()`ing the
inner future)