Re: [PATCH proxmox-backup v4 4/7] api: add PUT endpoint for move_group

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

One note inline.

On 3/11/26 4:13 PM, Hannes Laimer wrote:
> Requires DATASTORE_MODIFY on both the source and target namespaces.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 76 ++++++++++++++++++++++++++++++++++++-
>   1 file changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index cca34055..77a88e51 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -69,7 +69,9 @@ use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
>   
>   use crate::api2::backup::optional_ns_param;
>   use crate::api2::node::rrd::create_value_from_rrd;
> -use crate::backup::{check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK};
> +use crate::backup::{
> +    check_ns_privs, check_ns_privs_full, ListAccessibleBackupGroups, VerifyWorker, NS_PRIVS_OK,
> +};
>   use crate::server::jobstate::{compute_schedule_status, Job, JobState};
>   use crate::tools::{backup_info_to_snapshot_list_item, get_all_snapshot_files, read_backup_index};
>   
> @@ -278,6 +280,77 @@ pub async fn delete_group(
>       .await?
>   }
>   
> +#[api(
> +    input: {
> +        properties: {
> +            store: { schema: DATASTORE_SCHEMA },
> +            ns: {
> +                type: BackupNamespace,
> +                optional: true,
> +            },
> +            group: {
> +                type: pbs_api_types::BackupGroup,
> +                flatten: true,
> +            },
> +            "new-ns": {
> +                type: BackupNamespace,
> +            },
> +        },
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Anybody,
> +        description: "Requires DATASTORE_MODIFY on both the source and target namespace.",
> +    },
> +)]
> +/// Move a backup group to a different namespace within the same datastore.
> +pub fn move_group(
> +    store: String,
> +    ns: Option<BackupNamespace>,
> +    group: pbs_api_types::BackupGroup,
> +    new_ns: BackupNamespace,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let ns = ns.unwrap_or_default();
> +
> +    check_ns_privs(&store, &ns, &auth_id, PRIV_DATASTORE_MODIFY)?;
> +    check_ns_privs(&store, &new_ns, &auth_id, PRIV_DATASTORE_MODIFY)?;
> +
> +    let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;

Please note that the operation parameter to 
DataStore::lookup_datastore() has been made non-optional in a very 
recent change [0] applied after you send the patch series. Please adapt 
this accordingly for a new version of the patch series, thanks!

[0] 
https://git.proxmox.com/?p=proxmox-backup.git;a=commit;h=c0a9b6513a065ae0a1dc92e4b8c22721db752023

> +
> +    // Best-effort pre-checks for a fast synchronous error before spawning a worker.
> +    if ns == new_ns {
> +        bail!("source and target namespace must be different");
> +    }
> +    if !datastore.namespace_exists(&new_ns) {
> +        bail!("target namespace '{new_ns}' does not exist");
> +    }
> +    let source_group = datastore.backup_group(ns.clone(), group.clone());
> +    if !source_group.exists() {
> +        bail!("group '{group}' does not exist in namespace '{ns}'");
> +    }
> +    let target_group = datastore.backup_group(new_ns.clone(), group.clone());
> +    if target_group.exists() {
> +        bail!("group '{group}' already exists in target namespace '{new_ns}'");
> +    }
> +
> +    let worker_id = format!("{store}:{ns}:{group}");
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> +    let upid_str = WorkerTask::new_thread(
> +        "move-group",
> +        Some(worker_id),
> +        auth_id.to_string(),
> +        to_stdout,
> +        move |_worker| datastore.move_group(&ns, &group, &new_ns),
> +    )?;
> +
> +    Ok(json!(upid_str))
> +}
> +
>   #[api(
>       input: {
>           properties: {
> @@ -2828,6 +2901,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>           "groups",
>           &Router::new()
>               .get(&API_METHOD_LIST_GROUPS)
> +            .put(&API_METHOD_MOVE_GROUP)
>               .delete(&API_METHOD_DELETE_GROUP),
>       ),
>       ("mount", &Router::new().post(&API_METHOD_MOUNT)),