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 74DC297893 for ; 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 ; 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 ; 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 ; 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: From: "Hannes Laimer" To: "Christian Ebner" , "Proxmox Backup Server development discussion" X-Mailer: aerc 0.17.0-78-g4ffbaa6b3946 References: <20240416152416.96561-1-h.laimer@proxmox.com> <20240416152416.96561-7-h.laimer@proxmox.com> In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 > > --- > > 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) -> 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= { > > + 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 { > > + 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),