[pbs-devel] [PATCH vma-to-pbs v5 0/4] add support for bulk import of a dump directory

[pbs-devel] [PATCH vma-to-pbs v5 0/4] add support for bulk import of a dump directory

[Filip Schauer]

When a path to a directory is provided in the vma_file argument, try to
upload all VMA backups in the directory. This also handles compressed
VMA files, notes and logs. If a vmid is specified with --vmid, only the
backups of that particular vmid are uploaded.

Also improve the readability of the log messages to keep track of all
imported backups.

Changed since v4:
* Switch grouped_vmas from Vec<Vec<>> to HashMap<String, Vec<>>
* Remove dependency on itertools
* bail when no backups were found
* Default to yes on the bulk import confirmation prompt
* bail on invalid input to the bulk import confirmation prompt

Changed since v3:
* Mention in the description of the --vmid argument, that it is required
  if a single VMA file is provided
* Construct grouped_vmas in place
* Add debug logs when gathering files for bulk import
* Log a summary of the files gathered for bulk import
* Remove the "confusing VMA file path" error message in the second
  commit
* Switch chunk_stats from Arc<Mutex<[u64; 256]>> to
  Arc<[AtomicU64; 256]> and use fetch_add to atomically increment and
  fetch the chunk stat
* Ask for confirmation before bulk import
* Add --yes option to skip the confirmation prompt

Changed since v2:
* Make skipping a VMID on error optional with the --skip-failed option
* Switch log output from stderr to stdout
* Bump itertools to 0.13

Changed since v1:
* Do not recurse through dump directory
* Compile regex once before iterating over the files in the dump
  directory
* Use extract on regex capture groups
* Do not use deprecated method `chrono::NaiveDateTime::timestamp`
* Use proxmox_sys::fs::file_read_optional_string
* Group VMA files by VMID and continue with next VMID on error
* Move the BackupVmaToPbsArgs split into its own commit
* Remove hard coded occurences of 255
* Use level-based logging instead of println

Filip Schauer (4):
  add support for bulk import of a dump directory
  add option to skip vmids whose backups failed to upload
  use level-based logging instead of println
  log device upload progress as a percentage

 Cargo.toml     |   4 +
 src/main.rs    | 193 +++++++++++++++++++++++++++++++++++++++++++++----
 src/vma2pbs.rs | 110 ++++++++++++++++++++++------
 3 files changed, 272 insertions(+), 35 deletions(-)

-- 
2.39.5



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

[pbs-devel] [PATCH vma-to-pbs v5 1/4] add support for bulk import of a dump directory

[Filip Schauer]

When a path to a directory is provided in the vma_file argument, try to
upload all VMA backups in the directory. This also handles compressed
VMA files, notes and logs. If a vmid is specified with --vmid, only the
backups of that particular vmid are uploaded.

This is intended for use on a dump directory:

PBS_FINGERPRINT='PBS_FINGERPRINT' vma-to-pbs \
        --repository 'user@realm!token@server:port:datastore' \
        /var/lib/vz/dump

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 Cargo.toml     |   2 +
 src/main.rs    | 167 +++++++++++++++++++++++++++++++++++++++++++++----
 src/vma2pbs.rs |  64 ++++++++++++++++---
 3 files changed, 214 insertions(+), 19 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index cd13426..ad80304 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -7,9 +7,11 @@ edition = "2021"
 [dependencies]
 anyhow = "1.0"
 bincode = "1.3"
+chrono = "0.4"
 hyper = "0.14.5"
 pico-args = "0.5"
 md5 = "0.7.0"
+regex = "1.7"
 scopeguard = "1.1.0"
 serde = "1.0"
 serde_json = "1.0"
diff --git a/src/main.rs b/src/main.rs
index 3e25591..a394078 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -1,26 +1,35 @@
+use std::collections::HashMap;
 use std::ffi::OsString;
+use std::fs::read_dir;
+use std::io::{BufRead, BufReader, Write};
+use std::path::PathBuf;
 
 use anyhow::{bail, Context, Error};
+use chrono::NaiveDateTime;
 use proxmox_sys::linux::tty;
 use proxmox_time::epoch_i64;
+use regex::Regex;
 
 mod vma;
 mod vma2pbs;
-use vma2pbs::{vma2pbs, BackupVmaToPbsArgs, PbsArgs, VmaBackupArgs};
+use vma2pbs::{vma2pbs, BackupVmaToPbsArgs, Compression, PbsArgs, VmaBackupArgs};
 
 const CMD_HELP: &str = "\
 Usage: vma-to-pbs [OPTIONS] --repository <auth_id@host:port:datastore> --vmid <VMID> [vma_file]
 
 Arguments:
-  [vma_file]
+  [vma_file | dump_directory]
 
 Options:
       --repository <auth_id@host:port:datastore>
           Repository URL
       [--ns <NAMESPACE>]
           Namespace
-      --vmid <VMID>
+      [--vmid <VMID>]
           Backup ID
+          This is required if a single VMA file is provided.
+          If not specified, bulk import all VMA backups in the provided directory.
+          If specified with a dump directory, only import backups of the specified vmid.
       [--backup-time <EPOCH>]
           Backup timestamp
       --fingerprint <FINGERPRINT>
@@ -41,6 +50,8 @@ Options:
           File containing a comment/notes
       [--log-file <LOG_FILE>]
           Log file
+  -y, --yes
+          Automatic yes to prompts
   -h, --help
           Print help
   -V, --version
@@ -52,7 +63,16 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
     args.remove(0); // remove the executable path.
 
     let mut first_later_args_index = 0;
-    let options = ["-h", "--help", "-c", "--compress", "-e", "--encrypt"];
+    let options = [
+        "-h",
+        "--help",
+        "-c",
+        "--compress",
+        "-e",
+        "--encrypt",
+        "-y",
+        "--yes",
+    ];
 
     for (i, arg) in args.iter().enumerate() {
         if let Some(arg) = arg.to_str() {
@@ -87,7 +107,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
 
     let pbs_repository = args.value_from_str("--repository")?;
     let namespace = args.opt_value_from_str("--ns")?;
-    let vmid = args.value_from_str("--vmid")?;
+    let vmid: Option<String> = args.opt_value_from_str("--vmid")?;
     let backup_time: Option<i64> = args.opt_value_from_str("--backup-time")?;
     let backup_time = backup_time.unwrap_or_else(epoch_i64);
     let fingerprint = args.opt_value_from_str("--fingerprint")?;
@@ -99,6 +119,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
     let key_password_file: Option<OsString> = args.opt_value_from_str("--key-password-file")?;
     let notes_file: Option<OsString> = args.opt_value_from_str("--notes-file")?;
     let log_file_path: Option<OsString> = args.opt_value_from_str("--log-file")?;
+    let yes = args.contains(["-y", "--yes"]);
 
     match (encrypt, keyfile.is_some()) {
         (true, false) => bail!("--encrypt requires a --keyfile!"),
@@ -196,15 +217,137 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
         encrypt,
     };
 
-    let vma_args = VmaBackupArgs {
-        vma_file_path: vma_file_path.cloned(),
-        backup_id: vmid,
-        backup_time,
-        notes,
-        log_file_path,
+    let bulk =
+        vma_file_path
+            .map(PathBuf::from)
+            .and_then(|path| if path.is_dir() { Some(path) } else { None });
+
+    let grouped_vmas = if let Some(dump_dir_path) = bulk {
+        let re = Regex::new(
+            r"vzdump-qemu-(\d+)-(\d{4}_\d{2}_\d{2}-\d{2}_\d{2}_\d{2}).vma(|.zst|.lzo|.gz)$",
+        )?;
+
+        let mut vmas = Vec::new();
+
+        for entry in read_dir(dump_dir_path)? {
+            let entry = entry?;
+            let path = entry.path();
+
+            if !path.is_file() {
+                continue;
+            }
+
+            if let Some(file_name) = path.file_name().and_then(|n| n.to_str()) {
+                let Some((_, [backup_id, timestr, ext])) =
+                    re.captures(file_name).map(|c| c.extract())
+                else {
+                    // Skip the file, since it is not a VMA backup
+                    continue;
+                };
+
+                if let Some(ref vmid) = vmid {
+                    if backup_id != vmid {
+                        // Skip the backup, since it does not match the specified vmid
+                        continue;
+                    }
+                }
+
+                let compression = match ext {
+                    "" => None,
+                    ".zst" => Some(Compression::Zstd),
+                    ".lzo" => Some(Compression::Lzo),
+                    ".gz" => Some(Compression::GZip),
+                    _ => bail!("Unexpected file extension: {ext}"),
+                };
+
+                let backup_time = NaiveDateTime::parse_from_str(timestr, "%Y_%m_%d-%H_%M_%S")?
+                    .and_utc()
+                    .timestamp();
+
+                let notes_path = path.with_file_name(format!("{}.notes", file_name));
+                let notes = proxmox_sys::fs::file_read_optional_string(notes_path)?;
+
+                let log_path = path.with_file_name(format!("{}.log", file_name));
+                let log_file_path = if log_path.exists() {
+                    Some(log_path.to_path_buf().into_os_string())
+                } else {
+                    None
+                };
+
+                let backup_args = VmaBackupArgs {
+                    vma_file_path: Some(path.clone().into()),
+                    compression,
+                    backup_id: backup_id.to_string(),
+                    backup_time,
+                    notes,
+                    log_file_path,
+                };
+                vmas.push(backup_args);
+            }
+        }
+
+        vmas.sort_by_key(|d| d.backup_time);
+        let total_vma_count = vmas.len();
+        let grouped_vmas = vmas.into_iter().fold(
+            HashMap::new(),
+            |mut grouped: HashMap<String, Vec<VmaBackupArgs>>, vma_args| {
+                grouped
+                    .entry(vma_args.backup_id.clone())
+                    .or_default()
+                    .push(vma_args);
+                grouped
+            },
+        );
+
+        if grouped_vmas.is_empty() {
+            bail!("Did not find any backup archives");
+        }
+
+        println!(
+            "Found {} backup archive(s) of {} different VMID(s):",
+            total_vma_count,
+            grouped_vmas.len()
+        );
+
+        for (backup_id, vma_group) in &grouped_vmas {
+            println!("- VMID {}: {} backups", backup_id, vma_group.len());
+        }
+
+        if !yes {
+            eprint!("Proceed with the bulk import? (Y/n): ");
+            std::io::stdout().flush()?;
+            let mut line = String::new();
+
+            BufReader::new(std::io::stdin()).read_line(&mut line)?;
+            let trimmed = line.trim();
+            match trimmed {
+                "y" | "Y" | "" => {}
+                "n" | "N" => bail!("Bulk import was not confirmed."),
+                _ => bail!("Unexpected choice '{trimmed}'!"),
+            }
+        }
+
+        grouped_vmas
+    } else if let Some(vmid) = vmid {
+        HashMap::from([(
+            vmid.clone(),
+            vec![VmaBackupArgs {
+                vma_file_path: vma_file_path.cloned(),
+                compression: None,
+                backup_id: vmid,
+                backup_time,
+                notes,
+                log_file_path,
+            }],
+        )])
+    } else {
+        bail!("No vmid specified for single backup file");
     };
 
-    let options = BackupVmaToPbsArgs { pbs_args, vma_args };
+    let options = BackupVmaToPbsArgs {
+        pbs_args,
+        grouped_vmas,
+    };
 
     Ok(options)
 }
diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
index a888a7b..95ede9b 100644
--- a/src/vma2pbs.rs
+++ b/src/vma2pbs.rs
@@ -4,6 +4,7 @@ use std::collections::HashMap;
 use std::ffi::{c_char, CStr, CString, OsString};
 use std::fs::File;
 use std::io::{stdin, BufRead, BufReader, Read};
+use std::process::{Command, Stdio};
 use std::ptr;
 use std::time::SystemTime;
 
@@ -30,7 +31,7 @@ const VMA_CLUSTER_SIZE: usize = 65536;
 
 pub struct BackupVmaToPbsArgs {
     pub pbs_args: PbsArgs,
-    pub vma_args: VmaBackupArgs,
+    pub grouped_vmas: HashMap<String, Vec<VmaBackupArgs>>,
 }
 
 pub struct PbsArgs {
@@ -45,8 +46,15 @@ pub struct PbsArgs {
     pub encrypt: bool,
 }
 
+pub enum Compression {
+    Zstd,
+    Lzo,
+    GZip,
+}
+
 pub struct VmaBackupArgs {
     pub vma_file_path: Option<OsString>,
+    pub compression: Option<Compression>,
     pub backup_id: String,
     pub backup_time: i64,
     pub notes: Option<String>,
@@ -467,7 +475,19 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
 
     let start_transfer_time = SystemTime::now();
 
-    upload_vma_file(pbs_args, &args.vma_args)?;
+    for (_, vma_group) in args.grouped_vmas {
+        for backup_args in vma_group {
+            if let Err(e) = upload_vma_file(pbs_args, &backup_args) {
+                eprintln!(
+                    "Failed to upload vma file at {:?} - {}",
+                    backup_args.vma_file_path.unwrap_or("(stdin)".into()),
+                    e
+                );
+                println!("Skipping VMID {}", backup_args.backup_id);
+                break;
+            }
+        }
+    }
 
     let transfer_duration = SystemTime::now().duration_since(start_transfer_time)?;
     let total_seconds = transfer_duration.as_secs();
@@ -480,13 +500,43 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
 }
 
 fn upload_vma_file(pbs_args: &PbsArgs, backup_args: &VmaBackupArgs) -> Result<(), Error> {
-    let vma_file: Box<dyn BufRead> = match &backup_args.vma_file_path {
-        Some(vma_file_path) => match File::open(vma_file_path) {
-            Err(why) => return Err(anyhow!("Couldn't open file: {}", why)),
-            Ok(file) => Box::new(BufReader::new(file)),
+    match &backup_args.vma_file_path {
+        Some(vma_file_path) => println!("Uploading VMA backup from {:?}", vma_file_path),
+        None => println!("Uploading VMA backup from (stdin)"),
+    };
+
+    let vma_file: Box<dyn BufRead> = match &backup_args.compression {
+        Some(compression) => {
+            let vma_file_path = backup_args
+                .vma_file_path
+                .as_ref()
+                .expect("No VMA file path provided");
+            let mut cmd = match compression {
+                Compression::Zstd => {
+                    let mut cmd = Command::new("zstd");
+                    cmd.args(["-q", "-d", "-c"]);
+                    cmd
+                }
+                Compression::Lzo => {
+                    let mut cmd = Command::new("lzop");
+                    cmd.args(["-d", "-c"]);
+                    cmd
+                }
+                Compression::GZip => Command::new("zcat"),
+            };
+            let process = cmd.arg(vma_file_path).stdout(Stdio::piped()).spawn()?;
+            let stdout = process.stdout.expect("Failed to capture stdout");
+            Box::new(BufReader::new(stdout))
+        }
+        None => match &backup_args.vma_file_path {
+            Some(vma_file_path) => match File::open(vma_file_path) {
+                Err(why) => return Err(anyhow!("Couldn't open file: {}", why)),
+                Ok(file) => Box::new(BufReader::new(file)),
+            },
+            None => Box::new(BufReader::new(stdin())),
         },
-        None => Box::new(BufReader::new(stdin())),
     };
+
     let vma_reader = VmaReader::new(vma_file)?;
 
     let pbs = create_pbs_backup_task(pbs_args, backup_args)?;
-- 
2.39.5



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

Re: [pbs-devel] [PATCH vma-to-pbs v5 1/4] add support for bulk import of a dump directory

[Shannon Sterz]

comments in-line:

On Mon Nov 11, 2024 at 2:08 PM CET, Filip Schauer wrote:
> When a path to a directory is provided in the vma_file argument, try to
> upload all VMA backups in the directory. This also handles compressed
> VMA files, notes and logs. If a vmid is specified with --vmid, only the
> backups of that particular vmid are uploaded.
>
> This is intended for use on a dump directory:
>
> PBS_FINGERPRINT='PBS_FINGERPRINT' vma-to-pbs \
>         --repository 'user@realm!token@server:port:datastore' \
>         /var/lib/vz/dump
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  Cargo.toml     |   2 +
>  src/main.rs    | 167 +++++++++++++++++++++++++++++++++++++++++++++----
>  src/vma2pbs.rs |  64 ++++++++++++++++---
>  3 files changed, 214 insertions(+), 19 deletions(-)
>
> diff --git a/Cargo.toml b/Cargo.toml
> index cd13426..ad80304 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -7,9 +7,11 @@ edition = "2021"
>  [dependencies]
>  anyhow = "1.0"
>  bincode = "1.3"
> +chrono = "0.4"
>  hyper = "0.14.5"
>  pico-args = "0.5"
>  md5 = "0.7.0"
> +regex = "1.7"
>  scopeguard = "1.1.0"
>  serde = "1.0"
>  serde_json = "1.0"
> diff --git a/src/main.rs b/src/main.rs
> index 3e25591..a394078 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -1,26 +1,35 @@
> +use std::collections::HashMap;
>  use std::ffi::OsString;
> +use std::fs::read_dir;
> +use std::io::{BufRead, BufReader, Write};
> +use std::path::PathBuf;
>
>  use anyhow::{bail, Context, Error};
> +use chrono::NaiveDateTime;
>  use proxmox_sys::linux::tty;
>  use proxmox_time::epoch_i64;
> +use regex::Regex;
>
>  mod vma;
>  mod vma2pbs;
> -use vma2pbs::{vma2pbs, BackupVmaToPbsArgs, PbsArgs, VmaBackupArgs};
> +use vma2pbs::{vma2pbs, BackupVmaToPbsArgs, Compression, PbsArgs, VmaBackupArgs};
>
>  const CMD_HELP: &str = "\
>  Usage: vma-to-pbs [OPTIONS] --repository <auth_id@host:port:datastore> --vmid <VMID> [vma_file]
>
>  Arguments:
> -  [vma_file]
> +  [vma_file | dump_directory]
>
>  Options:
>        --repository <auth_id@host:port:datastore>
>            Repository URL
>        [--ns <NAMESPACE>]
>            Namespace
> -      --vmid <VMID>
> +      [--vmid <VMID>]

nit: this is marked as optional here (and in the code), but the usage
line above still make it look like it's required.

>            Backup ID
> +          This is required if a single VMA file is provided.
> +          If not specified, bulk import all VMA backups in the provided directory.
> +          If specified with a dump directory, only import backups of the specified vmid.
>        [--backup-time <EPOCH>]
>            Backup timestamp
>        --fingerprint <FINGERPRINT>
> @@ -41,6 +50,8 @@ Options:
>            File containing a comment/notes
>        [--log-file <LOG_FILE>]
>            Log file
> +  -y, --yes
> +          Automatic yes to prompts
>    -h, --help
>            Print help
>    -V, --version
> @@ -52,7 +63,16 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>      args.remove(0); // remove the executable path.
>
>      let mut first_later_args_index = 0;
> -    let options = ["-h", "--help", "-c", "--compress", "-e", "--encrypt"];
> +    let options = [
> +        "-h",
> +        "--help",
> +        "-c",
> +        "--compress",
> +        "-e",
> +        "--encrypt",
> +        "-y",
> +        "--yes",
> +    ];
>
>      for (i, arg) in args.iter().enumerate() {
>          if let Some(arg) = arg.to_str() {
> @@ -87,7 +107,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>
>      let pbs_repository = args.value_from_str("--repository")?;
>      let namespace = args.opt_value_from_str("--ns")?;
> -    let vmid = args.value_from_str("--vmid")?;
> +    let vmid: Option<String> = args.opt_value_from_str("--vmid")?;
>      let backup_time: Option<i64> = args.opt_value_from_str("--backup-time")?;
>      let backup_time = backup_time.unwrap_or_else(epoch_i64);
>      let fingerprint = args.opt_value_from_str("--fingerprint")?;
> @@ -99,6 +119,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>      let key_password_file: Option<OsString> = args.opt_value_from_str("--key-password-file")?;
>      let notes_file: Option<OsString> = args.opt_value_from_str("--notes-file")?;
>      let log_file_path: Option<OsString> = args.opt_value_from_str("--log-file")?;
> +    let yes = args.contains(["-y", "--yes"]);
>
>      match (encrypt, keyfile.is_some()) {
>          (true, false) => bail!("--encrypt requires a --keyfile!"),
> @@ -196,15 +217,137 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>          encrypt,
>      };
>
> -    let vma_args = VmaBackupArgs {
> -        vma_file_path: vma_file_path.cloned(),
> -        backup_id: vmid,
> -        backup_time,
> -        notes,
> -        log_file_path,
> +    let bulk =
> +        vma_file_path
> +            .map(PathBuf::from)
> +            .and_then(|path| if path.is_dir() { Some(path) } else { None });
> +
> +    let grouped_vmas = if let Some(dump_dir_path) = bulk {
> +        let re = Regex::new(
> +            r"vzdump-qemu-(\d+)-(\d{4}_\d{2}_\d{2}-\d{2}_\d{2}_\d{2}).vma(|.zst|.lzo|.gz)$",
> +        )?;
> +
> +        let mut vmas = Vec::new();
> +
> +        for entry in read_dir(dump_dir_path)? {
> +            let entry = entry?;
> +            let path = entry.path();
> +
> +            if !path.is_file() {
> +                continue;
> +            }
> +
> +            if let Some(file_name) = path.file_name().and_then(|n| n.to_str()) {
> +                let Some((_, [backup_id, timestr, ext])) =
> +                    re.captures(file_name).map(|c| c.extract())
> +                else {
> +                    // Skip the file, since it is not a VMA backup
> +                    continue;
> +                };
> +
> +                if let Some(ref vmid) = vmid {
> +                    if backup_id != vmid {
> +                        // Skip the backup, since it does not match the specified vmid
> +                        continue;
> +                    }
> +                }
> +
> +                let compression = match ext {
> +                    "" => None,
> +                    ".zst" => Some(Compression::Zstd),
> +                    ".lzo" => Some(Compression::Lzo),
> +                    ".gz" => Some(Compression::GZip),
> +                    _ => bail!("Unexpected file extension: {ext}"),
> +                };
> +
> +                let backup_time = NaiveDateTime::parse_from_str(timestr, "%Y_%m_%d-%H_%M_%S")?
> +                    .and_utc()
> +                    .timestamp();
> +
> +                let notes_path = path.with_file_name(format!("{}.notes", file_name));
> +                let notes = proxmox_sys::fs::file_read_optional_string(notes_path)?;
> +
> +                let log_path = path.with_file_name(format!("{}.log", file_name));
> +                let log_file_path = if log_path.exists() {
> +                    Some(log_path.to_path_buf().into_os_string())
> +                } else {
> +                    None
> +                };
> +
> +                let backup_args = VmaBackupArgs {
> +                    vma_file_path: Some(path.clone().into()),
> +                    compression,
> +                    backup_id: backup_id.to_string(),
> +                    backup_time,
> +                    notes,
> +                    log_file_path,
> +                };
> +                vmas.push(backup_args);
> +            }
> +        }
> +
> +        vmas.sort_by_key(|d| d.backup_time);
> +        let total_vma_count = vmas.len();
> +        let grouped_vmas = vmas.into_iter().fold(
> +            HashMap::new(),
> +            |mut grouped: HashMap<String, Vec<VmaBackupArgs>>, vma_args| {
> +                grouped
> +                    .entry(vma_args.backup_id.clone())
> +                    .or_default()
> +                    .push(vma_args);
> +                grouped
> +            },
> +        );
> +
> +        if grouped_vmas.is_empty() {
> +            bail!("Did not find any backup archives");
> +        }
> +
> +        println!(
> +            "Found {} backup archive(s) of {} different VMID(s):",
> +            total_vma_count,
> +            grouped_vmas.len()
> +        );
> +
> +        for (backup_id, vma_group) in &grouped_vmas {
> +            println!("- VMID {}: {} backups", backup_id, vma_group.len());

nit: this should be:

```rs
println!("- VMID {backup_id}: {} backups", vma_group.len());
```

> +        }
> +
> +        if !yes {
> +            eprint!("Proceed with the bulk import? (Y/n): ");
> +            std::io::stdout().flush()?;
> +            let mut line = String::new();
> +
> +            BufReader::new(std::io::stdin()).read_line(&mut line)?;
> +            let trimmed = line.trim();
> +            match trimmed {
> +                "y" | "Y" | "" => {}
> +                "n" | "N" => bail!("Bulk import was not confirmed."),
> +                _ => bail!("Unexpected choice '{trimmed}'!"),
> +            }
> +        }
> +
> +        grouped_vmas
> +    } else if let Some(vmid) = vmid {
> +        HashMap::from([(
> +            vmid.clone(),
> +            vec![VmaBackupArgs {
> +                vma_file_path: vma_file_path.cloned(),
> +                compression: None,
> +                backup_id: vmid,
> +                backup_time,
> +                notes,
> +                log_file_path,
> +            }],
> +        )])
> +    } else {
> +        bail!("No vmid specified for single backup file");
>      };
>
> -    let options = BackupVmaToPbsArgs { pbs_args, vma_args };
> +    let options = BackupVmaToPbsArgs {
> +        pbs_args,
> +        grouped_vmas,
> +    };
>
>      Ok(options)
>  }
> diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
> index a888a7b..95ede9b 100644
> --- a/src/vma2pbs.rs
> +++ b/src/vma2pbs.rs
> @@ -4,6 +4,7 @@ use std::collections::HashMap;
>  use std::ffi::{c_char, CStr, CString, OsString};
>  use std::fs::File;
>  use std::io::{stdin, BufRead, BufReader, Read};
> +use std::process::{Command, Stdio};
>  use std::ptr;
>  use std::time::SystemTime;
>
> @@ -30,7 +31,7 @@ const VMA_CLUSTER_SIZE: usize = 65536;
>
>  pub struct BackupVmaToPbsArgs {
>      pub pbs_args: PbsArgs,
> -    pub vma_args: VmaBackupArgs,
> +    pub grouped_vmas: HashMap<String, Vec<VmaBackupArgs>>,
>  }
>
>  pub struct PbsArgs {
> @@ -45,8 +46,15 @@ pub struct PbsArgs {
>      pub encrypt: bool,
>  }
>
> +pub enum Compression {
> +    Zstd,
> +    Lzo,
> +    GZip,
> +}
> +
>  pub struct VmaBackupArgs {
>      pub vma_file_path: Option<OsString>,
> +    pub compression: Option<Compression>,
>      pub backup_id: String,
>      pub backup_time: i64,
>      pub notes: Option<String>,
> @@ -467,7 +475,19 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>
>      let start_transfer_time = SystemTime::now();
>
> -    upload_vma_file(pbs_args, &args.vma_args)?;
> +    for (_, vma_group) in args.grouped_vmas {
> +        for backup_args in vma_group {
> +            if let Err(e) = upload_vma_file(pbs_args, &backup_args) {
> +                eprintln!(
> +                    "Failed to upload vma file at {:?} - {}",
> +                    backup_args.vma_file_path.unwrap_or("(stdin)".into()),
> +                    e

nit: same as above, move `e` into the format string

> +                );
> +                println!("Skipping VMID {}", backup_args.backup_id);
> +                break;
> +            }
> +        }
> +    }
>
>      let transfer_duration = SystemTime::now().duration_since(start_transfer_time)?;
>      let total_seconds = transfer_duration.as_secs();
> @@ -480,13 +500,43 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>  }
>
>  fn upload_vma_file(pbs_args: &PbsArgs, backup_args: &VmaBackupArgs) -> Result<(), Error> {
> -    let vma_file: Box<dyn BufRead> = match &backup_args.vma_file_path {
> -        Some(vma_file_path) => match File::open(vma_file_path) {
> -            Err(why) => return Err(anyhow!("Couldn't open file: {}", why)),
> -            Ok(file) => Box::new(BufReader::new(file)),
> +    match &backup_args.vma_file_path {
> +        Some(vma_file_path) => println!("Uploading VMA backup from {:?}", vma_file_path),

nit: this could be

```rs
           Some(vma_file_path) => println!("Uploading VMA backup from {vma_file_path:?}"),
```

> +        None => println!("Uploading VMA backup from (stdin)"),
> +    };
> +
> +    let vma_file: Box<dyn BufRead> = match &backup_args.compression {
> +        Some(compression) => {
> +            let vma_file_path = backup_args
> +                .vma_file_path
> +                .as_ref()
> +                .expect("No VMA file path provided");
> +            let mut cmd = match compression {
> +                Compression::Zstd => {
> +                    let mut cmd = Command::new("zstd");
> +                    cmd.args(["-q", "-d", "-c"]);
> +                    cmd

i think the following would be more elegant here:


```rs
                   Compression::Zstd => Command::new("zstd")
                       .args(["-q", "-d", "-c"]),
```

it's a bit more concise imo

> +                }
> +                Compression::Lzo => {
> +                    let mut cmd = Command::new("lzop");
> +                    cmd.args(["-d", "-c"]);
> +                    cmd

same as above

> +                }
> +                Compression::GZip => Command::new("zcat"),
> +            };
> +            let process = cmd.arg(vma_file_path).stdout(Stdio::piped()).spawn()?;
> +            let stdout = process.stdout.expect("Failed to capture stdout");
> +            Box::new(BufReader::new(stdout))
> +        }
> +        None => match &backup_args.vma_file_path {
> +            Some(vma_file_path) => match File::open(vma_file_path) {
> +                Err(why) => return Err(anyhow!("Couldn't open file: {}", why)),

nit: `why` can be moved into the format string here

> +                Ok(file) => Box::new(BufReader::new(file)),
> +            },
> +            None => Box::new(BufReader::new(stdin())),
>          },
> -        None => Box::new(BufReader::new(stdin())),
>      };
> +
>      let vma_reader = VmaReader::new(vma_file)?;
>
>      let pbs = create_pbs_backup_task(pbs_args, backup_args)?;



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

Re: [pbs-devel] [PATCH vma-to-pbs v5 1/4] add support for bulk import of a dump directory

[Filip Schauer]

On 13/11/2024 12:41, Shannon Sterz wrote:
>>   const CMD_HELP: &str = "\
>>   Usage: vma-to-pbs [OPTIONS] --repository <auth_id@host:port:datastore> --vmid <VMID> [vma_file]
>>
>>   Arguments:
>> -  [vma_file]
>> +  [vma_file | dump_directory]
>>
>>   Options:
>>         --repository <auth_id@host:port:datastore>
>>             Repository URL
>>         [--ns <NAMESPACE>]
>>             Namespace
>> -      --vmid <VMID>
>> +      [--vmid <VMID>]
> nit: this is marked as optional here (and in the code), but the usage
> line above still make it look like it's required.

That usage line describes the command for a single VMA file. In that
case `--vmid` actually is required. It is however not required for bulk
import. So to make that clearer I made two usage lines in v6.


On 13/11/2024 12:41, Shannon Sterz wrote:
>> +    let vma_file: Box<dyn BufRead> = match &backup_args.compression {
>> +        Some(compression) => {
>> +            let vma_file_path = backup_args
>> +                .vma_file_path
>> +                .as_ref()
>> +                .expect("No VMA file path provided");
>> +            let mut cmd = match compression {
>> +                Compression::Zstd => {
>> +                    let mut cmd = Command::new("zstd");
>> +                    cmd.args(["-q", "-d", "-c"]);
>> +                    cmd
> i think the following would be more elegant here:
>
>
> ```rs
>                     Compression::Zstd => Command::new("zstd")
>                         .args(["-q", "-d", "-c"]),
> ```
>
> it's a bit more concise imo
>
>> +                }
>> +                Compression::Lzo => {
>> +                    let mut cmd = Command::new("lzop");
>> +                    cmd.args(["-d", "-c"]);
>> +                    cmd
> same as above

Yeah I tried that, but unfortunatelly it does not compile:

```
error[E0716]: temporary value dropped while borrowed
    --> src/vma2pbs.rs:532:38
     |
531 |             let mut cmd = match compression {
     |                 ------- borrow later stored here
532 |                 Compression::Zstd => 
Command::new("zstd").args(["-q", "-d", "-c"]),
     | ^^^^^^^^^^^^^^^^^^^^                        - temporary value is 
freed at the end of this statement
     |                                      |
     |                                      creates a temporary value 
which is freed while still in use
     |
     = note: consider using a `let` binding to create a longer lived value
```



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

[pbs-devel] [PATCH vma-to-pbs v5 2/4] add option to skip vmids whose backups failed to upload

[Filip Schauer]

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/main.rs    |  6 ++++++
 src/vma2pbs.rs | 13 ++++++++++---
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/main.rs b/src/main.rs
index a394078..d4b36fa 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -50,6 +50,9 @@ Options:
           File containing a comment/notes
       [--log-file <LOG_FILE>]
           Log file
+      --skip-failed
+          Skip VMIDs that failed to be uploaded and continue onto the next VMID if a dump directory
+          is specified.
   -y, --yes
           Automatic yes to prompts
   -h, --help
@@ -70,6 +73,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
         "--compress",
         "-e",
         "--encrypt",
+        "--skip-failed",
         "-y",
         "--yes",
     ];
@@ -119,6 +123,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
     let key_password_file: Option<OsString> = args.opt_value_from_str("--key-password-file")?;
     let notes_file: Option<OsString> = args.opt_value_from_str("--notes-file")?;
     let log_file_path: Option<OsString> = args.opt_value_from_str("--log-file")?;
+    let skip_failed = args.contains("--skip-failed");
     let yes = args.contains(["-y", "--yes"]);
 
     match (encrypt, keyfile.is_some()) {
@@ -347,6 +352,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
     let options = BackupVmaToPbsArgs {
         pbs_args,
         grouped_vmas,
+        skip_failed,
     };
 
     Ok(options)
diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
index 95ede9b..a5b4027 100644
--- a/src/vma2pbs.rs
+++ b/src/vma2pbs.rs
@@ -32,6 +32,7 @@ const VMA_CLUSTER_SIZE: usize = 65536;
 pub struct BackupVmaToPbsArgs {
     pub pbs_args: PbsArgs,
     pub grouped_vmas: HashMap<String, Vec<VmaBackupArgs>>,
+    pub skip_failed: bool,
 }
 
 pub struct PbsArgs {
@@ -478,13 +479,19 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
     for (_, vma_group) in args.grouped_vmas {
         for backup_args in vma_group {
             if let Err(e) = upload_vma_file(pbs_args, &backup_args) {
-                eprintln!(
+                let err_msg = format!(
                     "Failed to upload vma file at {:?} - {}",
                     backup_args.vma_file_path.unwrap_or("(stdin)".into()),
                     e
                 );
-                println!("Skipping VMID {}", backup_args.backup_id);
-                break;
+
+                if args.skip_failed {
+                    eprintln!("{}", err_msg);
+                    println!("Skipping VMID {}", backup_args.backup_id);
+                    break;
+                } else {
+                    bail!(err_msg);
+                }
             }
         }
     }
-- 
2.39.5



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

Re: [pbs-devel] [PATCH vma-to-pbs v5 2/4] add option to skip vmids whose backups failed to upload

[Shannon Sterz]

comments in-line:

On Mon Nov 11, 2024 at 2:08 PM CET, Filip Schauer wrote:
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/main.rs    |  6 ++++++
>  src/vma2pbs.rs | 13 ++++++++++---
>  2 files changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/src/main.rs b/src/main.rs
> index a394078..d4b36fa 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -50,6 +50,9 @@ Options:
>            File containing a comment/notes
>        [--log-file <LOG_FILE>]
>            Log file
> +      --skip-failed
> +          Skip VMIDs that failed to be uploaded and continue onto the next VMID if a dump directory
> +          is specified.
>    -y, --yes
>            Automatic yes to prompts
>    -h, --help
> @@ -70,6 +73,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>          "--compress",
>          "-e",
>          "--encrypt",
> +        "--skip-failed",
>          "-y",
>          "--yes",
>      ];
> @@ -119,6 +123,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>      let key_password_file: Option<OsString> = args.opt_value_from_str("--key-password-file")?;
>      let notes_file: Option<OsString> = args.opt_value_from_str("--notes-file")?;
>      let log_file_path: Option<OsString> = args.opt_value_from_str("--log-file")?;
> +    let skip_failed = args.contains("--skip-failed");
>      let yes = args.contains(["-y", "--yes"]);
>
>      match (encrypt, keyfile.is_some()) {
> @@ -347,6 +352,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>      let options = BackupVmaToPbsArgs {
>          pbs_args,
>          grouped_vmas,
> +        skip_failed,
>      };
>
>      Ok(options)
> diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
> index 95ede9b..a5b4027 100644
> --- a/src/vma2pbs.rs
> +++ b/src/vma2pbs.rs
> @@ -32,6 +32,7 @@ const VMA_CLUSTER_SIZE: usize = 65536;
>  pub struct BackupVmaToPbsArgs {
>      pub pbs_args: PbsArgs,
>      pub grouped_vmas: HashMap<String, Vec<VmaBackupArgs>>,
> +    pub skip_failed: bool,
>  }
>
>  pub struct PbsArgs {
> @@ -478,13 +479,19 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>      for (_, vma_group) in args.grouped_vmas {
>          for backup_args in vma_group {
>              if let Err(e) = upload_vma_file(pbs_args, &backup_args) {
> -                eprintln!(
> +                let err_msg = format!(
>                      "Failed to upload vma file at {:?} - {}",
>                      backup_args.vma_file_path.unwrap_or("(stdin)".into()),
>                      e

nit: i'd move `e` into the format string, since you are basically
already touching these lines :)

>                  );
> -                println!("Skipping VMID {}", backup_args.backup_id);
> -                break;
> +
> +                if args.skip_failed {
> +                    eprintln!("{}", err_msg);
> +                    println!("Skipping VMID {}", backup_args.backup_id);
> +                    break;
> +                } else {
> +                    bail!(err_msg);
> +                }
>              }
>          }
>      }



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

[pbs-devel] [PATCH vma-to-pbs v5 3/4] use level-based logging instead of println

[Filip Schauer]

Use log level "info" by default and prevent spamming messages for every
single chunk uploaded. To re-enable these messages, set the RUST_LOG
environment variable to "debug".

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 Cargo.toml     |  2 ++
 src/main.rs    | 28 ++++++++++++++++++++++------
 src/vma2pbs.rs | 38 ++++++++++++++++++++------------------
 3 files changed, 44 insertions(+), 24 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index ad80304..7951bbc 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -8,7 +8,9 @@ edition = "2021"
 anyhow = "1.0"
 bincode = "1.3"
 chrono = "0.4"
+env_logger = "0.10"
 hyper = "0.14.5"
+log = "0.4"
 pico-args = "0.5"
 md5 = "0.7.0"
 regex = "1.7"
diff --git a/src/main.rs b/src/main.rs
index d4b36fa..203196b 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -6,6 +6,7 @@ use std::path::PathBuf;
 
 use anyhow::{bail, Context, Error};
 use chrono::NaiveDateTime;
+use env_logger::Target;
 use proxmox_sys::linux::tty;
 use proxmox_time::epoch_i64;
 use regex::Regex;
@@ -128,7 +129,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
 
     match (encrypt, keyfile.is_some()) {
         (true, false) => bail!("--encrypt requires a --keyfile!"),
-        (false, true) => println!(
+        (false, true) => log::info!(
             "--keyfile given, but --encrypt not set -> backup will be signed, but not encrypted!"
         ),
         _ => {}
@@ -190,7 +191,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
 
             Some(key_password)
         } else if vma_file_path.is_none() {
-            println!(
+            log::info!(
                 "Please use --key-password-file to provide the password when passing the VMA file \
                 to stdin, if required."
             );
@@ -246,13 +247,17 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
                 let Some((_, [backup_id, timestr, ext])) =
                     re.captures(file_name).map(|c| c.extract())
                 else {
-                    // Skip the file, since it is not a VMA backup
+                    log::debug!("Skip \"{file_name}\", since it is not a VMA backup");
                     continue;
                 };
 
                 if let Some(ref vmid) = vmid {
                     if backup_id != vmid {
-                        // Skip the backup, since it does not match the specified vmid
+                        log::debug!(
+                            "Skip backup with VMID {}, since it does not match specified VMID {}",
+                            backup_id,
+                            vmid
+                        );
                         continue;
                     }
                 }
@@ -308,14 +313,14 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
             bail!("Did not find any backup archives");
         }
 
-        println!(
+        log::info!(
             "Found {} backup archive(s) of {} different VMID(s):",
             total_vma_count,
             grouped_vmas.len()
         );
 
         for (backup_id, vma_group) in &grouped_vmas {
-            println!("- VMID {}: {} backups", backup_id, vma_group.len());
+            log::info!("- VMID {}: {} backups", backup_id, vma_group.len());
         }
 
         if !yes {
@@ -358,7 +363,18 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
     Ok(options)
 }
 
+fn init_cli_logger() {
+    env_logger::Builder::from_env(env_logger::Env::new().filter_or("RUST_LOG", "info"))
+        .format_level(false)
+        .format_target(false)
+        .format_timestamp(None)
+        .target(Target::Stdout)
+        .init();
+}
+
 fn main() -> Result<(), Error> {
+    init_cli_logger();
+
     let args = parse_args()?;
     vma2pbs(args)?;
 
diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
index a5b4027..0517251 100644
--- a/src/vma2pbs.rs
+++ b/src/vma2pbs.rs
@@ -82,8 +82,8 @@ fn create_pbs_backup_task(
     pbs_args: &PbsArgs,
     backup_args: &VmaBackupArgs,
 ) -> Result<*mut ProxmoxBackupHandle, Error> {
-    println!(
-        "backup time: {}",
+    log::info!(
+        "\tbackup time: {}",
         epoch_to_rfc3339(backup_args.backup_time)?
     );
 
@@ -152,7 +152,7 @@ where
         let config_name = config.name;
         let config_data = config.content;
 
-        println!("CFG: size: {} name: {}", config_data.len(), config_name);
+        log::info!("\tCFG: size: {} name: {}", config_data.len(), config_name);
 
         let config_name_cstr = CString::new(config_name)?;
 
@@ -190,9 +190,11 @@ where
         let device_name = vma_reader.get_device_name(device_id.try_into()?)?;
         let device_size = vma_reader.get_device_size(device_id.try_into()?)?;
 
-        println!(
-            "DEV: dev_id={} size: {} devname: {}",
-            device_id, device_size, device_name
+        log::info!(
+            "\tDEV: dev_id={} size: {} devname: {}",
+            device_id,
+            device_size,
+            device_name
         );
 
         let device_name_cstr = CString::new(device_name)?;
@@ -276,8 +278,8 @@ where
         };
 
         let pbs_upload_chunk = |pbs_chunk_buffer: Option<&[u8]>| {
-            println!(
-                "Uploading dev_id: {} offset: {:#0X} - {:#0X}",
+            log::debug!(
+                "\tUploading dev_id: {} offset: {:#0X} - {:#0X}",
                 dev_id,
                 pbs_chunk_offset,
                 pbs_chunk_offset + pbs_chunk_size,
@@ -466,13 +468,13 @@ fn set_notes(
 
 pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
     let pbs_args = &args.pbs_args;
-    println!("PBS repository: {}", pbs_args.pbs_repository);
+    log::info!("PBS repository: {}", pbs_args.pbs_repository);
     if let Some(ns) = &pbs_args.namespace {
-        println!("PBS namespace: {}", ns);
+        log::info!("PBS namespace: {}", ns);
     }
-    println!("PBS fingerprint: {}", pbs_args.fingerprint);
-    println!("compress: {}", pbs_args.compress);
-    println!("encrypt: {}", pbs_args.encrypt);
+    log::info!("PBS fingerprint: {}", pbs_args.fingerprint);
+    log::info!("compress: {}", pbs_args.compress);
+    log::info!("encrypt: {}", pbs_args.encrypt);
 
     let start_transfer_time = SystemTime::now();
 
@@ -486,8 +488,8 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
                 );
 
                 if args.skip_failed {
-                    eprintln!("{}", err_msg);
-                    println!("Skipping VMID {}", backup_args.backup_id);
+                    log::warn!("{}", err_msg);
+                    log::info!("Skipping VMID {}", backup_args.backup_id);
                     break;
                 } else {
                     bail!(err_msg);
@@ -501,15 +503,15 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
     let minutes = total_seconds / 60;
     let seconds = total_seconds % 60;
     let milliseconds = transfer_duration.as_millis() % 1000;
-    println!("Backup finished within {minutes} minutes, {seconds} seconds and {milliseconds} ms");
+    log::info!("Backup finished within {minutes} minutes, {seconds} seconds and {milliseconds} ms");
 
     Ok(())
 }
 
 fn upload_vma_file(pbs_args: &PbsArgs, backup_args: &VmaBackupArgs) -> Result<(), Error> {
     match &backup_args.vma_file_path {
-        Some(vma_file_path) => println!("Uploading VMA backup from {:?}", vma_file_path),
-        None => println!("Uploading VMA backup from (stdin)"),
+        Some(vma_file_path) => log::info!("Uploading VMA backup from {:?}", vma_file_path),
+        None => log::info!("Uploading VMA backup from (stdin)"),
     };
 
     let vma_file: Box<dyn BufRead> = match &backup_args.compression {
-- 
2.39.5



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

Re: [pbs-devel] [PATCH vma-to-pbs v5 3/4] use level-based logging instead of println

[Shannon Sterz]

comments in-line:

On Mon Nov 11, 2024 at 2:08 PM CET, Filip Schauer wrote:
> Use log level "info" by default and prevent spamming messages for every
> single chunk uploaded. To re-enable these messages, set the RUST_LOG
> environment variable to "debug".
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  Cargo.toml     |  2 ++
>  src/main.rs    | 28 ++++++++++++++++++++++------
>  src/vma2pbs.rs | 38 ++++++++++++++++++++------------------
>  3 files changed, 44 insertions(+), 24 deletions(-)
>
> diff --git a/Cargo.toml b/Cargo.toml
> index ad80304..7951bbc 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -8,7 +8,9 @@ edition = "2021"
>  anyhow = "1.0"
>  bincode = "1.3"
>  chrono = "0.4"
> +env_logger = "0.10"
>  hyper = "0.14.5"
> +log = "0.4"
>  pico-args = "0.5"
>  md5 = "0.7.0"
>  regex = "1.7"
> diff --git a/src/main.rs b/src/main.rs
> index d4b36fa..203196b 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -6,6 +6,7 @@ use std::path::PathBuf;
>
>  use anyhow::{bail, Context, Error};
>  use chrono::NaiveDateTime;
> +use env_logger::Target;
>  use proxmox_sys::linux::tty;
>  use proxmox_time::epoch_i64;
>  use regex::Regex;
> @@ -128,7 +129,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>
>      match (encrypt, keyfile.is_some()) {
>          (true, false) => bail!("--encrypt requires a --keyfile!"),
> -        (false, true) => println!(
> +        (false, true) => log::info!(
>              "--keyfile given, but --encrypt not set -> backup will be signed, but not encrypted!"
>          ),
>          _ => {}
> @@ -190,7 +191,7 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>
>              Some(key_password)
>          } else if vma_file_path.is_none() {
> -            println!(
> +            log::info!(
>                  "Please use --key-password-file to provide the password when passing the VMA file \
>                  to stdin, if required."
>              );
> @@ -246,13 +247,17 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>                  let Some((_, [backup_id, timestr, ext])) =
>                      re.captures(file_name).map(|c| c.extract())
>                  else {
> -                    // Skip the file, since it is not a VMA backup
> +                    log::debug!("Skip \"{file_name}\", since it is not a VMA backup");
>                      continue;
>                  };
>
>                  if let Some(ref vmid) = vmid {
>                      if backup_id != vmid {
> -                        // Skip the backup, since it does not match the specified vmid
> +                        log::debug!(
> +                            "Skip backup with VMID {}, since it does not match specified VMID {}",
> +                            backup_id,
> +                            vmid

nit: you can use format strings here

> +                        );
>                          continue;
>                      }
>                  }
> @@ -308,14 +313,14 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>              bail!("Did not find any backup archives");
>          }
>
> -        println!(
> +        log::info!(
>              "Found {} backup archive(s) of {} different VMID(s):",
>              total_vma_count,
>              grouped_vmas.len()
>          );
>
>          for (backup_id, vma_group) in &grouped_vmas {
> -            println!("- VMID {}: {} backups", backup_id, vma_group.len());
> +            log::info!("- VMID {}: {} backups", backup_id, vma_group.len());
>          }

nit: if you are already touching this, move this over to format strings
as well

>
>          if !yes {
> @@ -358,7 +363,18 @@ fn parse_args() -> Result<BackupVmaToPbsArgs, Error> {
>      Ok(options)
>  }
>
> +fn init_cli_logger() {
> +    env_logger::Builder::from_env(env_logger::Env::new().filter_or("RUST_LOG", "info"))
> +        .format_level(false)
> +        .format_target(false)
> +        .format_timestamp(None)
> +        .target(Target::Stdout)
> +        .init();
> +}
> +
>  fn main() -> Result<(), Error> {
> +    init_cli_logger();
> +
>      let args = parse_args()?;
>      vma2pbs(args)?;
>
> diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
> index a5b4027..0517251 100644
> --- a/src/vma2pbs.rs
> +++ b/src/vma2pbs.rs
> @@ -82,8 +82,8 @@ fn create_pbs_backup_task(
>      pbs_args: &PbsArgs,
>      backup_args: &VmaBackupArgs,
>  ) -> Result<*mut ProxmoxBackupHandle, Error> {
> -    println!(
> -        "backup time: {}",
> +    log::info!(
> +        "\tbackup time: {}",
>          epoch_to_rfc3339(backup_args.backup_time)?
>      );
>
> @@ -152,7 +152,7 @@ where
>          let config_name = config.name;
>          let config_data = config.content;
>
> -        println!("CFG: size: {} name: {}", config_data.len(), config_name);
> +        log::info!("\tCFG: size: {} name: {}", config_data.len(), config_name);

nit: move `config_name` into the format string

>
>          let config_name_cstr = CString::new(config_name)?;
>
> @@ -190,9 +190,11 @@ where
>          let device_name = vma_reader.get_device_name(device_id.try_into()?)?;
>          let device_size = vma_reader.get_device_size(device_id.try_into()?)?;
>
> -        println!(
> -            "DEV: dev_id={} size: {} devname: {}",
> -            device_id, device_size, device_name
> +        log::info!(
> +            "\tDEV: dev_id={} size: {} devname: {}",

nit: format string

> +            device_id,
> +            device_size,
> +            device_name
>          );
>
>          let device_name_cstr = CString::new(device_name)?;
> @@ -276,8 +278,8 @@ where
>          };
>
>          let pbs_upload_chunk = |pbs_chunk_buffer: Option<&[u8]>| {
> -            println!(
> -                "Uploading dev_id: {} offset: {:#0X} - {:#0X}",
> +            log::debug!(
> +                "\tUploading dev_id: {} offset: {:#0X} - {:#0X}",

nit: format string, for example:

`\tUploading dev_id: {dev_id} offset: {pbs_chunk_offset:#0X} - {:#0X}`

>                  dev_id,
>                  pbs_chunk_offset,
>                  pbs_chunk_offset + pbs_chunk_size,
> @@ -466,13 +468,13 @@ fn set_notes(
>
>  pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>      let pbs_args = &args.pbs_args;
> -    println!("PBS repository: {}", pbs_args.pbs_repository);
> +    log::info!("PBS repository: {}", pbs_args.pbs_repository);
>      if let Some(ns) = &pbs_args.namespace {
> -        println!("PBS namespace: {}", ns);
> +        log::info!("PBS namespace: {}", ns);

nit: format string

>      }
> -    println!("PBS fingerprint: {}", pbs_args.fingerprint);
> -    println!("compress: {}", pbs_args.compress);
> -    println!("encrypt: {}", pbs_args.encrypt);
> +    log::info!("PBS fingerprint: {}", pbs_args.fingerprint);
> +    log::info!("compress: {}", pbs_args.compress);
> +    log::info!("encrypt: {}", pbs_args.encrypt);
>
>      let start_transfer_time = SystemTime::now();
>
> @@ -486,8 +488,8 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>                  );
>
>                  if args.skip_failed {
> -                    eprintln!("{}", err_msg);
> -                    println!("Skipping VMID {}", backup_args.backup_id);
> +                    log::warn!("{}", err_msg);
> +                    log::info!("Skipping VMID {}", backup_args.backup_id);
>                      break;
>                  } else {
>                      bail!(err_msg);
> @@ -501,15 +503,15 @@ pub fn vma2pbs(args: BackupVmaToPbsArgs) -> Result<(), Error> {
>      let minutes = total_seconds / 60;
>      let seconds = total_seconds % 60;
>      let milliseconds = transfer_duration.as_millis() % 1000;
> -    println!("Backup finished within {minutes} minutes, {seconds} seconds and {milliseconds} ms");
> +    log::info!("Backup finished within {minutes} minutes, {seconds} seconds and {milliseconds} ms");
>
>      Ok(())
>  }
>
>  fn upload_vma_file(pbs_args: &PbsArgs, backup_args: &VmaBackupArgs) -> Result<(), Error> {
>      match &backup_args.vma_file_path {
> -        Some(vma_file_path) => println!("Uploading VMA backup from {:?}", vma_file_path),
> -        None => println!("Uploading VMA backup from (stdin)"),
> +        Some(vma_file_path) => log::info!("Uploading VMA backup from {:?}", vma_file_path),

nit: format string

> +        None => log::info!("Uploading VMA backup from (stdin)"),
>      };
>
>      let vma_file: Box<dyn BufRead> = match &backup_args.compression {



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

Re: [pbs-devel] [PATCH vma-to-pbs v5 3/4] use level-based logging instead of println

[Filip Schauer]

On 13/11/2024 12:41, Shannon Sterz wrote:
>> -                        // Skip the backup, since it does not match the specified vmid
>> +                        log::debug!(
>> +                            "Skip backup with VMID {}, since it does not match specified VMID {}",
>> +                            backup_id,
>> +                            vmid
> nit: you can use format strings here

I could, but then the line would exceed the 100 character limit and I
would rather not split the string over multiple lines.



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

[pbs-devel] [PATCH vma-to-pbs v5 4/4] log device upload progress as a percentage

[Filip Schauer]

Log the upload progress of a device as a percentage with log level info
every 1000 chunks.

Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
---
 src/vma2pbs.rs | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
index 0517251..f469053 100644
--- a/src/vma2pbs.rs
+++ b/src/vma2pbs.rs
@@ -6,6 +6,8 @@ use std::fs::File;
 use std::io::{stdin, BufRead, BufReader, Read};
 use std::process::{Command, Stdio};
 use std::ptr;
+use std::sync::atomic::{AtomicU64, Ordering};
+use std::sync::Arc;
 use std::time::SystemTime;
 
 use anyhow::{anyhow, bail, Error};
@@ -234,6 +236,8 @@ where
         non_zero_mask: u64,
     }
 
+    let chunk_stats = Arc::new([const { AtomicU64::new(0) }; VMA_MAX_DEVICES]);
+
     let images_chunks: RefCell<HashMap<u8, HashMap<u64, ImageChunk>>> =
         RefCell::new(HashMap::new());
 
@@ -284,6 +288,11 @@ where
                 pbs_chunk_offset,
                 pbs_chunk_offset + pbs_chunk_size,
             );
+            let chunk_stat = chunk_stats[dev_id as usize].fetch_add(1, Ordering::SeqCst);
+            if (chunk_stat % 1000) == 0 {
+                let percentage = 100 * PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE * chunk_stat / device_size;
+                log::info!("\tUploading dev_id: {} ({}%)", dev_id, percentage);
+            }
 
             let mut pbs_err: *mut c_char = ptr::null_mut();
 
-- 
2.39.5



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

Re: [pbs-devel] [PATCH vma-to-pbs v5 4/4] log device upload progress as a percentage

[Shannon Sterz]

comments in-line:

On Mon Nov 11, 2024 at 2:08 PM CET, Filip Schauer wrote:
> Log the upload progress of a device as a percentage with log level info
> every 1000 chunks.
>
> Signed-off-by: Filip Schauer <f.schauer@proxmox.com>
> ---
>  src/vma2pbs.rs | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/vma2pbs.rs b/src/vma2pbs.rs
> index 0517251..f469053 100644
> --- a/src/vma2pbs.rs
> +++ b/src/vma2pbs.rs
> @@ -6,6 +6,8 @@ use std::fs::File;
>  use std::io::{stdin, BufRead, BufReader, Read};
>  use std::process::{Command, Stdio};
>  use std::ptr;
> +use std::sync::atomic::{AtomicU64, Ordering};
> +use std::sync::Arc;
>  use std::time::SystemTime;
>
>  use anyhow::{anyhow, bail, Error};
> @@ -234,6 +236,8 @@ where
>          non_zero_mask: u64,
>      }
>
> +    let chunk_stats = Arc::new([const { AtomicU64::new(0) }; VMA_MAX_DEVICES]);
> +
>      let images_chunks: RefCell<HashMap<u8, HashMap<u64, ImageChunk>>> =
>          RefCell::new(HashMap::new());
>
> @@ -284,6 +288,11 @@ where
>                  pbs_chunk_offset,
>                  pbs_chunk_offset + pbs_chunk_size,
>              );
> +            let chunk_stat = chunk_stats[dev_id as usize].fetch_add(1, Ordering::SeqCst);
> +            if (chunk_stat % 1000) == 0 {
> +                let percentage = 100 * PROXMOX_BACKUP_DEFAULT_CHUNK_SIZE * chunk_stat / device_size;
> +                log::info!("\tUploading dev_id: {} ({}%)", dev_id, percentage);

nit: format string

> +            }
>
>              let mut pbs_err: *mut c_char = ptr::null_mut();
>

Other than the nits across the four patches, consider this series:

Reviewed-by: Shannon Sterz <s.sterz@proxmox.com>



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

Re: [pbs-devel] [PATCH vma-to-pbs v5 4/4] log device upload progress as a percentage

[Filip Schauer]

On 13/11/2024 12:41, Shannon Sterz wrote:
> Other than the nits across the four patches, consider this series:
>
> Reviewed-by: Shannon Sterz<s.sterz@proxmox.com>

Thanks for the review!
The nits are addressed in v6.

Superseded by:
https://lists.proxmox.com/pipermail/pbs-devel/2024-November/011465.html



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