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 19925964B7 for ; Mon, 15 Apr 2024 17:50:23 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id EC221E239 for ; Mon, 15 Apr 2024 17:50:22 +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 ; Mon, 15 Apr 2024 17:50:21 +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 9AFAA44C5F for ; Mon, 15 Apr 2024 17:50:21 +0200 (CEST) Message-ID: <4fb80cd2-ea6c-42b1-abf9-308a9a1debf2@proxmox.com> Date: Mon, 15 Apr 2024 17:50:19 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird To: Proxmox Backup Server development discussion , Hannes Laimer References: <20240409110012.166472-1-h.laimer@proxmox.com> <20240409110012.166472-7-h.laimer@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20240409110012.166472-7-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.030 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: Mon, 15 Apr 2024 15:50:23 -0000 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(-) > > diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.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_property_string(str).ok()) > .and_then(|value| MaintenanceMode::deserialize(value).ok()) > } > + > + pub fn set_maintenance_mode(&mut self, mode: Option) { > + self.maintenance_mode = mode.and_then(|mode| { > + let mut writer = String::new(); > + mode.serialize(proxmox_schema::ser::PropertyStringSerializer::new( > + &mut writer, > + &MaintenanceMode::API_SCHEMA, > + )) > + .ok()?; > + Some(writer) > + }); > + } > } > > #[api( > diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.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 { > } > > impl MaintenanceMode { > + pub fn new(ty: MaintenanceType, message: Option) -> Self { > + Self { ty, message } > + } > + > /// Used for deciding whether the datastore is cleared from the internal 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, CreateOptions, > }; > -use proxmox_sys::{task_log, task_warn}; > +use proxmox_sys::{task_log, task_warn, WorkerTaskContext}; > > use pxar::accessor::aio::Accessor; > use pxar::EntryKind; > > use pbs_api_types::{ > print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType, > - Counts, CryptMode, DataStoreListItem, DataStoreStatus, GarbageCollectionStatus, GroupListItem, > - KeepOptions, Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem, > - SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, > - BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, > - 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, > + Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreStatus, > + GarbageCollectionStatus, GroupListItem, KeepOptions, MaintenanceMode, MaintenanceType, > + Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem, SnapshotVerifyState, > + BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA, > + BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, 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; > > 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? > } > > +pub fn do_mount_device( > + _lock: Option, > + mut config: SectionConfigData, > + mut datastore: DataStoreConfig, > + worker: Option<&dyn WorkerTaskContext>, > +) -> Result<(), Error> { > + if let Some(uuid) = 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 = std::path::Path::new(&datastore.path); > + if let Some(worker) = 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 removable.", > + &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) = pbs_config::datastore::config()?; > + let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?; > + > + if datastore.backing_device.is_none() { > + bail!("datastore '{}' is not removable", &store); > + } > + > + let lock = pbs_config::datastore::lock_config()?; > + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; > + let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI; > + > + let upid = WorkerTask::new_thread( > + "mount-device", > + Some(store), > + auth_id.to_string(), > + to_stdout, > + move |worker| do_mount_device(Some(lock), section_config, datastore, Some(&worker)), > + )?; What is the motivation for doing this inside a dedicated worker task? Is this expected to take a considerable amount of time in some cases? > + > + Ok(json!(upid)) > +} > + > +fn do_unmount_device( > + force: bool, > + datastore: DataStoreConfig, > + worker: Option<&dyn WorkerTaskContext>, > +) -> Result<(), Error> { > + if force { > + let _ = crate::tools::disks::unmount_by_mountpoint(&datastore.path); > + return Ok(()); > + } > + > + let mut active_operations = task_tracking::get_active_operations(&datastore.name)?; > + while active_operations.read + active_operations.write > 0 { > + if let Some(worker) = worker { > + if worker.abort_requested() { > + bail!("aborted, due to user request"); > + } > + task_log!( > + worker, > + "can't unmount yet, still {} read and {} write operations active", > + active_operations.read, > + active_operations.write > + ); > + } > + > + std::thread::sleep(std::time::Duration::new(5, 0)); > + active_operations = task_tracking::get_active_operations(&datastore.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 operations.", > + 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 = pbs_config::datastore::lock_config()?; > + let (mut section_config, _digest) = pbs_config::datastore::config()?; > + let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?; > + > + if datastore.backing_device.is_none() { > + bail!("datastore '{}' is not removable", &store); > + } > + > + let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?; > + let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI; > + > + datastore.set_maintenance_mode(Some(MaintenanceMode::new(MaintenanceType::Unplugged, None))); > + section_config.set_data(&datastore.name, "datastore", &datastore)?; > + pbs_config::datastore::save_config(§ion_config)?; > + drop(_lock); > + > + if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN) > + { > + let sock = proxmox_rest_server::ctrl_sock_from_pid(proxy_pid); > + let _ = 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 reference > + ), > + ) > + .await; > + } > + > + let upid = 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 = &[ > ( > @@ -2278,6 +2447,7 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[ > .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 = &[ > .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),