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 03B1E96EE3
for <pbs-devel@lists.proxmox.com>; Tue, 16 Apr 2024 16:25:10 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id CFD231D056
for <pbs-devel@lists.proxmox.com>; Tue, 16 Apr 2024 16:25:09 +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>; Tue, 16 Apr 2024 16:25:08 +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 7976845208
for <pbs-devel@lists.proxmox.com>; Tue, 16 Apr 2024 16:25:08 +0200 (CEST)
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Tue, 16 Apr 2024 16:25:06 +0200
Message-Id: <D0LM6SYRVJI2.2DBOTADDQCV6E@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: <20240409110012.166472-1-h.laimer@proxmox.com>
<20240409110012.166472-7-h.laimer@proxmox.com>
<4fb80cd2-ea6c-42b1-abf9-308a9a1debf2@proxmox.com>
In-Reply-To: <4fb80cd2-ea6c-42b1-abf9-308a9a1debf2@proxmox.com>
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.008 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
Subject: Re: [pbs-devel] [PATCH proxmox-backup v3 06/24] 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: Tue, 16 Apr 2024 14:25:10 -0000
On Mon Apr 15, 2024 at 5:50 PM CEST, Christian Ebner wrote:
> a few comments inline
>
> On 4/9/24 12:59, Hannes Laimer wrote:
> > Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> > ---
> > pbs-api-types/src/datastore.rs | 12 ++
> > pbs-api-types/src/maintenance.rs | 4 +
> > src/api2/admin/datastore.rs | 189 +++++++++++++++++++++++++++++-=
-
> > 3 files changed, 196 insertions(+), 9 deletions(-)
> >=20
> > diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datasto=
re.rs
> > index f57957d2..b6d238ed 100644
> > --- a/pbs-api-types/src/datastore.rs
> > +++ b/pbs-api-types/src/datastore.rs
> > @@ -351,6 +351,18 @@ impl DataStoreConfig {
> > .and_then(|str| MaintenanceMode::API_SCHEMA.parse_propert=
y_string(str).ok())
> > .and_then(|value| MaintenanceMode::deserialize(value).ok(=
))
> > }
> > +
> > + pub fn set_maintenance_mode(&mut self, mode: Option<MaintenanceMod=
e>) {
> > + self.maintenance_mode =3D mode.and_then(|mode| {
> > + let mut writer =3D String::new();
> > + mode.serialize(proxmox_schema::ser::PropertyStringSerializ=
er::new(
> > + &mut writer,
> > + &MaintenanceMode::API_SCHEMA,
> > + ))
> > + .ok()?;
> > + Some(writer)
> > + });
> > + }
> > }
> > =20
> > #[api(
> > diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maint=
enance.rs
> > index e7ffbcd0..3868688c 100644
> > --- a/pbs-api-types/src/maintenance.rs
> > +++ b/pbs-api-types/src/maintenance.rs
> > @@ -78,6 +78,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 f7164b87..92f6adc2 100644
> > --- a/src/api2/admin/datastore.rs
> > +++ b/src/api2/admin/datastore.rs
> > @@ -26,23 +26,24 @@ 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, MaintenanceMo=
de, MaintenanceType,
> > + Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListIte=
m, SnapshotVerifyState,
> > + BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCH=
EMA, BACKUP_TIME_SCHEMA,
> > + BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHE=
MA, 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;
> > +use pbs_config::{BackupLockGuard, CachedUserInfo};
> > use pbs_datastore::backup_info::BackupInfo;
> > use pbs_datastore::cached_chunk_reader::CachedChunkReader;
> > use pbs_datastore::catalog::{ArchiveEntry, CatalogReader};
> > @@ -59,6 +60,7 @@ use pbs_datastore::{
> > };
> > use pbs_tools::json::required_string_param;
> > use proxmox_rest_server::{formatter, WorkerTask};
> > +use proxmox_section_config::SectionConfigData;
> > =20
> > use crate::api2::backup::optional_ns_param;
> > use crate::api2::node::rrd::create_value_from_rrd;
> > @@ -2240,6 +2242,173 @@ pub async fn set_backup_owner(
> > .await?
> > }
> > =20
> > +pub fn do_mount_device(
> > + _lock: Option<BackupLockGuard>,
> > + mut config: SectionConfigData,
> > + mut datastore: DataStoreConfig,
> > + worker: Option<&dyn WorkerTaskContext>,
> > +) -> Result<(), Error> {
> > + if let Some(uuid) =3D datastore.backing_device.as_ref() {
> > + if pbs_datastore::check_if_available(&datastore).is_ok() {
> > + return Err(format_err!("device '{}' is already mounted", &=
uuid));
> > + }
>
> use `bail` here and inline the `uuid`:
>
> bail!("device '{uuid}' is already mounted");
>
> > + let mount_point_path =3D std::path::Path::new(&datastore.path)=
;
> > + if let Some(worker) =3D worker {
> > + task_log!(worker, "mounting '{}' to '{}'", uuid, datastore=
.path);
>
> inline `uuid`
>
> > + }
> > + crate::tools::disks::mount_by_uuid(uuid, mount_point_path)?;
> > +
> > + datastore.set_maintenance_mode(None);
> > + config.set_data(&datastore.name, "datastore", &datastore)?;
> > + pbs_config::datastore::save_config(&config)?;
> > +
> > + Ok(())
> > + } else {
> > + Err(format_err!(
> > + "Datastore '{}' can't be mounted because it is not removab=
le.",
> > + &datastore.name
>
> this does not need to be a reference, you can pass in `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);
> > + }
> > +
> > + let lock =3D pbs_config::datastore::lock_config()?;
> > + 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(Some(lock), section_config, data=
store, Some(&worker)),
> > + )?;
>
> What is the motivation for doing this inside a dedicated worker task? Is=
=20
> this expected to take a considerable amount of time in some cases?
>
mostly so auto mounts appear in the task log, and sometimes it takes a
few seconds
> > +
> > + 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)?;
> > + while active_operations.read + active_operations.write > 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 operati=
ons active",
> > + active_operations.read,
> > + active_operations.write
> > + );
> > + }
> > +
> > + std::thread::sleep(std::time::Duration::new(5, 0));
> > + 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 (mut section_config, _digest) =3D pbs_config::datastore::confi=
g()?;
> > + let mut datastore: DataStoreConfig =3D section_config.lookup("data=
store", &store)?;
> > +
> > + if datastore.backing_device.is_none() {
> > + bail!("datastore '{}' is not removable", &store);
> > + }
> > +
> > + let auth_id: Authid =3D rpcenv.get_auth_id().unwrap().parse()?;
> > + let to_stdout =3D rpcenv.env_type() =3D=3D RpcEnvironmentType::CLI=
;
> > +
> > + datastore.set_maintenance_mode(Some(MaintenanceMode::new(Maintenan=
ceType::Unplugged, None)));
> > + section_config.set_data(&datastore.name, "datastore", &datastore)?=
;
> > + pbs_config::datastore::save_config(§ion_config)?;
> > + drop(_lock);
> > +
> > + 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\":\"{=
}\"}}\n",
> > + &store
>
> `store` can be inlined as well here, also no need for taking it by refere=
nce
>
> > + ),
> > + )
> > + .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 &[
> > (
> > @@ -2278,6 +2447,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?!
> > @@ -2312,6 +2482,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),