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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

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