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

[pbs-devel] [PATCH proxmox-backup 00/23] 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
removable datastores are marked as 'unplugged', unless they are already
mounted.

b404eea5 and e5863f7e are not strictly needed, but they made sense in
this context, so I kept them in this series.

Hannes Laimer (23):
  tools: add disks utility functions
  pbs-api-types: add backing-device to DataStoreConfig
  maintenance: add 'Unpplugged' maintenance type
  disks: add UUID to partition info
  api2: admin: add (un)mount endpoint for removable datastores
  add helper for checking if a removable datastore is available
  api2: removable datastore creation
  ui: add partition selector form
  api2: disks list: add only-unused flag
  ui: add removable datastore creation support
  ui: add (un)mount button to summary
  pbs-api-types: datastore: use new proxmox_scheme::de for
    deserialization
  pbs-api-types: add removable/is-available flag to DataStoreListItem
  ui: display removable datastores in list
  ui: utils: render unplugged maintenance mode 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
  ui: maintenance: disable edit if unplugged
  pb-manager: add (un)mount command
  add auto-mounting for removable datastores
  api: mark removable datastores as unplugged after restart
  datastore: handle deletion of removable datastore properly

 debian/proxmox-backup-server.install        |   1 +
 debian/proxmox-backup-server.udev           |   3 +
 etc/Makefile                                |   3 +-
 etc/removable-device-attach@.service.in     |   8 +
 pbs-api-types/src/datastore.rs              |  40 ++++-
 pbs-api-types/src/maintenance.rs            |  35 +++-
 pbs-datastore/src/datastore.rs              |  28 +++-
 pbs-datastore/src/lib.rs                    |   2 +-
 src/api2/admin/datastore.rs                 | 175 +++++++++++++++++++-
 src/api2/config/datastore.rs                |  68 +++++++-
 src/api2/node/disks/mod.rs                  |   8 +
 src/api2/status.rs                          |  18 +-
 src/bin/proxmox-backup-api.rs               |  18 ++
 src/bin/proxmox_backup_manager/datastore.rs |  60 ++++++-
 src/tools/disks/mod.rs                      |  96 +++++++++--
 www/Makefile                                |   1 +
 www/NavigationTree.js                       |  10 +-
 www/Utils.js                                |  20 ++-
 www/css/ext6-pbs.css                        |  20 +++
 www/datastore/DataStoreListSummary.js       |   1 +
 www/datastore/Summary.js                    | 108 ++++++++++--
 www/form/PartitionSelector.js               |  59 +++++++
 www/window/DataStoreEdit.js                 |  47 ++++++
 www/window/MaintenanceOptions.js            |  16 +-
 24 files changed, 785 insertions(+), 60 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 01/23] 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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 72e374ca..1cf6385b 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -1159,3 +1159,23 @@ 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("-l");
+    command.arg(path);
+
+    proxmox_sys::command::run_command(command, None)?;
+    Ok(())
+}
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 02/23] 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 | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 73c4890e..a5cc1bd4 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -268,6 +268,11 @@ 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 device, iff the datastore is removable.",
+            optional: true,
+            type: String,
+        }
     }
 )]
 #[derive(Serialize, Deserialize, Updater, Clone, PartialEq)]
@@ -311,6 +316,10 @@ 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(iff removable)
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub backing_device: Option<String>,
 }
 
 impl DataStoreConfig {
@@ -327,6 +336,7 @@ impl DataStoreConfig {
             notify: None,
             tuning: None,
             maintenance_mode: None,
+            backing_device: None,
         }
     }
 
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 03/23] maintenance: add 'Unpplugged' maintenance type

[Hannes Laimer]

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

diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index 1b03ca94..caab9eb5 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -38,7 +38,6 @@ pub enum Operation {
 /// Maintenance type.
 pub enum MaintenanceType {
     // TODO:
-    //  - Add "unmounting" once we got pluggable datastores
     //  - Add "GarbageCollection" or "DeleteOnly" as type and track GC (or all deletes) as separate
     //    operation, so that one can enable a mode where nothing new can be added but stuff can be
     //    cleaned
@@ -48,6 +47,8 @@ pub enum MaintenanceType {
     Offline,
     /// The datastore is being deleted.
     Delete,
+    /// The removable datastore is unplugged
+    Unplugged,
 }
 serde_plain::derive_display_from_serialize!(MaintenanceType);
 serde_plain::derive_fromstr_from_deserialize!(MaintenanceType);
@@ -82,6 +83,10 @@ impl MaintenanceMode {
             bail!("datastore is being deleted");
         }
 
+        if self.ty == MaintenanceType::Unplugged {
+            bail!("datastore is not plugged in");
+        }
+
         let message = percent_encoding::percent_decode_str(self.message.as_deref().unwrap_or(""))
             .decode_utf8()
             .unwrap_or(Cow::Borrowed(""));
@@ -98,3 +103,27 @@ impl MaintenanceMode {
         Ok(())
     }
 }
+
+#[test]
+fn test_check() {
+    let ro_mode = MaintenanceMode::new(MaintenanceType::ReadOnly, None);
+    let offline_mode = MaintenanceMode::new(MaintenanceType::Offline, None);
+    let delete_mode = MaintenanceMode::new(MaintenanceType::Delete, None);
+    let unplugged_mode = MaintenanceMode::new(MaintenanceType::Unplugged, None);
+
+    assert!(ro_mode.check(Some(Operation::Lookup)).is_ok());
+    assert!(ro_mode.check(Some(Operation::Read)).is_ok());
+    assert!(ro_mode.check(Some(Operation::Write)).is_err());
+
+    assert!(offline_mode.check(Some(Operation::Lookup)).is_ok());
+    assert!(offline_mode.check(Some(Operation::Read)).is_err());
+    assert!(offline_mode.check(Some(Operation::Write)).is_err());
+
+    assert!(delete_mode.check(Some(Operation::Lookup)).is_err());
+    assert!(delete_mode.check(Some(Operation::Read)).is_err());
+    assert!(delete_mode.check(Some(Operation::Write)).is_err());
+
+    assert!(unplugged_mode.check(Some(Operation::Lookup)).is_err());
+    assert!(unplugged_mode.check(Some(Operation::Read)).is_err());
+    assert!(unplugged_mode.check(Some(Operation::Write)).is_err());
+}
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 04/23] 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 1cf6385b..7d52e039 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -57,6 +57,8 @@ pub struct LsblkInfo {
     /// File system label.
     #[serde(rename = "fstype")]
     file_system_type: Option<String>,
+    /// File system UUID.
+    uuid: Option<String>,
 }
 
 impl DiskManage {
@@ -559,7 +561,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)?;
 
@@ -643,6 +645,8 @@ pub struct PartitionInfo {
     pub size: Option<u64>,
     /// GPT partition
     pub gpt: bool,
+    /// UUID
+    pub uuid: Option<String>,
 }
 
 #[api(
@@ -833,8 +837,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,
@@ -857,6 +863,7 @@ fn get_partitions_info(
                 filesystem,
                 size: disk.size().ok(),
                 gpt: disk.has_gpt(),
+                uuid,
             }
         })
         .collect()
-- 
2.39.2

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

[Hannes Laimer]

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

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index a5cc1bd4..4495d7fa 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -346,6 +346,18 @@ impl DataStoreConfig {
             .and_then(|str| MaintenanceMode::API_SCHEMA.parse_property_string(str).ok())
             .and_then(|value| MaintenanceMode::deserialize(value).ok())
     }
+
+    pub fn set_maintenance_mode(&mut self, mode: Option<MaintenanceMode>) {
+        self.maintenance_mode = mode.and_then(|mode| {
+            let mut writer = String::new();
+            mode.serialize(proxmox_schema::ser::PropertyStringSerializer::new(
+                &mut writer,
+                &MaintenanceMode::API_SCHEMA,
+            ))
+            .ok()?;
+            Some(writer)
+        });
+    }
 }
 
 #[api(
diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index caab9eb5..790704d9 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -78,6 +78,10 @@ pub struct MaintenanceMode {
 }
 
 impl MaintenanceMode {
+    pub fn new(ty: MaintenanceType, message: Option<String>) -> Self {
+        Self { ty, message }
+    }
+
     pub fn check(&self, operation: Option<Operation>) -> Result<(), Error> {
         if self.ty == MaintenanceType::Delete {
             bail!("datastore is being deleted");
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a95031e7..cda0b677 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -26,23 +26,24 @@ use proxmox_sortable_macro::sortable;
 use proxmox_sys::fs::{
     file_read_firstline, file_read_optional_string, replace_file, CreateOptions,
 };
-use proxmox_sys::{task_log, task_warn};
+use proxmox_sys::{task_log, task_warn, WorkerTaskContext};
 
 use pxar::accessor::aio::Accessor;
 use pxar::EntryKind;
 
 use pbs_api_types::{
     print_ns_and_snapshot, print_store_and_ns, Authid, BackupContent, BackupNamespace, BackupType,
-    Counts, CryptMode, DataStoreListItem, DataStoreStatus, GarbageCollectionStatus, GroupListItem,
-    KeepOptions, Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem,
-    SnapshotVerifyState, BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA,
-    BACKUP_TIME_SCHEMA, BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA,
-    MAX_NAMESPACE_DEPTH, NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
-    PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY,
-    UPID_SCHEMA, VERIFICATION_OUTDATED_AFTER_SCHEMA,
+    Counts, CryptMode, DataStoreConfig, DataStoreListItem, DataStoreStatus,
+    GarbageCollectionStatus, GroupListItem, KeepOptions, MaintenanceMode, MaintenanceType,
+    Operation, PruneJobOptions, RRDMode, RRDTimeFrame, SnapshotListItem, SnapshotVerifyState,
+    BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_NAMESPACE_SCHEMA, BACKUP_TIME_SCHEMA,
+    BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, MAX_NAMESPACE_DEPTH,
+    NS_MAX_DEPTH_SCHEMA, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY,
+    PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ, PRIV_DATASTORE_VERIFY, UPID_SCHEMA,
+    VERIFICATION_OUTDATED_AFTER_SCHEMA,
 };
 use pbs_client::pxar::{create_tar, create_zip};
-use pbs_config::CachedUserInfo;
+use pbs_config::{BackupLockGuard, CachedUserInfo};
 use pbs_datastore::backup_info::BackupInfo;
 use pbs_datastore::cached_chunk_reader::CachedChunkReader;
 use pbs_datastore::catalog::{ArchiveEntry, CatalogReader};
@@ -59,6 +60,7 @@ use pbs_datastore::{
 };
 use pbs_tools::json::required_string_param;
 use proxmox_rest_server::{formatter, WorkerTask};
+use proxmox_section_config::SectionConfigData;
 
 use crate::api2::backup::optional_ns_param;
 use crate::api2::node::rrd::create_value_from_rrd;
@@ -2239,6 +2241,158 @@ pub async fn set_backup_owner(
     .await?
 }
 
+pub fn do_mount_device(
+    _lock: Option<BackupLockGuard>,
+    mut config: SectionConfigData,
+    mut datastore: DataStoreConfig,
+    worker: Option<&dyn WorkerTaskContext>,
+) -> Result<(), Error> {
+    if let Some(uuid) = datastore.backing_device.as_ref() {
+        if pbs_datastore::check_if_available(&datastore).is_ok() {
+            return Err(format_err!("device '{}' is already mounted", &uuid));
+        }
+        let mount_point_path = std::path::Path::new(&datastore.path);
+        if let Some(worker) = worker {
+            task_log!(worker, "mounting '{}' to '{}'", uuid, datastore.path);
+        }
+        crate::tools::disks::mount_by_uuid(uuid, mount_point_path)?;
+
+        datastore.set_maintenance_mode(None);
+        config.set_data(&datastore.name, "datastore", &datastore)?;
+        pbs_config::datastore::save_config(&config)?;
+
+        Ok(())
+    } else {
+        Err(format_err!(
+            "Datastore '{}' can't be mounted because it is not removable.",
+            &datastore.name
+        ))
+    }
+}
+
+#[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 lock = pbs_config::datastore::lock_config()?;
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+    let upid = WorkerTask::new_thread(
+        "mount-device",
+        Some(store),
+        auth_id.to_string(),
+        to_stdout,
+        move |worker| do_mount_device(Some(lock), section_config, datastore, Some(&worker)),
+    )?;
+
+    Ok(json!(upid))
+}
+
+fn do_unmount_device(
+    _lock: BackupLockGuard,
+    force: bool,
+    mut config: SectionConfigData,
+    mut datastore: DataStoreConfig,
+    worker: Option<&dyn WorkerTaskContext>,
+) -> Result<(), Error> {
+    datastore.set_maintenance_mode(Some(MaintenanceMode::new(MaintenanceType::Unplugged, None)));
+    config.set_data(&datastore.name, "datastore", &datastore)?;
+    pbs_config::datastore::save_config(&config)?;
+    drop(_lock);
+
+    if force {
+        let _ = crate::tools::disks::unmount_by_mountpoint(&datastore.path);
+        return Ok(());
+    }
+
+    let mut active_operations = task_tracking::get_active_operations(&datastore.name)?;
+    while active_operations.read + active_operations.write > 0 {
+        if let Some(worker) = worker {
+            if worker.abort_requested() {
+                bail!("aborted, due to user request");
+            }
+            task_log!(
+                worker,
+                "can't unmount yet, still {} read and {} write operations active",
+                active_operations.read,
+                active_operations.write
+            );
+        }
+
+        std::thread::sleep(std::time::Duration::new(5, 0));
+        active_operations = task_tracking::get_active_operations(&datastore.name)?;
+    }
+    crate::tools::disks::unmount_by_mountpoint(&datastore.path)?;
+
+    Ok(())
+}
+
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            store: { schema: DATASTORE_SCHEMA },
+            force: {
+                type: Boolean,
+                description: "Force unmount even if there are active operations.",
+                optional: true,
+                default: false,
+            },
+        },
+    },
+    returns: {
+        schema: UPID_SCHEMA,
+    },
+    access: {
+        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
+    }
+)]
+/// Unmount a removable device that is associated with the datastore
+pub fn unmount(store: String, force: bool, 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 lock = pbs_config::datastore::lock_config()?;
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+
+    let upid = WorkerTask::new_thread(
+        "unmount-device",
+        Some(store),
+        auth_id.to_string(),
+        to_stdout,
+        move |worker| do_unmount_device(lock, force, section_config, datastore, Some(&worker)),
+    )?;
+
+    Ok(json!(upid))
+}
+
 #[sortable]
 const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
     (
@@ -2277,6 +2431,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?!
@@ -2311,6 +2466,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 06/23] 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 | 18 ++++++++++++++++++
 pbs-datastore/src/lib.rs       |  2 +-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 29b9c779..ae9589d2 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -49,6 +49,22 @@ pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error>
     Ok(())
 }
 
+pub fn check_if_available(config: &DataStoreConfig) -> Result<(), Error> {
+    config.backing_device.as_ref().map_or(Ok(()), |uuid| {
+        let mut command = std::process::Command::new("findmnt");
+        command.args(["-n", "-o", "TARGET", "--source", &format!("UUID={}", uuid)]);
+
+        match proxmox_sys::command::run_command(command, None) {
+            Ok(mount_point) if mount_point.trim_end() == config.path => Ok(()),
+            _ => Err(format_err!(
+                "device for datastore '{}' has to be mounted at '{}'",
+                config.name,
+                config.path
+            )),
+        }
+    })
+}
+
 /// Datastore Management
 ///
 /// A Datastore can store severals backups, and provides the
@@ -225,6 +241,8 @@ impl DataStore {
     ) -> Result<Arc<Self>, Error> {
         let name = config.name.clone();
 
+        check_if_available(&config)?;
+
         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 b09fd114..52326b4d 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, check_if_available, DataStore};
 
 mod hierarchy;
 pub use hierarchy::{
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 07/23] 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 5e013c39..de683d73 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 pbs_api_types::{
     Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreNotify, DatastoreTuning,
@@ -19,7 +19,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;
 use crate::api2::config::sync::delete_sync_job;
@@ -71,6 +72,31 @@ 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 = 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(None, config.clone(), 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)?;
+            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(
@@ -124,6 +150,29 @@ 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 08/23] ui: add partition selector form

[Hannes Laimer]

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

diff --git a/www/Makefile b/www/Makefile
index 04c12b31..02ccd444 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -48,6 +48,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..6246e958
--- /dev/null
+++ b/www/form/PartitionSelector.js
@@ -0,0 +1,59 @@
+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?include-partitions=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,
+    valueField: 'uuid',
+    displayField: 'devpath',
+
+    store: {
+	model: 'pbs-partition-list',
+	autoLoad: true,
+	sorters: 'devpath',
+    },
+
+    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 09/23] api2: disks list: add only-unused 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        | 67 ++++++++++++++++++++++++++++++-----
 www/form/PartitionSelector.js |  2 +-
 3 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/src/api2/node/disks/mod.rs b/src/api2/node/disks/mod.rs
index 5ee959cd..73cda339 100644
--- a/src/api2/node/disks/mod.rs
+++ b/src/api2/node/disks/mod.rs
@@ -40,6 +40,12 @@ pub mod zfs;
                 optional: true,
                 default: false,
             },
+            "only-unused": {
+                description: "Only list partitions not used for removable datastores or mounted directories.",
+                type: bool,
+                optional: true,
+                default: false,
+            },
             "usage-type": {
                 type: DiskUsageType,
                 optional: true,
@@ -61,6 +67,7 @@ pub mod zfs;
 pub fn list_disks(
     skipsmart: bool,
     include_partitions: bool,
+    only_unused: bool,
     usage_type: Option<DiskUsageType>,
 ) -> Result<Vec<DiskUsageInfo>, Error> {
     let mut list = Vec::new();
@@ -68,6 +75,7 @@ pub fn list_disks(
     for (_, info) in DiskUsageQuery::new()
         .smart(!skipsmart)
         .partitions(include_partitions)
+        .only_not_in_use(only_unused)
         .query()?
     {
         if let Some(ref usage_type) = usage_type {
diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index 7d52e039..0f1c0e0b 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -18,6 +18,7 @@ use proxmox_lang::error::io_err_other;
 use proxmox_lang::{io_bail, io_format_err};
 use proxmox_schema::api;
 use proxmox_sys::linux::procfs::{mountinfo::Device, MountInfo};
+use serde_json::Value;
 
 use pbs_api_types::BLOCKDEVICE_NAME_REGEX;
 
@@ -30,6 +31,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! {
@@ -770,6 +772,7 @@ fn scan_partitions(
 pub struct DiskUsageQuery {
     smart: bool,
     partitions: bool,
+    only_not_in_use: bool,
 }
 
 impl DiskUsageQuery {
@@ -777,6 +780,7 @@ impl DiskUsageQuery {
         Self {
             smart: true,
             partitions: false,
+            only_not_in_use: false,
         }
     }
 
@@ -790,12 +794,22 @@ impl DiskUsageQuery {
         self
     }
 
+    pub fn only_not_in_use(&mut self, only_not_in_use: bool) -> &mut Self {
+        self.only_not_in_use = only_not_in_use;
+        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.only_not_in_use)
     }
 
     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.only_not_in_use,
+        )?;
         if let Some(info) = map.remove(disk) {
             Ok(info)
         } else {
@@ -804,7 +818,12 @@ 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.only_not_in_use,
+        )
     }
 }
 
@@ -877,6 +896,8 @@ fn get_disks(
     no_smart: bool,
     // include partitions
     include_partitions: bool,
+    // skip partitions which are in use
+    only_not_in_use: bool,
 ) -> Result<HashMap<String, DiskUsageInfo>, Error> {
     let disk_manager = DiskManage::new();
 
@@ -894,6 +915,30 @@ fn get_disks(
 
     // fixme: ceph journals/volumes
 
+    let uuids_in_use = if only_not_in_use && 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)?
@@ -966,12 +1011,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
diff --git a/www/form/PartitionSelector.js b/www/form/PartitionSelector.js
index 6246e958..64e7990a 100644
--- a/www/form/PartitionSelector.js
+++ b/www/form/PartitionSelector.js
@@ -3,7 +3,7 @@ Ext.define('pbs-partition-list', {
     fields: ['name', 'uuid', 'filesystem', 'devpath', 'size'],
     proxy: {
 	type: 'proxmox',
-	url: "/api2/json/nodes/localhost/disks/list?include-partitions=1",
+	url: "/api2/json/nodes/localhost/disks/list?include-partitions=1&only-unused=1",
 	reader: {
 	    transform: (rawData) => rawData.data
 		.flatMap(disk => (disk.partitions ?? [])
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 10/23] ui: add removable datastore creation support

[Hannes Laimer]

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

diff --git a/www/window/DataStoreEdit.js b/www/window/DataStoreEdit.js
index aecf6b8d..d44cdf24 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: false,
+			},
+			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,28 @@ Ext.define('PBS.DataStoreEdit', {
 		    },
 		],
 		columnB: [
+		    {
+			xtype: 'checkbox',
+			boxLabel: gettext('Removable datastore'),
+			listeners: {
+			    change: function(checkbox, newValue) {
+				const inputPanel = checkbox.up('inputpanel');
+				const pathField = inputPanel.down('[name=path]');
+				const uuidField = inputPanel.down('[name=backing-device]');
+
+				pathField.setValue('');
+				uuidField.setValue('');
+
+				const [activeField, inactiveField] =
+				    newValue ? [uuidField, pathField] : [pathField, uuidField];
+
+				activeField.allowBlank = false;
+				inactiveField.allowBlank = true;
+				activeField.show();
+				inactiveField.hide();
+			    },
+			},
+		    },
 		    {
 			xtype: 'textfield',
 			name: 'comment',
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 11/23] 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 | 88 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 84 insertions(+), 4 deletions(-)

diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index d67e81cc..dfff6e7f 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -218,8 +218,6 @@ Ext.define('PBS.DataStoreSummary', {
 	padding: 5,
     },
 
-    tbar: ['->', { xtype: 'proxmoxRRDTypeSelector' }],
-
     items: [
 	{
 	    xtype: 'container',
@@ -292,7 +290,76 @@ 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;
+			}
+			const maintenanceString = response.result.data['maintenance-mode'];
+			const [type, _msg] = PBS.Utils.parseMaintenanceMode(maintenanceString);
+			const isUnplugged = type === 'unplugged';
+
+			unmountBtn.setDisabled(isUnplugged);
+			mountBtn.setDisabled(!isUnplugged);
+		    },
+		});
+	    } 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) {
@@ -305,11 +372,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);
 	    },
@@ -327,6 +400,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 12/23] pbs-api-types: datastore: use new proxmox_scheme::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 4495d7fa..a09df7a3 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -341,10 +341,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()
+        })
     }
 
     pub fn set_maintenance_mode(&mut self, mode: Option<MaintenanceMode>) {
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 13/23] 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 |  7 ++++++-
 src/api2/admin/datastore.rs    |  1 +
 src/api2/status.rs             | 18 +++++++++++++++---
 3 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index a09df7a3..cfeacb94 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -385,6 +385,8 @@ impl DataStoreConfig {
 pub struct DataStoreListItem {
     pub store: String,
     pub comment: Option<String>,
+    /// Datastore is removable
+    pub removable: bool,
     /// If the datastore is in maintenance mode, information about it
     #[serde(skip_serializing_if = "Option::is_none")]
     pub maintenance: Option<String>,
@@ -1331,6 +1333,8 @@ pub struct DataStoreStatusListItem {
     pub used: i64,
     /// The available bytes of the underlying storage. (-1 on error)
     pub avail: i64,
+    /// 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>>>,
@@ -1355,12 +1359,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: -1,
             used: -1,
             avail: -1,
+            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 cda0b677..f1976c21 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1244,6 +1244,7 @@ pub fn get_datastore_list(
                 } else {
                     data["comment"].as_str().map(String::from)
                 },
+                removable: data["backing-device"].as_str().is_some(),
                 maintenance: data["maintenance-mode"].as_str().map(String::from),
             });
         }
diff --git a/src/api2/status.rs b/src/api2/status.rs
index ff7d4cbd..65c5f770 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::{check_if_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 check_if_available(&store_config).is_err() {
+            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: status.total as i64,
             used: status.used as i64,
             avail: status.available as i64,
+            is_available: true,
             history: None,
             history_start: None,
             history_delta: None,
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 14/23] 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..08c796b2 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'];
+		const mainIcon = `fa fa-${records[i].data.removable ? 'usb' : '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 5fd65d25..2abff495 100644
--- a/www/css/ext6-pbs.css
+++ b/www/css/ext6-pbs.css
@@ -267,6 +267,10 @@ span.snapshot-comment-column {
     content: "\ ";
 }
 
+.x-treelist-item-icon.fa-usb, .pmx-tree-icon-custom.fa-usb {
+    font-size: 12px;
+}
+
 /* datastore maintenance */
 .pmx-tree-icon-custom.maintenance:after {
     content: "\f0ad";
@@ -286,6 +290,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 968239b0..c8eaca9f 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 15/23] ui: utils: render unplugged maintenance mode correctly

[Hannes Laimer]

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

diff --git a/www/Utils.js b/www/Utils.js
index 2eca600e..56b1de30 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -702,6 +702,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 16/23] ui: utils: make parseMaintenanceMode more robust

[Hannes Laimer]

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

diff --git a/www/Utils.js b/www/Utils.js
index 56b1de30..c84227ca 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -662,14 +662,18 @@ Ext.define('PBS.Utils', {
 	return `${icon} ${value}`;
     },
 
-    // FIXME: this "parser" is brittle and relies on the order the arguments will appear in
     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 17/23] 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 | 20 ++++++++++++--------
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/www/css/ext6-pbs.css b/www/css/ext6-pbs.css
index 2abff495..6f04d7e7 100644
--- a/www/css/ext6-pbs.css
+++ b/www/css/ext6-pbs.css
@@ -257,6 +257,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: "\f287";
+}
+
 /* 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 dfff6e7f..d80c4a08 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -61,16 +61,20 @@ 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 {
+			const maintenanceString = response.result.data['maintenance-mode'];
+			if (!maintenanceString) {
 			    me.view.el.mask(gettext('Datastore is not available'));
+			    return;
 			}
+
+			const [type, msg] = PBS.Utils.parseMaintenanceMode(maintenanceString);
+			const isUnplugged = type === 'unplugged';
+			const maskMessage = isUnplugged
+			    ? gettext('Datastore is unplugged')
+			    : `${gettext('Datastore is in maintenance mode')}${msg ? ': ' + msg : ''}`;
+
+			const 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 18/23] 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

[pbs-devel] [PATCH proxmox-backup 19/23] ui: maintenance: disable edit if unplugged

[Hannes Laimer]

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

diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
index 527c3698..e50e0f2f 100644
--- a/www/window/MaintenanceOptions.js
+++ b/www/window/MaintenanceOptions.js
@@ -52,6 +52,7 @@ Ext.define('PBS.window.MaintenanceOptions', {
 	items: [
 	    {
 		xtype: 'pbsMaintenanceType',
+		id: 'type-field',
 		name: 'maintenance-type',
 		fieldLabel: gettext('Maintenance Type'),
 		value: '__default__',
@@ -84,7 +85,15 @@ Ext.define('PBS.window.MaintenanceOptions', {
 		'maintenance-msg': message ?? '',
 	    };
 	}
+	const unplugged = options['maintenance-type'] === 'unplugged';
+	const defaultType = options['maintenance-type'] === '__default__';
+	if (unplugged) {
+	    options['maintenance-type'] = '';
+	}
 
 	me.callParent([options]);
+
+	Ext.ComponentManager.get('type-field').setDisabled(unplugged);
+	Ext.ComponentManager.get('message-field').setDisabled(unplugged || defaultType);
     },
 });
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 20/23] pb-manager: add (un)mount command

[Hannes Laimer]

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/bin/proxmox_backup_manager/datastore.rs | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 383bcd24..88cea6b0 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};
@@ -8,6 +8,7 @@ use pbs_api_types::{DataStoreConfig, DATASTORE_SCHEMA, PROXMOX_CONFIG_DIGEST_SCH
 use pbs_client::view_task_result;
 
 use proxmox_backup::api2;
+use proxmox_backup::api2::admin::datastore::{API_METHOD_MOUNT, API_METHOD_UNMOUNT};
 use proxmox_backup::client_helpers::connect_to_localhost;
 
 #[api(
@@ -142,6 +143,7 @@ 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))
         .insert(
             "show",
             CliCommand::new(&API_METHOD_SHOW_DATASTORE)
@@ -152,6 +154,7 @@ pub fn datastore_commands() -> CommandLineInterface {
             "create",
             CliCommand::new(&API_METHOD_CREATE_DATASTORE).arg_param(&["name", "path"]),
         )
+        .insert("unmount", CliCommand::new(&API_METHOD_UNMOUNT))
         .insert(
             "update",
             CliCommand::new(&api2::config::datastore::API_METHOD_UPDATE_DATASTORE)
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 21/23] 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 | 55 +++++++++++++++++++++
 5 files changed, 69 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..fe256548
--- /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 --uuid %i
\ No newline at end of file
diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
index 88cea6b0..d2f2db38 100644
--- a/src/bin/proxmox_backup_manager/datastore.rs
+++ b/src/bin/proxmox_backup_manager/datastore.rs
@@ -140,6 +140,60 @@ async fn delete_datastore(mut param: Value, rpcenv: &mut dyn RpcEnvironment) ->
     Ok(())
 }
 
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            uuid: {
+                type: String,
+                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(mut param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let output_format = extract_output_format(&mut param);
+    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 client = connect_to_localhost()?;
+        let result = client
+            .post(
+                &format!("api2/json/admin/datastore/{}/mount", store_name),
+                None,
+            )
+            .await?;
+
+        view_task_result(&client, result, &output_format).await?;
+        Ok(Value::Null)
+    } else {
+        Err(format_err!(
+            "'{}' 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))
@@ -169,6 +223,7 @@ pub fn datastore_commands() -> CommandLineInterface {
                     pbs_config::datastore::complete_calendar_event,
                 ),
         )
+        .insert("uuid-mount", CliCommand::new(&API_METHOD_UUID_MOUNT))
         .insert(
             "remove",
             CliCommand::new(&API_METHOD_DELETE_DATASTORE)
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 22/23] api: mark removable datastores as unplugged after restart

[Hannes Laimer]

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

diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index c6c24449..8c95844d 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -10,6 +10,8 @@ use proxmox_lang::try_block;
 use proxmox_router::RpcEnvironmentType;
 use proxmox_sys::fs::CreateOptions;
 
+use pbs_api_types::{DataStoreConfig, MaintenanceMode, MaintenanceType};
+use pbs_datastore::check_if_available;
 use proxmox_rest_server::{daemon, ApiConfig, RestServer};
 
 use proxmox_backup::auth_helpers::*;
@@ -73,6 +75,8 @@ async fn run() -> Result<(), Error> {
 
     proxmox_backup::auth_helpers::setup_auth_context(true);
 
+    mark_removable_datastores_unplugged()?;
+
     let backup_user = pbs_config::backup_user()?;
     let mut commando_sock = proxmox_rest_server::CommandSocket::new(
         proxmox_rest_server::our_ctrl_sock(),
@@ -161,3 +165,17 @@ async fn run() -> Result<(), Error> {
 
     Ok(())
 }
+
+fn mark_removable_datastores_unplugged() -> Result<(), Error> {
+    let (mut config, _digest) = pbs_config::datastore::config()?;
+    let list: Vec<DataStoreConfig> = config.convert_to_typed_array("datastore")?;
+    for mut datastore in list {
+        if check_if_available(&datastore).is_err() {
+            datastore
+                .set_maintenance_mode(Some(MaintenanceMode::new(MaintenanceType::Unplugged, None)));
+            config.set_data(&datastore.name, "datastore", &datastore)?;
+        }
+    }
+    pbs_config::datastore::save_config(&config)?;
+    Ok(())
+}
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 23/23] 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   | 15 +++++++++++++++
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index ae9589d2..ed616cfe 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -1438,10 +1438,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 de683d73..aba12e4d 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -28,9 +28,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::check_if_available;
 use proxmox_rest_server::WorkerTask;
 
 use crate::server::jobstate;
+use crate::tools::disks::unmount_by_mountpoint;
 
 #[api(
     input: {
@@ -477,6 +479,15 @@ 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 && check_if_available(&store_config).is_err() {
+        http_bail!(
+            BAD_REQUEST,
+            "can't destroy data on '{}' unless the device is plugged in",
+            name
+        );
+    }
+
     if !keep_job_configs {
         for job in list_verification_jobs(Some(name.clone()), Value::Null, rpcenv)? {
             delete_verification_job(job.config.id, None, rpcenv)?
@@ -515,6 +526,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

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

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> b404eea5 and e5863f7e are not strictly needed, but they made sense in
> this context, so I kept them in this series.
> 

Please refer to the commit message/patch number directly - there is no 
way to find out for me which commits are meant by that :)

Gave these patches a try on the latest master, here is what I found:

- Datastore add UI: Might make sense to restructure the dialog, the 
checkbox for 'removable datastore' influences UI elements that are 
before the checkbox - a user might have entered something to the 
datastore path field before toggling the checkbox.

- I'm not sure if the USB icon is the best choice for a removable data 
store. it could be a hot plugged SATA disk, iSCSI, etc. - maybe change 
this to something more generic if possible?

- UI: datastore summary page: underscore in `/mnt/removeable_datastore` 
is not rendered in my firefox (117.0.1)

- Mounting/Unmounting does not seem to work properly for me, here is 
what I did:
   - New VM with PBS, latest updates
   - Attached second disk
   - Created ext4 partition
   - created new removabale datastore from GUI
     - unmount from CLI (promxox-backup-manager datastore unmount --store
       test) does not seem to have any effect
     - pressing on 'unmount' in the GUI seems to work (verified by
       `mount`), but the GUI still considers the datastore as
        mounted - the `mount` button remains disabled
     - a `promxox-backup-manager datastore mount --store test` on the
       CLI does not seem to have any effect


- There is no way to prepare a disk for the use as a removable datastore 
from the UI alone. We can create a GPT partition table and create an 
ext4 partition on it (via 'Directory'), however this creates a mount 
point for the new partition, so it cannot be used any more when creating 
a datastore. Maybe creating the mount point could be optional?



-- 
- Lukas

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

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> +/// 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)]);

nit: you can use `format!("UUID={uuid}")` here

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 02/23] pbs-api-types: add backing-device to DataStoreConfig

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> +        "backing-device": {
> +            description: "The UUID of the device, iff the datastore is removable.",
                                                       ^
I don't think that we should use 'iff' in documentation. I am not sure 
if people outside of the academic world know that 'iff' means 'if and 
only if' ;)

> +
> +    /// The UUID of the device(iff removable)
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub backing_device: Option<String>,
>   }

Same here


-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 03/23] maintenance: add 'Unpplugged' maintenance type

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> +
> +#[test]
> +fn test_check() {
> +    let ro_mode = MaintenanceMode::new(MaintenanceType::ReadOnly, None);
> +    let offline_mode = MaintenanceMode::new(MaintenanceType::Offline, None);
> +    let delete_mode = MaintenanceMode::new(MaintenanceType::Delete, None);
> +    let unplugged_mode = MaintenanceMode::new(MaintenanceType::Unplugged, None);
> +
> +    assert!(ro_mode.check(Some(Operation::Lookup)).is_ok());
> +    assert!(ro_mode.check(Some(Operation::Read)).is_ok());
> +    assert!(ro_mode.check(Some(Operation::Write)).is_err());
> +
> +    assert!(offline_mode.check(Some(Operation::Lookup)).is_ok());
> +    assert!(offline_mode.check(Some(Operation::Read)).is_err());
> +    assert!(offline_mode.check(Some(Operation::Write)).is_err());
> +
> +    assert!(delete_mode.check(Some(Operation::Lookup)).is_err());
> +    assert!(delete_mode.check(Some(Operation::Read)).is_err());
> +    assert!(delete_mode.check(Some(Operation::Write)).is_err());
> +
> +    assert!(unplugged_mode.check(Some(Operation::Lookup)).is_err());
> +    assert!(unplugged_mode.check(Some(Operation::Read)).is_err());
> +    assert!(unplugged_mode.check(Some(Operation::Write)).is_err());
> +}

It's always advisable to have tests in separate submodule:

#[cfg(test)]
mod tests {
     use super::*;

     #[test]
     fn test_check() {
        ...
     }
}

This makes sure that the test(s) is/are not compiled during a normal 
build :)

-- 
- Lukas

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

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> +pub fn do_mount_device(
> +        datastore.set_maintenance_mode(None);

I think this overrides existing maintenance modes... e.g. if a datastore
is 'read-only', unmounted and mounted again, then the 'read-only' mode 
will be gone.

> +        config.set_data(&datastore.name, "datastore", &datastore)?;
> +        pbs_config::datastore::save_config(&config)?;
> +
> +        Ok(())
> +    } else {
> +        Err(format_err!(
> +            "Datastore '{}' can't be mounted because it is not removable.",
> +            &datastore.name
> +        ))
> +    }
> +}
> +


-- 
- Lukas

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

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> +pub fn check_if_available(config: &DataStoreConfig) -> Result<(), Error> {
> +    config.backing_device.as_ref().map_or(Ok(()), |uuid| {
> +        let mut command = std::process::Command::new("findmnt");
> +        command.args(["-n", "-o", "TARGET", "--source", &format!("UUID={}", uuid)]);

nit: uuid can be written inside placeholder: {uuid}


-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 07/23] api2: removable datastore creation

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> +        if let Some(datastore_name) = already_used_by {
> +            param_bail!(
> +                "backing-device",
> +                "device already in use by datastore '{}'",
> +                datastore_name
> +            );
Nit: you can write {datastore_name}

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 10/23] ui: add removable datastore creation support

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:

> +			    change: function(checkbox, newValue) {
> +				const inputPanel = checkbox.up('inputpanel');
> +				const pathField = inputPanel.down('[name=path]');
> +				const uuidField = inputPanel.down('[name=backing-device]');
> +

To quote our JS style guide [1]:
   Avoid using const for everything, but rather in the sense of
   constants. JavaScript is to dynamic for it to provide actual benefits
   if used as default.

... So maybe rather use `let` :). Also applies to some of the following 
commits.

[1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 11/23] ui: add (un)mount button to summary

[Lukas Wagner]

Same nit as in 10/23 regarding the use of `const`

On 9/15/23 08:54, Hannes Laimer wrote:
> 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 | 88 ++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 84 insertions(+), 4 deletions(-)
> 
> diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
> index d67e81cc..dfff6e7f 100644
> --- a/www/datastore/Summary.js
> +++ b/www/datastore/Summary.js
> @@ -218,8 +218,6 @@ Ext.define('PBS.DataStoreSummary', {
>   	padding: 5,
>       },
>   
> -    tbar: ['->', { xtype: 'proxmoxRRDTypeSelector' }],
> -
>       items: [
>   	{
>   	    xtype: 'container',
> @@ -292,7 +290,76 @@ 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;
> +			}
> +			const maintenanceString = response.result.data['maintenance-mode'];
> +			const [type, _msg] = PBS.Utils.parseMaintenanceMode(maintenanceString);
> +			const isUnplugged = type === 'unplugged';
> +
> +			unmountBtn.setDisabled(isUnplugged);
> +			mountBtn.setDisabled(!isUnplugged);
> +		    },
> +		});
> +	    } 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) {
> @@ -305,11 +372,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);
>   	    },
> @@ -327,6 +400,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);

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 16/23] ui: utils: make parseMaintenanceMode more robust

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:

> -    // FIXME: this "parser" is brittle and relies on the order the arguments will appear in
>       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) {

I know, we don't really do this so far our code base, but for functions 
like these it would certainly be nice to document any function contracts 
(input format, function outputs, side effects).

It's quite hard to review the code for correctness if this information 
is not provided.

What do you think?

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 17/23] ui: add datastore status mask for unplugged removable datastores

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
> index dfff6e7f..d80c4a08 100644
> --- a/www/datastore/Summary.js
> +++ b/www/datastore/Summary.js
> @@ -61,16 +61,20 @@ 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 {
> +			const maintenanceString = response.result.data['maintenance-mode'];
> +			if (!maintenanceString) {
>   			    me.view.el.mask(gettext('Datastore is not available'));
> +			    return;
>   			}
> +
> +			const [type, msg] = PBS.Utils.parseMaintenanceMode(maintenanceString);
> +			const isUnplugged = type === 'unplugged';
> +			const maskMessage = isUnplugged
> +			    ? gettext('Datastore is unplugged')
> +			    : `${gettext('Datastore is in maintenance mode')}${msg ? ': ' + msg : ''}`;

I know this expression existed before, but maybe use the opportunity to 
make it a bit more readable? It's a bit hard to parse for my brain :)

-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 20/23] pb-manager: add (un)mount command

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
>   src/bin/proxmox_backup_manager/datastore.rs | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/bin/proxmox_backup_manager/datastore.rs b/src/bin/proxmox_backup_manager/datastore.rs
> index 383bcd24..88cea6b0 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};

Nit: `format_err` is not needed in this commit, i guess this belongs in 
21/23?

-- 
- Lukas

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

[Lukas Wagner]

Oh, just one more thing. Some of the commit messages were a bit short 
and could use a bit more details about *why* the changes are necessary.

-- 
- Lukas

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

[Thomas Lamprecht]

Am 19/09/2023 um 15:37 schrieb Lukas Wagner:
> - I'm not sure if the USB icon is the best choice for a removable data
> store.  it could be a hot plugged SATA disk, iSCSI, etc. - maybe
> change this to something more generic if possible?

Yeah, USB can be confusing; to avoid decision fatigue I'd suggest one
of:

- fa-plug (for plug-able) https://fontawesome.com/v4/icon/plug

- fa-chain-broken (not used anywhere, and it sorta looks like it would
  fit but def. more abstract)
  https://fontawesome.com/v4/icon/chain-broken

> - There is no way to prepare a disk for the use as a removable
> datastore from the UI alone. We can create a GPT partition table and
> create an ext4 partition on it (via 'Directory'), however this creates
> a mount point for the new partition, so it cannot be used any more
> when creating a datastore. Maybe creating the mount point could be
> optional?

While true and definitively wanted in the longer run, I think this might
be out of scope for this series, to avoid blowing the yak-shaving up too
much  ^^

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

[Lukas Wagner]

On 9/19/23 17:01, Thomas Lamprecht wrote:
> Am 19/09/2023 um 15:37 schrieb Lukas Wagner:
>> - I'm not sure if the USB icon is the best choice for a removable data
>> store.  it could be a hot plugged SATA disk, iSCSI, etc. - maybe
>> change this to something more generic if possible?
> 
> Yeah, USB can be confusing; to avoid decision fatigue I'd suggest one
> of:
> 
> - fa-plug (for plug-able) https://fontawesome.com/v4/icon/plug
> 
> - fa-chain-broken (not used anywhere, and it sorta looks like it would
>    fit but def. more abstract)
>    https://fontawesome.com/v4/icon/chain-broken
> 

Out of those two I think I'd prefer fa-plug.

>> - There is no way to prepare a disk for the use as a removable
>> datastore from the UI alone. We can create a GPT partition table and
>> create an ext4 partition on it (via 'Directory'), however this creates
>> a mount point for the new partition, so it cannot be used any more
>> when creating a datastore. Maybe creating the mount point could be
>> optional?
> 
> While true and definitively wanted in the longer run, I think this might
> be out of scope for this series, to avoid blowing the yak-shaving up too
> much  ^^

True, I just wanted to mention it.

-- 
- Lukas

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

[Lukas Wagner]

On 9/15/23 08:54, Hannes Laimer wrote:
> 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.

Something else that I've found: These changes somehow break adding a 
regular, non-removable datastore for me, the 'Add' button always stays 
disabled.

-- 
- Lukas

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

[Lukas Wagner]

On 9/19/23 15:37, Lukas Wagner wrote:
> - Mounting/Unmounting does not seem to work properly for me, here is 
> what I did:
>    - New VM with PBS, latest updates
>    - Attached second disk
>    - Created ext4 partition
>    - created new removabale datastore from GUI
>      - unmount from CLI (promxox-backup-manager datastore unmount --store
>        test) does not seem to have any effect
>      - pressing on 'unmount' in the GUI seems to work (verified by
>        `mount`), but the GUI still considers the datastore as
>         mounted - the `mount` button remains disabled
>      - a `promxox-backup-manager datastore mount --store test` on the
>        CLI does not seem to have any effect
> 

Turns out this was caused by an old version of `proxmox-schema`. Since 
there was not mention of this dependency in the cover letter, I assumed 
that the latest packaged version would be recent enough. However,
the change "schema: serialize enum unit variants" is required, which has 
not been packaged yet.

I'll do more thorough testing once v2 with the requested changes is posted.


-- 
- Lukas

Re: [pbs-devel] [PATCH proxmox-backup 03/23] maintenance: add 'Unpplugged' maintenance type

[Hannes Laimer]

On 9/19/23 15:37, Lukas Wagner wrote:
> 
> 
> On 9/15/23 08:54, Hannes Laimer wrote:
>> +
>> +#[test]
>> +fn test_check() {
>> +    let ro_mode = MaintenanceMode::new(MaintenanceType::ReadOnly, None);
>> +    let offline_mode = MaintenanceMode::new(MaintenanceType::Offline, 
>> None);
>> +    let delete_mode = MaintenanceMode::new(MaintenanceType::Delete, 
>> None);
>> +    let unplugged_mode = 
>> MaintenanceMode::new(MaintenanceType::Unplugged, None);
>> +
>> +    assert!(ro_mode.check(Some(Operation::Lookup)).is_ok());
>> +    assert!(ro_mode.check(Some(Operation::Read)).is_ok());
>> +    assert!(ro_mode.check(Some(Operation::Write)).is_err());
>> +
>> +    assert!(offline_mode.check(Some(Operation::Lookup)).is_ok());
>> +    assert!(offline_mode.check(Some(Operation::Read)).is_err());
>> +    assert!(offline_mode.check(Some(Operation::Write)).is_err());
>> +
>> +    assert!(delete_mode.check(Some(Operation::Lookup)).is_err());
>> +    assert!(delete_mode.check(Some(Operation::Read)).is_err());
>> +    assert!(delete_mode.check(Some(Operation::Write)).is_err());
>> +
>> +    assert!(unplugged_mode.check(Some(Operation::Lookup)).is_err());
>> +    assert!(unplugged_mode.check(Some(Operation::Read)).is_err());
>> +    assert!(unplugged_mode.check(Some(Operation::Write)).is_err());
>> +}
> 
> It's always advisable to have tests in separate submodule:
> 
> #[cfg(test)]
> mod tests {
>      use super::*;
> 
>      #[test]
>      fn test_check() {
>         ...
>      }
> }
> 
> This makes sure that the test(s) is/are not compiled during a normal 
> build :)
> 
Correct me if I'm wrong but, AFAIK `#[test]` does that, and since this
is the only and very simple test a whole mod thing seemed a bit
overkill, so I stuck with just `#[test]`.

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

[Hannes Laimer]

On 9/19/23 15:38, Lukas Wagner wrote:
> 
> 
> On 9/15/23 08:54, Hannes Laimer wrote:
>> +pub fn do_mount_device(
>> +        datastore.set_maintenance_mode(None);
> 
> I think this overrides existing maintenance modes... e.g. if a datastore
> is 'read-only', unmounted and mounted again, then the 'read-only' mode 
> will be gone.
> 
Yes, it does. Why should it not? The only situation where that could be
a problem is if a removable datastore should not be written to at all,
so it would either be unplugged or ro. But in this case, why setup sync
jos or backups to a datastore that should not be written to in the first
place?

>> +        config.set_data(&datastore.name, "datastore", &datastore)?;
>> +        pbs_config::datastore::save_config(&config)?;
>> +
>> +        Ok(())
>> +    } else {
>> +        Err(format_err!(
>> +            "Datastore '{}' can't be mounted because it is not 
>> removable.",
>> +            &datastore.name
>> +        ))
>> +    }
>> +}
>> +
> 
> 

Re: [pbs-devel] [PATCH proxmox-backup 03/23] maintenance: add 'Unpplugged' maintenance type

[Lukas Wagner]

On 9/21/23 09:41, Hannes Laimer wrote:
>>
>> It's always advisable to have tests in separate submodule:
>>
>> #[cfg(test)]
>> mod tests {
>>      use super::*;
>>
>>      #[test]
>>      fn test_check() {
>>         ...
>>      }
>> }
>>
>> This makes sure that the test(s) is/are not compiled during a normal 
>> build :)
>>
> Correct me if I'm wrong but, AFAIK `#[test]` does that, and since this
> is the only and very simple test a whole mod thing seemed a bit
> overkill, so I stuck with just `#[test]`.

I stand corrected, you are indeed right. To quote [1]:
   The test attribute marks a function to be executed as a test. These
   functions are only compiled when in test mode.

Maybe I misremember some edge case where this was not the case, or maybe 
it was the way I described some time ago and they changed it. TIL.

Anyway, I still think it is good practice to put tests in a separate 
module, as it cleanly separates test code from production code. For a 
single function this does not make much difference, but maybe somebody 
else will add some more tests, extract common test setup code into 
helpers, etc. - then they would need to touch the existing code anyway 
and move it in a module. In other words, I think it's good to do it the 
'clean' way right from the start. :)

[1] https://doc.rust-lang.org/reference/attributes/testing.html

-- 
- Lukas

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

[Lukas Wagner]

On 9/21/23 09:50, Hannes Laimer wrote:
> On 9/19/23 15:38, Lukas Wagner wrote:
>>
>>
>> On 9/15/23 08:54, Hannes Laimer wrote:
>>> +pub fn do_mount_device(
>>> +        datastore.set_maintenance_mode(None);
>>
>> I think this overrides existing maintenance modes... e.g. if a datastore
>> is 'read-only', unmounted and mounted again, then the 'read-only' mode 
>> will be gone.
>>
> Yes, it does. Why should it not? The only situation where that could be
> a problem is if a removable datastore should not be written to at all,
> so it would either be unplugged or ro. But in this case, why setup sync
> jos or backups to a datastore that should not be written to in the first
> place?
> 

I agree that there might be limited number of use cases where this is 
actually a problem. That being said, from a user's perspective I would 
find this behavior quite surprising if I were to stumble across it. I 
guess that's due to the fact that it is not really apparent that 'not 
mounted' is also handled as a maintenance mode, that replaces others.

If you keep this behavior as is, it should at least be documented in the 
admin guide.

-- 
- Lukas

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

[Hannes Laimer]

On 9/21/23 10:37, Lukas Wagner wrote:
> 
> 
> On 9/21/23 09:50, Hannes Laimer wrote:
>> On 9/19/23 15:38, Lukas Wagner wrote:
>>>
>>>
>>> On 9/15/23 08:54, Hannes Laimer wrote:
>>>> +pub fn do_mount_device(
>>>> +        datastore.set_maintenance_mode(None);
>>>
>>> I think this overrides existing maintenance modes... e.g. if a datastore
>>> is 'read-only', unmounted and mounted again, then the 'read-only' 
>>> mode will be gone.
>>>
>> Yes, it does. Why should it not? The only situation where that could be
>> a problem is if a removable datastore should not be written to at all,
>> so it would either be unplugged or ro. But in this case, why setup sync
>> jos or backups to a datastore that should not be written to in the first
>> place?
>>
> 
> I agree that there might be limited number of use cases where this is 
> actually a problem. That being said, from a user's perspective I would 
> find this behavior quite surprising if I were to stumble across it. I 
> guess that's due to the fact that it is not really apparent that 'not 
> mounted' is also handled as a maintenance mode, that replaces others.
> 
> If you keep this behavior as is, it should at least be documented in the 
> admin guide.
> 
Makes sense, should add some docs anyway