From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <h.laimer@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 74DC297893
 for <pbs-devel@lists.proxmox.com>; Wed, 17 Apr 2024 11:19:11 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 5E3B91742
 for <pbs-devel@lists.proxmox.com>; Wed, 17 Apr 2024 11:19:11 +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
 for <pbs-devel@lists.proxmox.com>; Wed, 17 Apr 2024 11:19:10 +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 3E81C41D94
 for <pbs-devel@lists.proxmox.com>; Wed, 17 Apr 2024 11:19:10 +0200 (CEST)
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Wed, 17 Apr 2024 11:19:08 +0200
Message-Id: <D0MAB31HNIDY.1QYQ9HLIY8HN6@proxmox.com>
From: "Hannes Laimer" <h.laimer@proxmox.com>
To: "Christian Ebner" <c.ebner@proxmox.com>, "Proxmox Backup Server
 development discussion" <pbs-devel@lists.proxmox.com>
X-Mailer: aerc 0.17.0-78-g4ffbaa6b3946
References: <20240416152416.96561-1-h.laimer@proxmox.com>
 <20240416152416.96561-7-h.laimer@proxmox.com>
 <fd467cef-4e5e-4994-8ea2-66c39069b092@proxmox.com>
In-Reply-To: <fd467cef-4e5e-4994-8ea2-66c39069b092@proxmox.com>
X-SPAM-LEVEL: Spam detection results:  0
 AWL -0.005 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 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.name, datastore.rs, maintenance.rs]
Subject: Re: [pbs-devel] [PATCH proxmox-backup v4 06/22] api2: admin: add
 (un)mount endpoint for removable datastores
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, 17 Apr 2024 09:19:11 -0000

On Wed Apr 17, 2024 at 10:08 AM CEST, Christian Ebner wrote:
> 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(-)
> >=20
> > diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maint=
enance.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 {
> >   }
> >  =20
> >   impl MaintenanceMode {
> > +    pub fn new(ty: MaintenanceType, message: Option<String>) -> Self {
> > +        Self { ty, message }
> > +    }
> > +
> >       /// Used for deciding whether the datastore is cleared from the i=
nternal 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, Cre=
ateOptions,
> >   };
> > -use proxmox_sys::{task_log, task_warn};
> > +use proxmox_sys::{task_log, task_warn, WorkerTaskContext};
> >  =20
> >   use pxar::accessor::aio::Accessor;
> >   use pxar::EntryKind;
> >  =20
> >   use pbs_api_types::{
> >       print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent,=
 BackupNamespace, BackupType,
> > -    Counts, CryptMode, DataStoreListItem, DataStoreStatus, GarbageColl=
ectionStatus, GroupListItem,
> > -    KeepOptions, Operation, PruneJobOptions, RRDMode, RRDTimeFrame, Sn=
apshotListItem,
> > -    SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA,=
 BACKUP_NAMESPACE_SCHEMA,
> > -    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_V=
ERIFIED_BACKUPS_SCHEMA,
> > -    MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PR=
IV_DATASTORE_BACKUP,
> > -    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, =
PRIV_DATASTORE_VERIFY,
> > -    UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
> > +    Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreSt=
atus,
> > +    GarbageCollectionStatus, GroupListItem, KeepOptions, Operation, Pr=
uneJobOptions, RRDMode,
> > +    RRDTimeFrame, SnapshotListItem, SnapshotVerifyState, BACKUP_ARCHIV=
E_NAME_SCHEMA,
> > +    BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, BAC=
KUP_TYPE_SCHEMA,
> > +    DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DE=
PTH, NS_MAX_DEPTH_SCHEMA,
> > +    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY=
, PRIV_DATASTORE_PRUNE,
> > +    PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID_SCHEMA, VERIFICAT=
ION_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?
> >   }
> >  =20
> > +pub fn do_mount_device(
> > +    datastore: DataStoreConfig,
> > +    worker: Option<&dyn WorkerTaskContext>,
> > +) -> Result<(), Error> {
> > +    if let Some(uuid) =3D datastore.backing_device.as_ref() {
> > +        if pbs_datastore::is_datastore_available(&datastore) {
> > +            bail!("device '{uuid}' is already mounted");
>
> this...
>

this also happens when creating a datastore, so creating one would fail
with mentioning a store that doesn't exist, which might be a little
weird. but no hard feelings=20

> > +        }
> > +        let mount_point_path =3D std::path::Path::new(&datastore.path)=
;
> > +        if let Some(worker) =3D worker {
> > +            task_log!(worker, "mounting '{uuid}' to '{}'", datastore.p=
ath);
>
> ... 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}' ...`
>

here it would make a lot of easier

> > +        }
> > +        crate::tools::disks::mount_by_uuid(uuid, mount_point_path)?;
> > +
> > +        Ok(())
> > +    } else {
> > +        Err(format_err!(
> > +            "Datastore '{}' can't be mounted because it is not removab=
le.",
>
> 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) =3D pbs_config::datastore::config()?=
;
> > +    let datastore: DataStoreConfig =3D section_config.lookup("datastor=
e", &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 =3D rpcenv.get_auth_id().unwrap().parse()?;
> > +    let to_stdout =3D rpcenv.env_type() =3D=3D RpcEnvironmentType::CLI=
;
> > +
> > +    let upid =3D 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 _ =3D crate::tools::disks::unmount_by_mountpoint(&datastor=
e.path);
> > +        return Ok(());
> > +    }
> > +
> > +    let mut active_operations =3D task_tracking::get_active_operations=
(&datastore.name)?;
> > +    let mut counter =3D 0;
> > +    while active_operations.read + active_operations.write > 0 {
> > +        if counter =3D=3D 0 {
> > +            if let Some(worker) =3D worker {
> > +                if worker.abort_requested() {
> > +                    bail!("aborted, due to user request");
> > +                }
> > +                task_log!(
> > +                    worker,
> > +                    "can't unmount yet, still {} read and {} write ope=
rations active",
>
> s/can't/cannot
>
> > +                    active_operations.read,
> > +                    active_operations.write
> > +                );
> > +            }
> > +            counter =3D 5000;
> > +        }
> > +        counter -=3D 1;
> > +        std::thread::sleep(std::time::Duration::from_millis(1));
> > +        active_operations =3D task_tracking::get_active_operations(&da=
tastore.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 o=
perations.",
> > +                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 =3D pbs_config::datastore::lock_config()?;
> > +    let (section_config, _digest) =3D pbs_config::datastore::config()?=
;
> > +    let datastore: DataStoreConfig =3D section_config.lookup("datastor=
e", &store)?;
> > +
> > +    if datastore.backing_device.is_none() {
> > +        bail!("datastore '{}' is not removable", &store);
> > +    }
> > +    drop(_lock);
> > +
> > +    let auth_id: Authid =3D rpcenv.get_auth_id().unwrap().parse()?;
> > +    let to_stdout =3D rpcenv.env_type() =3D=3D RpcEnvironmentType::CLI=
;
> > +
> > +    if let Ok(proxy_pid) =3D proxmox_rest_server::read_pid(pbs_buildcf=
g::PROXMOX_BACKUP_PROXY_PID_FN)
> > +    {
> > +        let sock =3D proxmox_rest_server::ctrl_sock_from_pid(proxy_pid=
);
> > +        let _ =3D proxmox_rest_server::send_raw_command(
> > +            sock,
> > +            &format!("{{\"command\":\"update-datastore-cache\",\"args\=
":\"{store}\"}}\n"),
> > +        )
> > +        .await;
> > +    }
> > +
> > +    let upid =3D 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 =3D &[
> >       (
> > @@ -2316,6 +2473,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap =3D &[
> >               .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 =3D &[
> >               .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),