From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <d.csapak@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id 6345D6FDFE
for <pbs-devel@lists.proxmox.com>; Wed, 1 Sep 2021 16:49:31 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 59E839DD3
for <pbs-devel@lists.proxmox.com>; Wed, 1 Sep 2021 16:49:01 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS id 3AE1E9D5D
for <pbs-devel@lists.proxmox.com>; Wed, 1 Sep 2021 16:48:57 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 09693444AB;
Wed, 1 Sep 2021 16:48:57 +0200 (CEST)
Message-ID: <2ae56265-0733-1ae7-91bd-584a3b3648cd@proxmox.com>
Date: Wed, 1 Sep 2021 16:48:55 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:92.0) Gecko/20100101
Thunderbird/92.0
Content-Language: en-US
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>, Hannes Laimer <h.laimer@proxmox.com>
References: <20210830111505.38694-1-h.laimer@proxmox.com>
<20210830111505.38694-8-h.laimer@proxmox.com>
From: Dominik Csapak <d.csapak@proxmox.com>
In-Reply-To: <20210830111505.38694-8-h.laimer@proxmox.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.917 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
NICE_REPLY_A -0.932 Looks like a legit reply (A)
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See
http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more
information. [datastore.rs]
Subject: Re: [pbs-devel] [PATCH v2 proxmox-backup 07/15] api2: add (un)mount
endpoint for removable ds's
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
<pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>,
<mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 01 Sep 2021 14:49:31 -0000
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()
>