[pbs-devel] [PATCH proxmox-backup v4 00/22] add removable datastores

[pbs-devel] [PATCH proxmox-backup v4 00/22] add removable datastores

[Hannes Laimer]

These patches add support for removable datastores. All removable
datastores have a backing-device(a UUID) associated with them. Removable
datastores work like normal ones, just that they can be unplugged. It is
possible to create a removable datastore, sync backups onto it, unplug
it and use it on a different PBS.

The datastore path is also the mountpoint for the removable datastore.
By default when creating one through the web UI it will be
`/mnt/removable-datastores/<UUID>`, using the CLI it is possible to
specify something else. Since a removable datastore is associated with
the UUID of a partition, it is technically possible to have two
removable datastores on the same device, but I don't think there is a
use-case that couldn't also be done using namespaces.

When a removable datastore is deleted and 'destroy-data' is set, the
device has to be plugged in. If 'destroy-data' is not set the datastore
can be deleted even if the device is not present. Removable datastores
are automatically mounted when plugged in. At the API service start all

Patche 20/22 is not strictly needed, but they made sense in
this context, so I kept them in this series. 


 
v4: thanks a lot @Dietmar and @Christian
 * make check if mounted wayyy faster, checking takes ~0.1ms, only
    happens once per operation and only on removable datastores so that
    seems fast enough to just do it instead of keeping track of the state
 * don't keep track of mounting state
 * drop Unplugged maintenance mode
 * use UUID_FORMAT for uuid field
 * a lot of small things, like use of bail!, inline format!, ...
 * include improvement to cache handling

v3:
 * remove lazy unmounting (since 9cba51ac782d04085c0af55128f32178e5132358 is applied)
 * fix CLI (un)mount command, thanks @Gabriel
 * add removable datastore CLI autocomplete helper
 * rebase onto master
 * move ui patches to the end

thanks @Lukas and @Thomas for the feedback
v2:
 * fix datastore 'add' button in the UI
 * some format!("{}", a) -> format!("{a}")
 * replace `const` with `let` in js code
 * change icon `fa-usb` -> `fa-plug`
 * add some docs
 * add JDoc for parseMaintenanceMode
 * proxmox-schema dep bump

Hannes Laimer (22):
  tools: add disks utility functions
  datastore: make dropping from cache more efficient
  pbs-api-types: add backing-device to DataStoreConfig
  disks: add UUID to partition info
  add helper for checking if a removable datastore is available
  api2: admin: add (un)mount endpoint for removable datastores
  api2: removable datastore creation
  api2: disks list: add only-unused flag
  pbs-api-types: datastore: use new proxmox_schema::de for
    deserialization
  pbs-api-types: add removable/is-available flag to DataStoreListItem
  pb-manager: add (un)mount command
  add auto-mounting for removable datastores
  datastore: handle deletion of removable datastore properly
  docs: mention maintenance mode reset when removable datastore is
    unplugged
  ui: add partition selector form
  ui: add removable datastore creation support
  ui: add (un)mount button to summary
  ui: display removable datastores in list
  ui: utils: render unplugged datastores correctly
  ui: utils: make parseMaintenanceMode more robust
  ui: add datastore status mask for unplugged removable datastores
  ui: maintenance: fix disable msg field if no type is selected

 debian/proxmox-backup-server.install        |   1 +
 debian/proxmox-backup-server.udev           |   3 +
 docs/maintenance.rst                        |   2 +
 etc/Makefile                                |   3 +-
 etc/removable-device-attach@.service.in     |   8 +
 pbs-api-types/src/datastore.rs              |  32 +++-
 pbs-api-types/src/maintenance.rs            |   4 +
 pbs-config/src/datastore.rs                 |  14 ++
 pbs-datastore/src/datastore.rs              | 119 ++++++++----
 pbs-datastore/src/lib.rs                    |   2 +-
 src/api2/admin/datastore.rs                 | 192 ++++++++++++++++++--
 src/api2/config/datastore.rs                |  75 +++++++-
 src/api2/node/disks/mod.rs                  |   8 +
 src/api2/status.rs                          |  18 +-
 src/bin/proxmox_backup_manager/datastore.rs | 130 ++++++++++++-
 src/tools/disks/mod.rs                      |  90 ++++++++-
 www/Makefile                                |   1 +
 www/NavigationTree.js                       |  15 +-
 www/Utils.js                                |  31 +++-
 www/css/ext6-pbs.css                        |  20 ++
 www/datastore/DataStoreListSummary.js       |   1 +
 www/datastore/Summary.js                    | 103 ++++++++++-
 www/form/PartitionSelector.js               |  74 ++++++++
 www/window/DataStoreEdit.js                 |  51 ++++++
 www/window/MaintenanceOptions.js            |   7 +-
 25 files changed, 903 insertions(+), 101 deletions(-)
 create mode 100644 etc/removable-device-attach@.service.in
 create mode 100644 www/form/PartitionSelector.js

-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 01/22] tools: add disks utility functions

[Hannes Laimer]

... for mounting and unmounting

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/tools/disks/mod.rs | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 94f89e0a..68854918 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -1323,3 +1323,22 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
 
     bail!("get_fs_uuid failed - missing UUID");
 }
+
+/// Mount a disk by its UUID and the mount point.
+pub fn mount_by_uuid(uuid: &str, mount_point: &Path) -> Result<(), Error> {
+    let mut command = std::process::Command::new("mount");
+    command.args([&format!("UUID={uuid}")]);
+    command.arg(mount_point);
+
+    proxmox_sys::command::run_command(command, None)?;
+    Ok(())
+}
+
+/// Unmount a disk by its mount point.
+pub fn unmount_by_mountpoint(path: &str) -> Result<(), Error> {
+    let mut command = std::process::Command::new("umount");
+    command.arg(path);
+
+    proxmox_sys::command::run_command(command, None)?;
+    Ok(())
+}
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 02/22] datastore: make dropping from cache more efficient

[Hannes Laimer]

... by not having to read the whole config file on every drop,
but assume calling `update-datastore-cache` through the scoket implies
it should be dropped and doing the checking on the sending side.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 60 +++++++++++++++-------------------
 src/api2/config/datastore.rs   |  8 +++--
 2 files changed, 31 insertions(+), 37 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 0685cc84..a7fe3b8c 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -34,7 +34,7 @@ use crate::task_tracking::{self, update_active_operations};
 use crate::DataBlob;
 
 lazy_static! {
-    static ref DATASTORE_MAP: Mutex<HashMap<String, Arc<DataStoreImpl>>> =
+    static ref DATASTORE_MAP: Mutex<HashMap<String, (Arc<DataStoreImpl>, bool)>> =
         Mutex::new(HashMap::new());
 }
 
@@ -111,24 +111,15 @@ impl Drop for DataStore {
                     last_task = updated_operations.read + updated_operations.write == 0;
                 }
             }
-
-            // remove datastore from cache iff
-            //  - last task finished, and
-            //  - datastore is in a maintenance mode that mandates it
-            let remove_from_cache = last_task
-                && pbs_config::datastore::config()
-                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", self.name()))
-                    .map_or(false, |c| {
-                        c.get_maintenance_mode().map_or(false, |m| m.is_offline())
-                    });
-
-            if remove_from_cache {
-                DATASTORE_MAP.lock().unwrap().remove(self.name());
+            if last_task {
+                let mut cache = DATASTORE_MAP.lock().unwrap();
+                if let Some((_, true)) = cache.get(self.name()) {
+                    cache.remove(self.name());
+                }
             }
         }
     }
 }
-
 impl DataStore {
     // This one just panics on everything
     #[doc(hidden)]
@@ -169,7 +160,7 @@ impl DataStore {
         let entry = datastore_cache.get(name);
 
         // reuse chunk store so that we keep using the same process locker instance!
-        let chunk_store = if let Some(datastore) = &entry {
+        let (chunk_store, drop_from_cache) = if let Some((datastore, drop_from_cache)) = &entry {
             let last_digest = datastore.last_digest.as_ref();
             if let Some(true) = last_digest.map(|last_digest| last_digest == &digest) {
                 return Ok(Arc::new(Self {
@@ -177,23 +168,26 @@ impl DataStore {
                     operation,
                 }));
             }
-            Arc::clone(&datastore.chunk_store)
+            (Arc::clone(&datastore.chunk_store), *drop_from_cache)
         } else {
             let tuning: DatastoreTuning = serde_json::from_value(
                 DatastoreTuning::API_SCHEMA
                     .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
             )?;
-            Arc::new(ChunkStore::open(
-                name,
-                &config.path,
-                tuning.sync_level.unwrap_or_default(),
-            )?)
+            (
+                Arc::new(ChunkStore::open(
+                    name,
+                    &config.path,
+                    tuning.sync_level.unwrap_or_default(),
+                )?),
+                false,
+            )
         };
 
         let datastore = DataStore::with_store_and_config(chunk_store, config, Some(digest))?;
 
         let datastore = Arc::new(datastore);
-        datastore_cache.insert(name.to_string(), datastore.clone());
+        datastore_cache.insert(name.to_string(), (datastore.clone(), drop_from_cache));
 
         Ok(Arc::new(Self {
             inner: datastore,
@@ -211,20 +205,18 @@ impl DataStore {
         Ok(())
     }
 
-    /// trigger clearing cache entry based on maintenance mode. Entry will only
-    /// be cleared iff there is no other task running, if there is, the end of the
+    /// trigger clearing of cache entry. Entry will only be cleared iff
+    /// there is no other task running, if there is, the end of the
     /// last running task will trigger the clearing of the cache entry.
     pub fn update_datastore_cache(name: &str) -> Result<(), Error> {
-        let (config, _digest) = pbs_config::datastore::config()?;
-        let datastore: DataStoreConfig = config.lookup("datastore", name)?;
-        if datastore
-            .get_maintenance_mode()
-            .map_or(false, |m| m.is_offline())
-        {
-            // the datastore drop handler does the checking if tasks are running and clears the
-            // cache entry, so we just have to trigger it here
-            let _ = DataStore::lookup_datastore(name, Some(Operation::Lookup));
+        let _store = DataStore::lookup_datastore(name, Some(Operation::Lookup));
+
+        let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
+        if let Some((_, drop_from_cache)) = datastore_cache.get_mut(name) {
+            *drop_from_cache = true;
         }
+        drop(datastore_cache);
+        drop(_store);
 
         Ok(())
     }
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 3081e1f4..0b3d92f9 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -389,10 +389,12 @@ pub fn update_datastore(
         data.tuning = update.tuning;
     }
 
-    let mut maintenance_mode_changed = false;
+    let mut drop_store_from_cache = false;
     if update.maintenance_mode.is_some() {
-        maintenance_mode_changed = data.maintenance_mode != update.maintenance_mode;
         data.maintenance_mode = update.maintenance_mode;
+        drop_store_from_cache = data
+            .get_maintenance_mode()
+            .map_or(false, |m| m.is_offline());
     }
 
     config.set_data(&name, "datastore", &data)?;
@@ -406,7 +408,7 @@ pub fn update_datastore(
     }
 
     // tell the proxy it might have to clear a cache entry
-    if maintenance_mode_changed {
+    if drop_store_from_cache {
         tokio::spawn(async move {
             if let Ok(proxy_pid) =
                 proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 03/22] pbs-api-types: add backing-device to DataStoreConfig

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 5e13c157..898149ea 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -273,6 +273,12 @@ pub const DATASTORE_TUNING_STRING_SCHEMA: Schema = StringSchema::new("Datastore
             format: &ApiStringFormat::PropertyString(&MaintenanceMode::API_SCHEMA),
             type: String,
         },
+        "backing-device": {
+            description: "The UUID of the filesystem partition for removable datastores.",
+            optional: true,
+            format: &proxmox_schema::api_types::UUID_FORMAT,
+            type: String,
+        }
     }
 )]
 #[derive(Serialize, Deserialize, Updater, Clone, PartialEq)]
@@ -316,6 +322,11 @@ pub struct DataStoreConfig {
     /// Maintenance mode, type is either 'offline' or 'read-only', message should be enclosed in "
     #[serde(skip_serializing_if = "Option::is_none")]
     pub maintenance_mode: Option<String>,
+
+    /// The UUID of the device(for removable datastores)
+    #[updater(skip)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub backing_device: Option<String>,
 }
 
 impl DataStoreConfig {
@@ -332,6 +343,7 @@ impl DataStoreConfig {
             notify: None,
             tuning: None,
             maintenance_mode: None,
+            backing_device: None,
         }
     }
 
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 04/22] disks: add UUID to partition info

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/tools/disks/mod.rs | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 68854918..eaf016df 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -59,6 +59,8 @@ pub struct LsblkInfo {
     /// File system label.
     #[serde(rename = "fstype")]
     file_system_type: Option<String>,
+    /// File system UUID.
+    uuid: Option<String>,
 }
 
 impl DiskManage {
@@ -617,7 +619,7 @@ pub struct BlockDevStat {
 /// Use lsblk to read partition type uuids and file system types.
 pub fn get_lsblk_info() -> Result<Vec<LsblkInfo>, Error> {
     let mut command = std::process::Command::new("lsblk");
-    command.args(["--json", "-o", "path,parttype,fstype"]);
+    command.args(["--json", "-o", "path,parttype,fstype,uuid"]);
 
     let output = proxmox_sys::command::run_command(command, None)?;
 
@@ -701,6 +703,8 @@ pub struct PartitionInfo {
     pub size: Option<u64>,
     /// GPT partition
     pub gpt: bool,
+    /// UUID
+    pub uuid: Option<String>,
 }
 
 #[api(
@@ -891,8 +895,10 @@ fn get_partitions_info(
 
             let mounted = disk.is_mounted().unwrap_or(false);
             let mut filesystem = None;
+            let mut uuid = None;
             if let (Some(devpath), Some(infos)) = (devpath.as_ref(), lsblk_infos.as_ref()) {
                 for info in infos.iter().filter(|i| i.path.eq(devpath)) {
+                    uuid = info.uuid.clone();
                     used = match info.partition_type.as_deref() {
                         Some("21686148-6449-6e6f-744e-656564454649") => PartitionUsageType::BIOS,
                         Some("c12a7328-f81f-11d2-ba4b-00a0c93ec93b") => PartitionUsageType::EFI,
@@ -915,6 +921,7 @@ fn get_partitions_info(
                 filesystem,
                 size: disk.size().ok(),
                 gpt: disk.has_gpt(),
+                uuid,
             }
         })
         .collect()
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 05/22] add helper for checking if a removable datastore is available

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 49 ++++++++++++++++++++++++++++++++++
 pbs-datastore/src/lib.rs       |  2 +-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index a7fe3b8c..ff2ca95f 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -14,6 +14,7 @@ use proxmox_schema::ApiType;
 use proxmox_sys::error::SysError;
 use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions};
 use proxmox_sys::fs::{lock_dir_noblock, DirLockGuard};
+use proxmox_sys::linux::procfs::MountInfo;
 use proxmox_sys::process_locker::ProcessLockSharedGuard;
 use proxmox_sys::WorkerTaskContext;
 use proxmox_sys::{task_log, task_warn};
@@ -49,6 +50,35 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error>
     Ok(())
 }
 
+pub fn is_datastore_available(config: &DataStoreConfig) -> bool {
+    config.backing_device.as_ref().map_or(true, |uuid| {
+        if let Ok(store_dev_path) = std::fs::read_link(Path::new("/dev/disk/by-uuid").join(uuid))
+            .map(|p| p.to_string_lossy().into_owned())
+            .map(|p| p.replace("../..", "/dev"))
+            .map(PathBuf::from)
+        {
+            let store_mount_point = PathBuf::from(&config.path);
+
+            MountInfo::read().map_or(false, |mount_info| {
+                mount_info
+                    .iter()
+                    .filter_map(|(_, entry)| {
+                        entry
+                            .mount_source
+                            .as_ref()
+                            .map(|source| (&entry.mount_point, source))
+                    })
+                    .any(|(mount_point, mount_source)| {
+                        PathBuf::from(&mount_point) == store_mount_point
+                            && PathBuf::from(&mount_source) == store_dev_path
+                    })
+            })
+        } else {
+            false
+        }
+    })
+}
+
 /// Datastore Management
 ///
 /// A Datastore can store severals backups, and provides the
@@ -149,6 +179,21 @@ impl DataStore {
             }
         }
 
+        if config.backing_device.is_some() {
+            let is_mounted = is_datastore_available(&config);
+            let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
+            if let Some((_, true)) = datastore_cache.get(&config.name) {
+                if !is_mounted {
+                    datastore_cache.remove(&config.name);
+                }
+                bail!("Removable Datastore in progress to being removed.");
+            }
+            if !is_mounted {
+                datastore_cache.remove(&config.name);
+                bail!("Removable Datastore is not mounted");
+            }
+        }
+
         if let Some(operation) = operation {
             update_active_operations(name, operation, 1)?;
         }
@@ -253,6 +298,10 @@ impl DataStore {
     ) -> Result<Arc<Self>, Error> {
         let name = config.name.clone();
 
+        if !is_datastore_available(&config) {
+            bail!("Datastore is not available")
+        }
+
         let tuning: DatastoreTuning = serde_json::from_value(
             DatastoreTuning::API_SCHEMA
                 .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 43050162..458f93d9 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -206,7 +206,7 @@ pub use manifest::BackupManifest;
 pub use store_progress::StoreProgress;
 
 mod datastore;
-pub use datastore::{check_backup_owner, DataStore};
+pub use datastore::{check_backup_owner, is_datastore_available, DataStore};
 
 mod hierarchy;
 pub use hierarchy::{
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 06/22] api2: admin: add (un)mount endpoint for removable datastores

[Hannes Laimer]

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(-)

diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.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 {
 }
 
 impl MaintenanceMode {
+    pub fn new(ty: MaintenanceType, message: Option<String>) -> 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 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, 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, 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;
@@ -2278,6 +2278,163 @@ pub async fn set_backup_owner(
     .await?
 }
 
+pub fn do_mount_device(
+    datastore: DataStoreConfig,
+    worker: Option<&dyn WorkerTaskContext>,
+) -> Result<(), Error> {
+    if let Some(uuid) = datastore.backing_device.as_ref() {
+        if pbs_datastore::is_datastore_available(&datastore) {
+            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 '{uuid}' to '{}'", datastore.path);
+        }
+        crate::tools::disks::mount_by_uuid(uuid, mount_point_path)?;
+
+        Ok(())
+    } else {
+        Err(format_err!(
+            "Datastore '{}' can't be mounted because it is not removable.",
+            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) = 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 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(datastore, Some(&worker)),
+    )?;
+
+    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)?;
+    let mut counter = 0;
+    while active_operations.read + active_operations.write > 0 {
+        if counter == 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
+                );
+            }
+            counter = 5000;
+        }
+        counter -= 1;
+        std::thread::sleep(std::time::Duration::from_millis(1));
+        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<Value, Error> {
+    let _lock = pbs_config::datastore::lock_config()?;
+    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);
+    }
+    drop(_lock);
+
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+    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\":\"{store}\"}}\n"),
+        )
+        .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 = &[
     (
@@ -2316,6 +2473,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?!
@@ -2350,6 +2508,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),
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 07/22] api2: removable datastore creation

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/config/datastore.rs | 53 ++++++++++++++++++++++++++++++++++--
 1 file changed, 51 insertions(+), 2 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 0b3d92f9..3a075674 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -8,7 +8,7 @@ use serde_json::Value;
 use proxmox_router::{http_bail, Permission, Router, RpcEnvironment, RpcEnvironmentType};
 use proxmox_schema::{api, param_bail, ApiType};
 use proxmox_section_config::SectionConfigData;
-use proxmox_sys::{task_warn, WorkerTaskContext};
+use proxmox_sys::{task_log, task_warn, WorkerTaskContext};
 use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
@@ -20,7 +20,8 @@ use pbs_config::BackupLockGuard;
 use pbs_datastore::chunk_store::ChunkStore;
 
 use crate::api2::admin::{
-    prune::list_prune_jobs, sync::list_sync_jobs, verify::list_verification_jobs,
+    datastore::do_mount_device, prune::list_prune_jobs, sync::list_sync_jobs,
+    verify::list_verification_jobs,
 };
 use crate::api2::config::prune::{delete_prune_job, do_create_prune_job};
 use crate::api2::config::sync::delete_sync_job;
@@ -72,6 +73,32 @@ pub(crate) fn do_create_datastore(
     datastore: DataStoreConfig,
     worker: Option<&dyn WorkerTaskContext>,
 ) -> Result<(), Error> {
+    if datastore.backing_device.is_some() {
+        let mut mount_point = PathBuf::from(&datastore.path);
+
+        let default_options = proxmox_sys::fs::CreateOptions::new();
+        proxmox_sys::fs::create_path(
+            &mount_point,
+            Some(default_options.clone()),
+            Some(default_options),
+        )?;
+        do_mount_device(datastore.clone(), worker)?;
+
+        mount_point.push(".chunks");
+        if mount_point.is_dir() {
+            config.set_data(&datastore.name, "datastore", &datastore)?;
+            pbs_config::datastore::save_config(&config)?;
+            jobstate::create_state_file("garbage_collection", &datastore.name)?;
+            if let Some(worker) = worker {
+                task_log!(
+                    worker,
+                    "created removable datastore, chunkstore already exists"
+                );
+            }
+            return Ok(());
+        }
+    }
+
     let path: PathBuf = datastore.path.clone().into();
 
     let tuning: DatastoreTuning = serde_json::from_value(
@@ -122,6 +149,28 @@ pub fn create_datastore(
         param_bail!("name", "datastore '{}' already exists.", config.name);
     }
 
+    if let Some(uuid) = &config.backing_device {
+        let already_used_by = section_config
+            .sections
+            .iter()
+            .flat_map(|(datastore_name, (_, config))| {
+                config
+                    .as_object()
+                    .and_then(|cfg| cfg.get("backing-device"))
+                    .and_then(|backing_device| backing_device.as_str())
+                    .filter(|&device_uuid| device_uuid == uuid)
+                    .map(|_| datastore_name)
+            })
+            .next();
+
+        if let Some(datastore_name) = already_used_by {
+            param_bail!(
+                "backing-device",
+                "device already in use by datastore '{datastore_name}'",
+            );
+        }
+    }
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
 
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 08/22] api2: disks list: add exclude-used flag

[Hannes Laimer]

... used by the partition selector for removable datastore creation.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/node/disks/mod.rs |  8 +++++
 src/tools/disks/mod.rs     | 62 ++++++++++++++++++++++++++++++++------
 2 files changed, 61 insertions(+), 9 deletions(-)

diff --git a/src/api2/node/disks/mod.rs b/src/api2/node/disks/mod.rs
index 711dae7b..f63bbd9b 100644
--- a/src/api2/node/disks/mod.rs
+++ b/src/api2/node/disks/mod.rs
@@ -41,6 +41,12 @@ pub mod zfs;
                 optional: true,
                 default: false,
             },
+            "exclude-used": {
+                description: "Exclude partitions already used for removable datastores or mounted directories.",
+                type: bool,
+                optional: true,
+                default: false,
+            },
             "usage-type": {
                 type: DiskUsageType,
                 optional: true,
@@ -62,6 +68,7 @@ pub mod zfs;
 pub fn list_disks(
     skipsmart: bool,
     include_partitions: bool,
+    exclude_used: bool,
     usage_type: Option<DiskUsageType>,
 ) -> Result<Vec<DiskUsageInfo>, Error> {
     let mut list = Vec::new();
@@ -69,6 +76,7 @@ pub fn list_disks(
     for (_, info) in DiskUsageQuery::new()
         .smart(!skipsmart)
         .partitions(include_partitions)
+        .exclude_used(exclude_used)
         .query()?
     {
         if let Some(ref usage_type) = usage_type {
diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index eaf016df..d27f5086 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -20,6 +20,7 @@ use proxmox_rest_server::WorkerTask;
 use proxmox_schema::api;
 use proxmox_sys::linux::procfs::{mountinfo::Device, MountInfo};
 use proxmox_sys::task_log;
+use serde_json::Value;
 
 use pbs_api_types::{BLOCKDEVICE_DISK_AND_PARTITION_NAME_REGEX, BLOCKDEVICE_NAME_REGEX};
 
@@ -32,6 +33,7 @@ pub use zpool_list::*;
 mod lvm;
 pub use lvm::*;
 mod smart;
+use crate::api2::node::disks::directory::list_datastore_mounts;
 pub use smart::*;
 
 lazy_static::lazy_static! {
@@ -828,6 +830,7 @@ fn scan_partitions(
 pub struct DiskUsageQuery {
     smart: bool,
     partitions: bool,
+    exclude_used: bool,
 }
 
 impl DiskUsageQuery {
@@ -835,6 +838,7 @@ impl DiskUsageQuery {
         Self {
             smart: true,
             partitions: false,
+            exclude_used: false,
         }
     }
 
@@ -848,12 +852,22 @@ impl DiskUsageQuery {
         self
     }
 
+    pub fn exclude_used(&mut self, exclude_used: bool) -> &mut Self {
+        self.exclude_used = exclude_used;
+        self
+    }
+
     pub fn query(&self) -> Result<HashMap<String, DiskUsageInfo>, Error> {
-        get_disks(None, !self.smart, self.partitions)
+        get_disks(None, !self.smart, self.partitions, self.exclude_used)
     }
 
     pub fn find(&self, disk: &str) -> Result<DiskUsageInfo, Error> {
-        let mut map = get_disks(Some(vec![disk.to_string()]), !self.smart, self.partitions)?;
+        let mut map = get_disks(
+            Some(vec![disk.to_string()]),
+            !self.smart,
+            self.partitions,
+            self.exclude_used,
+        )?;
         if let Some(info) = map.remove(disk) {
             Ok(info)
         } else {
@@ -862,7 +876,7 @@ impl DiskUsageQuery {
     }
 
     pub fn find_all(&self, disks: Vec<String>) -> Result<HashMap<String, DiskUsageInfo>, Error> {
-        get_disks(Some(disks), !self.smart, self.partitions)
+        get_disks(Some(disks), !self.smart, self.partitions, self.exclude_used)
     }
 }
 
@@ -935,6 +949,8 @@ fn get_disks(
     no_smart: bool,
     // include partitions
     include_partitions: bool,
+    // skip partitions which are in use
+    exclude_used: bool,
 ) -> Result<HashMap<String, DiskUsageInfo>, Error> {
     let disk_manager = DiskManage::new();
 
@@ -952,6 +968,30 @@ fn get_disks(
 
     // fixme: ceph journals/volumes
 
+    let uuids_in_use = if exclude_used && include_partitions {
+        let (config, _digest) = pbs_config::datastore::config()?;
+
+        let uuids_from_datastores: Vec<String> = config
+            .sections
+            .iter()
+            .filter_map(|(_, (_, data))| {
+                data.as_object()
+                    .and_then(|cfg| cfg.get("backing-device"))
+                    .and_then(Value::as_str)
+                    .map(String::from)
+            })
+            .collect();
+
+        let uuids_from_mounts: Vec<String> = list_datastore_mounts()?
+            .into_iter()
+            .filter_map(|mount| mount.device.split('/').last().map(String::from))
+            .collect();
+
+        [&uuids_from_datastores[..], &uuids_from_mounts[..]].concat()
+    } else {
+        Vec::new()
+    };
+
     let mut result = HashMap::new();
 
     for item in proxmox_sys::fs::scan_subdir(libc::AT_FDCWD, "/sys/block", &BLOCKDEVICE_NAME_REGEX)?
@@ -1024,12 +1064,16 @@ fn get_disks(
 
         let partitions: Option<Vec<PartitionInfo>> = if include_partitions {
             disk.partitions().map_or(None, |parts| {
-                Some(get_partitions_info(
-                    parts,
-                    &lvm_devices,
-                    &zfs_devices,
-                    &file_system_devices,
-                ))
+                let infos =
+                    get_partitions_info(parts, &lvm_devices, &zfs_devices, &file_system_devices)
+                        .into_iter()
+                        .filter(|part| {
+                            part.uuid
+                                .as_ref()
+                                .map_or(true, |u| !uuids_in_use.contains(u))
+                        })
+                        .collect();
+                Some(infos)
             })
         } else {
             None
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 09/22] pbs-api-types: datastore: use new proxmox_schema::de for deserialization

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 898149ea..b5b9784f 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -348,10 +348,13 @@ impl DataStoreConfig {
     }
 
     pub fn get_maintenance_mode(&self) -> Option<MaintenanceMode> {
-        self.maintenance_mode
-            .as_ref()
-            .and_then(|str| MaintenanceMode::API_SCHEMA.parse_property_string(str).ok())
-            .and_then(|value| MaintenanceMode::deserialize(value).ok())
+        self.maintenance_mode.as_ref().and_then(|str| {
+            MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
+                str,
+                &MaintenanceMode::API_SCHEMA,
+            ))
+            .ok()
+        })
     }
 }
 
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 10/22] pbs-api-types: add removable/is-available flag to DataStoreListItem

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/datastore.rs |  9 ++++++++-
 src/api2/admin/datastore.rs    | 17 +++++++++--------
 src/api2/status.rs             | 18 +++++++++++++++---
 3 files changed, 32 insertions(+), 12 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index b5b9784f..73521539 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -380,6 +380,10 @@ impl DataStoreConfig {
 pub struct DataStoreListItem {
     pub store: String,
     pub comment: Option<String>,
+    /// Datastore is removable
+    pub removable: bool,
+    /// Datastore is available
+    pub available: bool,
     /// If the datastore is in maintenance mode, information about it
     #[serde(skip_serializing_if = "Option::is_none")]
     pub maintenance: Option<String>,
@@ -1347,6 +1351,8 @@ pub struct DataStoreStatusListItem {
     /// The available bytes of the underlying storage. (-1 on error)
     #[serde(skip_serializing_if = "Option::is_none")]
     pub avail: Option<u64>,
+    /// The datastore is available, relevant if removable
+    pub is_available: bool,
     /// A list of usages of the past (last Month).
     #[serde(skip_serializing_if = "Option::is_none")]
     pub history: Option<Vec<Option<f64>>>,
@@ -1371,12 +1377,13 @@ pub struct DataStoreStatusListItem {
 }
 
 impl DataStoreStatusListItem {
-    pub fn empty(store: &str, err: Option<String>) -> Self {
+    pub fn empty(store: &str, err: Option<String>, is_available: bool) -> Self {
         DataStoreStatusListItem {
             store: store.to_owned(),
             total: None,
             used: None,
             avail: None,
+            is_available,
             history: None,
             history_start: None,
             history_delta: None,
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 57167a82..46cb75ff 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1261,8 +1261,8 @@ pub fn get_datastore_list(
 
     let mut list = Vec::new();
 
-    for (store, (_, data)) in &config.sections {
-        let acl_path = &["datastore", store];
+    for (store, (_, data)) in config.sections {
+        let acl_path = &["datastore", &store];
         let user_privs = user_info.lookup_privs(&auth_id, acl_path);
         let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
 
@@ -1273,15 +1273,16 @@ pub fn get_datastore_list(
             }
         }
 
+        let store_config: DataStoreConfig = serde_json::from_value(data)?;
+        let is_available = pbs_datastore::is_datastore_available(&store_config);
+
         if allowed || allow_id {
             list.push(DataStoreListItem {
                 store: store.clone(),
-                comment: if !allowed {
-                    None
-                } else {
-                    data["comment"].as_str().map(String::from)
-                },
-                maintenance: data["maintenance-mode"].as_str().map(String::from),
+                comment: if !allowed { None } else { store_config.comment },
+                removable: store_config.backing_device.is_some(),
+                available: is_available,
+                maintenance: store_config.maintenance_mode,
             });
         }
     }
diff --git a/src/api2/status.rs b/src/api2/status.rs
index 78bc06b5..e1d33ccf 100644
--- a/src/api2/status.rs
+++ b/src/api2/status.rs
@@ -13,7 +13,7 @@ use pbs_api_types::{
 };
 
 use pbs_config::CachedUserInfo;
-use pbs_datastore::DataStore;
+use pbs_datastore::{is_datastore_available, DataStore};
 
 use crate::rrd_cache::extract_rrd_data;
 use crate::tools::statistics::linear_regression;
@@ -48,10 +48,17 @@ pub async fn datastore_status(
     for (store, (_, _)) in &config.sections {
         let user_privs = user_info.lookup_privs(&auth_id, &["datastore", store]);
         let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
+
+        let store_config = config.lookup("datastore", store)?;
+        if !is_datastore_available(&store_config) {
+            list.push(DataStoreStatusListItem::empty(store, None, false));
+            continue;
+        }
+
         if !allowed {
             if let Ok(datastore) = DataStore::lookup_datastore(store, Some(Operation::Lookup)) {
                 if can_access_any_namespace(datastore, &auth_id, &user_info) {
-                    list.push(DataStoreStatusListItem::empty(store, None));
+                    list.push(DataStoreStatusListItem::empty(store, None, true));
                 }
             }
             continue;
@@ -60,7 +67,11 @@ pub async fn datastore_status(
         let datastore = match DataStore::lookup_datastore(store, Some(Operation::Read)) {
             Ok(datastore) => datastore,
             Err(err) => {
-                list.push(DataStoreStatusListItem::empty(store, Some(err.to_string())));
+                list.push(DataStoreStatusListItem::empty(
+                    store,
+                    Some(err.to_string()),
+                    true,
+                ));
                 continue;
             }
         };
@@ -71,6 +82,7 @@ pub async fn datastore_status(
             total: Some(status.total),
             used: Some(status.used),
             avail: Some(status.available),
+            is_available: true,
             history: None,
             history_start: None,
             history_delta: None,
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 11/22] pb-manager: add (un)mount command

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-config/src/datastore.rs                 | 14 ++++
 src/bin/proxmox_backup_manager/datastore.rs | 76 ++++++++++++++++++++-
 2 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/pbs-config/src/datastore.rs b/pbs-config/src/datastore.rs
index 5844a174..a540788f 100644
--- a/pbs-config/src/datastore.rs
+++ b/pbs-config/src/datastore.rs
@@ -63,6 +63,20 @@ pub fn complete_datastore_name(_arg: &str, _param: &HashMap<String, String>) ->
     }
 }
 
+pub fn complete_removable_datastore_name(
+    _arg: &str,
+    _param: &HashMap<String, String>,
+) -> Vec<String> {
+    match config() {
+        Ok((data, _digest)) => data
+            .sections
+            .into_iter()
+            .filter_map(|(name, (_, c))| c.get("backing-device").map(|_| name))
+            .collect(),
+        Err(_) => Vec::new(),
+    }
+}
+
 pub fn complete_acl_path(_arg: &str, _param: &HashMap<String, String>) -> Vec<String> {
     let mut list = vec![
         String::from("/"),
diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 383bcd24..f8462459 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -1,4 +1,4 @@
-use anyhow::Error;
+use anyhow::{format_err, Error};
 use serde_json::Value;
 
 use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
@@ -40,6 +40,34 @@ fn list_datastores(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
     Ok(Value::Null)
 }
 
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+            digest: {
+                optional: true,
+                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
+            },
+        },
+    },
+)]
+/// Mount a removable a datastore configuration.
+async fn mount_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
+    param["node"] = "localhost".into();
+
+    let info = &api2::admin::datastore::API_METHOD_MOUNT;
+    let result = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(param, info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
+    Ok(())
+}
+
 #[api(
     input: {
         properties: {
@@ -99,6 +127,34 @@ async fn create_datastore(mut param: Value) -> Result<Value, Error> {
     Ok(Value::Null)
 }
 
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+            digest: {
+                optional: true,
+                schema: PROXMOX_CONFIG_DIGEST_SCHEMA,
+            },
+        },
+    },
+)]
+/// Unmunt a removable a datastore configuration.
+async fn unmount_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<(), Error> {
+    param["node"] = "localhost".into();
+
+    let info = &api2::admin::datastore::API_METHOD_UNMOUNT;
+    let result = match info.handler {
+        ApiHandler::Async(handler) => (handler)(param, info, rpcenv).await?,
+        _ => unreachable!(),
+    };
+
+    crate::wait_for_local_worker(result.as_str().unwrap()).await?;
+    Ok(())
+}
+
 #[api(
     protected: true,
     input: {
@@ -142,6 +198,15 @@ async fn delete_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) ->
 pub fn datastore_commands() -> CommandLineInterface {
     let cmd_def = CliCommandMap::new()
         .insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORES))
+        .insert(
+            "mount",
+            CliCommand::new(&API_METHOD_MOUNT_DATASTORE)
+                .arg_param(&["store"])
+                .completion_cb(
+                    "store",
+                    pbs_config::datastore::complete_removable_datastore_name,
+                ),
+        )
         .insert(
             "show",
             CliCommand::new(&API_METHOD_SHOW_DATASTORE)
@@ -152,6 +217,15 @@ pub fn datastore_commands() -> CommandLineInterface {
             "create",
             CliCommand::new(&API_METHOD_CREATE_DATASTORE).arg_param(&["name", "path"]),
         )
+        .insert(
+            "unmount",
+            CliCommand::new(&API_METHOD_UNMOUNT_DATASTORE)
+                .arg_param(&["store"])
+                .completion_cb(
+                    "store",
+                    pbs_config::datastore::complete_removable_datastore_name,
+                ),
+        )
         .insert(
             "update",
             CliCommand::new(&api2::config::datastore::API_METHOD_UPDATE_DATASTORE)
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 12/22] add auto-mounting for removable datastores

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 debian/proxmox-backup-server.install        |  1 +
 debian/proxmox-backup-server.udev           |  3 ++
 etc/Makefile                                |  3 +-
 etc/removable-device-attach@.service.in     |  8 +++
 src/bin/proxmox_backup_manager/datastore.rs | 54 +++++++++++++++++++++
 5 files changed, 68 insertions(+), 1 deletion(-)
 create mode 100644 etc/removable-device-attach@.service.in

diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
index ee114ea3..cda01069 100644
--- a/debian/proxmox-backup-server.install
+++ b/debian/proxmox-backup-server.install
@@ -4,6 +4,7 @@ etc/proxmox-backup-daily-update.service /lib/systemd/system/
 etc/proxmox-backup-daily-update.timer /lib/systemd/system/
 etc/proxmox-backup-proxy.service /lib/systemd/system/
 etc/proxmox-backup.service /lib/systemd/system/
+etc/removable-device-attach@.service /lib/systemd/system/
 usr/bin/pmt
 usr/bin/pmtx
 usr/bin/proxmox-tape
diff --git a/debian/proxmox-backup-server.udev b/debian/proxmox-backup-server.udev
index afdfb2bc..e21b8bc7 100644
--- a/debian/proxmox-backup-server.udev
+++ b/debian/proxmox-backup-server.udev
@@ -16,3 +16,6 @@ SUBSYSTEM=="scsi_generic", SUBSYSTEMS=="scsi", ATTRS{type}=="1", ENV{ID_SCSI_SER
   SYMLINK+="tape/by-id/scsi-$env{ID_SCSI_SERIAL}-sg"
 
 LABEL="persistent_storage_tape_end"
+
+# triggers the mounting of a removable device
+ACTION=="add", SUBSYSTEM=="block", ENV{ID_FS_UUID}!="", TAG+="systemd", ENV{SYSTEMD_WANTS}="removable-device-attach@$env{ID_FS_UUID}"
\ No newline at end of file
diff --git a/etc/Makefile b/etc/Makefile
index 42f639f6..730de4f8 100644
--- a/etc/Makefile
+++ b/etc/Makefile
@@ -7,7 +7,8 @@ DYNAMIC_UNITS := \
 	proxmox-backup-banner.service \
 	proxmox-backup-daily-update.service \
 	proxmox-backup.service \
-	proxmox-backup-proxy.service
+	proxmox-backup-proxy.service \
+	removable-device-attach@.service
 
 all: $(UNITS) $(DYNAMIC_UNITS) pbs-enterprise.list
 
diff --git a/etc/removable-device-attach@.service.in b/etc/removable-device-attach@.service.in
new file mode 100644
index 00000000..e10d1ea3
--- /dev/null
+++ b/etc/removable-device-attach@.service.in
@@ -0,0 +1,8 @@
+[Unit]
+Description=Try to mount the removable device of a datastore with uuid '%i'.
+After=proxmox-backup-proxy.service
+Requires=proxmox-backup-proxy.service
+
+[Service]
+Type=simple
+ExecStart=/usr/sbin/proxmox-backup-manager datastore uuid-mount %i
diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index f8462459..7b727230 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -195,6 +195,56 @@ async fn delete_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) ->
     Ok(())
 }
 
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            uuid: {
+                type: String,
+                format: &proxmox_schema::api_types::UUID_FORMAT,
+                description: "The UUID of the device that should be mounted",
+            },
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+        },
+    },
+)]
+/// Try mounting a removable datastore given the UUID.
+async fn uuid_mount(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let uuid = param["uuid"]
+        .as_str()
+        .ok_or_else(|| format_err!("uuid has to be specified"))?;
+
+    let info = &api2::config::datastore::API_METHOD_LIST_DATASTORES;
+    let data: Value = match info.handler {
+        ApiHandler::Sync(handler) => (handler)(serde_json::json!({}), info, rpcenv)?,
+        _ => unreachable!(),
+    };
+
+    let store_name = data.as_array().and_then(|list| {
+        list.iter()
+            .filter_map(Value::as_object)
+            .find(|store| store.get("backing-device").map_or(false, |d| d.eq(&uuid)))
+            .and_then(|s| s.get("name").and_then(Value::as_str))
+    });
+
+    if let Some(store_name) = store_name {
+        let info = &api2::admin::datastore::API_METHOD_MOUNT;
+        let mount_param = serde_json::json!({
+            "store": store_name,
+        });
+        let result = match info.handler {
+            ApiHandler::Sync(handler) => (handler)(mount_param, info, rpcenv)?,
+            _ => unreachable!(),
+        };
+        crate::wait_for_local_worker(result.as_str().unwrap()).await?;
+        return Ok(Value::Null);
+    }
+    anyhow::bail!("'{}' is not associated with any datastore", uuid)
+}
+
 pub fn datastore_commands() -> CommandLineInterface {
     let cmd_def = CliCommandMap::new()
         .insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORES))
@@ -240,6 +290,10 @@ pub fn datastore_commands() -> CommandLineInterface {
                     pbs_config::datastore::complete_calendar_event,
                 ),
         )
+        .insert(
+            "uuid-mount",
+            CliCommand::new(&API_METHOD_UUID_MOUNT).arg_param(&["uuid"]),
+        )
         .insert(
             "remove",
             CliCommand::new(&API_METHOD_DELETE_DATASTORE)
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 13/22] datastore: handle deletion of removable datastore properly

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 10 ++++++----
 src/api2/config/datastore.rs   | 14 ++++++++++++++
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index ff2ca95f..70324a01 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1504,10 +1504,12 @@ impl DataStore {
                         // weird, but ok
                     }
                     Err(err) if err.is_errno(nix::errno::Errno::EBUSY) => {
-                        task_warn!(
-                            worker,
-                            "Cannot delete datastore directory (is it a mount point?)."
-                        )
+                        if datastore_config.backing_device.is_none() {
+                            task_warn!(
+                                worker,
+                                "Cannot delete datastore directory (is it a mount point?)."
+                            )
+                        }
                     }
                     Err(err) if err.is_errno(nix::errno::Errno::ENOTEMPTY) => {
                         task_warn!(worker, "Datastore directory not empty, not deleting.")
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 3a075674..5729071d 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -29,9 +29,11 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
 use crate::api2::config::verify::delete_verification_job;
 use pbs_config::CachedUserInfo;
 
+use pbs_datastore::is_datastore_available;
 use proxmox_rest_server::WorkerTask;
 
 use crate::server::jobstate;
+use crate::tools::disks::unmount_by_mountpoint;
 
 #[api(
     input: {
@@ -531,6 +533,14 @@ pub async fn delete_datastore(
         http_bail!(NOT_FOUND, "datastore '{}' does not exist.", name);
     }
 
+    let store_config: DataStoreConfig = config.lookup("datastore", &name)?;
+    if destroy_data && is_datastore_available(&store_config) {
+        http_bail!(
+            BAD_REQUEST,
+            "can't destroy data on '{name}' unless the datastore is mounted"
+        );
+    }
+
     if !keep_job_configs {
         for job in list_verification_jobs(Some(name.clone()), Value::Null, rpcenv)? {
             delete_verification_job(job.config.id, None, rpcenv)?
@@ -569,6 +579,10 @@ pub async fn delete_datastore(
             // ignore errors
             let _ = jobstate::remove_state_file("prune", &name);
             let _ = jobstate::remove_state_file("garbage_collection", &name);
+            if destroy_data && store_config.backing_device.is_some() {
+                let _ = unmount_by_mountpoint(&store_config.path);
+                let _ = std::fs::remove_dir(&store_config.path);
+            }
 
             if let Err(err) =
                 proxmox_async::runtime::block_on(crate::server::notify_datastore_removed())
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 14/22] docs: mention maintenance mode reset when removable datastore is unplugged

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 docs/maintenance.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/docs/maintenance.rst b/docs/maintenance.rst
index 6dbb6941..9f18cfdb 100644
--- a/docs/maintenance.rst
+++ b/docs/maintenance.rst
@@ -314,3 +314,5 @@ Internally Proxmox Backup Server tracks whether each datastore access is a
 write or read operation, so that it can gracefully enter the respective mode,
 by allowing conflicting operations that started before enabling the maintenance
 mode to finish.
+
+For removable datastores the maintenance mode is reset after they are unplugged.
\ No newline at end of file
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 15/22] ui: add partition selector form

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/Makefile                  |  1 +
 www/form/PartitionSelector.js | 74 +++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)
 create mode 100644 www/form/PartitionSelector.js

diff --git a/www/Makefile b/www/Makefile
index 79cb4c04..762d90c5 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -49,6 +49,7 @@ JSSRC=							\
 	form/NamespaceMaxDepth.js			\
 	form/CalendarEvent.js				\
 	form/PermissionPathSelector.js			\
+	form/PartitionSelector.js			\
 	form/GroupSelector.js				\
 	form/GroupFilter.js				\
 	form/VerifyOutdatedAfter.js			\
diff --git a/www/form/PartitionSelector.js b/www/form/PartitionSelector.js
new file mode 100644
index 00000000..604975a6
--- /dev/null
+++ b/www/form/PartitionSelector.js
@@ -0,0 +1,74 @@
+Ext.define('pbs-partition-list', {
+    extend: 'Ext.data.Model',
+    fields: ['name', 'uuid', 'filesystem', 'devpath', 'size'],
+    proxy: {
+	type: 'proxmox',
+	url: "/api2/json/nodes/localhost/disks/list?skipsmart=1&include-partitions=1&exclude-used=1",
+	reader: {
+	    transform: (rawData) => rawData.data
+		.flatMap(disk => (disk.partitions ?? [])
+		    .filter(partition => partition.used === 'filesystem')),
+	},
+    },
+    idProperty: 'devpath',
+
+});
+
+Ext.define('PBS.form.PartitionSelector', {
+    extend: 'Proxmox.form.ComboGrid',
+    alias: 'widget.pbsPartitionSelector',
+
+    allowBlank: false,
+    autoSelect: false,
+    submitEmpty: false,
+    valueField: 'uuid',
+    displayField: 'devpath',
+
+    store: {
+	model: 'pbs-partition-list',
+	autoLoad: true,
+	sorters: 'devpath',
+    },
+    getSubmitData: function() {
+	let me = this;
+	let data = null;
+	if (!me.disabled && me.submitValue && !me.isFileUpload()) {
+	    let val = me.getSubmitValue();
+	    if (val !== undefined && val !== null && val !== '') {
+		data = {};
+		data[me.getName()] = val;
+	    } else if (me.getDeleteEmpty()) {
+		data = {};
+		data.delete = me.getName();
+	    }
+	}
+	return data;
+    },
+    listConfig: {
+	columns: [
+	    {
+		header: gettext('Path'),
+		sortable: true,
+		dataIndex: 'devpath',
+		renderer: (v, metaData, rec) => Ext.String.htmlEncode(v),
+		flex: 1,
+	    },
+	    {
+		header: gettext('Filesystem'),
+		sortable: true,
+		dataIndex: 'filesystem',
+		flex: 1,
+	    },
+	    {
+		header: gettext('Size'),
+		sortable: true,
+		dataIndex: 'size',
+		renderer: Proxmox.Utils.format_size,
+		flex: 1,
+	    },
+	],
+	viewConfig: {
+	    emptyText: 'No partitions present',
+	},
+    },
+});
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 16/22] ui: add removable datastore creation support

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/window/DataStoreEdit.js | 51 +++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/www/window/DataStoreEdit.js b/www/window/DataStoreEdit.js
index aecf6b8d..9114cf4f 100644
--- a/www/window/DataStoreEdit.js
+++ b/www/window/DataStoreEdit.js
@@ -59,6 +59,31 @@ Ext.define('PBS.DataStoreEdit', {
 			fieldLabel: gettext('Backing Path'),
 			emptyText: gettext('An absolute path'),
 		    },
+		    {
+			xtype: 'pmxDisplayEditField',
+			fieldLabel: gettext('Device'),
+			name: 'backing-device',
+			hidden: true,
+			cbind: {
+			    editable: '{isCreate}',
+			}, editConfig: {
+			    xtype: 'pbsPartitionSelector',
+			    allowBlank: true,
+			},
+			emptyText: gettext('Device path'),
+			listeners: {
+			    change(field, newValue, oldValue) {
+				const form = field.up('form');
+				const pathField = form.down('[name=path]');
+
+				if (newValue) {
+				    pathField.setValue(`/mnt/removable_datastore/${newValue}`);
+				} else if (oldValue) {
+				    pathField.setValue('');
+				}
+			    },
+			},
+		    },
 		],
 		column2: [
 		    {
@@ -84,6 +109,32 @@ Ext.define('PBS.DataStoreEdit', {
 		    },
 		],
 		columnB: [
+		    {
+			xtype: 'checkbox',
+			boxLabel: gettext('Removable datastore'),
+			submitValue: false,
+			listeners: {
+			    change: function(checkbox, isRemovable) {
+				let inputPanel = checkbox.up('inputpanel');
+				let pathField = inputPanel.down('[name=path]');
+				let uuidField = inputPanel.down('[name=backing-device]');
+
+				pathField.setValue('');
+				uuidField.setValue('');
+
+				pathField.allowBlank = isRemovable;
+				uuidField.allowBlank = !isRemovable;
+
+				if (isRemovable) {
+				    pathField.hide();
+				    uuidField.show();
+				} else {
+				    pathField.show();
+				    uuidField.hide();
+				}
+			    },
+			},
+		    },
 		    {
 			xtype: 'textfield',
 			name: 'comment',
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 17/22] ui: add (un)mount button to summary

[Hannes Laimer]

And only try to load datastore information if the datastore is
available.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/datastore/Summary.js | 82 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)

diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index a932b4e0..ee8bad16 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -309,7 +309,72 @@ Ext.define('PBS.DataStoreSummary', {
 	    model: 'pve-rrd-datastore',
 	});
 
-	me.callParent();
+	me.statusStore = Ext.create('Proxmox.data.ObjectStore', {
+	    url: `/api2/json/admin/datastore/${me.datastore}/status`,
+	    interval: 1000,
+	});
+
+	let unmountBtn = Ext.create('Ext.Button', {
+	    text: gettext('Unmount'),
+	    hidden: true,
+	    handler: () => {
+		Proxmox.Utils.API2Request({
+		    url: `/admin/datastore/${me.datastore}/unmount`,
+		    method: 'POST',
+		    failure: function(response) {
+			Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+		    },
+		    success: function(response, options) {
+			Ext.create('Proxmox.window.TaskViewer', {
+			    upid: response.result.data,
+			}).show();
+		    },
+		});
+	    },
+	});
+
+	let mountBtn = Ext.create('Ext.Button', {
+	    text: gettext('Mount'),
+	    hidden: true,
+	    handler: () => {
+		Proxmox.Utils.API2Request({
+		    url: `/admin/datastore/${me.datastore}/mount`,
+		    method: 'POST',
+		    failure: function(response) {
+			Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+		    },
+		    success: function(response, options) {
+			Ext.create('Proxmox.window.TaskViewer', {
+			    upid: response.result.data,
+			}).show();
+		    },
+		});
+	    },
+	});
+
+	Ext.apply(me, {
+	    tbar: [unmountBtn, mountBtn, '->', { xtype: 'proxmoxRRDTypeSelector' }],
+	});
+
+	me.mon(me.statusStore, 'load', (s, records, success) => {
+	    if (!success) {
+		me.down('pbsDataStoreInfo').fireEvent('deactivate');
+		Proxmox.Utils.API2Request({
+		    url: `/config/datastore/${me.datastore}`,
+		    success: response => {
+			if (!response.result.data['backing-device']) {
+			    return;
+			}
+			unmountBtn.setDisabled(true);
+			mountBtn.setDisabled(false);
+		    },
+		});
+	    } else {
+		me.down('pbsDataStoreInfo').fireEvent('activate');
+		unmountBtn.setDisabled(false);
+		mountBtn.setDisabled(true);
+	    }
+	});
 
 	let sp = Ext.state.Manager.getProvider();
 	me.mon(sp, 'statechange', function(provider, key, value) {
@@ -322,11 +387,17 @@ Ext.define('PBS.DataStoreSummary', {
 	    Proxmox.Utils.updateColumns(me);
 	});
 
+	me.callParent();
+
 	Proxmox.Utils.API2Request({
 	    url: `/config/datastore/${me.datastore}`,
 	    waitMsgTarget: me.down('pbsDataStoreInfo'),
 	    success: function(response) {
-		let path = Ext.htmlEncode(response.result.data.path);
+		let data = response.result.data;
+		let path = Ext.htmlEncode(data.path);
+		const removable = Object.prototype.hasOwnProperty.call(data, "backing-device");
+		unmountBtn.setHidden(!removable);
+		mountBtn.setHidden(!removable);
 		me.down('pbsDataStoreInfo').setTitle(`${me.datastore} (${path})`);
 		me.down('pbsDataStoreNotes').setNotes(response.result.data.comment);
 	    },
@@ -344,6 +415,13 @@ Ext.define('PBS.DataStoreSummary', {
 	    let hasIoTicks = records?.some((rec) => rec?.data?.io_ticks !== undefined);
 	    me.down('#ioDelayChart').setVisible(!success || hasIoTicks);
 	}, undefined, { single: true });
+	me.on('afterrender', () => {
+	    me.statusStore.startUpdate();
+	});
+
+	me.on('destroy', () => {
+	    me.statusStore.stopUpdate();
+	});
 
 	me.query('proxmoxRRDChart').forEach((chart) => {
 	    chart.setStore(me.rrdstore);
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 18/22] ui: display removable datastores in list

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/NavigationTree.js                 | 10 +++++++---
 www/css/ext6-pbs.css                  |  8 ++++++++
 www/datastore/DataStoreListSummary.js |  1 +
 3 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/www/NavigationTree.js b/www/NavigationTree.js
index 3e0040f7..9ce93799 100644
--- a/www/NavigationTree.js
+++ b/www/NavigationTree.js
@@ -261,13 +261,17 @@ Ext.define('PBS.view.main.NavigationTree', {
 		    j++;
 		}
 
-		let [qtip, iconCls] = ['', 'fa fa-database'];
+		let mainIcon = `fa fa-${records[i].data.removable ? 'plug' : 'database'}`;
+		let [qtip, iconCls] = ['', mainIcon];
 		const maintenance = records[i].data.maintenance;
 		if (maintenance) {
 		    const [type, message] = PBS.Utils.parseMaintenanceMode(maintenance);
 		    qtip = `${type}${message ? ': ' + message : ''}`;
-		    let mainenanceTypeCls = type === 'delete' ? 'destroying' : 'maintenance';
-		    iconCls = `fa fa-database pmx-tree-icon-custom ${mainenanceTypeCls}`;
+		    const maintenanceTypeCls = {
+			'delete': 'destroying',
+			'unplugged': 'unplugged',
+		    }[type] ?? 'maintenance';
+		    iconCls = `${mainIcon} pmx-tree-icon-custom ${maintenanceTypeCls}`;
 		}
 
 		if (getChildTextAt(j).localeCompare(name) !== 0) {
diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
index c33ce684..706e681e 100644
--- a/www/css/ext6-pbs.css
+++ b/www/css/ext6-pbs.css
@@ -271,6 +271,10 @@ span.snapshot-comment-column {
     content: "\ ";
 }
 
+.x-treelist-item-icon.fa-plug, .pmx-tree-icon-custom.fa-plug {
+    font-size: 12px;
+}
+
 /* datastore maintenance */
 .pmx-tree-icon-custom.maintenance:after {
     content: "\f0ad";
@@ -290,6 +294,10 @@ span.snapshot-comment-column {
     color: #888;
 }
 
+.pmx-tree-icon-custom.unplugged:before {
+    color: #888;
+}
+
 /*' PBS specific icons */
 
 .pbs-icon-tape {
diff --git a/www/datastore/DataStoreListSummary.js b/www/datastore/DataStoreListSummary.js
index b908034d..f7ea83e7 100644
--- a/www/datastore/DataStoreListSummary.js
+++ b/www/datastore/DataStoreListSummary.js
@@ -22,6 +22,7 @@ Ext.define('PBS.datastore.DataStoreListSummary', {
 	    stillbad: 0,
 	    deduplication: 1.0,
 	    error: "",
+	    removable: false,
 	    maintenance: '',
 	},
     },
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 19/22] ui: utils: render unplugged datastores correctly

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/NavigationTree.js | 13 +++++++++----
 www/Utils.js          |  2 ++
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/www/NavigationTree.js b/www/NavigationTree.js
index 9ce93799..dc4a3dc7 100644
--- a/www/NavigationTree.js
+++ b/www/NavigationTree.js
@@ -264,13 +264,18 @@ Ext.define('PBS.view.main.NavigationTree', {
 		let mainIcon = `fa fa-${records[i].data.removable ? 'plug' : 'database'}`;
 		let [qtip, iconCls] = ['', mainIcon];
 		const maintenance = records[i].data.maintenance;
+		const removable_not_available = records[i].data.removable && !records[i].data.available;
+		if (removable_not_available) {
+		    iconCls = `${mainIcon} pmx-tree-icon-custom unplugged`;
+		    qtip = gettext('Removable datastore not mounted');
+		}
 		if (maintenance) {
 		    const [type, message] = PBS.Utils.parseMaintenanceMode(maintenance);
 		    qtip = `${type}${message ? ': ' + message : ''}`;
-		    const maintenanceTypeCls = {
-			'delete': 'destroying',
-			'unplugged': 'unplugged',
-		    }[type] ?? 'maintenance';
+		    let maintenanceTypeCls = 'maintenance';
+		    if (type === 'delete') {
+			maintenanceTypeCls = 'destroying';
+		    }
 		    iconCls = `${mainIcon} pmx-tree-icon-custom ${maintenanceTypeCls}`;
 		}
 
diff --git a/www/Utils.js b/www/Utils.js
index 5357949b..0ee6609c 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -703,6 +703,8 @@ Ext.define('PBS.Utils', {
 		break;
 	    case 'offline': modeText = gettext("Offline");
 		break;
+	    case 'unplugged': modeText = gettext("Unplugged");
+		break;
 	}
 	return `${modeText} ${extra}`;
     },
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 20/22] ui: utils: make parseMaintenanceMode more robust

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/Utils.js | 29 ++++++++++++++++++++++-------
 1 file changed, 22 insertions(+), 7 deletions(-)

diff --git a/www/Utils.js b/www/Utils.js
index 0ee6609c..1df46318 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -663,14 +663,29 @@ Ext.define('PBS.Utils', {
 	return `${icon} ${value}`;
     },
 
-    // FIXME: this "parser" is brittle and relies on the order the arguments will appear in
+    /**
+     * Parses maintenance mode property string.
+     * Examples:
+     *   "offline,message=foo" -> ["offline", "foo"]
+     *   "offline" -> ["offline", null]
+     *   "message=foo,offline" -> ["offline", "foo"]
+     *   null/undefined -> [null, null]
+     *
+     * @param {string|null} mode - Maintenance mode string to parse.
+     * @return {Array<string|null>} - Parsed maintenance mode values.
+     */
     parseMaintenanceMode: function(mode) {
-	let [type, message] = mode.split(/,(.+)/);
-	type = type.split("=").pop();
-	message = message ? message.split("=")[1]
-	    .replace(/^"(.*)"$/, '$1')
-	    .replaceAll('\\"', '"') : null;
-	return [type, message];
+	if (!mode) {
+	    return [null, null];
+	}
+	return mode.split(',').reduce(([m, msg], pair) => {
+	    const [key, value] = pair.split('=');
+	    if (key === 'message') {
+		return [m, value.replace(/^"(.*)"$/, '$1').replace(/\\"/g, '"')];
+	    } else {
+		return [value ?? key, msg];
+	    }
+	}, [null, null]);
     },
 
     renderMaintenance: function(mode, activeTasks) {
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 21/22] ui: add datastore status mask for unplugged removable datastores

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/css/ext6-pbs.css     | 12 ++++++++++++
 www/datastore/Summary.js | 21 +++++++++++++--------
 2 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
index 706e681e..891189ae 100644
--- a/www/css/ext6-pbs.css
+++ b/www/css/ext6-pbs.css
@@ -261,6 +261,18 @@ span.snapshot-comment-column {
     content: "\f0ad";
 }
 
+.pbs-unplugged-mask div.x-mask-msg-text {
+    background: None;
+    padding: 12px 0 0;
+}
+
+.pbs-unplugged-mask:before {
+    font-size: 3em;
+    display: flex;
+    justify-content: center;
+    content: "\f1e6";
+}
+
 /* the small icons TODO move to proxmox-widget-toolkit */
 .pmx-tree-icon-custom:after {
     position: relative;
diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index ee8bad16..d1c515b0 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -61,16 +61,21 @@ Ext.define('PBS.DataStoreInfo', {
 		Proxmox.Utils.API2Request({
 		    url: `/config/datastore/${me.view.datastore}`,
 		    success: function(response) {
-			const config = response.result.data;
-			if (config['maintenance-mode']) {
-			    const [_type, msg] = PBS.Utils.parseMaintenanceMode(config['maintenance-mode']);
-			    me.view.el.mask(
-				`${gettext('Datastore is in maintenance mode')}${msg ? ': ' + msg : ''}`,
-				'fa pbs-maintenance-mask',
-			    );
-			} else {
+			let maintenanceString = response.result.data['maintenance-mode'];
+			let removable = !!response.result.data['backing-device'];
+			if (!maintenanceString && !removable) {
 			    me.view.el.mask(gettext('Datastore is not available'));
+			    return;
 			}
+
+			let [_type, msg] = PBS.Utils.parseMaintenanceMode(maintenanceString);
+			let isUnplugged = !maintenanceString && removable;
+			let maskMessage = isUnplugged
+			    ? gettext('Datastore is not mounted')
+			    : `${gettext('Datastore is in maintenance mode')}${msg ? ': ' + msg : ''}`;
+
+			let maskIcon = isUnplugged ? 'fa pbs-unplugged-mask' : 'fa pbs-maintenance-mask';
+			me.view.el.mask(maskMessage, maskIcon);
 		    },
 		});
 		return;
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup v4 22/22] ui: maintenance: fix disable msg field if no type is selected

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/window/MaintenanceOptions.js | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
index 1ee92542..527c3698 100644
--- a/www/window/MaintenanceOptions.js
+++ b/www/window/MaintenanceOptions.js
@@ -56,12 +56,17 @@ Ext.define('PBS.window.MaintenanceOptions', {
 		fieldLabel: gettext('Maintenance Type'),
 		value: '__default__',
 		deleteEmpty: true,
+		listeners: {
+		    change: (field, newValue) => {
+			Ext.getCmp('message-field').setDisabled(newValue === '__default__');
+		    },
+		},
 	    },
 	    {
 		xtype: 'proxmoxtextfield',
+		id: 'message-field',
 		name: 'maintenance-msg',
 		fieldLabel: gettext('Description'),
-		// FIXME: disable if maintenance type is none
 	    },
 	],
     },
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup v4 02/22] datastore: make dropping from cache more efficient

[Christian Ebner]

On 4/16/24 17:23, Hannes Laimer wrote:
> ... by not having to read the whole config file on every drop,
> but assume calling `update-datastore-cache` through the scoket implies
> it should be dropped and doing the checking on the sending side.

s/scoket/socket, but this might be amended before applying I guess.

Re: [pbs-devel] [PATCH proxmox-backup v4 05/22] add helper for checking if a removable datastore is available

[Christian Ebner]

two comments inline

On 4/16/24 17:23, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   pbs-datastore/src/datastore.rs | 49 ++++++++++++++++++++++++++++++++++
>   pbs-datastore/src/lib.rs       |  2 +-
>   2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index a7fe3b8c..ff2ca95f 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -14,6 +14,7 @@ use proxmox_schema::ApiType;
>   use proxmox_sys::error::SysError;
>   use proxmox_sys::fs::{file_read_optional_string, replace_file, CreateOptions};
>   use proxmox_sys::fs::{lock_dir_noblock, DirLockGuard};
> +use proxmox_sys::linux::procfs::MountInfo;
>   use proxmox_sys::process_locker::ProcessLockSharedGuard;
>   use proxmox_sys::WorkerTaskContext;
>   use proxmox_sys::{task_log, task_warn};
> @@ -49,6 +50,35 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error>
>       Ok(())
>   }
>   
> +pub fn is_datastore_available(config: &DataStoreConfig) -> bool {
> +    config.backing_device.as_ref().map_or(true, |uuid| {
> +        if let Ok(store_dev_path) = std::fs::read_link(Path::new("/dev/disk/by-uuid").join(uuid))
> +            .map(|p| p.to_string_lossy().into_owned())
> +            .map(|p| p.replace("../..", "/dev"))
> +            .map(PathBuf::from)
> +        {
> +            let store_mount_point = PathBuf::from(&config.path);
> +
> +            MountInfo::read().map_or(false, |mount_info| {
> +                mount_info
> +                    .iter()
> +                    .filter_map(|(_, entry)| {
> +                        entry
> +                            .mount_source
> +                            .as_ref()
> +                            .map(|source| (&entry.mount_point, source))
> +                    })
> +                    .any(|(mount_point, mount_source)| {
> +                        PathBuf::from(&mount_point) == store_mount_point
> +                            && PathBuf::from(&mount_source) == store_dev_path
> +                    })
> +            })
> +        } else {
> +            false
> +        }
> +    })
> +}
> +


Nice work on this one! Way better to look up the device name and getting 
the mountpoint by that via procfs.

>   /// Datastore Management
>   ///
>   /// A Datastore can store severals backups, and provides the
> @@ -149,6 +179,21 @@ impl DataStore {
>               }
>           }
>   
> +        if config.backing_device.is_some() {
> +            let is_mounted = is_datastore_available(&config);
> +            let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
> +            if let Some((_, true)) = datastore_cache.get(&config.name) {
> +                if !is_mounted {
> +                    datastore_cache.remove(&config.name);
> +                }
> +                bail!("Removable Datastore in progress to being removed.");

Should maybe be rephrased to `Removable Datastore in progress of being 
removed.` or `Pending removal of datastore.`

> +            }
> +            if !is_mounted {
> +                datastore_cache.remove(&config.name);
> +                bail!("Removable Datastore is not mounted");
> +            }
> +        }
> +
>           if let Some(operation) = operation {
>               update_active_operations(name, operation, 1)?;
>           }
> @@ -253,6 +298,10 @@ impl DataStore {
>       ) -> Result<Arc<Self>, Error> {
>           let name = config.name.clone();
>   
> +        if !is_datastore_available(&config) {
> +            bail!("Datastore is not available")
> +        }
> +
>           let tuning: DatastoreTuning = serde_json::from_value(
>               DatastoreTuning::API_SCHEMA
>                   .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
> diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
> index 43050162..458f93d9 100644
> --- a/pbs-datastore/src/lib.rs
> +++ b/pbs-datastore/src/lib.rs
> @@ -206,7 +206,7 @@ pub use manifest::BackupManifest;
>   pub use store_progress::StoreProgress;
>   
>   mod datastore;
> -pub use datastore::{check_backup_owner, DataStore};
> +pub use datastore::{check_backup_owner, is_datastore_available, DataStore};
>   
>   mod hierarchy;
>   pub use hierarchy::{

Re: [pbs-devel] [PATCH proxmox-backup v4 06/22] api2: admin: add (un)mount endpoint for removable datastores

[Christian Ebner]

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(-)
> 
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.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 {
>   }
>   
>   impl MaintenanceMode {
> +    pub fn new(ty: MaintenanceType, message: Option<String>) -> 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 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, 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, 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;
> @@ -2278,6 +2278,163 @@ pub async fn set_backup_owner(
>       .await?
>   }
>   
> +pub fn do_mount_device(
> +    datastore: DataStoreConfig,
> +    worker: Option<&dyn WorkerTaskContext>,
> +) -> Result<(), Error> {
> +    if let Some(uuid) = datastore.backing_device.as_ref() {
> +        if pbs_datastore::is_datastore_available(&datastore) {
> +            bail!("device '{uuid}' is already mounted");

this...

> +        }
> +        let mount_point_path = std::path::Path::new(&datastore.path);
> +        if let Some(worker) = worker {
> +            task_log!(worker, "mounting '{uuid}' to '{}'", datastore.path);

... 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}' ...`

> +        }
> +        crate::tools::disks::mount_by_uuid(uuid, mount_point_path)?;
> +
> +        Ok(())
> +    } else {
> +        Err(format_err!(
> +            "Datastore '{}' can't be mounted because it is not removable.",

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) = pbs_config::datastore::config()?;
> +    let datastore: DataStoreConfig = section_config.lookup("datastore", &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 = 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(datastore, Some(&worker)),
> +    )?;
> +
> +    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)?;
> +    let mut counter = 0;
> +    while active_operations.read + active_operations.write > 0 {
> +        if counter == 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",

s/can't/cannot

> +                    active_operations.read,
> +                    active_operations.write
> +                );
> +            }
> +            counter = 5000;
> +        }
> +        counter -= 1;
> +        std::thread::sleep(std::time::Duration::from_millis(1));
> +        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<Value, Error> {
> +    let _lock = pbs_config::datastore::lock_config()?;
> +    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);
> +    }
> +    drop(_lock);
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +
> +    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\":\"{store}\"}}\n"),
> +        )
> +        .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 = &[
>       (
> @@ -2316,6 +2473,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?!
> @@ -2350,6 +2508,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),

Re: [pbs-devel] [PATCH proxmox-backup v4 09/22] pbs-api-types: datastore: use new proxmox_schema::de for deserialization

[Christian Ebner]

Still not to happy about the commit title, but no hurt feelings

Re: [pbs-devel] [PATCH proxmox-backup v4 11/22] pb-manager: add (un)mount command

[Christian Ebner]

As Dietmar already mentioned in the last series, the binary is called 
`proxmox-backup-manager`, which is rather log as prefix, so maybe add
`bin: manager:` instead?

Re: [pbs-devel] [PATCH proxmox-backup v4 12/22] add auto-mounting for removable datastores

[Christian Ebner]

nit inline
On 4/16/24 17:24, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   debian/proxmox-backup-server.install        |  1 +
>   debian/proxmox-backup-server.udev           |  3 ++
>   etc/Makefile                                |  3 +-
>   etc/removable-device-attach@.service.in     |  8 +++
>   src/bin/proxmox_backup_manager/datastore.rs | 54 +++++++++++++++++++++
>   5 files changed, 68 insertions(+), 1 deletion(-)
>   create mode 100644 etc/removable-device-attach@.service.in
> 
> diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install
> index ee114ea3..cda01069 100644
> --- a/debian/proxmox-backup-server.install
> +++ b/debian/proxmox-backup-server.install
> @@ -4,6 +4,7 @@ etc/proxmox-backup-daily-update.service /lib/systemd/system/
>   etc/proxmox-backup-daily-update.timer /lib/systemd/system/
>   etc/proxmox-backup-proxy.service /lib/systemd/system/
>   etc/proxmox-backup.service /lib/systemd/system/
> +etc/removable-device-attach@.service /lib/systemd/system/
>   usr/bin/pmt
>   usr/bin/pmtx
>   usr/bin/proxmox-tape
> diff --git a/debian/proxmox-backup-server.udev b/debian/proxmox-backup-server.udev
> index afdfb2bc..e21b8bc7 100644
> --- a/debian/proxmox-backup-server.udev
> +++ b/debian/proxmox-backup-server.udev
> @@ -16,3 +16,6 @@ SUBSYSTEM=="scsi_generic", SUBSYSTEMS=="scsi", ATTRS{type}=="1", ENV{ID_SCSI_SER
>     SYMLINK+="tape/by-id/scsi-$env{ID_SCSI_SERIAL}-sg"
>   
>   LABEL="persistent_storage_tape_end"
> +
> +# triggers the mounting of a removable device
> +ACTION=="add", SUBSYSTEM=="block", ENV{ID_FS_UUID}!="", TAG+="systemd", ENV{SYSTEMD_WANTS}="removable-device-attach@$env{ID_FS_UUID}"
> \ No newline at end of file
> diff --git a/etc/Makefile b/etc/Makefile
> index 42f639f6..730de4f8 100644
> --- a/etc/Makefile
> +++ b/etc/Makefile
> @@ -7,7 +7,8 @@ DYNAMIC_UNITS := \
>   	proxmox-backup-banner.service \
>   	proxmox-backup-daily-update.service \
>   	proxmox-backup.service \
> -	proxmox-backup-proxy.service
> +	proxmox-backup-proxy.service \
> +	removable-device-attach@.service
>   
>   all: $(UNITS) $(DYNAMIC_UNITS) pbs-enterprise.list
>   
> diff --git a/etc/removable-device-attach@.service.in b/etc/removable-device-attach@.service.in
> new file mode 100644
> index 00000000..e10d1ea3
> --- /dev/null
> +++ b/etc/removable-device-attach@.service.in
> @@ -0,0 +1,8 @@
> +[Unit]
> +Description=Try to mount the removable device of a datastore with uuid '%i'.
> +After=proxmox-backup-proxy.service
> +Requires=proxmox-backup-proxy.service
> +
> +[Service]
> +Type=simple
> +ExecStart=/usr/sbin/proxmox-backup-manager datastore uuid-mount %i
> diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
> index f8462459..7b727230 100644
> --- a/src/bin/proxmox_backup_manager/datastore.rs
> +++ b/src/bin/proxmox_backup_manager/datastore.rs
> @@ -195,6 +195,56 @@ async fn delete_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) ->
>       Ok(())
>   }
>   
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            uuid: {
> +                type: String,
> +                format: &proxmox_schema::api_types::UUID_FORMAT,
> +                description: "The UUID of the device that should be mounted",
> +            },
> +            "output-format": {
> +                schema: OUTPUT_FORMAT,
> +                optional: true,
> +            },
> +        },
> +    },
> +)]
> +/// Try mounting a removable datastore given the UUID.
> +async fn uuid_mount(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> +    let uuid = param["uuid"]
> +        .as_str()
> +        .ok_or_else(|| format_err!("uuid has to be specified"))?;
> +
> +    let info = &api2::config::datastore::API_METHOD_LIST_DATASTORES;
> +    let data: Value = match info.handler {
> +        ApiHandler::Sync(handler) => (handler)(serde_json::json!({}), info, rpcenv)?,
> +        _ => unreachable!(),
> +    };
> +
> +    let store_name = data.as_array().and_then(|list| {
> +        list.iter()
> +            .filter_map(Value::as_object)
> +            .find(|store| store.get("backing-device").map_or(false, |d| d.eq(&uuid)))
> +            .and_then(|s| s.get("name").and_then(Value::as_str))
> +    });
> +
> +    if let Some(store_name) = store_name {
> +        let info = &api2::admin::datastore::API_METHOD_MOUNT;
> +        let mount_param = serde_json::json!({
> +            "store": store_name,
> +        });
> +        let result = match info.handler {
> +            ApiHandler::Sync(handler) => (handler)(mount_param, info, rpcenv)?,
> +            _ => unreachable!(),
> +        };
> +        crate::wait_for_local_worker(result.as_str().unwrap()).await?;
> +        return Ok(Value::Null);
> +    }
> +    anyhow::bail!("'{}' is not associated with any datastore", uuid)

inline `uuid` and add `bail` to the use statements at the beginning of 
the file instead of scoping here.

> +}
> +
>   pub fn datastore_commands() -> CommandLineInterface {
>       let cmd_def = CliCommandMap::new()
>           .insert("list", CliCommand::new(&API_METHOD_LIST_DATASTORES))
> @@ -240,6 +290,10 @@ pub fn datastore_commands() -> CommandLineInterface {
>                       pbs_config::datastore::complete_calendar_event,
>                   ),
>           )
> +        .insert(
> +            "uuid-mount",
> +            CliCommand::new(&API_METHOD_UUID_MOUNT).arg_param(&["uuid"]),

+1 from me, the `arg_param` increases usability for this command, thx!

> +        )
>           .insert(
>               "remove",
>               CliCommand::new(&API_METHOD_DELETE_DATASTORE)

Re: [pbs-devel] [PATCH proxmox-backup v4 13/22] datastore: handle deletion of removable datastore properly

[Christian Ebner]

nit inline

On 4/16/24 17:24, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   pbs-datastore/src/datastore.rs | 10 ++++++----
>   src/api2/config/datastore.rs   | 14 ++++++++++++++
>   2 files changed, 20 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index ff2ca95f..70324a01 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -1504,10 +1504,12 @@ impl DataStore {
>                           // weird, but ok
>                       }
>                       Err(err) if err.is_errno(nix::errno::Errno::EBUSY) => {
> -                        task_warn!(
> -                            worker,
> -                            "Cannot delete datastore directory (is it a mount point?)."
> -                        )
> +                        if datastore_config.backing_device.is_none() {
> +                            task_warn!(
> +                                worker,
> +                                "Cannot delete datastore directory (is it a mount point?)."
> +                            )
> +                        }
>                       }
>                       Err(err) if err.is_errno(nix::errno::Errno::ENOTEMPTY) => {
>                           task_warn!(worker, "Datastore directory not empty, not deleting.")
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 3a075674..5729071d 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -29,9 +29,11 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
>   use crate::api2::config::verify::delete_verification_job;
>   use pbs_config::CachedUserInfo;
>   
> +use pbs_datastore::is_datastore_available;
>   use proxmox_rest_server::WorkerTask;
>   
>   use crate::server::jobstate;
> +use crate::tools::disks::unmount_by_mountpoint;
>   
>   #[api(
>       input: {
> @@ -531,6 +533,14 @@ pub async fn delete_datastore(
>           http_bail!(NOT_FOUND, "datastore '{}' does not exist.", name);
>       }
>   
> +    let store_config: DataStoreConfig = config.lookup("datastore", &name)?;
> +    if destroy_data && is_datastore_available(&store_config) {
> +        http_bail!(
> +            BAD_REQUEST,
> +            "can't destroy data on '{name}' unless the datastore is mounted"

nit: s/can't/cannot

> +        );
> +    }
> +
>       if !keep_job_configs {
>           for job in list_verification_jobs(Some(name.clone()), Value::Null, rpcenv)? {
>               delete_verification_job(job.config.id, None, rpcenv)?
> @@ -569,6 +579,10 @@ pub async fn delete_datastore(
>               // ignore errors
>               let _ = jobstate::remove_state_file("prune", &name);
>               let _ = jobstate::remove_state_file("garbage_collection", &name);
> +            if destroy_data && store_config.backing_device.is_some() {
> +                let _ = unmount_by_mountpoint(&store_config.path);
> +                let _ = std::fs::remove_dir(&store_config.path);
> +            }
>   
>               if let Err(err) =
>                   proxmox_async::runtime::block_on(crate::server::notify_datastore_removed())

Re: [pbs-devel] [PATCH proxmox-backup v4 14/22] docs: mention maintenance mode reset when removable datastore is unplugged

[Hannes Laimer]

ignore this, it's wrong and doesn't apply in v4

On Tue Apr 16, 2024 at 5:24 PM CEST, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  docs/maintenance.rst | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/docs/maintenance.rst b/docs/maintenance.rst
> index 6dbb6941..9f18cfdb 100644
> --- a/docs/maintenance.rst
> +++ b/docs/maintenance.rst
> @@ -314,3 +314,5 @@ Internally Proxmox Backup Server tracks whether each datastore access is a
>  write or read operation, so that it can gracefully enter the respective mode,
>  by allowing conflicting operations that started before enabling the maintenance
>  mode to finish.
> +
> +For removable datastores the maintenance mode is reset after they are unplugged.
> \ No newline at end of file

Re: [pbs-devel] [PATCH proxmox-backup v4 14/22] docs: mention maintenance mode reset when removable datastore is unplugged

[Christian Ebner]

On 4/17/24 10:46, Hannes Laimer wrote:
> ignore this, it's wrong and doesn't apply in v4

okay, could you add a few lines to the documentation describing what the 
removable datastore is, and I think you should mention the automount 
feature, which is really nice!

Re: [pbs-devel] [PATCH proxmox-backup v4 09/22] pbs-api-types: datastore: use new proxmox_schema::de for deserialization

[Hannes Laimer]

On Wed Apr 17, 2024 at 10:13 AM CEST, Christian Ebner wrote:
> Still not to happy about the commit title, but no hurt feelings

actually we don't even need this anymore for this series, but yes,
missed it, no reason for not changing the message

Re: [pbs-devel] [PATCH proxmox-backup v4 00/22] add removable datastores

[Christian Ebner]

Had a look at the new version of the patches and must say nice work.

My comments are mostly me being picky and do not block this being applied.

So please consider this:
Tested-by: Christian Ebner <c.ebner@proxmox.com>

and for the Rust part also:
Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

Some ideas for possible future improvements:

What I am still missing is a way to partition and format the disk for it 
being usable as reusable datastore directly via the UI. So a workflow 
like, attaching new disk, wipe its contents, create partition and 
filesystem and create removable datastore on it, where the latter to 
might be combined.

An other feature which might be interesting, are automated job 
executions once the datastore is online after being mounted,
e.g. start a verification job or perform a garbage collection.

Re: [pbs-devel] [PATCH proxmox-backup v4 06/22] api2: admin: add (un)mount endpoint for removable datastores

[Hannes Laimer]

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(-)
> > 
> > diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.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 {
> >   }
> >   
> >   impl MaintenanceMode {
> > +    pub fn new(ty: MaintenanceType, message: Option<String>) -> 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 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, 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, 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;
> > @@ -2278,6 +2278,163 @@ pub async fn set_backup_owner(
> >       .await?
> >   }
> >   
> > +pub fn do_mount_device(
> > +    datastore: DataStoreConfig,
> > +    worker: Option<&dyn WorkerTaskContext>,
> > +) -> Result<(), Error> {
> > +    if let Some(uuid) = 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 

> > +        }
> > +        let mount_point_path = std::path::Path::new(&datastore.path);
> > +        if let Some(worker) = worker {
> > +            task_log!(worker, "mounting '{uuid}' to '{}'", datastore.path);
>
> ... 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 removable.",
>
> 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) = pbs_config::datastore::config()?;
> > +    let datastore: DataStoreConfig = section_config.lookup("datastore", &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 = 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(datastore, Some(&worker)),
> > +    )?;
> > +
> > +    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)?;
> > +    let mut counter = 0;
> > +    while active_operations.read + active_operations.write > 0 {
> > +        if counter == 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",
>
> s/can't/cannot
>
> > +                    active_operations.read,
> > +                    active_operations.write
> > +                );
> > +            }
> > +            counter = 5000;
> > +        }
> > +        counter -= 1;
> > +        std::thread::sleep(std::time::Duration::from_millis(1));
> > +        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<Value, Error> {
> > +    let _lock = pbs_config::datastore::lock_config()?;
> > +    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);
> > +    }
> > +    drop(_lock);
> > +
> > +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> > +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> > +
> > +    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\":\"{store}\"}}\n"),
> > +        )
> > +        .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 = &[
> >       (
> > @@ -2316,6 +2473,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?!
> > @@ -2350,6 +2508,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),

Re: [pbs-devel] [PATCH proxmox-backup v4 06/22] api2: admin: add (un)mount endpoint for removable datastores

[Christian Ebner]

On 4/17/24 11:19, Hannes Laimer wrote:
> 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

But then it would simply state that the device with `uuid` intended to 
be used for the `store` (which will be created) is already mounted. Not 
that the datastore is already there. So not really wrong in my opinion.

Re: [pbs-devel] [PATCH proxmox-backup v4 02/22] datastore: make dropping from cache more efficient

[Dietmar Maurer]

First, I think the commit message it totally misleading. This is not for efficiency, instead, this replaces the former MaintenanceType::Unplugged functionality?

But we still have MaintenanceType::Delete, and I am quite unsure it we should handle those states differently.


> On 16.4.2024 17:23 CEST Hannes Laimer <h.laimer@proxmox.com> wrote:
> 
>  
> ... by not having to read the whole config file on every drop,
> but assume calling `update-datastore-cache` through the scoket implies
> it should be dropped and doing the checking on the sending side.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 60 +++++++++++++++-------------------
>  src/api2/config/datastore.rs   |  8 +++--
>  2 files changed, 31 insertions(+), 37 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 0685cc84..a7fe3b8c 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -34,7 +34,7 @@ use crate::task_tracking::{self, update_active_operations};
>  use crate::DataBlob;
>  
>  lazy_static! {
> -    static ref DATASTORE_MAP: Mutex<HashMap<String, Arc<DataStoreImpl>>> =
> +    static ref DATASTORE_MAP: Mutex<HashMap<String, (Arc<DataStoreImpl>, bool)>> =
>          Mutex::new(HashMap::new());
>  }
>  
> @@ -111,24 +111,15 @@ impl Drop for DataStore {
>                      last_task = updated_operations.read + updated_operations.write == 0;
>                  }
>              }
> -
> -            // remove datastore from cache iff
> -            //  - last task finished, and
> -            //  - datastore is in a maintenance mode that mandates it
> -            let remove_from_cache = last_task
> -                && pbs_config::datastore::config()
> -                    .and_then(|(s, _)| s.lookup::<DataStoreConfig>("datastore", self.name()))
> -                    .map_or(false, |c| {
> -                        c.get_maintenance_mode().map_or(false, |m| m.is_offline())
> -                    });
> -
> -            if remove_from_cache {
> -                DATASTORE_MAP.lock().unwrap().remove(self.name());
> +            if last_task {
> +                let mut cache = DATASTORE_MAP.lock().unwrap();
> +                if let Some((_, true)) = cache.get(self.name()) {
> +                    cache.remove(self.name());
> +                }
>              }
>          }
>      }
>  }
> -
>  impl DataStore {
>      // This one just panics on everything
>      #[doc(hidden)]
> @@ -169,7 +160,7 @@ impl DataStore {
>          let entry = datastore_cache.get(name);
>  
>          // reuse chunk store so that we keep using the same process locker instance!
> -        let chunk_store = if let Some(datastore) = &entry {
> +        let (chunk_store, drop_from_cache) = if let Some((datastore, drop_from_cache)) = &entry {
>              let last_digest = datastore.last_digest.as_ref();
>              if let Some(true) = last_digest.map(|last_digest| last_digest == &digest) {
>                  return Ok(Arc::new(Self {
> @@ -177,23 +168,26 @@ impl DataStore {
>                      operation,
>                  }));
>              }
> -            Arc::clone(&datastore.chunk_store)
> +            (Arc::clone(&datastore.chunk_store), *drop_from_cache)
>          } else {
>              let tuning: DatastoreTuning = serde_json::from_value(
>                  DatastoreTuning::API_SCHEMA
>                      .parse_property_string(config.tuning.as_deref().unwrap_or(""))?,
>              )?;
> -            Arc::new(ChunkStore::open(
> -                name,
> -                &config.path,
> -                tuning.sync_level.unwrap_or_default(),
> -            )?)
> +            (
> +                Arc::new(ChunkStore::open(
> +                    name,
> +                    &config.path,
> +                    tuning.sync_level.unwrap_or_default(),
> +                )?),
> +                false,
> +            )
>          };
>  
>          let datastore = DataStore::with_store_and_config(chunk_store, config, Some(digest))?;
>  
>          let datastore = Arc::new(datastore);
> -        datastore_cache.insert(name.to_string(), datastore.clone());
> +        datastore_cache.insert(name.to_string(), (datastore.clone(), drop_from_cache));
>  
>          Ok(Arc::new(Self {
>              inner: datastore,
> @@ -211,20 +205,18 @@ impl DataStore {
>          Ok(())
>      }
>  
> -    /// trigger clearing cache entry based on maintenance mode. Entry will only
> -    /// be cleared iff there is no other task running, if there is, the end of the
> +    /// trigger clearing of cache entry. Entry will only be cleared iff
> +    /// there is no other task running, if there is, the end of the
>      /// last running task will trigger the clearing of the cache entry.
>      pub fn update_datastore_cache(name: &str) -> Result<(), Error> {
> -        let (config, _digest) = pbs_config::datastore::config()?;
> -        let datastore: DataStoreConfig = config.lookup("datastore", name)?;
> -        if datastore
> -            .get_maintenance_mode()
> -            .map_or(false, |m| m.is_offline())
> -        {
> -            // the datastore drop handler does the checking if tasks are running and clears the
> -            // cache entry, so we just have to trigger it here
> -            let _ = DataStore::lookup_datastore(name, Some(Operation::Lookup));
> +        let _store = DataStore::lookup_datastore(name, Some(Operation::Lookup));
> +
> +        let mut datastore_cache = DATASTORE_MAP.lock().unwrap();
> +        if let Some((_, drop_from_cache)) = datastore_cache.get_mut(name) {
> +            *drop_from_cache = true;
>          }
> +        drop(datastore_cache);
> +        drop(_store);
>  
>          Ok(())
>      }
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 3081e1f4..0b3d92f9 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -389,10 +389,12 @@ pub fn update_datastore(
>          data.tuning = update.tuning;
>      }
>  
> -    let mut maintenance_mode_changed = false;
> +    let mut drop_store_from_cache = false;
>      if update.maintenance_mode.is_some() {
> -        maintenance_mode_changed = data.maintenance_mode != update.maintenance_mode;
>          data.maintenance_mode = update.maintenance_mode;
> +        drop_store_from_cache = data
> +            .get_maintenance_mode()
> +            .map_or(false, |m| m.is_offline());
>      }
>  
>      config.set_data(&name, "datastore", &data)?;
> @@ -406,7 +408,7 @@ pub fn update_datastore(
>      }
>  
>      // tell the proxy it might have to clear a cache entry
> -    if maintenance_mode_changed {
> +    if drop_store_from_cache {
>          tokio::spawn(async move {
>              if let Ok(proxy_pid) =
>                  proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v4 09/22] pbs-api-types: datastore: use new proxmox_schema::de for deserialization

[Christian Ebner]

On 4/17/24 11:08, Hannes Laimer wrote:
> On Wed Apr 17, 2024 at 10:13 AM CEST, Christian Ebner wrote:
>> Still not to happy about the commit title, but no hurt feelings
> 
> actually we don't even need this anymore for this series, but yes,
> missed it, no reason for not changing the message

but it does not hurt either, and is more readable than the previous 
deserialization.

Now that I had another look at this, you might bring the 
`SchemaDeserializer` in scope with a `use` statement at the top so you 
don't need it below, that might help to make this even more readable.