Re: [pbs-devel] [PATCH proxmox-backup v4 06/22] api2: admin: add (un)mount endpoint for removable datastores

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

some comments inline

On 4/16/24 17:24, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   pbs-api-types/src/maintenance.rs |   4 +
>   src/api2/admin/datastore.rs      | 175 +++++++++++++++++++++++++++++--
>   2 files changed, 171 insertions(+), 8 deletions(-)
> 
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index a605cc17..56d6bcb5 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -77,6 +77,10 @@ pub struct MaintenanceMode {
>   }
>   
>   impl MaintenanceMode {
> +    pub fn new(ty: MaintenanceType, message: Option<String>) -> Self {
> +        Self { ty, message }
> +    }
> +
>       /// Used for deciding whether the datastore is cleared from the internal cache after the last
>       /// task finishes, so all open files are closed.
>       pub fn is_offline(&self) -> bool {
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 35628c59..57167a82 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -26,20 +26,20 @@ use proxmox_sortable_macro::sortable;
>   use proxmox_sys::fs::{
>       file_read_firstline, file_read_optional_string, replace_file, CreateOptions,
>   };
> -use proxmox_sys::{task_log, task_warn};
> +use proxmox_sys::{task_log, task_warn, WorkerTaskContext};
>   
>   use pxar::accessor::aio::Accessor;
>   use pxar::EntryKind;
>   
>   use pbs_api_types::{
>       print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType,
> -    Counts, CryptMode, DataStoreListItem, DataStoreStatus, GarbageCollectionStatus, GroupListItem,
> -    KeepOptions, Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem,
> -    SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
> -    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA,
> -    MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
> -    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY,
> -    UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
> +    Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreStatus,
> +    GarbageCollectionStatus, GroupListItem, KeepOptions, Operation, PruneJobOptions, RRDMode,
> +    RRDTimeFrame, SnapshotListItem, SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA,
> +    BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA,
> +    DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA,
> +    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
> +    PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
>   };
>   use pbs_client::pxar::{create_tar, create_zip};
>   use pbs_config::CachedUserInfo;
> @@ -2278,6 +2278,163 @@ pub async fn set_backup_owner(
>       .await?
>   }
>   
> +pub fn do_mount_device(
> +    datastore: DataStoreConfig,
> +    worker: Option<&dyn WorkerTaskContext>,
> +) -> Result<(), Error> {
> +    if let Some(uuid) = datastore.backing_device.as_ref() {
> +        if pbs_datastore::is_datastore_available(&datastore) {
> +            bail!("device '{uuid}' is already mounted");

this...

> +        }
> +        let mount_point_path = std::path::Path::new(&datastore.path);
> +        if let Some(worker) = worker {
> +            task_log!(worker, "mounting '{uuid}' to '{}'", datastore.path);

... and this might include also the datastore name in the message,
so it is easier to relate them to the store as just by the `uuid`.
Something like:
`... '{uuid}' for store '{store_name}' to '{store_path}' ...`

> +        }
> +        crate::tools::disks::mount_by_uuid(uuid, mount_point_path)?;
> +
> +        Ok(())
> +    } else {
> +        Err(format_err!(
> +            "Datastore '{}' can't be mounted because it is not removable.",

s/can't/cannot

> +            datastore.name
> +        ))
> +    }
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +        }
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
> +    },
> +)]
> +/// Mount removable datastore.
> +pub fn mount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> +    let (section_config, _digest) = pbs_config::datastore::config()?;
> +    let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> +
> +    if datastore.backing_device.is_none() {
> +        bail!("datastore '{}' is not removable", &store);

store can be inlined and does not need to be taken by reference here

> +    }
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> +    let upid = WorkerTask::new_thread(
> +        "mount-device",
> +        Some(store),
> +        auth_id.to_string(),
> +        to_stdout,
> +        move |worker| do_mount_device(datastore, Some(&worker)),
> +    )?;
> +
> +    Ok(json!(upid))
> +}
> +
> +fn do_unmount_device(
> +    force: bool,
> +    datastore: DataStoreConfig,
> +    worker: Option<&dyn WorkerTaskContext>,
> +) -> Result<(), Error> {
> +    if force {
> +        let _ = crate::tools::disks::unmount_by_mountpoint(&datastore.path);
> +        return Ok(());
> +    }
> +
> +    let mut active_operations = task_tracking::get_active_operations(&datastore.name)?;
> +    let mut counter = 0;
> +    while active_operations.read + active_operations.write > 0 {
> +        if counter == 0 {
> +            if let Some(worker) = worker {
> +                if worker.abort_requested() {
> +                    bail!("aborted, due to user request");
> +                }
> +                task_log!(
> +                    worker,
> +                    "can't unmount yet, still {} read and {} write operations active",

s/can't/cannot

> +                    active_operations.read,
> +                    active_operations.write
> +                );
> +            }
> +            counter = 5000;
> +        }
> +        counter -= 1;
> +        std::thread::sleep(std::time::Duration::from_millis(1));
> +        active_operations = task_tracking::get_active_operations(&datastore.name)?;
> +    }
> +    crate::tools::disks::unmount_by_mountpoint(&datastore.path)?;
> +
> +    Ok(())
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: { schema: DATASTORE_SCHEMA },
> +            force: {
> +                type: Boolean,
> +                description: "Force unmount even if there are active operations.",
> +                optional: true,
> +                default: false,
> +            },
> +        },
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
> +    }
> +)]
> +/// Unmount a removable device that is associated with the datastore
> +pub async fn unmount(
> +    store: String,
> +    force: bool,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<Value, Error> {
> +    let _lock = pbs_config::datastore::lock_config()?;
> +    let (section_config, _digest) = pbs_config::datastore::config()?;
> +    let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> +
> +    if datastore.backing_device.is_none() {
> +        bail!("datastore '{}' is not removable", &store);
> +    }
> +    drop(_lock);
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> +    if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
> +    {
> +        let sock = proxmox_rest_server::ctrl_sock_from_pid(proxy_pid);
> +        let _ = proxmox_rest_server::send_raw_command(
> +            sock,
> +            &format!("{{\"command\":\"update-datastore-cache\",\"args\":\"{store}\"}}\n"),
> +        )
> +        .await;
> +    }
> +
> +    let upid = WorkerTask::new_thread(
> +        "unmount-device",
> +        Some(store),
> +        auth_id.to_string(),
> +        to_stdout,
> +        move |worker| do_unmount_device(force, datastore, Some(&worker)),
> +    )?;
> +
> +    Ok(json!(upid))
> +}
> +
>   #[sortable]
>   const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>       (
> @@ -2316,6 +2473,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>               .get(&API_METHOD_LIST_GROUPS)
>               .delete(&API_METHOD_DELETE_GROUP),
>       ),
> +    ("mount", &Router::new().post(&API_METHOD_MOUNT)),
>       (
>           "namespace",
>           // FIXME: move into datastore:: sub-module?!
> @@ -2350,6 +2508,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>               .delete(&API_METHOD_DELETE_SNAPSHOT),
>       ),
>       ("status", &Router::new().get(&API_METHOD_STATUS)),
> +    ("unmount", &Router::new().post(&API_METHOD_UNMOUNT)),
>       (
>           "upload-backup-log",
>           &Router::new().upload(&API_METHOD_UPLOAD_BACKUP_LOG),