[pbs-devel] [RFC proxmox-backup 0/3] Encode creation parameters into pxar archive

[pbs-devel] [RFC proxmox-backup 0/3] Encode creation parameters into pxar archive

[Gabriel Goller]

To better debug the creation of the pxar archive, we encode the 
creation_params (the parameters passed to the cli) inside the pxar
archive. Like this we just need to locate the `.pxar_creation_params`
file in the archive and we can check which arguments have been used. 

To realize this, we unify all the parameters of the `create_backup`
function so that we don't need an additional struct (which in turn
introduces additional maintenance efforts).

An example `.pxar_creation_params` file would look like this:

    {
      "backupspec": [
        "test.pxar:linux"
      ],
      "chunk-size": 4096,
      "exclude": [
        "MAINTAINERS"
      ],
      "skip-e2big-xattr": true
    }


Some background: 
The original idea came from Dietmar and while it is surely useful, I
don't really like my implementation (most likely this is a skill-issue
from my side). So suggestions/critique of any kind is welcome!  

proxmox-backup:

Gabriel Goller (3):
  pxar: factor out encode_file
  client: unify parameters and write to file
  pxar: added creation parameters

 pbs-client/src/pxar/create.rs                 | 23 ++++++-
 pbs-client/src/pxar_backup_stream.rs          |  5 +-
 proxmox-backup-client/src/main.rs             | 29 +++++---
 .../src/proxmox_restore_daemon/api.rs         |  2 +-
 pxar-bin/Cargo.toml                           |  1 +
 pxar-bin/src/main.rs                          | 67 ++++++++++++-------
 tests/catar.rs                                |  1 +
 7 files changed, 89 insertions(+), 39 deletions(-)


Summary over all repositories:
  7 files changed, 89 insertions(+), 39 deletions(-)

-- 
Generated by git-murpp 0.5.0

[pbs-devel] [PATCH proxmox-backup 1/3] pxar: factor out encode_file

[Gabriel Goller]

When writing the `--exclude` params to the '.pxarexclude-cli' file, we
use a function `encode_pxarexclude_cli`. Factored it out, so we can use
it for other files as well.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-client/src/pxar/create.rs | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 60efb0ce..0d5e8b68 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -395,6 +395,16 @@ impl Archiver {
         patterns_count: usize,
     ) -> Result<(), Error> {
         let content = generate_pxar_excludes_cli(&self.patterns[..patterns_count]);
+        self.encode_file(encoder, file_name, &content).await?;
+        Ok(())
+    }
+
+    async fn encode_file<T: SeqWrite + Send>(
+        &mut self,
+        encoder: &mut Encoder<'_, T>,
+        file_name: &CStr,
+        content: &[u8],
+    ) -> Result<(), Error> {
         if let Some(ref catalog) = self.catalog {
             catalog
                 .lock()
@@ -406,9 +416,9 @@ impl Archiver {
         metadata.stat.mode = pxar::format::mode::IFREG | 0o600;
 
         let mut file = encoder
-            .create_file(&metadata, ".pxarexclude-cli", content.len() as u64)
+            .create_file(&metadata, file_name.to_str()?, content.len() as u64)
             .await?;
-        file.write_all(&content).await?;
+        file.write_all(content).await?;
 
         Ok(())
     }
-- 
2.43.0

[pbs-devel] [PATCH proxmox-backup 2/3] client: unify parameters and write to file

[Gabriel Goller]

Merge all the existing parameters of `create_backup` to the
`serde_json::Value` parameter. Now only a single variable needs to be
written to the `.pxar_creation_params` file.
Pass the parameters down and encode it into the pxar archive.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pbs-client/src/pxar/create.rs                 |  9 ++++++
 pbs-client/src/pxar_backup_stream.rs          |  5 +++-
 proxmox-backup-client/src/main.rs             | 29 ++++++++++++-------
 .../src/proxmox_restore_daemon/api.rs         |  2 +-
 pxar-bin/src/main.rs                          |  1 +
 tests/catar.rs                                |  1 +
 6 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/pbs-client/src/pxar/create.rs b/pbs-client/src/pxar/create.rs
index 0d5e8b68..437bf05b 100644
--- a/pbs-client/src/pxar/create.rs
+++ b/pbs-client/src/pxar/create.rs
@@ -142,6 +142,7 @@ pub async fn create_archive<T, F>(
     callback: F,
     catalog: Option<Arc<Mutex<dyn BackupCatalogWriter + Send>>>,
     options: PxarCreateOptions,
+    creation_command: String,
 ) -> Result<(), Error>
 where
     T: SeqWrite + Send,
@@ -199,6 +200,14 @@ where
         skip_e2big_xattr: options.skip_e2big_xattr,
     };
 
+    archiver
+        .encode_file(
+            &mut encoder,
+            &CString::new(".pxar_creation_params").unwrap(),
+            creation_command.as_bytes(),
+        )
+        .await
+        .unwrap();
     archiver
         .archive_dir_contents(&mut encoder, source_dir, true)
         .await?;
diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/pxar_backup_stream.rs
index 22a6ffdc..52ed8f3c 100644
--- a/pbs-client/src/pxar_backup_stream.rs
+++ b/pbs-client/src/pxar_backup_stream.rs
@@ -40,6 +40,7 @@ impl PxarBackupStream {
         dir: Dir,
         catalog: Arc<Mutex<CatalogWriter<W>>>,
         options: crate::pxar::PxarCreateOptions,
+        creation_command: String,
     ) -> Result<Self, Error> {
         let (tx, rx) = std::sync::mpsc::sync_channel(10);
 
@@ -64,6 +65,7 @@ impl PxarBackupStream {
                 },
                 Some(catalog),
                 options,
+                creation_command,
             )
             .await
             {
@@ -87,10 +89,11 @@ impl PxarBackupStream {
         dirname: &Path,
         catalog: Arc<Mutex<CatalogWriter<W>>>,
         options: crate::pxar::PxarCreateOptions,
+        creation_command: String,
     ) -> Result<Self, Error> {
         let dir = nix::dir::Dir::open(dirname, OFlag::O_DIRECTORY, Mode::empty())?;
 
-        Self::new(dir, catalog, options)
+        Self::new(dir, catalog, options, creation_command)
     }
 }
 
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 546275cb..0160a177 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -191,8 +191,14 @@ async fn backup_directory<P: AsRef<Path>>(
     catalog: Arc<Mutex<CatalogWriter<TokioWriterAdapter<StdChannelWriter<Error>>>>>,
     pxar_create_options: pbs_client::pxar::PxarCreateOptions,
     upload_options: UploadOptions,
+    creation_command: String,
 ) -> Result<BackupStats, Error> {
-    let pxar_stream = PxarBackupStream::open(dir_path.as_ref(), catalog, pxar_create_options)?;
+    let pxar_stream = PxarBackupStream::open(
+        dir_path.as_ref(),
+        catalog,
+        pxar_create_options,
+        creation_command,
+    )?;
     let mut chunk_stream = ChunkStream::new(pxar_stream, chunk_size);
 
     let (tx, rx) = mpsc::channel(10); // allow to buffer 10 chunks
@@ -677,10 +683,6 @@ fn spawn_catalog_upload(
 /// Create (host) backup.
 async fn create_backup(
     param: Value,
-    all_file_systems: bool,
-    skip_lost_and_found: bool,
-    dry_run: bool,
-    skip_e2big_xattr: bool,
     _info: &ApiMethod,
     _rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
@@ -737,14 +739,14 @@ async fn create_backup(
         );
     }
 
-    let mut devices = if all_file_systems {
+    let mut devices = if param["all-file-systems"].as_bool().unwrap_or(false) {
         None
     } else {
         Some(HashSet::new())
     };
 
     if let Some(include_dev) = include_dev {
-        if all_file_systems {
+        if param["all-file-systems"].as_bool().unwrap_or(false) {
             bail!("option 'all-file-systems' conflicts with option 'include-dev'");
         }
 
@@ -940,12 +942,16 @@ async fn create_backup(
     let mut catalog_result_rx = None;
 
     let log_file = |desc: &str, file: &str, target: &str| {
-        let what = if dry_run { "Would upload" } else { "Upload" };
+        let what = if param["dry-run"].as_bool().unwrap_or(false) {
+            "Would upload"
+        } else {
+            "Upload"
+        };
         log::info!("{} {} '{}' to '{}' as {}", what, desc, file, repo, target);
     };
 
     for (backup_type, filename, target, size) in upload_list {
-        match (backup_type, dry_run) {
+        match (backup_type, param["dry-run"].as_bool().unwrap_or(false)) {
             // dry-run
             (BackupSpecificationType::CONFIG, true) => log_file("config file", &filename, &target),
             (BackupSpecificationType::LOGFILE, true) => log_file("log file", &filename, &target),
@@ -995,6 +1001,8 @@ async fn create_backup(
                     .unwrap()
                     .start_directory(std::ffi::CString::new(target.as_str())?.as_c_str())?;
 
+                let skip_lost_and_found = param["skip-lost-and-found"].as_bool().unwrap_or(false);
+                let skip_e2big_xattr = param["skip-e2big-xattr"].as_bool().unwrap_or(false);
                 let pxar_options = pbs_client::pxar::PxarCreateOptions {
                     device_set: devices.clone(),
                     patterns: pattern_list.clone(),
@@ -1018,6 +1026,7 @@ async fn create_backup(
                     catalog.clone(),
                     pxar_options,
                     upload_options,
+                    param.to_string(),
                 )
                 .await?;
                 manifest.add_file(target, stats.size, stats.csum, crypto.mode)?;
@@ -1041,7 +1050,7 @@ async fn create_backup(
         }
     }
 
-    if dry_run {
+    if param["dry-run"].as_bool().unwrap_or(false) {
         log::info!("dry-run: no upload happened");
         return Ok(Value::Null);
     }
diff --git a/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs b/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
index c2055222..d1f141af 100644
--- a/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
+++ b/proxmox-restore-daemon/src/proxmox_restore_daemon/api.rs
@@ -356,7 +356,7 @@ fn extract(
                     };
 
                     let pxar_writer = TokioWriter::new(writer);
-                    create_archive(dir, pxar_writer, Flags::DEFAULT, |_| Ok(()), None, options)
+                    create_archive(dir, pxar_writer, Flags::DEFAULT, |_| Ok(()), None, options, "".to_string())
                         .await
                 }
                 .await;
diff --git a/pxar-bin/src/main.rs b/pxar-bin/src/main.rs
index 2bbe90e3..277d0b46 100644
--- a/pxar-bin/src/main.rs
+++ b/pxar-bin/src/main.rs
@@ -384,6 +384,7 @@ async fn create_archive(
         },
         None,
         options,
+        "".to_string(),
     )
     .await?;
 
diff --git a/tests/catar.rs b/tests/catar.rs
index 36bb4f3b..22a23925 100644
--- a/tests/catar.rs
+++ b/tests/catar.rs
@@ -40,6 +40,7 @@ fn run_test(dir_name: &str) -> Result<(), Error> {
         |_| Ok(()),
         None,
         options,
+        "".to_string(),
     ))?;
 
     Command::new("cmp")
-- 
2.43.0

[pbs-devel] [PATCH proxmox-backup 3/3] pxar: added creation parameters

[Gabriel Goller]

This adds support for the creation parameters and the
`.pxar_creation_params` file into the `pxar` binary.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 pxar-bin/Cargo.toml  |  1 +
 pxar-bin/src/main.rs | 68 +++++++++++++++++++++++++++-----------------
 2 files changed, 43 insertions(+), 26 deletions(-)

diff --git a/pxar-bin/Cargo.toml b/pxar-bin/Cargo.toml
index d91c03d3..4ed5c29a 100644
--- a/pxar-bin/Cargo.toml
+++ b/pxar-bin/Cargo.toml
@@ -13,6 +13,7 @@ anyhow.workspace = true
 futures.workspace = true
 log.workspace = true
 nix.workspace = true
+serde.workspace = true
 serde_json.workspace = true
 tokio = { workspace = true, features = [ "rt", "rt-multi-thread" ] }
 
diff --git a/pxar-bin/src/main.rs b/pxar-bin/src/main.rs
index 277d0b46..f0db2440 100644
--- a/pxar-bin/src/main.rs
+++ b/pxar-bin/src/main.rs
@@ -9,6 +9,7 @@ use std::sync::Arc;
 use anyhow::{bail, format_err, Error};
 use futures::future::FutureExt;
 use futures::select;
+use serde_json::Value;
 use tokio::signal::unix::{signal, SignalKind};
 
 use pathpatterns::{MatchEntry, MatchType, PatternFlag};
@@ -234,46 +235,71 @@ fn extract_archive(
     Ok(())
 }
 
+#[derive(serde::Deserialize)]
+#[serde(rename_all = "kebab-case")]
+struct CreateArchiveParams {
+    archive: String,
+    source: String,
+    no_xattrs: Option<bool>,
+    no_fcaps: Option<bool>,
+    no_acls: Option<bool>,
+    all_file_systems: Option<bool>,
+    no_device_nodes: Option<bool>,
+    no_fifos: Option<bool>,
+    no_sockets: Option<bool>,
+    exclude: Option<Vec<String>>,
+    entries_max: Option<isize>,
+}
+
 #[api(
     input: {
         properties: {
             archive: {
+                type: String,
                 description: "Archive name.",
             },
             source: {
+                type: String,
                 description: "Source directory.",
             },
             "no-xattrs": {
+                type: Boolean,
                 description: "Ignore extended file attributes.",
                 optional: true,
                 default: false,
             },
             "no-fcaps": {
+                type: Boolean,
                 description: "Ignore file capabilities.",
                 optional: true,
                 default: false,
             },
             "no-acls": {
+                type: Boolean,
                 description: "Ignore access control list entries.",
                 optional: true,
                 default: false,
             },
             "all-file-systems": {
+                type: Boolean,
                 description: "Include mounted sudirs.",
                 optional: true,
                 default: false,
             },
             "no-device-nodes": {
+                type: Boolean,
                 description: "Ignore device nodes.",
                 optional: true,
                 default: false,
             },
             "no-fifos": {
+                type: Boolean,
                 description: "Ignore fifos.",
                 optional: true,
                 default: false,
             },
             "no-sockets": {
+                type: Boolean,
                 description: "Ignore sockets.",
                 optional: true,
                 default: false,
@@ -288,6 +314,7 @@ fn extract_archive(
                 },
             },
             "entries-max": {
+                type: Integer,
                 description: "Max number of entries loaded at once into memory",
                 optional: true,
                 default: ENCODER_MAX_ENTRIES as isize,
@@ -298,22 +325,11 @@ fn extract_archive(
     },
 )]
 /// Create a new .pxar archive.
-#[allow(clippy::too_many_arguments)]
-async fn create_archive(
-    archive: String,
-    source: String,
-    no_xattrs: bool,
-    no_fcaps: bool,
-    no_acls: bool,
-    all_file_systems: bool,
-    no_device_nodes: bool,
-    no_fifos: bool,
-    no_sockets: bool,
-    exclude: Option<Vec<String>>,
-    entries_max: isize,
-) -> Result<(), Error> {
+async fn create_archive(param: Value) -> Result<(), Error> {
+    let params: CreateArchiveParams = serde_json::from_value(param.clone()).unwrap();
+
     let patterns = {
-        let input = exclude.unwrap_or_default();
+        let input = params.exclude.unwrap_or_default();
         let mut patterns = Vec::with_capacity(input.len());
         for entry in input {
             patterns.push(
@@ -324,21 +340,21 @@ async fn create_archive(
         patterns
     };
 
-    let device_set = if all_file_systems {
+    let device_set = if params.all_file_systems.unwrap_or(false) {
         None
     } else {
         Some(HashSet::new())
     };
 
     let options = pbs_client::pxar::PxarCreateOptions {
-        entries_max: entries_max as usize,
+        entries_max: params.entries_max.unwrap_or(ENCODER_MAX_ENTRIES as isize) as usize,
         device_set,
         patterns,
         skip_lost_and_found: false,
         skip_e2big_xattr: false,
     };
 
-    let source = PathBuf::from(source);
+    let source = PathBuf::from(params.source);
 
     let dir = nix::dir::Dir::open(
         &source,
@@ -350,26 +366,26 @@ async fn create_archive(
         .create_new(true)
         .write(true)
         .mode(0o640)
-        .open(archive)?;
+        .open(params.archive)?;
 
     let writer = std::io::BufWriter::with_capacity(1024 * 1024, file);
     let mut feature_flags = Flags::DEFAULT;
-    if no_xattrs {
+    if params.no_xattrs.unwrap_or(false) {
         feature_flags.remove(Flags::WITH_XATTRS);
     }
-    if no_fcaps {
+    if params.no_fcaps.unwrap_or(false) {
         feature_flags.remove(Flags::WITH_FCAPS);
     }
-    if no_acls {
+    if params.no_acls.unwrap_or(false) {
         feature_flags.remove(Flags::WITH_ACL);
     }
-    if no_device_nodes {
+    if params.no_device_nodes.unwrap_or(false) {
         feature_flags.remove(Flags::WITH_DEVICE_NODES);
     }
-    if no_fifos {
+    if params.no_fifos.unwrap_or(false) {
         feature_flags.remove(Flags::WITH_FIFOS);
     }
-    if no_sockets {
+    if params.no_sockets.unwrap_or(false) {
         feature_flags.remove(Flags::WITH_SOCKETS);
     }
 
@@ -384,7 +400,7 @@ async fn create_archive(
         },
         None,
         options,
-        "".to_string(),
+        param.to_string(),
     )
     .await?;
 
-- 
2.43.0

Re: [pbs-devel] [RFC proxmox-backup 0/3] Encode creation parameters into pxar archive

[Christian Ebner]

> On 06.03.2024 15:34 CET Gabriel Goller <g.goller@proxmox.com> wrote:
> 
>  
> To better debug the creation of the pxar archive, we encode the 
> creation_params (the parameters passed to the cli) inside the pxar
> archive. Like this we just need to locate the `.pxar_creation_params`
> file in the archive and we can check which arguments have been used. 
> 
> To realize this, we unify all the parameters of the `create_backup`
> function so that we don't need an additional struct (which in turn
> introduces additional maintenance efforts).
> 
> An example `.pxar_creation_params` file would look like this:
> 
>     {
>       "backupspec": [
>         "test.pxar:linux"
>       ],
>       "chunk-size": 4096,
>       "exclude": [
>         "MAINTAINERS"
>       ],
>       "skip-e2big-xattr": true
>     }
>

While this approach has already been used up until now for storing the
pxar cli exclude patterns, allowing to encode such metadata inside the
archive as regular file without having any means other than the filename
to find and distinguish this information from other files seems not
ideal to me.

Wouldn't it make sense to extend the pxar file format by a dedicated
entry type to store such information? And handle these entries in a
dedicated manner? E.g. by a `PXAR_CLI_PARAMS` entry header?

This would of course require an updated pxar format version 2, which we
might need anyways if the patches regarding metadata based change
detection should be applied.

Cheers,
Chris

Re: [pbs-devel] [RFC proxmox-backup 0/3] Encode creation parameters into pxar archive

[Gabriel Goller]

Thanks for the review!

On Wed Mar 6, 2024 at 4:13 PM CET, Christian Ebner wrote:
> Wouldn't it make sense to extend the pxar file format by a dedicated
> entry type to store such information? And handle these entries in a
> dedicated manner? E.g. by a `PXAR_CLI_PARAMS` entry header?

I though about this but disregarded it ultimately because of the whole 
version update and backwards compat hassle. IMO this is 'debug data' 
and 'extra information' so it would fit better inside the archive (same
as the exclude-patterns, these are also not really important after the 
archive has been created (except for debugging)). If for some reason 
this data is missing it wouldn't be a problem at all.

> While this approach has already been used up until now for storing the
> pxar cli exclude patterns, allowing to encode such metadata inside the
> archive as regular file without having any means other than the filename
> to find and distinguish this information from other files seems not
> ideal to me.

We could also specify some kind of prefix for pxar-specific files,
something like '.pxar_***'? This way we stay consistent and users can
distinguish manually encoded files from their files.
But I also get your point about having regular files appear in the pxar
archive could be confusing to some. And obviously having 3+ of these
files is a no-go.

> This would of course require an updated pxar format version 2, which we
> might need anyways if the patches regarding metadata based change
> detection should be applied.

Although this is a good point, if we merge both series in the same
window, we would only have to do one update! 


For some reason—now that I am writing this obviously—I am currently more
inclined to your version. Mostly because the `.pxarexclude` file also
allows input (e.g. the user can create the file insert stuff), while 
the `.pxar_creation_params` does not allow input and would have to 
overwrite/ignore an existing file.

I am open to other suggestions/inputs!

Re: [pbs-devel] [RFC proxmox-backup 0/3] Encode creation parameters into pxar archive

[Thomas Lamprecht]

Am 06/03/2024 um 16:13 schrieb Christian Ebner:
>> On 06.03.2024 15:34 CET Gabriel Goller <g.goller@proxmox.com> wrote:
>>
>>  
>> To better debug the creation of the pxar archive, we encode the 
>> creation_params (the parameters passed to the cli) inside the pxar
>> archive. Like this we just need to locate the `.pxar_creation_params`
>> file in the archive and we can check which arguments have been used. 
>>
>> To realize this, we unify all the parameters of the `create_backup`
>> function so that we don't need an additional struct (which in turn
>> introduces additional maintenance efforts).
>>
>> An example `.pxar_creation_params` file would look like this:
>>
>>     {
>>       "backupspec": [
>>         "test.pxar:linux"
>>       ],
>>       "chunk-size": 4096,
>>       "exclude": [
>>         "MAINTAINERS"
>>       ],
>>       "skip-e2big-xattr": true
>>     }
>>
> 
> While this approach has already been used up until now for storing the
> pxar cli exclude patterns, allowing to encode such metadata inside the
> archive as regular file without having any means other than the filename
> to find and distinguish this information from other files seems not
> ideal to me.
> 
> Wouldn't it make sense to extend the pxar file format by a dedicated
> entry type to store such information? And handle these entries in a
> dedicated manner? E.g. by a `PXAR_CLI_PARAMS` entry header?

+1 – I do not like mixing it external (meta) data that has nothing to
do with the actual user data being backed up inside the archive.

> This would of course require an updated pxar format version 2, which we
> might need anyways if the patches regarding metadata based change
> detection should be applied.

Yes, if we need to bump that for your stuff it would be worth adding
above too (and maybe some more general way to extend metadata info for
the future without new version bumps required).
Otherwise, I'd rather just do not add it at all for now as just bumping
for this seems a bit overkill and as said, inside the archive is the
wrong place.