Re: [PATCH proxmox-backup v6 8/8] cli: add move-namespace and move-group commands

[Arthur Bied-Charreton <a.bied-charreton@proxmox.com>]

On Tue, Mar 31, 2026 at 02:34:09PM +0200, Hannes Laimer wrote:
> Add 'move-namespace' and 'move-group' subcommands to
> proxmox-backup-manager datastore. Both call the corresponding API
> handler and wait for the worker task to complete.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
Hey, one comment & one nit inline
> ---
>  src/bin/proxmox_backup_manager/datastore.rs | 84 ++++++++++++++++++++-
>  1 file changed, 83 insertions(+), 1 deletion(-)
> 
> diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
> index 5c65c5ec..2ee56fba 100644
> --- a/src/bin/proxmox_backup_manager/datastore.rs
> +++ b/src/bin/proxmox_backup_manager/datastore.rs
> @@ -1,5 +1,6 @@
>  use pbs_api_types::{
> -    DataStoreConfig, DataStoreConfigUpdater, DATASTORE_SCHEMA, PROXMOX_CONFIG_DIGEST_SCHEMA,
> +    BackupNamespace, DataStoreConfig, DataStoreConfigUpdater, DATASTORE_SCHEMA,
> +    PROXMOX_CONFIG_DIGEST_SCHEMA,
>  };
>  use pbs_client::view_task_result;
>  use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
> @@ -323,6 +324,75 @@ async fn uuid_mount(mut param: Value, _rpcenv: &mut dyn RpcEnvironment) -> Resul
>      Ok(Value::Null)
>  }
>  
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            ns: {
> +                type: BackupNamespace,
> +            },
> +            "new-ns": {
> +                type: BackupNamespace,
> +            },
> +        },
> +    },
> +)]
> +/// Move a backup namespace to a new location within the same datastore.
> +async fn cli_move_namespace(
> +    mut param: Value,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<(), Error> {
> +    param["node"] = "localhost".into();
> +
> +    let info = &api2::admin::namespace::API_METHOD_MOVE_NAMESPACE;
> +    let result = match info.handler {
> +        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
> +        _ => unreachable!(),
> +    };
> +
> +    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
> +    Ok(())
> +}
nit: The `move-namespace` command outputs a log message from
`DataStore::with_store_and_config` ("using datastore cache ...").
That's unrelated to these changes, but it's a little confusing since
that ends up being the only output you get when moving a namespace.
IMO this would benefit from some kind of success message to make it 
clear that the move happened.
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            ns: {
> +                type: BackupNamespace,
> +                optional: true,
> +            },
> +            group: {
> +                type: pbs_api_types::BackupGroup,
> +                flatten: true,
> +            },
> +            "new-ns": {
> +                type: BackupNamespace,
> +                optional: true,
> +            },
> +        },
> +    },
> +)]
> +/// Move a backup group to a different namespace within the same datastore.
> +async fn cli_move_group(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
> +    param["node"] = "localhost".into();
> +
> +    let info = &api2::admin::datastore::API_METHOD_MOVE_GROUP;
> +    let result = match info.handler {
> +        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
> +        _ => unreachable!(),
> +    };
> +
> +    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
> +    Ok(())
> +}
It is not clear to me why not setting a source/target operand should
automatically imply the root namespace. I assume this is to reflect the
actual namespace structure, where root is actually stored as '', but I
think it would be cleaner if there was a way to make it explicit, or if
that's not possible (not sure if there is even a keyword we can reserve
for this), at least document it.
>  #[api(
>      protected: true,
>      input: {
> @@ -407,6 +477,18 @@ pub fn datastore_commands() -> CommandLineInterface {
>              CliCommand::new(&API_METHOD_DELETE_DATASTORE)
>                  .arg_param(&["name"])
>                  .completion_cb("name", pbs_config::datastore::complete_datastore_name),
> +        )
> +        .insert(
> +            "move-namespace",
> +            CliCommand::new(&API_METHOD_CLI_MOVE_NAMESPACE)
> +                .arg_param(&["store"])
> +                .completion_cb("store", pbs_config::datastore::complete_datastore_name),
> +        )
> +        .insert(
> +            "move-group",
> +            CliCommand::new(&API_METHOD_CLI_MOVE_GROUP)
> +                .arg_param(&["store"])
> +                .completion_cb("store", pbs_config::datastore::complete_datastore_name),
>          );
>  
>      cmd_def.into()
> -- 
> 2.47.3
> 
> 
> 
> 
>