From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; 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: From: "Hannes Laimer" To: "Christian Ebner" , "Proxmox Backup Server development discussion" 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 > > --- > > 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) { > > + 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) -> 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, > > + 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= { > > + 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 { > > + 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),