Re: [RFC PATCH proxmox-backup 3/4] close #7333: tape: restore: allow using several drives in parallel

[Dominik Csapak <d.csapak@proxmox.com>]

comments inline

On 11/6/26 2:39 PM, Jakob-Tobias Winter wrote:
> Restoring a media set that spans several tapes from a single drive
> takes long, even if other drives of the library are idle. Add the
> optional 'additional-drives' parameter to the restore API and the
> proxmox-tape CLI, for up to three drives in addition to the given one.
> All drives are checked for the Tape.Read privilege, locked and marked
> as in use for the whole task.
> 
> A selective restore then reads the media of its first two phases in
> parallel. A full restore cannot simply do the same: a tape backup
> writes the chunks of a snapshot before its snapshot archive, possibly
> to an earlier media, so with several drives a snapshot could be added
> before all of its chunks are restored, and an aborted restore would
> leave it behind with chunks missing. So with several drives, a full
> restore uses the phases of the selective restore for all snapshots,
> which only add the snapshots once all chunks are restored. This needs
> the media set catalog, the snapshots only show up at the end of the
> restore, and each media is loaded twice, once for the small snapshot
> archives and once for the chunks. With a single drive, a full restore
> works as before.
> 
> Signed-off-by: Jakob-Tobias Winter <tobias@stuttgart.de>
> ---
>   src/api2/tape/restore.rs | 65 +++++++++++++++++++++++++++++++---------
>   src/bin/proxmox-tape.rs  | 10 +++++++
>   2 files changed, 61 insertions(+), 14 deletions(-)
> 
> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
> index ee8906f85..8040a6c21 100644
> --- a/src/api2/tape/restore.rs
> +++ b/src/api2/tape/restore.rs
> @@ -286,6 +286,16 @@ pub const ROUTER: Router = Router::new().post(&API_METHOD_RESTORE);
>               drive: {
>                   schema: DRIVE_NAME_SCHEMA,
>               },
> +            "additional-drives": {
> +                description: "Use these drives in addition to 'drive', to restore from several \
> +                    media in parallel.",
> +                type: Array,
> +                optional: true,
> +                max_length: 3,
> +                items: {
> +                    schema: DRIVE_NAME_SCHEMA,
> +                },
> +            },

as said in my answer to the cover letter, i'd prefer 'drives' as a
replacement for drives instead of an additional parameter.

>               "media-set": {
>                   description: "Media set UUID.",
>                   type: String,
> @@ -318,8 +328,9 @@ pub const ROUTER: Router = Router::new().post(&API_METHOD_RESTORE);
>       access: {
>           // Note: parameters are no uri parameter, so we need to test inside function body
>           description: "The user needs Tape.Read privilege on /tape/pool/{pool} and \
> -            /tape/device/{drive}, Datastore.Backup privilege on /datastore/{store}/[{namespace}], \
> -            Datastore.Modify privileges to create namespaces (if they don't exist).",
> +            /tape/device/{drive} (and each additional drive), Datastore.Backup privilege on \
> +            /datastore/{store}/[{namespace}], Datastore.Modify privileges to create namespaces \
> +            (if they don't exist).",
>           permission: &Permission::Anybody,
>       },
>   )]
> @@ -328,6 +339,7 @@ pub const ROUTER: Router = Router::new().post(&API_METHOD_RESTORE);
>   pub fn restore(
>       store: String,
>       drive: String,
> +    additional_drives: Option<Vec<String>>,
>       namespaces: Option<Vec<String>>,
>       media_set: String,
>       notify_user: Option<Userid>,
> @@ -374,7 +386,16 @@ pub fn restore(
>               }
>           }
>       }
> -    user_info.check_privs(&auth_id, &["tape", "device", &drive], PRIV_TAPE_READ, false)?;
> +    let mut drives = vec![drive];
> +    for drive in additional_drives.unwrap_or_default() {
> +        if drives.contains(&drive) {
> +            bail!("drive '{drive}' given more than once");

nit: if we want to enforce that here, i think a hashset is a better way,
since we have quadratic check cost here (shouldn't matter
that much though)

> +        }
> +        drives.push(drive);
> +    }
> +    for drive in drives.iter() {
> +        user_info.check_privs(&auth_id, &["tape", "device", drive], PRIV_TAPE_READ, false)?;
> +    }
>   
>       let media_set_uuid = media_set.parse()?;
>   
> @@ -388,7 +409,10 @@ pub fn restore(
>       let (drive_config, _digest) = pbs_config::drive::config()?;
>   
>       // early check/lock before starting worker
> -    let drive_lock = lock_tape_device(&drive_config, &drive)?;
> +    let drive_locks = drives
> +        .iter()
> +        .map(|drive| lock_tape_device(&drive_config, drive))
> +        .collect::<Result<Vec<_>, _>>()?;
>   
>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>   
> @@ -404,23 +428,28 @@ pub fn restore(
>           auth_id.to_string(),
>           to_stdout,
>           move |worker| {
> -            let _drive_lock = drive_lock; // keep lock guard
> +            let _drive_locks = drive_locks; // keep lock guards
>   
> -            set_tape_device_state(&drive, &worker.upid().to_string())?;
> +            for drive in drives.iter() {
> +                set_tape_device_state(drive, &worker.upid().to_string())?;
> +            }
>   
>               let restore_owner = owner.as_ref().unwrap_or(&auth_id);
>   
>               info!("Mediaset '{media_set}'");
>               info!("Pool: {pool}");
>   
> -            let res = if snapshots.is_some() || namespaces {
> +            // the chunks of a snapshot can be on an earlier media that another drive did not
> +            // restore yet, so with several drives a full restore uses the phases of the selective
> +            // restore, which only add the snapshots after all chunks are restored
> +            let res = if snapshots.is_some() || namespaces || drives.len() > 1 {
>                   restore_list_worker(
>                       worker,
> -                    snapshots.unwrap_or_default(),
> +                    snapshots,
>                       inventory,
>                       media_set_uuid,
>                       drive_config,
> -                    std::slice::from_ref(&drive),
> +                    &drives,
>                       store_map,
>                       restore_owner,
>                       &notification_mode,
> @@ -433,7 +462,7 @@ pub fn restore(
>                       inventory,
>                       media_set_uuid,
>                       drive_config,
> -                    &drive,
> +                    &drives[0],
>                       store_map,
>                       restore_owner,
>                       &notification_mode,
> @@ -443,8 +472,10 @@ pub fn restore(
>               if res.is_ok() {
>                   info!("Restore mediaset '{media_set}' done");
>               }
> -            if let Err(err) = set_tape_device_state(&drive, "") {
> -                info!("could not unset drive state for {drive}: {err}");
> +            for drive in drives.iter() {
> +                if let Err(err) = set_tape_device_state(drive, "") {
> +                    info!("could not unset drive state for {drive}: {err}");
> +                }
>               }
>   
>               res
> @@ -626,7 +657,7 @@ fn log_required_tapes<'a>(inventory: &Inventory, list: impl Iterator<Item = &'a
>   #[allow(clippy::too_many_arguments)]
>   fn restore_list_worker(
>       worker: Arc<WorkerTask>,
> -    snapshots: Vec<String>,
> +    snapshots: Option<Vec<String>>,

not sure why you change that to an option here?
an empty snapshot list should already be a good enough indicator?

i don't think it makes a good API do differentiate between
no list or an empty list...

>       inventory: Inventory,
>       media_set_uuid: Uuid,
>       drive_config: SectionConfigData,
> @@ -646,11 +677,17 @@ fn restore_list_worker(
>   
>       let res = proxmox_lang::try_block!({
>           // phase 0
> +        // without snapshots and namespace mapping, this is a full restore using several drives
> +        let restore_all = snapshots.is_none() && store_map.ns_map.is_none();

here we could just do an `is_empty()` check?

> +        let snapshots = snapshots.unwrap_or_default();

and this would stay where it was, before calling the function

>           let snapshots = if snapshots.is_empty() {
>               let mut restorable = Vec::new();
>               // restore source namespaces
>               for (store, snapshot) in catalog.list_snapshots() {
>                   let (ns, dir) = match parse_ns_and_snapshot(snapshot) {
> +                    Ok((ns, dir)) if restore_all && store_map.target_store(store).is_some() => {
> +                        (ns, dir)
> +                    }
>                       Ok((ns, dir)) if store_map.has_full_mapping(store, &ns) => (ns, dir),
>                       Err(err) => {
>                           warn!("couldn't parse snapshot {snapshot} - {err}");
> @@ -665,7 +702,7 @@ fn restore_list_worker(
>                       &snapshot,
>                       &ns,
>                       &dir,
> -                    false,
> +                    restore_all,
>                       &user_info,
>                       auth_id,
>                       restore_owner,
> diff --git a/src/bin/proxmox-tape.rs b/src/bin/proxmox-tape.rs
> index dcbf2d0fb..860ed38fc 100644
> --- a/src/bin/proxmox-tape.rs
> +++ b/src/bin/proxmox-tape.rs
> @@ -906,6 +906,16 @@ async fn backup(mut param: Value) -> Result<(), Error> {
>                   schema: DRIVE_NAME_SCHEMA,
>                   optional: true,
>               },
> +            "additional-drives": {
> +                description: "Use these drives in addition to 'drive', to restore from several \
> +                    media in parallel.",
> +                type: Array,
> +                optional: true,
> +                max_length: 3,
> +                items: {
> +                    schema: DRIVE_NAME_SCHEMA,
> +                },
> +            },
>               "media-set": {
>                   description: "Media set UUID.",
>                   type: String,