Re: [pbs-devel] [RFC v2 proxmox-backup 18/21] api: admin: implement endpoints to restore trashed contents

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

Thanks for feedback, will have a closer look next week.

Allow me two quick questions inline though...

On 5/9/25 14:27, Fabian Grünbichler wrote:
> On May 8, 2025 3:05 pm, Christian Ebner wrote:
>> Implements the api endpoints to restore trashed contents contained
>> within namespaces, backup groups or individual snapshots.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   src/api2/admin/datastore.rs | 173 +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 172 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index cbd24c729..eb033c3fc 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -51,7 +51,7 @@ use pbs_api_types::{
>>   };
>>   use pbs_client::pxar::{create_tar, create_zip};
>>   use pbs_config::CachedUserInfo;
>> -use pbs_datastore::backup_info::{BackupInfo, ListBackupFilter};
>> +use pbs_datastore::backup_info::{BackupInfo, ListBackupFilter, TRASH_MARKER_FILENAME};
>>   use pbs_datastore::cached_chunk_reader::CachedChunkReader;
>>   use pbs_datastore::catalog::{ArchiveEntry, CatalogReader};
>>   use pbs_datastore::data_blob::DataBlob;
>> @@ -2727,6 +2727,165 @@ pub async fn unmount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<V
>>       Ok(json!(upid))
>>   }
>>   
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            store: { schema: DATASTORE_SCHEMA },
>> +            ns: { type: BackupNamespace, },
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Anybody,
>> +        description: "Requires on /datastore/{store}[/{namespace}] either DATASTORE_MODIFY for any \
>> +            or DATASTORE_BACKUP and being the owner of the group",
>> +    },
>> +)]
>> +/// Recover trashed contents of a namespace.
>> +pub fn recover_namespace(
>> +    store: String,
>> +    ns: BackupNamespace,
>> +    rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<(), Error> {
>> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>> +    let limited = check_ns_privs_full(
>> +        &store,
>> +        &ns,
>> +        &auth_id,
>> +        PRIV_DATASTORE_MODIFY,
>> +        PRIV_DATASTORE_BACKUP,
>> +    )?;
>> +
>> +    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
>> +
>> +    for backup_group in datastore.iter_backup_groups(ns.clone())? {
>> +        let backup_group = backup_group?;
>> +        if limited {
>> +            let owner = datastore.get_owner(&ns, backup_group.group())?;
>> +            if check_backup_owner(&owner, &auth_id).is_err() {
>> +                continue;
>> +            }
>> +        }
>> +        do_recover_group(&backup_group)?;
>> +    }
>> +
>> +    Ok(())
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            store: { schema: DATASTORE_SCHEMA },
>> +            group: {
>> +                type: pbs_api_types::BackupGroup,
>> +                flatten: true,
>> +            },
>> +            ns: {
>> +                type: BackupNamespace,
>> +                optional: true,
>> +            },
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Anybody,
>> +        description: "Requires on /datastore/{store}[/{namespace}] either DATASTORE_MODIFY for any \
>> +            or DATASTORE_BACKUP and being the owner of the group",
>> +    },
>> +)]
>> +/// Recover trashed contents of a backup group.
>> +pub fn recover_group(
>> +    store: String,
>> +    group: pbs_api_types::BackupGroup,
>> +    ns: Option<BackupNamespace>,
>> +    rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<(), Error> {
>> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>> +    let ns = ns.unwrap_or_default();
>> +    let datastore = check_privs_and_load_store(
>> +        &store,
>> +        &ns,
>> +        &auth_id,
>> +        PRIV_DATASTORE_MODIFY,
>> +        PRIV_DATASTORE_BACKUP,
>> +        Some(Operation::Write),
>> +        &group,
>> +    )?;
>> +
>> +    let backup_group = datastore.backup_group(ns, group);
>> +    do_recover_group(&backup_group)?;
>> +
>> +    Ok(())
>> +}
>> +
>> +fn do_recover_group(backup_group: &BackupGroup) -> Result<(), Error> {
> 
> missing locking for the group?

Not sure about that one. After all the group is trashed at least as long 
as all the snapshots are trashed. And GC will only ever clean up the 
group folder if the trash marker is not set. So I do not see a reason 
why this should be locked.

> 
>> +    let trashed_snapshots = backup_group.list_backups(ListBackupFilter::Trashed)?;
>> +    for snapshot in trashed_snapshots {
>> +        do_recover_snapshot(&snapshot.backup_dir)?;
>> +    }
>> +
>> +    let group_trash_path = backup_group.full_group_path().join(TRASH_MARKER_FILENAME);
>> +    if let Err(err) = std::fs::remove_file(&group_trash_path) {
>> +        if err.kind() != std::io::ErrorKind::NotFound {
>> +            bail!("failed to remove group trash file {group_trash_path:?} - {err}");
>> +        }
>> +    }
>> +    Ok(())
>> +}
>> +
>> +#[api(
>> +    input: {
>> +        properties: {
>> +            store: { schema: DATASTORE_SCHEMA },
>> +            backup_dir: {
>> +                type: pbs_api_types::BackupDir,
>> +                flatten: true,
>> +            },
>> +            ns: {
>> +                type: BackupNamespace,
>> +                optional: true,
>> +            },
>> +        },
>> +    },
>> +    access: {
>> +        permission: &Permission::Anybody,
>> +        description: "Requires on /datastore/{store}[/{namespace}] either DATASTORE_MODIFY for any \
>> +            or DATASTORE_BACKUP and being the owner of the group",
>> +    },
>> +)]
>> +/// Recover trashed contents of a backup snapshot.
>> +pub fn recover_snapshot(
>> +    store: String,
>> +    backup_dir: pbs_api_types::BackupDir,
>> +    ns: Option<BackupNamespace>,
>> +    rpcenv: &mut dyn RpcEnvironment,
>> +) -> Result<(), Error> {
>> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>> +    let ns = ns.unwrap_or_default();
>> +    let datastore = check_privs_and_load_store(
>> +        &store,
>> +        &ns,
>> +        &auth_id,
>> +        PRIV_DATASTORE_MODIFY,
>> +        PRIV_DATASTORE_BACKUP,
>> +        Some(Operation::Write),
>> +        &backup_dir.group,
>> +    )?;
>> +
>> +    let snapshot = datastore.backup_dir(ns, backup_dir)?;
>> +    do_recover_snapshot(&snapshot)?;
>> +
>> +    Ok(())
>> +}
>> +
>> +fn do_recover_snapshot(snapshot_dir: &BackupDir) -> Result<(), Error> {
> 
> missing locking for the snapshot?

Why? remove_file() should be atomic?

> 
>> +    let trash_path = snapshot_dir.full_path().join(TRASH_MARKER_FILENAME);
>> +    if let Err(err) = std::fs::remove_file(&trash_path) {
>> +        if err.kind() != std::io::ErrorKind::NotFound {
>> +            bail!("failed to remove trash file {trash_path:?} - {err}");
>> +        }
>> +    }
>> +    Ok(())
>> +}
>> +
>>   #[sortable]
>>   const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>>       (
>> @@ -2792,6 +2951,18 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>>           "pxar-file-download",
>>           &Router::new().download(&API_METHOD_PXAR_FILE_DOWNLOAD),
>>       ),
>> +    (
>> +        "recover-group",
>> +        &Router::new().post(&API_METHOD_RECOVER_GROUP),
> 
> I am not sure whether those should be POST or PUT, they are modifying an
> existing (trashed) group/snapshot/.. after all?
> 
>> +    ),
>> +    (
>> +        "recover-namespace",
>> +        &Router::new().post(&API_METHOD_RECOVER_NAMESPACE),
>> +    ),
>> +    (
>> +        "recover-snapshot",
>> +        &Router::new().post(&API_METHOD_RECOVER_SNAPSHOT),
>> +    ),
>>       ("rrd", &Router::new().get(&API_METHOD_GET_RRD_STATS)),
>>       (
>>           "snapshots",
>> -- 
>> 2.39.5
>>
>>
>>
>> _______________________________________________
>> 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
> 
> 



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