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

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

On May 9, 2025 2:59 pm, Christian Ebner wrote:
> 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.

because logically, this is the inverse of BackupGroup::destroy with
skip_trash=false, and that locks the group..

else you could have a recovery and a full deletion running concurrently
for the same group. also, while you are recovering the group you
probably don't want to start a new backup snapshot, which would also be
possible with the missing lock.

>>> +    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?

but a skip_trash=true deletion might be going on already or some other
operation holding a lock on the snapshot that doesn't want the 'trash'
status being changed underneath it?

>> 
>>> +    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