[pbs-devel] [PATCH proxmox-backup 0/8] drop catalog encoding for split pxar archives

[pbs-devel] [PATCH proxmox-backup 0/8] drop catalog encoding for split pxar archives

[Christian Ebner]

The catalog stores metadata needed for fast lookups via e.g. the file
browser for single file restore.
Since it is now possible to create snapshots with split pxar archives,
the same metadata can be accessed in a reasonable performat way also via
the metadata archive, effectively making the catalog unneeded duplicate
data for this case.

This patch series therefore allows to lookup directory entries where
needed via the metadata archive instead of the catalog and conditionally
drops the catalog encoding for snapshots using split archive encoding.

The patches adapt not only the `catalog` api endpoint, used by the
Proxmox Backup WebUI to access the snapshots contents but also the
proxomox-file-restore, which is used by the Proxmox Virtual Environments
PBS storage plugin to access and restore contents.

Tested that regular backups can still be inspected and restored and that
the same functionality is still available for split archive backups,
without now having a catalog encoded.

Functionally was tested with the following path series applied to PVE:
https://lists.proxmox.com/pipermail/pve-devel/2024-May/064024.html

Please note: For directories with may entries a noticeable performance
drop is present, which might be improved upon by optimizing decoding.

Christian Ebner (8):
  api: datastore: factor out path decoding for catalog
  api: datastore: move reusable code out of thread
  client: tools: add helper to lookup `ArchiveEntry`s via pxar
  api: datastore: conditional lookup for catalog endpoint
  api: datastore: add optional archive-name to file-restore
  file-restore: fallback to mpxar if catalog not present
  www: content: lookup via metadata archive instead of catalog
  client: backup: conditionally write catalog for file level backups

 pbs-client/src/pxar_backup_stream.rs |  11 ++-
 pbs-client/src/tools/mod.rs          |  84 +++++++++++++++++
 proxmox-backup-client/src/main.rs    |  21 +++--
 proxmox-file-restore/src/main.rs     |  72 +++++++++++----
 src/api2/admin/datastore.rs          | 129 ++++++++++++++++++---------
 www/datastore/Content.js             |   3 +
 6 files changed, 251 insertions(+), 69 deletions(-)

-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 1/8] api: datastore: factor out path decoding for catalog

[Christian Ebner]

The file path passed to the catalog is base64 encoded, with an exception
for the root.
Factor this check and decoding step out into a helper function to make
it reusable when doing the same for lookups via the metadata archive
instead of the catalog.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 34a9105dd..854302999 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1636,6 +1636,14 @@ pub fn upload_backup_log(
     .boxed()
 }
 
+fn decode_path(path: &str) -> Result<Vec<u8>, Error> {
+    if path != "root" && path != "/" {
+        base64::decode(path).map_err(|err| format_err!("base64 decoding of path failed - {err}"))
+    } else {
+        Ok(vec![b'/'])
+    }
+}
+
 #[api(
     input: {
         properties: {
@@ -1709,12 +1717,7 @@ pub async fn catalog(
 
         let mut catalog_reader = CatalogReader::new(reader);
 
-        let path = if filepath != "root" && filepath != "/" {
-            base64::decode(filepath)?
-        } else {
-            vec![b'/']
-        };
-
+        let path = decode_path(&filepath)?;
         catalog_reader.list_dir_contents(&path)
     })
     .await?
-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 2/8] api: datastore: move reusable code out of thread

[Christian Ebner]

Move code that can be reused when having to  perform a lookup via the
pxar metadata archive instead of the catalog out of the thread.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 854302999..117dab080 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1678,30 +1678,30 @@ pub async fn catalog(
 ) -> Result<Vec<ArchiveEntry>, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
-    tokio::task::spawn_blocking(move || {
-        let ns = ns.unwrap_or_default();
+    let ns = ns.unwrap_or_default();
 
-        let datastore = check_privs_and_load_store(
-            &store,
-            &ns,
-            &auth_id,
-            PRIV_DATASTORE_READ,
-            PRIV_DATASTORE_BACKUP,
-            Some(Operation::Read),
-            &backup_dir.group,
-        )?;
+    let datastore = check_privs_and_load_store(
+        &store,
+        &ns,
+        &auth_id,
+        PRIV_DATASTORE_READ,
+        PRIV_DATASTORE_BACKUP,
+        Some(Operation::Read),
+        &backup_dir.group,
+    )?;
 
-        let backup_dir = datastore.backup_dir(ns, backup_dir)?;
+    let backup_dir = datastore.backup_dir(ns, backup_dir)?;
 
-        let file_name = CATALOG_NAME;
+    let file_name = CATALOG_NAME;
 
-        let (manifest, files) = read_backup_index(&backup_dir)?;
-        for file in files {
-            if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) {
-                bail!("cannot decode '{}' - is encrypted", file_name);
-            }
+    let (manifest, files) = read_backup_index(&backup_dir)?;
+    for file in files {
+        if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) {
+            bail!("cannot decode '{file_name}' - is encrypted");
         }
+    }
 
+    tokio::task::spawn_blocking(move || {
         let mut path = datastore.base_path();
         path.push(backup_dir.relative_path());
         path.push(file_name);
-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 3/8] client: tools: add helper to lookup `ArchiveEntry`s via pxar

[Christian Ebner]

In preparation to lookup entries via the pxar metadata archive
instead of the catalog, in order to drop encoding the catalog
for snapshots using split pxar archives altogehter.

This helper allows to lookup the directory entries via the provided
accessor instance and formats them to be compatible with the output
as produced by lookups via the catalog.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-client/src/tools/mod.rs | 84 +++++++++++++++++++++++++++++++++++++
 1 file changed, 84 insertions(+)

diff --git a/pbs-client/src/tools/mod.rs b/pbs-client/src/tools/mod.rs
index 29cd33df1..436475738 100644
--- a/pbs-client/src/tools/mod.rs
+++ b/pbs-client/src/tools/mod.rs
@@ -1,9 +1,12 @@
 //! Shared tools useful for common CLI clients.
 use std::collections::HashMap;
 use std::env::VarError::{NotPresent, NotUnicode};
+use std::ffi::OsStr;
 use std::fs::File;
 use std::io::{BufRead, BufReader};
+use std::os::unix::ffi::OsStrExt;
 use std::os::unix::io::FromRawFd;
+use std::path::PathBuf;
 use std::process::Command;
 
 use anyhow::{bail, format_err, Context, Error};
@@ -16,7 +19,12 @@ use proxmox_schema::*;
 use proxmox_sys::fs::file_get_json;
 
 use pbs_api_types::{Authid, BackupNamespace, RateLimitConfig, UserWithTokens, BACKUP_REPO_URL};
+use pbs_datastore::catalog::{ArchiveEntry, DirEntryAttribute};
 use pbs_datastore::BackupManifest;
+use pxar::accessor::aio::Accessor;
+use pxar::accessor::ReadAt;
+use pxar::format::SignedDuration;
+use pxar::{mode, EntryKind};
 
 use crate::{BackupRepository, HttpClient, HttpClientOptions};
 
@@ -635,3 +643,79 @@ pub fn raise_nofile_limit() -> Result<libc::rlimit64, Error> {
 
     Ok(old)
 }
+
+/// Look up the directory entries of the given directory `path` in a pxar archive via it's given
+/// `accessor` and return the entries formatted as [`ArchiveEntry`]'s, compatible with reading
+/// entries from the catalog.
+///
+/// If the optional `path_prefix` is given, all returned entry paths will be prefixed with it.
+pub async fn pxar_metadata_catalog_lookup<T: Clone + ReadAt>(
+    accessor: Accessor<T>,
+    path: &OsStr,
+    path_prefix: Option<&str>,
+) -> Result<Vec<ArchiveEntry>, Error> {
+    let root = accessor.open_root().await?;
+    let dir_entry = root
+        .lookup(&path)
+        .await
+        .map_err(|err| format_err!("lookup failed - {err}"))?
+        .ok_or_else(|| format_err!("lookup failed - error opening '{path:?}'"))?;
+
+    let mut entries = Vec::new();
+    if let EntryKind::Directory = dir_entry.kind() {
+        let dir_entry = dir_entry
+            .enter_directory()
+            .await
+            .map_err(|err| format_err!("failed to enter directory - {err}"))?;
+
+        let mut entries_iter = dir_entry.read_dir();
+        while let Some(entry) = entries_iter.next().await {
+            let entry = entry?.decode_entry().await?;
+
+            let entry_attr = match entry.kind() {
+                EntryKind::Version(_) | EntryKind::Prelude(_) | EntryKind::GoodbyeTable => continue,
+                EntryKind::Directory => DirEntryAttribute::Directory {
+                    start: entry.entry_range_info().entry_range.start,
+                },
+                EntryKind::File { size, .. } => {
+                    let mtime = match entry.metadata().mtime_as_duration() {
+                        SignedDuration::Positive(val) => i64::try_from(val.as_secs())?,
+                        SignedDuration::Negative(val) => -1 * i64::try_from(val.as_secs())?,
+                    };
+                    DirEntryAttribute::File { size: *size, mtime }
+                }
+                EntryKind::Device(_) => match entry.metadata().file_type() {
+                    mode::IFBLK => DirEntryAttribute::BlockDevice,
+                    mode::IFCHR => DirEntryAttribute::CharDevice,
+                    _ => bail!("encountered unknown device type"),
+                },
+                EntryKind::Symlink(_) => DirEntryAttribute::Symlink,
+                EntryKind::Hardlink(_) => DirEntryAttribute::Hardlink,
+                EntryKind::Fifo => DirEntryAttribute::Fifo,
+                EntryKind::Socket => DirEntryAttribute::Socket,
+            };
+
+            let entry_path = if let Some(prefix) = path_prefix {
+                let mut entry_path = PathBuf::from(prefix);
+                match entry.path().strip_prefix("/") {
+                    Ok(path) => entry_path.push(path),
+                    Err(_) => entry_path.push(entry.path()),
+                }
+                entry_path
+            } else {
+                PathBuf::from(entry.path())
+            };
+            entries.push(ArchiveEntry::new(
+                entry_path.as_os_str().as_bytes(),
+                Some(&entry_attr),
+            ));
+        }
+    } else {
+        bail!(format!(
+            "expected directory entry, got entry kind '{:?}'",
+            dir_entry.kind()
+        ));
+    }
+
+    Ok(entries)
+}
-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 4/8] api: datastore: conditional lookup for catalog endpoint

[Christian Ebner]

Add an optional `archive-name` parameter, indicating the metadata
archive to be used for directory content lookups instead of the
catalog. If provided, instead of the catalog reader, a pxar Accessor
instance is created to perform the lookup.

This is in preparation for dropping catalog encoding for snapshots
with split pxar archive encoding.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 73 ++++++++++++++++++++++++++++---------
 1 file changed, 55 insertions(+), 18 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 117dab080..e25a78bca 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1659,7 +1659,12 @@ fn decode_path(path: &str) -> Result<Vec<u8>, Error> {
             "filepath": {
                 description: "Base64 encoded path.",
                 type: String,
-            }
+            },
+            "archive-name": {
+                type: String,
+                description: "Name of the archive to lookup given filepath (base64 encoded)",
+                optional: true,
+            },
         },
     },
     access: {
@@ -1674,8 +1679,18 @@ pub async fn catalog(
     ns: Option<BackupNamespace>,
     backup_dir: pbs_api_types::BackupDir,
     filepath: String,
+    archive_name: Option<String>,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<ArchiveEntry>, Error> {
+    let file_name = if let Some(ref archive_name) = archive_name {
+        String::from_utf8(
+            base64::decode(archive_name)
+                .map_err(|err| format_err!("base64 decode of 'archive-name' failed - {err}"))?,
+        )?
+    } else {
+        CATALOG_NAME.to_string()
+    };
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     let ns = ns.unwrap_or_default();
@@ -1692,8 +1707,6 @@ pub async fn catalog(
 
     let backup_dir = datastore.backup_dir(ns, backup_dir)?;
 
-    let file_name = CATALOG_NAME;
-
     let (manifest, files) = read_backup_index(&backup_dir)?;
     for file in files {
         if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) {
@@ -1701,26 +1714,50 @@ pub async fn catalog(
         }
     }
 
-    tokio::task::spawn_blocking(move || {
-        let mut path = datastore.base_path();
-        path.push(backup_dir.relative_path());
-        path.push(file_name);
+    if archive_name.is_none() {
+        tokio::task::spawn_blocking(move || {
+            let mut path = datastore.base_path();
+            path.push(backup_dir.relative_path());
+            path.push(&file_name);
 
-        let index = DynamicIndexReader::open(&path)
-            .map_err(|err| format_err!("unable to read dynamic index '{:?}' - {}", &path, err))?;
+            let index = DynamicIndexReader::open(&path)
+                .map_err(|err| format_err!("unable to read dynamic index '{path:?}' - {err}"))?;
 
-        let (csum, size) = index.compute_csum();
-        manifest.verify_file(file_name, &csum, size)?;
+            let (csum, size) = index.compute_csum();
+            manifest.verify_file(&file_name, &csum, size)?;
 
-        let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
-        let reader = BufferedDynamicReader::new(index, chunk_reader);
+            let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
+            let reader = BufferedDynamicReader::new(index, chunk_reader);
 
-        let mut catalog_reader = CatalogReader::new(reader);
+            let mut catalog_reader = CatalogReader::new(reader);
 
-        let path = decode_path(&filepath)?;
-        catalog_reader.list_dir_contents(&path)
-    })
-    .await?
+            let path = decode_path(&filepath)?;
+            catalog_reader.list_dir_contents(&path)
+        })
+        .await?
+    } else {
+        let (archive_name, payload_archive_name) =
+            pbs_client::tools::get_pxar_archive_names(&file_name, &manifest)?;
+        let (reader, archive_size) =
+            get_local_pxar_reader(datastore.clone(), &manifest, &backup_dir, &archive_name)?;
+
+        let reader = if let Some(payload_archive_name) = payload_archive_name {
+            let payload_input =
+                get_local_pxar_reader(datastore, &manifest, &backup_dir, &payload_archive_name)?;
+            pxar::PxarVariant::Split(reader, payload_input)
+        } else {
+            pxar::PxarVariant::Unified(reader)
+        };
+        let accessor = Accessor::new(reader, archive_size).await?;
+
+        let file_path = decode_path(&filepath)?;
+        pbs_client::tools::pxar_metadata_catalog_lookup(
+            accessor,
+            OsStr::from_bytes(&file_path),
+            None,
+        )
+        .await
+    }
 }
 
 #[sortable]
-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 5/8] api: datastore: add optional archive-name to file-restore

[Christian Ebner]

Allow to pass the archive name as optional api call parameter instead
of having it as prefix to the path.
If this parameter is given, instead of splitting of the archive name
from the path, the parameter itself is used, leaving the path
untouched.

This allows to restore single files from the archive, without having
to artificially construct the path in case of file restores for split
pxar archives, where the response path of the listing does not
include the archive, as opposed to the response provided by lookup
via the catalog.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/admin/datastore.rs | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index e25a78bca..d33422d13 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1773,6 +1773,7 @@ pub const API_METHOD_PXAR_FILE_DOWNLOAD: ApiMethod = ApiMethod::new(
             ("backup-time", false, &BACKUP_TIME_SCHEMA),
             ("filepath", false, &StringSchema::new("Base64 encoded path").schema()),
             ("tar", true, &BooleanSchema::new("Download as .tar.zst").schema()),
+            ("archive-name", true, &StringSchema::new("Base64 encoded archive name").schema()),
         ]),
     )
 ).access(
@@ -1840,9 +1841,17 @@ pub fn pxar_file_download(
             components.remove(0);
         }
 
-        let mut split = components.splitn(2, |c| *c == b'/');
-        let pxar_name = std::str::from_utf8(split.next().unwrap())?;
-        let file_path = split.next().unwrap_or(b"/");
+        let (pxar_name, file_path) = if let Some(archive_name) = param["archive-name"].as_str() {
+            let archive_name = base64::decode(archive_name)
+                .map_err(|err| format_err!("base64 decode of archive-name failed - {err}"))?;
+            (archive_name, base64::decode(&filepath)?)
+        } else {
+            let mut split = components.splitn(2, |c| *c == b'/');
+            let pxar_name = split.next().unwrap();
+            let file_path = split.next().unwrap_or(b"/");
+            (pxar_name.to_owned(), file_path.to_owned())
+        };
+        let pxar_name = std::str::from_utf8(&pxar_name)?;
         let (manifest, files) = read_backup_index(&backup_dir)?;
         for file in files {
             if file.filename == pxar_name && file.crypt_mode == Some(CryptMode::Encrypt) {
@@ -1865,7 +1874,7 @@ pub fn pxar_file_download(
         let decoder = Accessor::new(reader, archive_size).await?;
 
         let root = decoder.open_root().await?;
-        let path = OsStr::from_bytes(file_path).to_os_string();
+        let path = OsStr::from_bytes(&file_path).to_os_string();
         let file = root
             .lookup(&path)
             .await?
-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 6/8] file-restore: fallback to mpxar if catalog not present

[Christian Ebner]

The `proxmox-file-restore list` command will uses the provided path to
lookup and list directory entries via the catalog. Fallback to using
the metadata archive if the catalog is not present for fast lookups in
a backup snapshot.

This is in preparation for dropping encoding of the catalog for
snapshots using split archive encoding. Proxmox VE's storage plugin
uses this to allow single file restore for LXCs.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 proxmox-file-restore/src/main.rs | 72 +++++++++++++++++++++++++-------
 1 file changed, 56 insertions(+), 16 deletions(-)

diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
index 38cc1ce85..a09873467 100644
--- a/proxmox-file-restore/src/main.rs
+++ b/proxmox-file-restore/src/main.rs
@@ -124,7 +124,8 @@ async fn list_files(
         ExtractPath::ListArchives => {
             let mut entries = vec![];
             for file in manifest.files() {
-                if !has_pxar_filename_extension(&file.filename, true)
+                if !file.filename.ends_with(".pxar.didx")
+                    && !file.filename.ends_with(".mpxar.didx")
                     && !file.filename.ends_with(".img.fidx")
                 {
                     continue;
@@ -146,24 +147,63 @@ async fn list_files(
             Ok(entries)
         }
         ExtractPath::Pxar(file, mut path) => {
-            let index = client
-                .download_dynamic_index(&manifest, CATALOG_NAME)
+            if let Ok(file_info) = manifest.lookup_file_info(CATALOG_NAME) {
+                let index = client
+                    .download_dynamic_index(&manifest, CATALOG_NAME)
+                    .await?;
+                let most_used = index.find_most_used_chunks(8);
+                let chunk_reader = RemoteChunkReader::new(
+                    client.clone(),
+                    crypt_config,
+                    file_info.chunk_crypt_mode(),
+                    most_used,
+                );
+                let reader = BufferedDynamicReader::new(index, chunk_reader);
+                let mut catalog_reader = CatalogReader::new(reader);
+
+                let mut fullpath = file.into_bytes();
+                fullpath.append(&mut path);
+
+                catalog_reader.list_dir_contents(&fullpath)
+            } else {
+                if path.is_empty() {
+                    path = vec![b'/'];
+                }
+
+                let (archive_name, payload_archive_name) =
+                    pbs_client::tools::get_pxar_archive_names(&file, &manifest)?;
+
+                let (reader, archive_size) = get_remote_pxar_reader(
+                    &archive_name,
+                    client.clone(),
+                    &manifest,
+                    crypt_config.clone(),
+                )
                 .await?;
-            let most_used = index.find_most_used_chunks(8);
-            let file_info = manifest.lookup_file_info(CATALOG_NAME)?;
-            let chunk_reader = RemoteChunkReader::new(
-                client.clone(),
-                crypt_config,
-                file_info.chunk_crypt_mode(),
-                most_used,
-            );
-            let reader = BufferedDynamicReader::new(index, chunk_reader);
-            let mut catalog_reader = CatalogReader::new(reader);
 
-            let mut fullpath = file.into_bytes();
-            fullpath.append(&mut path);
+                let reader = if let Some(payload_archive_name) = payload_archive_name {
+                    let (payload_reader, payload_size) = get_remote_pxar_reader(
+                        &payload_archive_name,
+                        client,
+                        &manifest,
+                        crypt_config,
+                    )
+                    .await?;
+                    pxar::PxarVariant::Split(reader, (payload_reader, payload_size))
+                } else {
+                    pxar::PxarVariant::Unified(reader)
+                };
+
+                let accessor = Accessor::new(reader, archive_size).await?;
+                let path = OsStr::from_bytes(&path);
 
-            catalog_reader.list_dir_contents(&fullpath)
+                pbs_client::tools::pxar_metadata_catalog_lookup(
+                    accessor,
+                    &path,
+                    Some(&archive_name),
+                )
+                .await
+            }
         }
         ExtractPath::VM(file, path) => {
             let details = SnapRestoreDetails {
-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 7/8] www: content: lookup via metadata archive instead of catalog

[Christian Ebner]

In case of pxar archives with split metadata and payload data, the
metadata archive has to be used to lookup entries for navigation
before performing a single file restore.

Decide based on the archive filename extension whether to use the
`catalog` or the `pxar-lookup` api endpoint.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 www/datastore/Content.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index 6dd1ab319..43be6a6c7 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -786,6 +786,9 @@ Ext.define('PBS.DataStoreContent', {
 		'backup-time': (time.getTime()/1000).toFixed(0),
 		'backup-type': type,
 	    };
+	    if (rec.data.filename.endsWith(".mpxar.didx")) {
+		extraParams['archive-name'] = btoa(rec.data.filename);
+	    }
 	    if (view.namespace && view.namespace !== '') {
 		extraParams.ns = view.namespace;
 	    }
-- 
2.39.2



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

[pbs-devel] [PATCH proxmox-backup 8/8] client: backup: conditionally write catalog for file level backups

[Christian Ebner]

Only write the catalog when using the regular backup mode, do not write
it when using the split archive mode.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-client/src/pxar_backup_stream.rs | 11 +++++++----
 proxmox-backup-client/src/main.rs    | 21 ++++++++++++---------
 2 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/pxar_backup_stream.rs
index f322566f0..cdbcdfec2 100644
--- a/pbs-client/src/pxar_backup_stream.rs
+++ b/pbs-client/src/pxar_backup_stream.rs
@@ -15,7 +15,7 @@ use nix::sys::stat::Mode;
 use proxmox_async::blocking::TokioWriterAdapter;
 use proxmox_io::StdChannelWriter;
 
-use pbs_datastore::catalog::CatalogWriter;
+use pbs_datastore::catalog::{BackupCatalogWriter, CatalogWriter};
 
 use crate::inject_reused_chunks::InjectChunks;
 use crate::pxar::create::PxarWriters;
@@ -42,7 +42,7 @@ impl Drop for PxarBackupStream {
 impl PxarBackupStream {
     pub fn new<W: Write + Send + 'static>(
         dir: Dir,
-        catalog: Arc<Mutex<CatalogWriter<W>>>,
+        catalog: Option<Arc<Mutex<CatalogWriter<W>>>>,
         options: crate::pxar::PxarCreateOptions,
         boundaries: Option<mpsc::Sender<InjectChunks>>,
         separate_payload_stream: bool,
@@ -82,7 +82,10 @@ impl PxarBackupStream {
         let handler = async move {
             if let Err(err) = crate::pxar::create_archive(
                 dir,
-                PxarWriters::new(writer, Some(catalog)),
+                PxarWriters::new(
+                    writer,
+                    catalog.map(|c| c as Arc<Mutex<dyn BackupCatalogWriter + Send>>),
+                ),
                 crate::pxar::Flags::DEFAULT,
                 move |path| {
                     log::debug!("{:?}", path);
@@ -122,7 +125,7 @@ impl PxarBackupStream {
 
     pub fn open<W: Write + Send + 'static>(
         dirname: &Path,
-        catalog: Arc<Mutex<CatalogWriter<W>>>,
+        catalog: Option<Arc<Mutex<CatalogWriter<W>>>>,
         options: crate::pxar::PxarCreateOptions,
         boundaries: Option<mpsc::Sender<InjectChunks>>,
         separate_payload_stream: bool,
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 09af912df..e50979d6e 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -192,7 +192,7 @@ async fn backup_directory<P: AsRef<Path>>(
     archive_name: &str,
     payload_target: Option<&str>,
     chunk_size: Option<usize>,
-    catalog: Arc<Mutex<CatalogWriter<TokioWriterAdapter<StdChannelWriter<Error>>>>>,
+    catalog: Option<Arc<Mutex<CatalogWriter<TokioWriterAdapter<StdChannelWriter<Error>>>>>>,
     pxar_create_options: pbs_client::pxar::PxarCreateOptions,
     upload_options: UploadOptions,
 ) -> Result<(BackupStats, Option<BackupStats>), Error> {
@@ -1059,19 +1059,20 @@ async fn create_backup(
                     };
 
                 // start catalog upload on first use
-                if catalog.is_none() {
+                if catalog.is_none() && !detection_mode.is_data() && !detection_mode.is_metadata() {
                     let catalog_upload_res =
                         spawn_catalog_upload(client.clone(), crypto.mode == CryptMode::Encrypt)?;
                     catalog = Some(catalog_upload_res.catalog_writer);
                     catalog_result_rx = Some(catalog_upload_res.result);
                 }
-                let catalog = catalog.as_ref().unwrap();
 
                 log_file("directory", &filename, &target);
-                catalog
-                    .lock()
-                    .unwrap()
-                    .start_directory(std::ffi::CString::new(target.as_str())?.as_c_str())?;
+                if let Some(catalog) = catalog.as_ref() {
+                    catalog
+                        .lock()
+                        .unwrap()
+                        .start_directory(std::ffi::CString::new(target.as_str())?.as_c_str())?;
+                }
 
                 let mut previous_ref = None;
                 let max_cache_size = if detection_mode.is_metadata() {
@@ -1139,7 +1140,7 @@ async fn create_backup(
                     &target,
                     payload_target.as_deref(),
                     chunk_size_opt,
-                    catalog.clone(),
+                    catalog.as_ref().map(|c| c.clone()),
                     pxar_options,
                     upload_options,
                 )
@@ -1155,7 +1156,9 @@ async fn create_backup(
                     )?;
                 }
                 manifest.add_file(target, stats.size, stats.csum, crypto.mode)?;
-                catalog.lock().unwrap().end_directory()?;
+                if let Some(catalog) = catalog.as_ref() {
+                    catalog.lock().unwrap().end_directory()?;
+                }
             }
             (BackupSpecificationType::IMAGE, false) => {
                 log_file("image", &filename, &target);
-- 
2.39.2



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

Re: [pbs-devel] [PATCH proxmox-backup 4/8] api: datastore: conditional lookup for catalog endpoint

[Fabian Grünbichler]

On June 7, 2024 11:43 am, Christian Ebner wrote:
> Add an optional `archive-name` parameter, indicating the metadata
> archive to be used for directory content lookups instead of the
> catalog. If provided, instead of the catalog reader, a pxar Accessor
> instance is created to perform the lookup.
> 
> This is in preparation for dropping catalog encoding for snapshots
> with split pxar archive encoding.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 73 ++++++++++++++++++++++++++++---------
>  1 file changed, 55 insertions(+), 18 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 117dab080..e25a78bca 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1659,7 +1659,12 @@ fn decode_path(path: &str) -> Result<Vec<u8>, Error> {
>              "filepath": {
>                  description: "Base64 encoded path.",
>                  type: String,
> -            }
> +            },
> +            "archive-name": {
> +                type: String,
> +                description: "Name of the archive to lookup given filepath (base64 encoded)",
> +                optional: true,
> +            },

why is this base64 encoded? the archive name is a safe ID..

>          },
>      },
>      access: {
> @@ -1674,8 +1679,18 @@ pub async fn catalog(
>      ns: Option<BackupNamespace>,
>      backup_dir: pbs_api_types::BackupDir,
>      filepath: String,
> +    archive_name: Option<String>,
>      rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<Vec<ArchiveEntry>, Error> {
> +    let file_name = if let Some(ref archive_name) = archive_name {
> +        String::from_utf8(
> +            base64::decode(archive_name)
> +                .map_err(|err| format_err!("base64 decode of 'archive-name' failed - {err}"))?,
> +        )?
> +    } else {
> +        CATALOG_NAME.to_string()
> +    };

without b64, this would just be an unwrap_or_else ;)

> +
>      let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>  
>      let ns = ns.unwrap_or_default();
> @@ -1692,8 +1707,6 @@ pub async fn catalog(
>  
>      let backup_dir = datastore.backup_dir(ns, backup_dir)?;
>  
> -    let file_name = CATALOG_NAME;
> -
>      let (manifest, files) = read_backup_index(&backup_dir)?;
>      for file in files {
>          if file.filename == file_name && file.crypt_mode == Some(CryptMode::Encrypt) {
> @@ -1701,26 +1714,50 @@ pub async fn catalog(
>          }
>      }
>  
> -    tokio::task::spawn_blocking(move || {
> -        let mut path = datastore.base_path();
> -        path.push(backup_dir.relative_path());
> -        path.push(file_name);
> +    if archive_name.is_none() {
> +        tokio::task::spawn_blocking(move || {
> +            let mut path = datastore.base_path();
> +            path.push(backup_dir.relative_path());
> +            path.push(&file_name);
>  
> -        let index = DynamicIndexReader::open(&path)
> -            .map_err(|err| format_err!("unable to read dynamic index '{:?}' - {}", &path, err))?;
> +            let index = DynamicIndexReader::open(&path)
> +                .map_err(|err| format_err!("unable to read dynamic index '{path:?}' - {err}"))?;
>  
> -        let (csum, size) = index.compute_csum();
> -        manifest.verify_file(file_name, &csum, size)?;
> +            let (csum, size) = index.compute_csum();
> +            manifest.verify_file(&file_name, &csum, size)?;
>  
> -        let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
> -        let reader = BufferedDynamicReader::new(index, chunk_reader);
> +            let chunk_reader = LocalChunkReader::new(datastore, None, CryptMode::None);
> +            let reader = BufferedDynamicReader::new(index, chunk_reader);
>  
> -        let mut catalog_reader = CatalogReader::new(reader);
> +            let mut catalog_reader = CatalogReader::new(reader);
>  
> -        let path = decode_path(&filepath)?;
> -        catalog_reader.list_dir_contents(&path)
> -    })
> -    .await?
> +            let path = decode_path(&filepath)?;
> +            catalog_reader.list_dir_contents(&path)
> +        })
> +        .await?
> +    } else {
> +        let (archive_name, payload_archive_name) =
> +            pbs_client::tools::get_pxar_archive_names(&file_name, &manifest)?;
> +        let (reader, archive_size) =
> +            get_local_pxar_reader(datastore.clone(), &manifest, &backup_dir, &archive_name)?;
> +
> +        let reader = if let Some(payload_archive_name) = payload_archive_name {
> +            let payload_input =
> +                get_local_pxar_reader(datastore, &manifest, &backup_dir, &payload_archive_name)?;
> +            pxar::PxarVariant::Split(reader, payload_input)
> +        } else {
> +            pxar::PxarVariant::Unified(reader)
> +        };
> +        let accessor = Accessor::new(reader, archive_size).await?;
> +
> +        let file_path = decode_path(&filepath)?;
> +        pbs_client::tools::pxar_metadata_catalog_lookup(
> +            accessor,
> +            OsStr::from_bytes(&file_path),
> +            None,
> +        )
> +        .await
> +    }
>  }
>  
>  #[sortable]
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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

Re: [pbs-devel] [PATCH proxmox-backup 5/8] api: datastore: add optional archive-name to file-restore

[Fabian Grünbichler]

On June 7, 2024 11:43 am, Christian Ebner wrote:
> Allow to pass the archive name as optional api call parameter instead
> of having it as prefix to the path.
> If this parameter is given, instead of splitting of the archive name
> from the path, the parameter itself is used, leaving the path
> untouched.
> 
> This allows to restore single files from the archive, without having
> to artificially construct the path in case of file restores for split
> pxar archives, where the response path of the listing does not
> include the archive, as opposed to the response provided by lookup
> via the catalog.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index e25a78bca..d33422d13 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1773,6 +1773,7 @@ pub const API_METHOD_PXAR_FILE_DOWNLOAD: ApiMethod = ApiMethod::new(
>              ("backup-time", false, &BACKUP_TIME_SCHEMA),
>              ("filepath", false, &StringSchema::new("Base64 encoded path").schema()),
>              ("tar", true, &BooleanSchema::new("Download as .tar.zst").schema()),
> +            ("archive-name", true, &StringSchema::new("Base64 encoded archive name").schema()),

same question here - this should be safe as regular string with the
appropriate schema?

>          ]),
>      )
>  ).access(
> @@ -1840,9 +1841,17 @@ pub fn pxar_file_download(
>              components.remove(0);
>          }
>  
> -        let mut split = components.splitn(2, |c| *c == b'/');
> -        let pxar_name = std::str::from_utf8(split.next().unwrap())?;
> -        let file_path = split.next().unwrap_or(b"/");
> +        let (pxar_name, file_path) = if let Some(archive_name) = param["archive-name"].as_str() {
> +            let archive_name = base64::decode(archive_name)
> +                .map_err(|err| format_err!("base64 decode of archive-name failed - {err}"))?;
> +            (archive_name, base64::decode(&filepath)?)
> +        } else {
> +            let mut split = components.splitn(2, |c| *c == b'/');
> +            let pxar_name = split.next().unwrap();
> +            let file_path = split.next().unwrap_or(b"/");
> +            (pxar_name.to_owned(), file_path.to_owned())
> +        };
> +        let pxar_name = std::str::from_utf8(&pxar_name)?;
>          let (manifest, files) = read_backup_index(&backup_dir)?;
>          for file in files {
>              if file.filename == pxar_name && file.crypt_mode == Some(CryptMode::Encrypt) {
> @@ -1865,7 +1874,7 @@ pub fn pxar_file_download(
>          let decoder = Accessor::new(reader, archive_size).await?;
>  
>          let root = decoder.open_root().await?;
> -        let path = OsStr::from_bytes(file_path).to_os_string();
> +        let path = OsStr::from_bytes(&file_path).to_os_string();
>          let file = root
>              .lookup(&path)
>              .await?
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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

Re: [pbs-devel] [PATCH proxmox-backup 6/8] file-restore: fallback to mpxar if catalog not present

[Fabian Grünbichler]

On June 7, 2024 11:43 am, Christian Ebner wrote:
> The `proxmox-file-restore list` command will uses the provided path to
> lookup and list directory entries via the catalog. Fallback to using
> the metadata archive if the catalog is not present for fast lookups in
> a backup snapshot.
> 
> This is in preparation for dropping encoding of the catalog for
> snapshots using split archive encoding. Proxmox VE's storage plugin
> uses this to allow single file restore for LXCs.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  proxmox-file-restore/src/main.rs | 72 +++++++++++++++++++++++++-------
>  1 file changed, 56 insertions(+), 16 deletions(-)
> 
> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
> index 38cc1ce85..a09873467 100644
> --- a/proxmox-file-restore/src/main.rs
> +++ b/proxmox-file-restore/src/main.rs
> @@ -124,7 +124,8 @@ async fn list_files(
>          ExtractPath::ListArchives => {
>              let mut entries = vec![];
>              for file in manifest.files() {
> -                if !has_pxar_filename_extension(&file.filename, true)
> +                if !file.filename.ends_with(".pxar.didx")
> +                    && !file.filename.ends_with(".mpxar.didx")
>                      && !file.filename.ends_with(".img.fidx")

is this hunk here stray? or why do we now list regular pxar files here
but didn't before? this seems unrelated to the rest of this patch?

>                  {
>                      continue;
> @@ -146,24 +147,63 @@ async fn list_files(
>              Ok(entries)
>          }
>          ExtractPath::Pxar(file, mut path) => {
> -            let index = client
> -                .download_dynamic_index(&manifest, CATALOG_NAME)
> +            if let Ok(file_info) = manifest.lookup_file_info(CATALOG_NAME) {
> +                let index = client
> +                    .download_dynamic_index(&manifest, CATALOG_NAME)
> +                    .await?;
> +                let most_used = index.find_most_used_chunks(8);
> +                let chunk_reader = RemoteChunkReader::new(
> +                    client.clone(),
> +                    crypt_config,
> +                    file_info.chunk_crypt_mode(),
> +                    most_used,
> +                );
> +                let reader = BufferedDynamicReader::new(index, chunk_reader);
> +                let mut catalog_reader = CatalogReader::new(reader);
> +
> +                let mut fullpath = file.into_bytes();
> +                fullpath.append(&mut path);
> +
> +                catalog_reader.list_dir_contents(&fullpath)
> +            } else {
> +                if path.is_empty() {
> +                    path = vec![b'/'];
> +                }
> +
> +                let (archive_name, payload_archive_name) =
> +                    pbs_client::tools::get_pxar_archive_names(&file, &manifest)?;
> +
> +                let (reader, archive_size) = get_remote_pxar_reader(
> +                    &archive_name,
> +                    client.clone(),
> +                    &manifest,
> +                    crypt_config.clone(),
> +                )
>                  .await?;
> -            let most_used = index.find_most_used_chunks(8);
> -            let file_info = manifest.lookup_file_info(CATALOG_NAME)?;
> -            let chunk_reader = RemoteChunkReader::new(
> -                client.clone(),
> -                crypt_config,
> -                file_info.chunk_crypt_mode(),
> -                most_used,
> -            );
> -            let reader = BufferedDynamicReader::new(index, chunk_reader);
> -            let mut catalog_reader = CatalogReader::new(reader);
>  
> -            let mut fullpath = file.into_bytes();
> -            fullpath.append(&mut path);
> +                let reader = if let Some(payload_archive_name) = payload_archive_name {
> +                    let (payload_reader, payload_size) = get_remote_pxar_reader(
> +                        &payload_archive_name,
> +                        client,
> +                        &manifest,
> +                        crypt_config,
> +                    )
> +                    .await?;
> +                    pxar::PxarVariant::Split(reader, (payload_reader, payload_size))
> +                } else {
> +                    pxar::PxarVariant::Unified(reader)
> +                };
> +
> +                let accessor = Accessor::new(reader, archive_size).await?;
> +                let path = OsStr::from_bytes(&path);
>  
> -            catalog_reader.list_dir_contents(&fullpath)
> +                pbs_client::tools::pxar_metadata_catalog_lookup(
> +                    accessor,
> +                    &path,
> +                    Some(&archive_name),
> +                )
> +                .await

so the new code here, and the one in the api are identical modulo
get_remote/local_pxar_read..

and those two are the only call sites of pxar_metadata_catalog_lookup..

so couldn't we just adapt the latter to take a closure returning the
readers for a given archive name and unify the rest? also technically,
we don't need the payload reader at all other than to not run afoul of
some invariants somewhere I guess? but that could be done as a follow-up
as well.

> +            }
>          }
>          ExtractPath::VM(file, path) => {
>              let details = SnapRestoreDetails {
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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

Re: [pbs-devel] [PATCH proxmox-backup 4/8] api: datastore: conditional lookup for catalog endpoint

[Christian Ebner]

On 6/7/24 12:23, Fabian Grünbichler wrote:
> On June 7, 2024 11:43 am, Christian Ebner wrote:
>> Add an optional `archive-name` parameter, indicating the metadata
>> archive to be used for directory content lookups instead of the
>> catalog. If provided, instead of the catalog reader, a pxar Accessor
>> instance is created to perform the lookup.
>>
>> This is in preparation for dropping catalog encoding for snapshots
>> with split pxar archive encoding.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs | 73 ++++++++++++++++++++++++++++---------
>>   1 file changed, 55 insertions(+), 18 deletions(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index 117dab080..e25a78bca 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -1659,7 +1659,12 @@ fn decode_path(path: &str) -> Result<Vec<u8>, Error> {
>>               "filepath": {
>>                   description: "Base64 encoded path.",
>>                   type: String,
>> -            }
>> +            },
>> +            "archive-name": {
>> +                type: String,
>> +                description: "Name of the archive to lookup given filepath (base64 encoded)",
>> +                optional: true,
>> +            },
> 
> why is this base64 encoded? the archive name is a safe ID..

My intention here was to be sure that I did not overlooked a edge case 
for the archive name and be consistent with the current filepath.

But you are right, will adapt this in a new version of the patch series.



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

Re: [pbs-devel] [PATCH proxmox-backup 6/8] file-restore: fallback to mpxar if catalog not present

[Christian Ebner]

On 6/7/24 12:32, Fabian Grünbichler wrote:
> On June 7, 2024 11:43 am, Christian Ebner wrote:
>> The `proxmox-file-restore list` command will uses the provided path to
>> lookup and list directory entries via the catalog. Fallback to using
>> the metadata archive if the catalog is not present for fast lookups in
>> a backup snapshot.
>>
>> This is in preparation for dropping encoding of the catalog for
>> snapshots using split archive encoding. Proxmox VE's storage plugin
>> uses this to allow single file restore for LXCs.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   proxmox-file-restore/src/main.rs | 72 +++++++++++++++++++++++++-------
>>   1 file changed, 56 insertions(+), 16 deletions(-)
>>
>> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
>> index 38cc1ce85..a09873467 100644
>> --- a/proxmox-file-restore/src/main.rs
>> +++ b/proxmox-file-restore/src/main.rs
>> @@ -124,7 +124,8 @@ async fn list_files(
>>           ExtractPath::ListArchives => {
>>               let mut entries = vec![];
>>               for file in manifest.files() {
>> -                if !has_pxar_filename_extension(&file.filename, true)
>> +                if !file.filename.ends_with(".pxar.didx")
>> +                    && !file.filename.ends_with(".mpxar.didx")
>>                       && !file.filename.ends_with(".img.fidx")
> 
> is this hunk here stray? or why do we now list regular pxar files here
> but didn't before? this seems unrelated to the rest of this patch?

This makes sure that the `.mpxar` is not listed as archive, e.g. when 
accessing the snapshot via the Proxmox VE file browser. (Please note the 
negation).

But I will split this off into a single patch, adding some context as 
commit message.

> 
>>                   {
>>                       continue;
>> @@ -146,24 +147,63 @@ async fn list_files(
>>               Ok(entries)
>>           }
>>           ExtractPath::Pxar(file, mut path) => {
>> -            let index = client
>> -                .download_dynamic_index(&manifest, CATALOG_NAME)
>> +            if let Ok(file_info) = manifest.lookup_file_info(CATALOG_NAME) {
>> +                let index = client
>> +                    .download_dynamic_index(&manifest, CATALOG_NAME)
>> +                    .await?;
>> +                let most_used = index.find_most_used_chunks(8);
>> +                let chunk_reader = RemoteChunkReader::new(
>> +                    client.clone(),
>> +                    crypt_config,
>> +                    file_info.chunk_crypt_mode(),
>> +                    most_used,
>> +                );
>> +                let reader = BufferedDynamicReader::new(index, chunk_reader);
>> +                let mut catalog_reader = CatalogReader::new(reader);
>> +
>> +                let mut fullpath = file.into_bytes();
>> +                fullpath.append(&mut path);
>> +
>> +                catalog_reader.list_dir_contents(&fullpath)
>> +            } else {
>> +                if path.is_empty() {
>> +                    path = vec![b'/'];
>> +                }
>> +
>> +                let (archive_name, payload_archive_name) =
>> +                    pbs_client::tools::get_pxar_archive_names(&file, &manifest)?;
>> +
>> +                let (reader, archive_size) = get_remote_pxar_reader(
>> +                    &archive_name,
>> +                    client.clone(),
>> +                    &manifest,
>> +                    crypt_config.clone(),
>> +                )
>>                   .await?;
>> -            let most_used = index.find_most_used_chunks(8);
>> -            let file_info = manifest.lookup_file_info(CATALOG_NAME)?;
>> -            let chunk_reader = RemoteChunkReader::new(
>> -                client.clone(),
>> -                crypt_config,
>> -                file_info.chunk_crypt_mode(),
>> -                most_used,
>> -            );
>> -            let reader = BufferedDynamicReader::new(index, chunk_reader);
>> -            let mut catalog_reader = CatalogReader::new(reader);
>>   
>> -            let mut fullpath = file.into_bytes();
>> -            fullpath.append(&mut path);
>> +                let reader = if let Some(payload_archive_name) = payload_archive_name {
>> +                    let (payload_reader, payload_size) = get_remote_pxar_reader(
>> +                        &payload_archive_name,
>> +                        client,
>> +                        &manifest,
>> +                        crypt_config,
>> +                    )
>> +                    .await?;
>> +                    pxar::PxarVariant::Split(reader, (payload_reader, payload_size))
>> +                } else {
>> +                    pxar::PxarVariant::Unified(reader)
>> +                };
>> +
>> +                let accessor = Accessor::new(reader, archive_size).await?;
>> +                let path = OsStr::from_bytes(&path);
>>   
>> -            catalog_reader.list_dir_contents(&fullpath)
>> +                pbs_client::tools::pxar_metadata_catalog_lookup(
>> +                    accessor,
>> +                    &path,
>> +                    Some(&archive_name),
>> +                )
>> +                .await
> 
> so the new code here, and the one in the api are identical modulo
> get_remote/local_pxar_read..
> 
> and those two are the only call sites of pxar_metadata_catalog_lookup..

Hmm, okay I will have a look on how to combine this!

> 
> so couldn't we just adapt the latter to take a closure returning the
> readers for a given archive name and unify the rest? also technically,
> we don't need the payload reader at all other than to not run afoul of
> some invariants somewhere I guess? but that could be done as a follow-up
> as well.

Yes, this is true since no payload data is accessed..



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

Re: [pbs-devel] [PATCH proxmox-backup 8/8] client: backup: conditionally write catalog for file level backups

[Fabian Grünbichler]

On June 7, 2024 11:43 am, Christian Ebner wrote:
> Only write the catalog when using the regular backup mode, do not write
> it when using the split archive mode.
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  pbs-client/src/pxar_backup_stream.rs | 11 +++++++----
>  proxmox-backup-client/src/main.rs    | 21 ++++++++++++---------
>  2 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/pbs-client/src/pxar_backup_stream.rs b/pbs-client/src/pxar_backup_stream.rs
> index f322566f0..cdbcdfec2 100644
> --- a/pbs-client/src/pxar_backup_stream.rs
> +++ b/pbs-client/src/pxar_backup_stream.rs
> @@ -15,7 +15,7 @@ use nix::sys::stat::Mode;
>  use proxmox_async::blocking::TokioWriterAdapter;
>  use proxmox_io::StdChannelWriter;
>  
> -use pbs_datastore::catalog::CatalogWriter;
> +use pbs_datastore::catalog::{BackupCatalogWriter, CatalogWriter};
>  
>  use crate::inject_reused_chunks::InjectChunks;
>  use crate::pxar::create::PxarWriters;
> @@ -42,7 +42,7 @@ impl Drop for PxarBackupStream {
>  impl PxarBackupStream {
>      pub fn new<W: Write + Send + 'static>(
>          dir: Dir,
> -        catalog: Arc<Mutex<CatalogWriter<W>>>,
> +        catalog: Option<Arc<Mutex<CatalogWriter<W>>>>,
>          options: crate::pxar::PxarCreateOptions,
>          boundaries: Option<mpsc::Sender<InjectChunks>>,
>          separate_payload_stream: bool,
> @@ -82,7 +82,10 @@ impl PxarBackupStream {
>          let handler = async move {
>              if let Err(err) = crate::pxar::create_archive(
>                  dir,
> -                PxarWriters::new(writer, Some(catalog)),
> +                PxarWriters::new(
> +                    writer,
> +                    catalog.map(|c| c as Arc<Mutex<dyn BackupCatalogWriter + Send>>),
> +                ),
>                  crate::pxar::Flags::DEFAULT,
>                  move |path| {
>                      log::debug!("{:?}", path);
> @@ -122,7 +125,7 @@ impl PxarBackupStream {
>  
>      pub fn open<W: Write + Send + 'static>(
>          dirname: &Path,
> -        catalog: Arc<Mutex<CatalogWriter<W>>>,
> +        catalog: Option<Arc<Mutex<CatalogWriter<W>>>>,
>          options: crate::pxar::PxarCreateOptions,
>          boundaries: Option<mpsc::Sender<InjectChunks>>,
>          separate_payload_stream: bool,
> diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
> index 09af912df..e50979d6e 100644
> --- a/proxmox-backup-client/src/main.rs
> +++ b/proxmox-backup-client/src/main.rs
> @@ -192,7 +192,7 @@ async fn backup_directory<P: AsRef<Path>>(
>      archive_name: &str,
>      payload_target: Option<&str>,
>      chunk_size: Option<usize>,
> -    catalog: Arc<Mutex<CatalogWriter<TokioWriterAdapter<StdChannelWriter<Error>>>>>,
> +    catalog: Option<Arc<Mutex<CatalogWriter<TokioWriterAdapter<StdChannelWriter<Error>>>>>>,
>      pxar_create_options: pbs_client::pxar::PxarCreateOptions,
>      upload_options: UploadOptions,
>  ) -> Result<(BackupStats, Option<BackupStats>), Error> {
> @@ -1059,19 +1059,20 @@ async fn create_backup(
>                      };
>  
>                  // start catalog upload on first use
> -                if catalog.is_none() {
> +                if catalog.is_none() && !detection_mode.is_data() && !detection_mode.is_metadata() {
>                      let catalog_upload_res =
>                          spawn_catalog_upload(client.clone(), crypto.mode == CryptMode::Encrypt)?;
>                      catalog = Some(catalog_upload_res.catalog_writer);
>                      catalog_result_rx = Some(catalog_upload_res.result);
>                  }
> -                let catalog = catalog.as_ref().unwrap();
>  
>                  log_file("directory", &filename, &target);
> -                catalog
> -                    .lock()
> -                    .unwrap()
> -                    .start_directory(std::ffi::CString::new(target.as_str())?.as_c_str())?;
> +                if let Some(catalog) = catalog.as_ref() {
> +                    catalog
> +                        .lock()
> +                        .unwrap()
> +                        .start_directory(std::ffi::CString::new(target.as_str())?.as_c_str())?;
> +                }
>  
>                  let mut previous_ref = None;
>                  let max_cache_size = if detection_mode.is_metadata() {
> @@ -1139,7 +1140,7 @@ async fn create_backup(
>                      &target,
>                      payload_target.as_deref(),
>                      chunk_size_opt,
> -                    catalog.clone(),
> +                    catalog.as_ref().map(|c| c.clone()),

nit: .cloned

>                      pxar_options,
>                      upload_options,
>                  )
> @@ -1155,7 +1156,9 @@ async fn create_backup(
>                      )?;
>                  }
>                  manifest.add_file(target, stats.size, stats.csum, crypto.mode)?;
> -                catalog.lock().unwrap().end_directory()?;
> +                if let Some(catalog) = catalog.as_ref() {
> +                    catalog.lock().unwrap().end_directory()?;
> +                }
>              }
>              (BackupSpecificationType::IMAGE, false) => {
>                  log_file("image", &filename, &target);
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 


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

Re: [pbs-devel] [PATCH proxmox-backup 6/8] file-restore: fallback to mpxar if catalog not present

[Fabian Grünbichler]

On June 7, 2024 12:43 pm, Christian Ebner wrote:
> On 6/7/24 12:32, Fabian Grünbichler wrote:
>> On June 7, 2024 11:43 am, Christian Ebner wrote:
>>> The `proxmox-file-restore list` command will uses the provided path to
>>> lookup and list directory entries via the catalog. Fallback to using
>>> the metadata archive if the catalog is not present for fast lookups in
>>> a backup snapshot.
>>>
>>> This is in preparation for dropping encoding of the catalog for
>>> snapshots using split archive encoding. Proxmox VE's storage plugin
>>> uses this to allow single file restore for LXCs.
>>>
>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>> ---
>>>   proxmox-file-restore/src/main.rs | 72 +++++++++++++++++++++++++-------
>>>   1 file changed, 56 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
>>> index 38cc1ce85..a09873467 100644
>>> --- a/proxmox-file-restore/src/main.rs
>>> +++ b/proxmox-file-restore/src/main.rs
>>> @@ -124,7 +124,8 @@ async fn list_files(
>>>           ExtractPath::ListArchives => {
>>>               let mut entries = vec![];
>>>               for file in manifest.files() {
>>> -                if !has_pxar_filename_extension(&file.filename, true)
>>> +                if !file.filename.ends_with(".pxar.didx")
>>> +                    && !file.filename.ends_with(".mpxar.didx")
>>>                       && !file.filename.ends_with(".img.fidx")
>> 
>> is this hunk here stray? or why do we now list regular pxar files here
>> but didn't before? this seems unrelated to the rest of this patch?
> 
> This makes sure that the `.mpxar` is not listed as archive, e.g. when 
> accessing the snapshot via the Proxmox VE file browser. (Please note the 
> negation).
> 
> But I will split this off into a single patch, adding some context as 
> commit message.

but the previous version using the helper had the same effect of not
listing mpxar archives, the only difference is that thew new variant
does list ppxar ones?


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

Re: [pbs-devel] [PATCH proxmox-backup 6/8] file-restore: fallback to mpxar if catalog not present

[Christian Ebner]

On 6/7/24 13:35, Fabian Grünbichler wrote:
> On June 7, 2024 12:43 pm, Christian Ebner wrote:
>> On 6/7/24 12:32, Fabian Grünbichler wrote:
>>> On June 7, 2024 11:43 am, Christian Ebner wrote:
>>>> The `proxmox-file-restore list` command will uses the provided path to
>>>> lookup and list directory entries via the catalog. Fallback to using
>>>> the metadata archive if the catalog is not present for fast lookups in
>>>> a backup snapshot.
>>>>
>>>> This is in preparation for dropping encoding of the catalog for
>>>> snapshots using split archive encoding. Proxmox VE's storage plugin
>>>> uses this to allow single file restore for LXCs.
>>>>
>>>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>>> ---
>>>>    proxmox-file-restore/src/main.rs | 72 +++++++++++++++++++++++++-------
>>>>    1 file changed, 56 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/proxmox-file-restore/src/main.rs b/proxmox-file-restore/src/main.rs
>>>> index 38cc1ce85..a09873467 100644
>>>> --- a/proxmox-file-restore/src/main.rs
>>>> +++ b/proxmox-file-restore/src/main.rs
>>>> @@ -124,7 +124,8 @@ async fn list_files(
>>>>            ExtractPath::ListArchives => {
>>>>                let mut entries = vec![];
>>>>                for file in manifest.files() {
>>>> -                if !has_pxar_filename_extension(&file.filename, true)
>>>> +                if !file.filename.ends_with(".pxar.didx")
>>>> +                    && !file.filename.ends_with(".mpxar.didx")
>>>>                        && !file.filename.ends_with(".img.fidx")
>>>
>>> is this hunk here stray? or why do we now list regular pxar files here
>>> but didn't before? this seems unrelated to the rest of this patch?
>>
>> This makes sure that the `.mpxar` is not listed as archive, e.g. when
>> accessing the snapshot via the Proxmox VE file browser. (Please note the
>> negation).
>>
>> But I will split this off into a single patch, adding some context as
>> commit message.
> 
> but the previous version using the helper had the same effect of not
> listing mpxar archives, the only difference is that thew new variant
> does list ppxar ones?

No, it skips over them, so ppxar are not listed anymore while pxar, 
mpxar and img are... (not clear without the code context).


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