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

[Christian Ebner <c.ebner@proxmox.com>]

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