Re: [pbs-devel] [PATCH v2 proxmox-backup 07/15] api2: add (un)mount endpoint for removable ds's

[Dominik Csapak <d.csapak@proxmox.com>]

Comments inline

On 8/30/21 13:14, Hannes Laimer wrote:
> Add endpoints for mounting and unmounting removable datastores by their
> name.
> ---
>   src/api2/admin/datastore.rs | 144 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 144 insertions(+)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index b236dfab..f6adc663 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -39,6 +39,7 @@ use crate::config::datastore;
>   use crate::config::cached_user_info::CachedUserInfo;
>   
>   use crate::server::{jobstate::Job, WorkerTask};
> +use crate::tools::disks::{mount_by_uuid, unmount_by_mount_point};
>   
>   use crate::config::acl::{
>       PRIV_DATASTORE_AUDIT,
> @@ -1859,6 +1860,139 @@ pub fn set_backup_owner(
>       Ok(())
>   }
>   
> +pub fn do_mount(store: String, auth_id: &Authid) -> Result<String, Error> {
> +    let (config, _digest) = datastore::config()?;
> +    let store_config = config.lookup("datastore", &store)?;
> +
> +    let upid_str = WorkerTask::new_thread(
> +        "mount",
> +        Some(store.clone()),
> +        auth_id.clone(),
> +        false,
> +        move |_worker| {
> +            if check_if_available(&store_config).is_ok() {
> +                bail!(
> +                    "Datastore '{}' can't be mounted because it is already available.",
> +                    &store_config.name
> +                );
> +            };
> +            if let (Some(uuid), Some(mount_point)) = (
> +                store_config.backing_device,
> +                store_config.backing_device_mount_point,
> +            ) {
> +                let mount_point_path = std::path::Path::new(&mount_point);
> +                mount_by_uuid(&uuid, &mount_point_path)
> +            } else {
> +                bail!(
> +                    "Datastore '{}' can't be mounted because it is not removable.",
> +                    &store_config.name
> +                );
> +            }
> +        },
> +    )?;
> +
> +    Ok(upid_str)
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +        }
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),

that permission looks wrong, maybe _MODIFY instead ?

> +    },
> +)]
> +/// Mount removable datastore.
> +pub fn mount(store: String, mut rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> +    let (_config, digest) = datastore::config()?;
> +
> +    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +
> +    do_mount(store, &auth_id).map(|upid_str| json!(upid_str))
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +        },
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),

same here

> +    },
> +)]
> +/// Unmount removable datastore.
> +pub fn unmount(store: String, mut rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> +    let (config, digest) = datastore::config()?;
> +
> +    let store_config = config.lookup("datastore", &store)?;
> +    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
> +
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +
> +    let upid_str = WorkerTask::new_thread(
> +        "unmount",
> +        Some(store.clone()),
> +        auth_id.clone(),
> +        to_stdout,
> +        move |_worker| {
> +            if !check_if_available(&store_config).is_ok() {
> +                bail!(
> +                    "Datastore '{}' can't be unmounted because it is not available.",
> +                    &store_config.name
> +                );
> +            };
> +            if let (Some(_uuid), Some(mount_point)) = (
> +                store_config.backing_device,
> +                store_config.backing_device_mount_point,
> +            ) {
> +                let mount_point_path = std::path::Path::new(&mount_point);
> +                for job in crate::server::TaskListInfoIterator::new(true)? {
> +                    let job = match job {
> +                        Ok(job) => job,
> +                        Err(_) => break,
> +                    };
> +
> +                    if !job.upid.worker_type.eq("unmount")
> +                        && job.upid.worker_id.map_or(false, |id| id.contains(&store))

i know its what we do for the task filtering, but i don't think its the
best approach for this

if my datastore name is 'host', i'll not be able to unmount it during
any completely unrelated backup of a 'host'

sadly i dont have a good alternate solution at hand
that can handle old daemons....
so we likely will have to go with this for now... but i would want it at
least mark it with a FIXME or TODO so we see that it should be done
right...

but, since we unmount lazy anyway, we should wait here until its really 
unmounted, else we might give users a wrong feeling of safety to remove
the drive, when in reality its still writing?

if we do that, we can toss the job checking code completely, unmount
it always lazy and wait until finished?


> +                    {
> +                        bail!(
> +                            "Can't unmount '{}' because there is a '{}' job still running!",
> +                            &store_config.name,
> +                            job.upid.worker_type
> +                        );
> +                    }
> +                }
> +                unmount_by_mount_point(&mount_point_path)
> +            } else {
> +                bail!(
> +                    "Datastore '{}' can't be mounted because it is not removable.",
> +                    &store_config.name
> +                );
> +            }
> +        },
> +    )?;
> +
> +    Ok(json!(upid_str))
> +}
> +
>   #[sortable]
>   const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>       (
> @@ -1904,6 +2038,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>               .get(&API_METHOD_LIST_GROUPS)
>               .delete(&API_METHOD_DELETE_GROUP)
>       ),
> +    (
> +        "mount",
> +        &Router::new()
> +            .post(&API_METHOD_MOUNT)
> +    ),
>       (
>           "notes",
>           &Router::new()
> @@ -1941,6 +2080,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>           &Router::new()
>               .get(&API_METHOD_STATUS)
>       ),
> +    (
> +        "unmount",
> +        &Router::new()
> +            .post(&API_METHOD_UNMOUNT)
> +    ),
>       (
>           "upload-backup-log",
>           &Router::new()
>