[pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Hannes Laimer]

v2(based on Lorenz Stechauner <l.stechauner@proxmox.com>'s feedback):
 - do not allow creation of removable datastores on a device that 
   is mounted at '/'
 - ui: also show the mount point in the Options-tab

Adds the possibility to create removable datastores. A removable
datastore has a UUID(the device on which the data is stored) and a
mount-point(where the device will be mounted), iff both are set the
datastore is removable. Everything else is identical to normal
datastores. Since config files for jobs, etc. are stored on the server,
all configuration can be done with the device plugged in or not. Certain
statistics about the datastore won't be available as long as it is not
plugged in. 
Removable datastores have to be unmounted before removing, it can only 
be unmounted if not jibs are running.
Removable datastores are mounted automatically when the device is plugged in, if it has
been unmounted, it has to be mounted manually through the WebUI or the Api.
Jobs will not be started if the datastore is not available, and
depending on the configuration, start when the device is plugged in the
next time.

Still todo:
 - make sync to local datastore more integrated
 - (add 'when plugged in'-option to job schedule?)
 - replace linux commands with internal functions in tools/disks, where
    possible

Hannes Laimer (15):
  tools: add disks utility functions
  config: add uuid+mountpoint to DataStoreConfig
  api2: add support for removable datastore creation
  backup: add check_if_available function to ds
  api2: add 'is_available' to DataStoreConfig
  api2: add 'removable' to DataStoreListItem
  api2: add (un)mount endpoint for removable ds's
  pbs: add mount-removable command to commandSocket
  pbs-manager: add 'send-command' command
  debian: add udev rule for removable datastores
  ui: show usb icon for removable datastore in list
  ui: add 'removable' checkbox in datastore creation
  ui: display row as disabled in ds statistics
  ui: show backing device UUID and mount-point in option tab
  ui: add (un)mount button to summary

 debian/proxmox-backup-server.udev    |   3 +
 pbs-api-types/src/lib.rs             |   7 ++
 src/api2/admin/datastore.rs          | 159 +++++++++++++++++++++++++++
 src/api2/config/datastore.rs         |  22 +++-
 src/api2/status.rs                   |  19 +++-
 src/api2/types/mod.rs                |   2 +
 src/backup/datastore.rs              |  23 ++++
 src/backup/mod.rs                    |   2 +-
 src/bin/proxmox-backup-api.rs        |  27 +++++
 src/bin/proxmox-backup-manager.rs    |  41 +++++++
 src/config/datastore.rs              |  16 +++
 src/tools/disks/mod.rs               |  53 +++++++++
 www/NavigationTree.js                |   3 +-
 www/dashboard/DataStoreStatistics.js |   3 +
 www/datastore/OptionView.js          |   6 +
 www/datastore/Summary.js             |  77 ++++++++++++-
 www/window/DataStoreEdit.js          |   5 +
 17 files changed, 460 insertions(+), 8 deletions(-)

-- 
2.30.2

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

[Hannes Laimer]

adds:
 - get_fs_uuid_by_disk_path: taken out of the already existing
    get_fs_uuid function
 - get_mount_point_by_uuid: returns the mount-point of a disk by a given
    uuid, fails if the disk is not mounted
 - get_fs_uuid_by_path: finds the uuid of the disk that the given path
    is on, can fail if there is no uuid for the disk
 - mount_by_uuid: mounts a disk identified by uuid to a given path
 - unmount_by_mount_point: basically linux 'umount -l', justification
    for the lazy option in code
---
 src/tools/disks/mod.rs | 53 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
index af7f7fe1..ec4d8742 100644
--- a/src/tools/disks/mod.rs
+++ b/src/tools/disks/mod.rs
@@ -1009,6 +1009,22 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
         None => bail!("disk {:?} has no node in /dev", disk.syspath()),
     };
 
+    get_fs_uuid_by_disk_path(&disk_path)
+}
+
+/// Get the FS UUID of the disk that the given path is on.
+pub fn get_fs_uuid_by_path(path: &Path) -> Result<String, Error> {
+    let mut command = std::process::Command::new("df");
+    command.args(&["--output=source"]);
+    command.arg(path);
+
+    let output = crate::tools::run_command(command, None)?;
+
+    get_fs_uuid_by_disk_path(Path::new(output.lines().skip(1).next().unwrap()))
+}
+
+/// Get the FS UUID of a disk with a given disk path.
+pub fn get_fs_uuid_by_disk_path(disk_path: &Path) -> Result<String, Error> {
     let mut command = std::process::Command::new("blkid");
     command.args(&["-o", "export"]);
     command.arg(disk_path);
@@ -1023,3 +1039,40 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
 
     bail!("get_fs_uuid failed - missing UUID");
 }
+
+/// Get mount point by UUID
+pub fn get_mount_point_by_uuid(uuid: &str) -> Result<String, Error> {
+    let mut command = std::process::Command::new("findmnt");
+    command.args(&["-rn", "-oTARGET", "-S"]);
+    command.arg(&format!("UUID={}", uuid));
+
+    let output = crate::tools::run_command(command, None)?;
+
+    if !output.is_empty() {
+        return Ok(String::from(output.trim()));
+    }
+
+    bail!("get_mount_point_by_uuid failed - device with uuid: {} is not mounted", 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);
+
+    crate::tools::run_command(command, None)?;
+    Ok(())
+}
+
+/// Unmount a disk by its mount point.
+pub fn unmount_by_mount_point(mount_point: &Path) -> Result<(), Error> {
+    let mut command = std::process::Command::new("umount");
+    // Datastores are only unmounted after a check for running jobs, '-l' needed due to some weird
+    // bug where some tokio-threads keep the .lock-file open (sometimes)
+    command.args(&["-l"]);
+    command.arg(mount_point);
+
+    crate::tools::run_command(command, None)?;
+    Ok(())
+}
-- 
2.30.2

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

[Dominik Csapak]

comments inline

On 8/30/21 13:14, Hannes Laimer wrote:
> adds:
>   - get_fs_uuid_by_disk_path: taken out of the already existing
>      get_fs_uuid function
>   - get_mount_point_by_uuid: returns the mount-point of a disk by a given
>      uuid, fails if the disk is not mounted
>   - get_fs_uuid_by_path: finds the uuid of the disk that the given path
>      is on, can fail if there is no uuid for the disk
>   - mount_by_uuid: mounts a disk identified by uuid to a given path
>   - unmount_by_mount_point: basically linux 'umount -l', justification
>      for the lazy option in code
> ---
>   src/tools/disks/mod.rs | 53 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 53 insertions(+)
> 
> diff --git a/src/tools/disks/mod.rs b/src/tools/disks/mod.rs
> index af7f7fe1..ec4d8742 100644
> --- a/src/tools/disks/mod.rs
> +++ b/src/tools/disks/mod.rs
> @@ -1009,6 +1009,22 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
>           None => bail!("disk {:?} has no node in /dev", disk.syspath()),
>       };
>   
> +    get_fs_uuid_by_disk_path(&disk_path)
> +}
> +
> +/// Get the FS UUID of the disk that the given path is on.
> +pub fn get_fs_uuid_by_path(path: &Path) -> Result<String, Error> {
> +    let mut command = std::process::Command::new("df");
> +    command.args(&["--output=source"]);
> +    command.arg(path);
> +
> +    let output = crate::tools::run_command(command, None)?;

AFAICS, we already have that information in
DiskManage::find_mounted_device ?

would it not make sense to add the uuid information there?

this would save us a fork

> +
> +    get_fs_uuid_by_disk_path(Path::new(output.lines().skip(1).next().unwrap()))

although it seems unlikely to make a problem (for now), please do
not use unwrap in the middle of code. when df sometime decides to change
output (or some other unpredictable thing happens) this will panic, and
depending where that code is executed, it can have bad effects on the
rest of the code (e.g. when holding a mutex)

instead use e.g. '.ok_or_else' or use something like
if let Some(foo) = output....next() { ... } else { bail!(...) }

> +}
> +
> +/// Get the FS UUID of a disk with a given disk path.
> +pub fn get_fs_uuid_by_disk_path(disk_path: &Path) -> Result<String, Error> {
>       let mut command = std::process::Command::new("blkid");
>       command.args(&["-o", "export"]);
>       command.arg(disk_path);
> @@ -1023,3 +1039,40 @@ pub fn get_fs_uuid(disk: &Disk) -> Result<String, Error> {
>   
>       bail!("get_fs_uuid failed - missing UUID");
>   }
> +
> +/// Get mount point by UUID
> +pub fn get_mount_point_by_uuid(uuid: &str) -> Result<String, Error> {
> +    let mut command = std::process::Command::new("findmnt");
> +    command.args(&["-rn", "-oTARGET", "-S"]);
> +    command.arg(&format!("UUID={}", uuid));
> +
> +    let output = crate::tools::run_command(command, None)?;
> +
> +    if !output.is_empty() {
> +        return Ok(String::from(output.trim()));
> +    }
> +
> +    bail!("get_mount_point_by_uuid failed - device with uuid: {} is not mounted", 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);
> +
> +    crate::tools::run_command(command, None)?;
> +    Ok(())
> +}
> +
> +/// Unmount a disk by its mount point.
> +pub fn unmount_by_mount_point(mount_point: &Path) -> Result<(), Error> {
> +    let mut command = std::process::Command::new("umount");
> +    // Datastores are only unmounted after a check for running jobs, '-l' needed due to some weird
> +    // bug where some tokio-threads keep the .lock-file open (sometimes)
> +    command.args(&["-l"]);
> +    command.arg(mount_point);
> +
> +    crate::tools::run_command(command, None)?;
> +    Ok(())
> +}
> 

[pbs-devel] [PATCH v2 proxmox-backup 02/15] config: add uuid+mountpoint to DataStoreConfig

[Hannes Laimer]

Expand the DataStoreConfig to also save the uuid and the
mount-point for removable datastores, a datastore is removable iff both
are set.
---
 src/config/datastore.rs | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/config/datastore.rs b/src/config/datastore.rs
index cfa03547..b225b20f 100644
--- a/src/config/datastore.rs
+++ b/src/config/datastore.rs
@@ -80,6 +80,16 @@ pub const DIR_NAME_SCHEMA: Schema = StringSchema::new("Directory name").schema()
             optional: true,
             type: bool,
         },
+        "backing-device": {
+            description: "The UUID of the device, iff the datastore is removable.",
+            optional: true,
+            type: String,
+        },
+        "backing-device-mount-point": {
+            description: "The mount point of the device, iff the datastore is removable.",
+            optional: true,
+            type: String,
+        },
     }
 )]
 #[derive(Serialize,Deserialize,Updater)]
@@ -117,6 +127,12 @@ pub struct DataStoreConfig {
     /// Send notification only for job errors
     #[serde(skip_serializing_if="Option::is_none")]
     pub notify: Option<String>,
+    /// The UUID of the device(iff removable)
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub backing_device: Option<String>,
+    /// The mount point of the device(iff removable)
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub backing_device_mount_point: Option<String>,
 }
 
 fn init() -> SectionConfig {
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 03/15] api2: add support for removable datastore creation

[Hannes Laimer]

---
 src/api2/config/datastore.rs | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index b0a5b0d2..742a298f 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -22,6 +22,7 @@ use crate::config::cached_user_info::CachedUserInfo;
 use crate::config::datastore::{self, DataStoreConfig, DataStoreConfigUpdater};
 use crate::config::acl::{PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY};
 use crate::server::{jobstate, WorkerTask};
+use crate::tools::disks::{get_fs_uuid_by_path, get_mount_point_by_uuid};
 
 #[api(
     input: {
@@ -83,6 +84,12 @@ pub(crate) fn do_create_datastore(
     protected: true,
     input: {
         properties: {
+            "removable": {
+                description: "The device to which the data is written is removable.",
+                type: bool,
+                optional: true,
+                default: false,
+            },
             config: {
                 type: DataStoreConfig,
                 flatten: true,
@@ -95,7 +102,8 @@ pub(crate) fn do_create_datastore(
 )]
 /// Create new datastore config.
 pub fn create_datastore(
-    config: DataStoreConfig,
+    mut config: DataStoreConfig,
+    removable: bool,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
 
@@ -107,6 +115,18 @@ pub fn create_datastore(
         bail!("datastore '{}' already exists.", config.name);
     }
 
+    if removable {
+        let path = std::path::Path::new(&config.path);
+        std::fs::create_dir_all(&path)?;
+        let uuid = get_fs_uuid_by_path(&path)?;
+        let mount_path = get_mount_point_by_uuid(&uuid)?;
+        if mount_path == "/" {
+            bail!("The device for a removable datastore cannot have '/' as its mountpoint.");
+        }
+        config.backing_device_mount_point = Some(mount_path);
+        config.backing_device = Some(uuid);
+    };
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     WorkerTask::new_thread(
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 04/15] backup: add check_if_available function to ds

[Hannes Laimer]

Makes sure a removable datastore is available, meaning it is plugged in
and mounted correctly. For not removable datastores the function will
never result in an Error, since they're always available.
---
 src/backup/datastore.rs | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 848459e8..ef3400d7 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -52,6 +52,24 @@ pub fn check_backup_owner(
     Ok(())
 }
 
+pub fn check_if_available(config: &datastore::DataStoreConfig) -> Result<(), Error> {
+    if let (Some(uuid), Some(config_mount_point)) = (
+        config.backing_device.clone(),
+        config.backing_device_mount_point.clone(),
+    ) {
+        match tools::disks::get_mount_point_by_uuid(&uuid) {
+            Ok(mount_point) if mount_point.eq(&config_mount_point) => Ok(()),
+            _ => Err(format_err!(
+                "The removale device for datastore '{}' has to be mounted at '{}'!",
+                config.name,
+                config_mount_point
+            )),
+        }
+    } else {
+        Ok(())
+    }
+}
+
 /// Datastore Management
 ///
 /// A Datastore can store severals backups, and provides the
@@ -69,6 +87,9 @@ impl DataStore {
 
         let (config, _digest) = datastore::config()?;
         let config: datastore::DataStoreConfig = config.lookup("datastore", name)?;
+
+        check_if_available(&config)?;
+
         let path = PathBuf::from(&config.path);
 
         let mut map = DATASTORE_MAP.lock().unwrap();
@@ -103,6 +124,8 @@ impl DataStore {
     }
 
     fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
+        check_if_available(&config)?;
+
         let chunk_store = ChunkStore::open(store_name, path)?;
 
         let mut gc_status_path = chunk_store.base_path();
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 05/15] api2: add 'is_available' to DataStoreConfig

[Hannes Laimer]

---
 src/api2/admin/datastore.rs | 14 ++++++++++++++
 src/api2/status.rs          | 19 ++++++++++++++++++-
 src/api2/types/mod.rs       |  2 ++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index f3c52413..16559678 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -606,6 +606,19 @@ pub fn status(
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<DataStoreStatus, Error> {
+    let (config, _digest) = datastore::config()?;
+    let store_config = config.lookup("datastore", &store)?;
+    if !check_if_available(&store_config).is_ok() {
+        return Ok(DataStoreStatus {
+            total: 0,
+            used: 0,
+            avail: 0,
+            is_available: false,
+            gc_status: None,
+            counts: None,
+        });
+    }
+
     let datastore = DataStore::lookup_datastore(&store)?;
     let storage = crate::tools::disks::disk_usage(&datastore.base_path())?;
     let (counts, gc_status) = if verbose {
@@ -631,6 +644,7 @@ pub fn status(
         total: storage.total,
         used: storage.used,
         avail: storage.avail,
+        is_available: true,
         gc_status,
         counts,
     })
diff --git a/src/api2/status.rs b/src/api2/status.rs
index 3aff91e7..06990743 100644
--- a/src/api2/status.rs
+++ b/src/api2/status.rs
@@ -21,7 +21,7 @@ use crate::api2::types::{
     Authid,
 };
 
-use crate::backup::DataStore;
+use crate::backup::{check_if_available, DataStore};
 use crate::config::datastore;
 use crate::tools::statistics::{linear_regression};
 use crate::config::cached_user_info::CachedUserInfo;
@@ -53,6 +53,10 @@ use crate::config::acl::{
                     type: Integer,
                     description: "The available bytes of the underlying storage",
                 },
+                "is-available": {
+                    type: bool,
+                    description: "The datastore is available, relevent if it is removable",
+                },
                 history: {
                     type: Array,
                     optional: true,
@@ -103,6 +107,18 @@ pub fn datastore_status(
             continue;
         }
 
+        let store_config = config.lookup("datastore", &store)?;
+        if !check_if_available(&store_config).is_ok() {
+            list.push(json!({
+                "store": store,
+                "total": -1,
+                "used": -1,
+                "avail": -1,
+                "is-available": false,
+            }));
+            continue;
+        }
+
         let datastore = match DataStore::lookup_datastore(&store) {
             Ok(datastore) => datastore,
             Err(err) => {
@@ -123,6 +139,7 @@ pub fn datastore_status(
             "total": status.total,
             "used": status.used,
             "avail": status.avail,
+            "is-available": true,
             "gc-status": datastore.last_gc_status(),
         });
 
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index bd3c7ac5..4d9ed691 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -371,6 +371,8 @@ pub struct DataStoreStatus {
     pub used: u64,
     /// Available space (bytes).
     pub avail: u64,
+    /// Datastore is available, relevant if it is removable
+    pub is_available: bool,
     /// Status of last GC
     #[serde(skip_serializing_if="Option::is_none")]
     pub gc_status: Option<GarbageCollectionStatus>,
-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 05/15] api2: add 'is_available' to DataStoreConfig

[Dominik Csapak]

comment inline:

On 8/30/21 13:14, Hannes Laimer wrote:
> ---
>   src/api2/admin/datastore.rs | 14 ++++++++++++++
>   src/api2/status.rs          | 19 ++++++++++++++++++-
>   src/api2/types/mod.rs       |  2 ++
>   3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index f3c52413..16559678 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -606,6 +606,19 @@ pub fn status(
>       _info: &ApiMethod,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<DataStoreStatus, Error> {
> +    let (config, _digest) = datastore::config()?;
> +    let store_config = config.lookup("datastore", &store)?;
> +    if !check_if_available(&store_config).is_ok() {
> +        return Ok(DataStoreStatus {
> +            total: 0,
> +            used: 0,
> +            avail: 0,
> +            is_available: false,
> +            gc_status: None,
> +            counts: None,
> +        });
> +    }
> +
>       let datastore = DataStore::lookup_datastore(&store)?;
>       let storage = crate::tools::disks::disk_usage(&datastore.base_path())?;
>       let (counts, gc_status) = if verbose {
> @@ -631,6 +644,7 @@ pub fn status(
>           total: storage.total,
>           used: storage.used,
>           avail: storage.avail,
> +        is_available: true,
>           gc_status,
>           counts,
>       })
> diff --git a/src/api2/status.rs b/src/api2/status.rs
> index 3aff91e7..06990743 100644
> --- a/src/api2/status.rs
> +++ b/src/api2/status.rs
> @@ -21,7 +21,7 @@ use crate::api2::types::{
>       Authid,
>   };
>   
> -use crate::backup::DataStore;
> +use crate::backup::{check_if_available, DataStore};
>   use crate::config::datastore;
>   use crate::tools::statistics::{linear_regression};
>   use crate::config::cached_user_info::CachedUserInfo;
> @@ -53,6 +53,10 @@ use crate::config::acl::{
>                       type: Integer,
>                       description: "The available bytes of the underlying storage",
>                   },
> +                "is-available": {
> +                    type: bool,
> +                    description: "The datastore is available, relevent if it is removable",
> +                },
>                   history: {
>                       type: Array,
>                       optional: true,
> @@ -103,6 +107,18 @@ pub fn datastore_status(
>               continue;
>           }
>   
> +        let store_config = config.lookup("datastore", &store)?;
> +        if !check_if_available(&store_config).is_ok() {
> +            list.push(json!({
> +                "store": store,
> +                "total": -1,
> +                "used": -1,
> +                "avail": -1,
> +                "is-available": false,
> +            }));

why -1 here, but 0 at the top?
the api users should be able to see that its not available by the:
'is-available' flag anyway

> +            continue;
> +        }
> +
>           let datastore = match DataStore::lookup_datastore(&store) {
>               Ok(datastore) => datastore,
>               Err(err) => {
> @@ -123,6 +139,7 @@ pub fn datastore_status(
>               "total": status.total,
>               "used": status.used,
>               "avail": status.avail,
> +            "is-available": true,
>               "gc-status": datastore.last_gc_status(),
>           });
>   
> diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
> index bd3c7ac5..4d9ed691 100644
> --- a/src/api2/types/mod.rs
> +++ b/src/api2/types/mod.rs
> @@ -371,6 +371,8 @@ pub struct DataStoreStatus {
>       pub used: u64,
>       /// Available space (bytes).
>       pub avail: u64,
> +    /// Datastore is available, relevant if it is removable
> +    pub is_available: bool,
>       /// Status of last GC
>       #[serde(skip_serializing_if="Option::is_none")]
>       pub gc_status: Option<GarbageCollectionStatus>,
> 

[pbs-devel] [PATCH v2 proxmox-backup 06/15] api2: add 'removable' to DataStoreListItem

[Hannes Laimer]

---
 pbs-api-types/src/lib.rs    | 7 +++++++
 src/api2/admin/datastore.rs | 1 +
 2 files changed, 8 insertions(+)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 576099eb..2965a11a 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -437,6 +437,12 @@ pub struct GroupListItem {
             optional: true,
             schema: SINGLE_LINE_COMMENT_SCHEMA,
         },
+        removable: {
+            description: "The datastore is marked as removable.",
+            type: bool,
+            optional: true,
+            default: false,
+        },
     },
 )]
 #[derive(Serialize, Deserialize)]
@@ -445,6 +451,7 @@ pub struct GroupListItem {
 pub struct DataStoreListItem {
     pub store: String,
     pub comment: Option<String>,
+    pub removable: bool,
 }
 
 #[api(
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 16559678..b236dfab 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1089,6 +1089,7 @@ pub fn get_datastore_list(
                 DataStoreListItem {
                     store: store.clone(),
                     comment: data["comment"].as_str().map(String::from),
+                    removable: data["backing-device"] != Value::Null,
                 }
             );
         }
-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 06/15] api2: add 'removable' to DataStoreListItem

[Dominik Csapak]

On 8/30/21 13:14, Hannes Laimer wrote:
> ---
>   pbs-api-types/src/lib.rs    | 7 +++++++
>   src/api2/admin/datastore.rs | 1 +
>   2 files changed, 8 insertions(+)
> 
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 576099eb..2965a11a 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -437,6 +437,12 @@ pub struct GroupListItem {
>               optional: true,
>               schema: SINGLE_LINE_COMMENT_SCHEMA,
>           },
> +        removable: {
> +            description: "The datastore is marked as removable.",
> +            type: bool,
> +            optional: true,
> +            default: false,
> +        },
>       },
>   )]
>   #[derive(Serialize, Deserialize)]
> @@ -445,6 +451,7 @@ pub struct GroupListItem {
>   pub struct DataStoreListItem {
>       pub store: String,
>       pub comment: Option<String>,
> +    pub removable: bool,
>   }
>   
>   #[api(
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 16559678..b236dfab 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1089,6 +1089,7 @@ pub fn get_datastore_list(
>                   DataStoreListItem {
>                       store: store.clone(),
>                       comment: data["comment"].as_str().map(String::from),
> +                    removable: data["backing-device"] != Value::Null,

isn't that check wrong? it is only removable if we have a
backing-device *and* as backing-device-mount-point?

>                   }
>               );
>           }
> 

[pbs-devel] [PATCH v2 proxmox-backup 07/15] api2: add (un)mount endpoint for removable ds's

[Hannes Laimer]

Add endpoints for mounting and unmounting removable datastores by their
name.
---
 src/api2/admin/datastore.rs | 144 ++++++++++++++++++++++++++++++++++++
 1 file changed, 144 insertions(+)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index b236dfab..f6adc663 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -39,6 +39,7 @@ use crate::config::datastore;
 use crate::config::cached_user_info::CachedUserInfo;
 
 use crate::server::{jobstate::Job, WorkerTask};
+use crate::tools::disks::{mount_by_uuid, unmount_by_mount_point};
 
 use crate::config::acl::{
     PRIV_DATASTORE_AUDIT,
@@ -1859,6 +1860,139 @@ pub fn set_backup_owner(
     Ok(())
 }
 
+pub fn do_mount(store: String, auth_id: &Authid) -> Result<String, Error> {
+    let (config, _digest) = datastore::config()?;
+    let store_config = config.lookup("datastore", &store)?;
+
+    let upid_str = WorkerTask::new_thread(
+        "mount",
+        Some(store.clone()),
+        auth_id.clone(),
+        false,
+        move |_worker| {
+            if check_if_available(&store_config).is_ok() {
+                bail!(
+                    "Datastore '{}' can't be mounted because it is already available.",
+                    &store_config.name
+                );
+            };
+            if let (Some(uuid), Some(mount_point)) = (
+                store_config.backing_device,
+                store_config.backing_device_mount_point,
+            ) {
+                let mount_point_path = std::path::Path::new(&mount_point);
+                mount_by_uuid(&uuid, &mount_point_path)
+            } else {
+                bail!(
+                    "Datastore '{}' can't be mounted because it is not removable.",
+                    &store_config.name
+                );
+            }
+        },
+    )?;
+
+    Ok(upid_str)
+}
+
+#[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, mut rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let (_config, digest) = datastore::config()?;
+
+    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
+
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+
+    do_mount(store, &auth_id).map(|upid_str| json!(upid_str))
+}
+
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+        },
+    },
+    returns: {
+        schema: UPID_SCHEMA,
+    },
+    access: {
+        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),
+    },
+)]
+/// Unmount removable datastore.
+pub fn unmount(store: String, mut rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
+    let (config, digest) = datastore::config()?;
+
+    let store_config = config.lookup("datastore", &store)?;
+    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
+
+    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+
+    let upid_str = WorkerTask::new_thread(
+        "unmount",
+        Some(store.clone()),
+        auth_id.clone(),
+        to_stdout,
+        move |_worker| {
+            if !check_if_available(&store_config).is_ok() {
+                bail!(
+                    "Datastore '{}' can't be unmounted because it is not available.",
+                    &store_config.name
+                );
+            };
+            if let (Some(_uuid), Some(mount_point)) = (
+                store_config.backing_device,
+                store_config.backing_device_mount_point,
+            ) {
+                let mount_point_path = std::path::Path::new(&mount_point);
+                for job in crate::server::TaskListInfoIterator::new(true)? {
+                    let job = match job {
+                        Ok(job) => job,
+                        Err(_) => break,
+                    };
+
+                    if !job.upid.worker_type.eq("unmount")
+                        && job.upid.worker_id.map_or(false, |id| id.contains(&store))
+                    {
+                        bail!(
+                            "Can't unmount '{}' because there is a '{}' job still running!",
+                            &store_config.name,
+                            job.upid.worker_type
+                        );
+                    }
+                }
+                unmount_by_mount_point(&mount_point_path)
+            } else {
+                bail!(
+                    "Datastore '{}' can't be mounted because it is not removable.",
+                    &store_config.name
+                );
+            }
+        },
+    )?;
+
+    Ok(json!(upid_str))
+}
+
 #[sortable]
 const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
     (
@@ -1904,6 +2038,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
             .get(&API_METHOD_LIST_GROUPS)
             .delete(&API_METHOD_DELETE_GROUP)
     ),
+    (
+        "mount",
+        &Router::new()
+            .post(&API_METHOD_MOUNT)
+    ),
     (
         "notes",
         &Router::new()
@@ -1941,6 +2080,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
         &Router::new()
             .get(&API_METHOD_STATUS)
     ),
+    (
+        "unmount",
+        &Router::new()
+            .post(&API_METHOD_UNMOUNT)
+    ),
     (
         "upload-backup-log",
         &Router::new()
-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 07/15] api2: add (un)mount endpoint for removable ds's

[Dominik Csapak]

Comments inline

On 8/30/21 13:14, Hannes Laimer wrote:
> Add endpoints for mounting and unmounting removable datastores by their
> name.
> ---
>   src/api2/admin/datastore.rs | 144 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 144 insertions(+)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index b236dfab..f6adc663 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -39,6 +39,7 @@ use crate::config::datastore;
>   use crate::config::cached_user_info::CachedUserInfo;
>   
>   use crate::server::{jobstate::Job, WorkerTask};
> +use crate::tools::disks::{mount_by_uuid, unmount_by_mount_point};
>   
>   use crate::config::acl::{
>       PRIV_DATASTORE_AUDIT,
> @@ -1859,6 +1860,139 @@ pub fn set_backup_owner(
>       Ok(())
>   }
>   
> +pub fn do_mount(store: String, auth_id: &Authid) -> Result<String, Error> {
> +    let (config, _digest) = datastore::config()?;
> +    let store_config = config.lookup("datastore", &store)?;
> +
> +    let upid_str = WorkerTask::new_thread(
> +        "mount",
> +        Some(store.clone()),
> +        auth_id.clone(),
> +        false,
> +        move |_worker| {
> +            if check_if_available(&store_config).is_ok() {
> +                bail!(
> +                    "Datastore '{}' can't be mounted because it is already available.",
> +                    &store_config.name
> +                );
> +            };
> +            if let (Some(uuid), Some(mount_point)) = (
> +                store_config.backing_device,
> +                store_config.backing_device_mount_point,
> +            ) {
> +                let mount_point_path = std::path::Path::new(&mount_point);
> +                mount_by_uuid(&uuid, &mount_point_path)
> +            } else {
> +                bail!(
> +                    "Datastore '{}' can't be mounted because it is not removable.",
> +                    &store_config.name
> +                );
> +            }
> +        },
> +    )?;
> +
> +    Ok(upid_str)
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +        }
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),

that permission looks wrong, maybe _MODIFY instead ?

> +    },
> +)]
> +/// Mount removable datastore.
> +pub fn mount(store: String, mut rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> +    let (_config, digest) = datastore::config()?;
> +
> +    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +
> +    do_mount(store, &auth_id).map(|upid_str| json!(upid_str))
> +}
> +
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +        },
> +    },
> +    returns: {
> +        schema: UPID_SCHEMA,
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT, false),

same here

> +    },
> +)]
> +/// Unmount removable datastore.
> +pub fn unmount(store: String, mut rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
> +    let (config, digest) = datastore::config()?;
> +
> +    let store_config = config.lookup("datastore", &store)?;
> +    rpcenv["digest"] = proxmox::tools::digest_to_hex(&digest).into();
> +
> +    let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +
> +    let upid_str = WorkerTask::new_thread(
> +        "unmount",
> +        Some(store.clone()),
> +        auth_id.clone(),
> +        to_stdout,
> +        move |_worker| {
> +            if !check_if_available(&store_config).is_ok() {
> +                bail!(
> +                    "Datastore '{}' can't be unmounted because it is not available.",
> +                    &store_config.name
> +                );
> +            };
> +            if let (Some(_uuid), Some(mount_point)) = (
> +                store_config.backing_device,
> +                store_config.backing_device_mount_point,
> +            ) {
> +                let mount_point_path = std::path::Path::new(&mount_point);
> +                for job in crate::server::TaskListInfoIterator::new(true)? {
> +                    let job = match job {
> +                        Ok(job) => job,
> +                        Err(_) => break,
> +                    };
> +
> +                    if !job.upid.worker_type.eq("unmount")
> +                        && job.upid.worker_id.map_or(false, |id| id.contains(&store))

i know its what we do for the task filtering, but i don't think its the
best approach for this

if my datastore name is 'host', i'll not be able to unmount it during
any completely unrelated backup of a 'host'

sadly i dont have a good alternate solution at hand
that can handle old daemons....
so we likely will have to go with this for now... but i would want it at
least mark it with a FIXME or TODO so we see that it should be done
right...

but, since we unmount lazy anyway, we should wait here until its really 
unmounted, else we might give users a wrong feeling of safety to remove
the drive, when in reality its still writing?

if we do that, we can toss the job checking code completely, unmount
it always lazy and wait until finished?


> +                    {
> +                        bail!(
> +                            "Can't unmount '{}' because there is a '{}' job still running!",
> +                            &store_config.name,
> +                            job.upid.worker_type
> +                        );
> +                    }
> +                }
> +                unmount_by_mount_point(&mount_point_path)
> +            } else {
> +                bail!(
> +                    "Datastore '{}' can't be mounted because it is not removable.",
> +                    &store_config.name
> +                );
> +            }
> +        },
> +    )?;
> +
> +    Ok(json!(upid_str))
> +}
> +
>   #[sortable]
>   const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>       (
> @@ -1904,6 +2038,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>               .get(&API_METHOD_LIST_GROUPS)
>               .delete(&API_METHOD_DELETE_GROUP)
>       ),
> +    (
> +        "mount",
> +        &Router::new()
> +            .post(&API_METHOD_MOUNT)
> +    ),
>       (
>           "notes",
>           &Router::new()
> @@ -1941,6 +2080,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>           &Router::new()
>               .get(&API_METHOD_STATUS)
>       ),
> +    (
> +        "unmount",
> +        &Router::new()
> +            .post(&API_METHOD_UNMOUNT)
> +    ),
>       (
>           "upload-backup-log",
>           &Router::new()
> 

[pbs-devel] [PATCH v2 proxmox-backup 08/15] pbs: add mount-removable command to commandSocket

[Hannes Laimer]

Adds 'mount-removable' command to superuser commandSocket, this command
is needed for automatic mountntig of removable datastores. When udev
rules are executed, the process and whatever is forked from it will be
killed almost immediately, therefore the mounting has to done
asynchronously.
---
 src/backup/mod.rs             |  2 +-
 src/bin/proxmox-backup-api.rs | 27 +++++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/src/backup/mod.rs b/src/backup/mod.rs
index c0ab041c..1c489471 100644
--- a/src/backup/mod.rs
+++ b/src/backup/mod.rs
@@ -86,7 +86,7 @@ pub use pbs_datastore::read_chunk::*;
 mod read_chunk;
 pub use read_chunk::*;
 
-mod datastore;
+pub mod datastore;
 pub use datastore::*;
 
 mod verify;
diff --git a/src/bin/proxmox-backup-api.rs b/src/bin/proxmox-backup-api.rs
index 75104205..fa6b6398 100644
--- a/src/bin/proxmox-backup-api.rs
+++ b/src/bin/proxmox-backup-api.rs
@@ -6,6 +6,7 @@ use proxmox::api::RpcEnvironmentType;
 
 use pbs_tools::auth::private_auth_key;
 
+use proxmox_backup::api2::types::Authid;
 use proxmox_backup::server::{
     self,
     auth::default_api_auth,
@@ -66,6 +67,32 @@ async fn run() -> Result<(), Error> {
 
     let mut commando_sock = server::CommandoSocket::new(server::our_ctrl_sock());
 
+    commando_sock.register_command("mount-removable".to_string(), |value| {
+        if let Some(serde_json::Value::String(uuid)) = value {
+            let (config, _digest) = proxmox_backup::config::datastore::config()?;
+            if let Some(store) = &config
+                .sections
+                .iter()
+                .filter_map(|(store, (_, _))| {
+                    config
+                        .lookup::<proxmox_backup::config::datastore::DataStoreConfig>(
+                            "datastore",
+                            &store,
+                        )
+                        .map_or(None, |config| match config.backing_device {
+                            Some(ref config_uuid) if config_uuid.eq(uuid) => Some(config.name),
+                            _ => None,
+                        })
+                })
+                .next()
+            {
+                let auth_id = Authid::root_auth_id().clone();
+                proxmox_backup::api2::admin::datastore::do_mount(String::from(store), &auth_id)?;
+            }
+        };
+        Ok(serde_json::Value::Null)
+    })?;
+
     config.enable_file_log(pbs_buildcfg::API_ACCESS_LOG_FN, &mut commando_sock)?;
 
     let rest_server = RestServer::new(config);
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 09/15] pbs-manager: add 'send-command' command

[Hannes Laimer]

Relays a command to the superuser commandSocket, this is necessary
because the path to the socket is prefixed with a '\0' and can therefore
not really be addressed from outside the rust code.
---
 src/bin/proxmox-backup-manager.rs | 41 +++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index 93d6de57..a07f69f1 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -197,6 +197,43 @@ async fn task_stop(param: Value) -> Result<Value, Error> {
     Ok(Value::Null)
 }
 
+#[api(
+    input: {
+        properties: {
+            command: {
+                description: "The command.",
+                type: String,
+            },
+            args: {
+                description: "The argument string.",
+                optional: true,
+                type: String,
+            }
+        }
+    }
+)]
+/// Send command to control socket.
+async fn send_command(param: Value) -> Result<Value, Error> {
+    let command_str = required_string_param(&param, "command")?;
+    let args = match required_string_param(&param, "args") {
+        Ok(args) => args,
+        Err(_) => "",
+    };
+
+    let api_pid = proxmox_backup::server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_API_PID_FN)?;
+    let sock = proxmox_backup::server::ctrl_sock_from_pid(api_pid);
+    proxmox_backup::server::send_raw_command(
+        sock,
+        &format!(
+            "{{\"command\":\"{}\",\"args\":\"{}\"}}\n",
+            command_str, args
+        ),
+    )
+    .await?;
+
+    Ok(Value::Null)
+}
+
 fn task_mgmt_cli() -> CommandLineInterface {
 
     let task_log_cmd_def = CliCommand::new(&API_METHOD_TASK_LOG)
@@ -392,6 +429,10 @@ fn main() {
         .insert("report",
             CliCommand::new(&API_METHOD_REPORT)
         )
+        .insert(
+            "send-command",
+            CliCommand::new(&API_METHOD_SEND_COMMAND).arg_param(&["command"]),
+        )
         .insert("versions",
             CliCommand::new(&API_METHOD_GET_VERSIONS)
         );
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 10/15] debian: add udev rule for removable datastores

[Hannes Laimer]

Adds an udev rules that triggers the mounting of a removable datastore
if the corresponding device is plugged in.
---
 debian/proxmox-backup-server.udev | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/debian/proxmox-backup-server.udev b/debian/proxmox-backup-server.udev
index afdfb2bc..23b93464 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 datastore if the corresponding device is plugged in
+ACTION=="add", SUBSYSTEM=="block", RUN+="/usr/sbin/proxmox-backup-manager send-command mount-removable --args $env{ID_FS_UUID}"
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 11/15] ui: show usb icon for removable datastore in list

[Hannes Laimer]

---
 www/NavigationTree.js | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/www/NavigationTree.js b/www/NavigationTree.js
index 6035526c..dc07e3bb 100644
--- a/www/NavigationTree.js
+++ b/www/NavigationTree.js
@@ -218,6 +218,7 @@ Ext.define('PBS.view.main.NavigationTree', {
 	    let existingChildren = {};
 	    for (let i = 0, j = 0, length = records.length; i < length; i++) {
 		let name = records[i].id;
+		let removable = records[i].data.removable;
 		existingChildren[name] = true;
 
 		while (name.localeCompare(getChildTextAt(j)) > 0 && (j+1) < list.childNodes.length) {
@@ -228,7 +229,7 @@ Ext.define('PBS.view.main.NavigationTree', {
 		    list.insertChild(j, {
 			text: name,
 			path: `DataStore-${name}`,
-			iconCls: 'fa fa-database',
+			iconCls: `fa fa-${removable ? 'usb' : 'database'}`,
 			leaf: true,
 		    });
 		}
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 12/15] ui: add 'removable' checkbox in datastore creation

[Hannes Laimer]

---
 www/window/DataStoreEdit.js | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/www/window/DataStoreEdit.js b/www/window/DataStoreEdit.js
index ed23ad11..4cede135 100644
--- a/www/window/DataStoreEdit.js
+++ b/www/window/DataStoreEdit.js
@@ -142,6 +142,11 @@ Ext.define('PBS.DataStoreEdit', {
 			fieldLabel: gettext('Backing Path'),
 			emptyText: gettext('An absolute path'),
 		    },
+		    {
+			xtype: 'proxmoxcheckbox',
+			name: 'removable',
+			fieldLabel: gettext('Removable'),
+		    },
 		],
 		column2: [
 		    {
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 13/15] ui: display row as disabled in ds statistics

[Hannes Laimer]

Show row in datastore statistics list as disable, if the datastore is
not available.
---
 www/dashboard/DataStoreStatistics.js | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/www/dashboard/DataStoreStatistics.js b/www/dashboard/DataStoreStatistics.js
index 544f40da..29d6e43d 100644
--- a/www/dashboard/DataStoreStatistics.js
+++ b/www/dashboard/DataStoreStatistics.js
@@ -68,6 +68,9 @@ Ext.define('PBS.DatastoreStatistics', {
 	    sortable: true,
 	    renderer: (value, metaData, record, rowIndex, colIndex, store) => {
 		let err = record.get('error');
+		if (!record.get('is-available')) {
+		    metaData.tdCls = 'proxmox-disabled-row';
+		}
 		if (err) {
 		    metaData.tdAttr = `data-qtip="${Ext.htmlEncode(err)}"`;
 		    metaData.tdCls = 'proxmox-invalid-row';
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 14/15] ui: show backing device UUID and mount-point in option tab

[Hannes Laimer]

---
 www/datastore/OptionView.js | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 5a5e85be..241cb12b 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -111,5 +111,11 @@ Ext.define('PBS.Datastore.Options', {
 		},
 	    },
 	},
+	"backing-device": {
+	    header: "Backing device UUID",
+	},
+	"backing-device-mount-point": {
+	    header: "Backing device mount point",
+	},
     },
 });
-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 15/15] ui: add (un)mount button to summary

[Hannes Laimer]

And only try to load datastore information if the datastore is
available.
---
 www/datastore/Summary.js | 77 +++++++++++++++++++++++++++++++++++++---
 1 file changed, 73 insertions(+), 4 deletions(-)

diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index 628f0c60..1c5f41d1 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -185,8 +185,6 @@ Ext.define('PBS.DataStoreSummary', {
 	padding: 5,
     },
 
-    tbar: ['->', { xtype: 'proxmoxRRDTypeSelector' }],
-
     items: [
 	{
 	    xtype: 'container',
@@ -257,7 +255,63 @@ 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) => {
+	    let available = s.getById('is-available').data.value;
+	    unmountBtn.setDisabled(!available);
+	    mountBtn.setDisabled(available);
+	    if (available) {
+		me.down('pbsDataStoreInfo').fireEvent('activate');
+	    } else {
+		me.down('pbsDataStoreInfo').fireEvent('deactivate');
+	    }
+	});
 
 	let sp = Ext.state.Manager.getProvider();
 	me.mon(sp, 'statechange', function(provider, key, value) {
@@ -267,11 +321,18 @@ 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);
+		let removable = Object.prototype.hasOwnProperty.call(data, "backing-device") &&
+		    Object.prototype.hasOwnProperty.call(data, "backing-device-mount-point");
+		unmountBtn.setHidden(!removable);
+		mountBtn.setHidden(!removable);
 		me.down('pbsDataStoreInfo').setTitle(`${me.datastore} (${path})`);
 		me.down('pbsDataStoreNotes').setNotes(response.result.data.comment);
 	    },
@@ -285,6 +346,14 @@ Ext.define('PBS.DataStoreSummary', {
 	    },
 	});
 
+	me.on('afterrender', () => {
+	    me.statusStore.startUpdate();
+	});
+
+	me.on('destroy', () => {
+	    me.statusStore.stopUpdate();
+	});
+
 	me.query('proxmoxRRDChart').forEach((chart) => {
 	    chart.setStore(me.rrdstore);
 	});
-- 
2.30.2

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

[Dominik Csapak]

comments inline

On 8/30/21 13:15, Hannes Laimer wrote:
> And only try to load datastore information if the datastore is
> available.
> ---
>   www/datastore/Summary.js | 77 +++++++++++++++++++++++++++++++++++++---
>   1 file changed, 73 insertions(+), 4 deletions(-)
> 
> diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
> index 628f0c60..1c5f41d1 100644
> --- a/www/datastore/Summary.js
> +++ b/www/datastore/Summary.js
> @@ -185,8 +185,6 @@ Ext.define('PBS.DataStoreSummary', {
>   	padding: 5,
>       },
>   
> -    tbar: ['->', { xtype: 'proxmoxRRDTypeSelector' }],
> -
>       items: [
>   	{
>   	    xtype: 'container',
> @@ -257,7 +255,63 @@ 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' }],
> +	});

would speak anything against making the buttons static as well, and
dynamically querying them down below ?
e.g. if you make the class a 'referenceHolder', you can use
references on the buttons and "me.lookup('reference')" to get them

> +
> +	me.mon(me.statusStore, 'load', (s, records, success) => {
> +	    let available = s.getById('is-available').data.value;
> +	    unmountBtn.setDisabled(!available);
> +	    mountBtn.setDisabled(available);
> +	    if (available) {
> +		me.down('pbsDataStoreInfo').fireEvent('activate');
> +	    } else {
> +		me.down('pbsDataStoreInfo').fireEvent('deactivate');
> +	    }
> +	});
>   
>   	let sp = Ext.state.Manager.getProvider();
>   	me.mon(sp, 'statechange', function(provider, key, value) {
> @@ -267,11 +321,18 @@ 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);
> +		let removable = Object.prototype.hasOwnProperty.call(data, "backing-device") &&
> +		    Object.prototype.hasOwnProperty.call(data, "backing-device-mount-point");
> +		unmountBtn.setHidden(!removable);
> +		mountBtn.setHidden(!removable);
>   		me.down('pbsDataStoreInfo').setTitle(`${me.datastore} (${path})`);
>   		me.down('pbsDataStoreNotes').setNotes(response.result.data.comment);
>   	    },
> @@ -285,6 +346,14 @@ Ext.define('PBS.DataStoreSummary', {
>   	    },
>   	});
>   
> +	me.on('afterrender', () => {
> +	    me.statusStore.startUpdate();
> +	});
> +
> +	me.on('destroy', () => {
> +	    me.statusStore.stopUpdate();
> +	});
> +
>   	me.query('proxmoxRRDChart').forEach((chart) => {
>   	    chart.setStore(me.rrdstore);
>   	});
> 

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Dominik Csapak]

High level comment here, rest in the relevant patches:

looks mostly ok, but one things stood out:

why do you add a command for the socket and the
manager 'send command' method? would it not have
been easier to call the mount api directly
from the proxmox-backup-manager?

also, would it not make sense to have
multiple uuids per datastore instead of only one?

i guess most users of this will want to have more than
one usb devices in rotation, and having multiple
datastores + multiple sync jobs is probably
a bit error prone. just allowing multiple
uuids for each datastore would solve that
problem nicely

On 8/30/21 13:14, Hannes Laimer wrote:
> v2(based on Lorenz Stechauner <l.stechauner@proxmox.com>'s feedback):
>   - do not allow creation of removable datastores on a device that
>     is mounted at '/'
>   - ui: also show the mount point in the Options-tab
> 
> Adds the possibility to create removable datastores. A removable
> datastore has a UUID(the device on which the data is stored) and a
> mount-point(where the device will be mounted), iff both are set the
> datastore is removable. Everything else is identical to normal
> datastores. Since config files for jobs, etc. are stored on the server,
> all configuration can be done with the device plugged in or not. Certain
> statistics about the datastore won't be available as long as it is not
> plugged in.
> Removable datastores have to be unmounted before removing, it can only
> be unmounted if not jibs are running.
> Removable datastores are mounted automatically when the device is plugged in, if it has
> been unmounted, it has to be mounted manually through the WebUI or the Api.
> Jobs will not be started if the datastore is not available, and
> depending on the configuration, start when the device is plugged in the
> next time.
> 
> Still todo:
>   - make sync to local datastore more integrated
>   - (add 'when plugged in'-option to job schedule?)
>   - replace linux commands with internal functions in tools/disks, where
>      possible
> 
> Hannes Laimer (15):
>    tools: add disks utility functions
>    config: add uuid+mountpoint to DataStoreConfig
>    api2: add support for removable datastore creation
>    backup: add check_if_available function to ds
>    api2: add 'is_available' to DataStoreConfig
>    api2: add 'removable' to DataStoreListItem
>    api2: add (un)mount endpoint for removable ds's
>    pbs: add mount-removable command to commandSocket
>    pbs-manager: add 'send-command' command
>    debian: add udev rule for removable datastores
>    ui: show usb icon for removable datastore in list
>    ui: add 'removable' checkbox in datastore creation
>    ui: display row as disabled in ds statistics
>    ui: show backing device UUID and mount-point in option tab
>    ui: add (un)mount button to summary
> 
>   debian/proxmox-backup-server.udev    |   3 +
>   pbs-api-types/src/lib.rs             |   7 ++
>   src/api2/admin/datastore.rs          | 159 +++++++++++++++++++++++++++
>   src/api2/config/datastore.rs         |  22 +++-
>   src/api2/status.rs                   |  19 +++-
>   src/api2/types/mod.rs                |   2 +
>   src/backup/datastore.rs              |  23 ++++
>   src/backup/mod.rs                    |   2 +-
>   src/bin/proxmox-backup-api.rs        |  27 +++++
>   src/bin/proxmox-backup-manager.rs    |  41 +++++++
>   src/config/datastore.rs              |  16 +++
>   src/tools/disks/mod.rs               |  53 +++++++++
>   www/NavigationTree.js                |   3 +-
>   www/dashboard/DataStoreStatistics.js |   3 +
>   www/datastore/OptionView.js          |   6 +
>   www/datastore/Summary.js             |  77 ++++++++++++-
>   www/window/DataStoreEdit.js          |   5 +
>   17 files changed, 460 insertions(+), 8 deletions(-)
> 

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Thomas Lamprecht]

On 01.09.21 16:48, Dominik Csapak wrote:
> 
> also, would it not make sense to have
> multiple uuids per datastore instead of only one?
> 
> i guess most users of this will want to have more than
> one usb devices in rotation, and having multiple
> datastores + multiple sync jobs is probably
> a bit error prone. just allowing multiple
> uuids for each datastore would solve that
> problem nicely

Hmm, what would you do if more than one of them are plugged in at the same time?
Maybe hard to time on a running system, but possible if I connect a hub with
devices already plugged in, and can definitively happen (by mistake) on cold boot.

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Dominik Csapak]

On 9/2/21 08:09, Thomas Lamprecht wrote:
> On 01.09.21 16:48, Dominik Csapak wrote:
>>
>> also, would it not make sense to have
>> multiple uuids per datastore instead of only one?
>>
>> i guess most users of this will want to have more than
>> one usb devices in rotation, and having multiple
>> datastores + multiple sync jobs is probably
>> a bit error prone. just allowing multiple
>> uuids for each datastore would solve that
>> problem nicely
> 
> Hmm, what would you do if more than one of them are plugged in at the same time?
> Maybe hard to time on a running system, but possible if I connect a hub with
> devices already plugged in, and can definitively happen (by mistake) on cold boot.
> 

my guess is that the udev rules do not trigger simultaneously ?
and even if, we could lock the mount/unmount so that this
does not make a problem and we simply mount the first
triggered one

but yeah, that was just an idea, not a hard requirement for me

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Thomas Lamprecht]

On 02.09.21 08:18, Dominik Csapak wrote:
> On 9/2/21 08:09, Thomas Lamprecht wrote:
>> On 01.09.21 16:48, Dominik Csapak wrote:
>>>
>>> also, would it not make sense to have
>>> multiple uuids per datastore instead of only one?
>>>
>>> i guess most users of this will want to have more than
>>> one usb devices in rotation, and having multiple
>>> datastores + multiple sync jobs is probably
>>> a bit error prone. just allowing multiple
>>> uuids for each datastore would solve that
>>> problem nicely
>>
>> Hmm, what would you do if more than one of them are plugged in at the same time?
>> Maybe hard to time on a running system, but possible if I connect a hub with
>> devices already plugged in, and can definitively happen (by mistake) on cold boot.
>>
> 
> my guess is that the udev rules do not trigger simultaneously ?

udev can coalesce events IIRC, and even if not it would not be guaranteed to
be stable so just relying on that fact would provide a bad UX IMO.

> and even if, we could lock the mount/unmount so that this
> does not make a problem and we simply mount the first
> triggered one

see above, IMO weird as the "first" is really not guaranteed to be
stable.

> but yeah, that was just an idea, not a hard requirement for me

IMO this is something we could always add later on without bending backwards,
if user really request it.

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Hannes Laimer]

Am 01.09.21 um 16:48 schrieb Dominik Csapak:
> High level comment here, rest in the relevant patches:
> 
> looks mostly ok, but one things stood out:
> 
> why do you add a command for the socket and the
> manager 'send command' method? would it not have
> been easier to call the mount api directly
> from the proxmox-backup-manager?
Since this is called by a udev rule it has to finish almost
immediately, calling the API(also through the pbs-manager)
takes to long, and everything forked will be killed. So
I had to somehow make the udev rule just 'dispatch' the mounting
command without actually doing it or starting another thread/process
to do it. The path to the socket file starts with a null byte and
is therefore not really accessible from outside of the rust code.
> 
> also, would it not make sense to have
> multiple uuids per datastore instead of only one?
> 
> i guess most users of this will want to have more than
> one usb devices in rotation, and having multiple
> datastores + multiple sync jobs is probably
> a bit error prone. just allowing multiple
> uuids for each datastore would solve that
> problem nicely
> 
> On 8/30/21 13:14, Hannes Laimer wrote:
>> v2(based on Lorenz Stechauner <l.stechauner@proxmox.com>'s feedback):
>>   - do not allow creation of removable datastores on a device that
>>     is mounted at '/'
>>   - ui: also show the mount point in the Options-tab
>>
>> Adds the possibility to create removable datastores. A removable
>> datastore has a UUID(the device on which the data is stored) and a
>> mount-point(where the device will be mounted), iff both are set the
>> datastore is removable. Everything else is identical to normal
>> datastores. Since config files for jobs, etc. are stored on the server,
>> all configuration can be done with the device plugged in or not. Certain
>> statistics about the datastore won't be available as long as it is not
>> plugged in.
>> Removable datastores have to be unmounted before removing, it can only
>> be unmounted if not jibs are running.
>> Removable datastores are mounted automatically when the device is
>> plugged in, if it has
>> been unmounted, it has to be mounted manually through the WebUI or the
>> Api.
>> Jobs will not be started if the datastore is not available, and
>> depending on the configuration, start when the device is plugged in the
>> next time.
>>
>> Still todo:
>>   - make sync to local datastore more integrated
>>   - (add 'when plugged in'-option to job schedule?)
>>   - replace linux commands with internal functions in tools/disks, where
>>      possible
>>
>> Hannes Laimer (15):
>>    tools: add disks utility functions
>>    config: add uuid+mountpoint to DataStoreConfig
>>    api2: add support for removable datastore creation
>>    backup: add check_if_available function to ds
>>    api2: add 'is_available' to DataStoreConfig
>>    api2: add 'removable' to DataStoreListItem
>>    api2: add (un)mount endpoint for removable ds's
>>    pbs: add mount-removable command to commandSocket
>>    pbs-manager: add 'send-command' command
>>    debian: add udev rule for removable datastores
>>    ui: show usb icon for removable datastore in list
>>    ui: add 'removable' checkbox in datastore creation
>>    ui: display row as disabled in ds statistics
>>    ui: show backing device UUID and mount-point in option tab
>>    ui: add (un)mount button to summary
>>
>>   debian/proxmox-backup-server.udev    |   3 +
>>   pbs-api-types/src/lib.rs             |   7 ++
>>   src/api2/admin/datastore.rs          | 159 +++++++++++++++++++++++++++
>>   src/api2/config/datastore.rs         |  22 +++-
>>   src/api2/status.rs                   |  19 +++-
>>   src/api2/types/mod.rs                |   2 +
>>   src/backup/datastore.rs              |  23 ++++
>>   src/backup/mod.rs                    |   2 +-
>>   src/bin/proxmox-backup-api.rs        |  27 +++++
>>   src/bin/proxmox-backup-manager.rs    |  41 +++++++
>>   src/config/datastore.rs              |  16 +++
>>   src/tools/disks/mod.rs               |  53 +++++++++
>>   www/NavigationTree.js                |   3 +-
>>   www/dashboard/DataStoreStatistics.js |   3 +
>>   www/datastore/OptionView.js          |   6 +
>>   www/datastore/Summary.js             |  77 ++++++++++++-
>>   www/window/DataStoreEdit.js          |   5 +
>>   17 files changed, 460 insertions(+), 8 deletions(-)
>>
> 
> 

Re: [pbs-devel] [PATCH v2 proxmox-backup 00/15] (partially)close #3156: Add support for removable datastores

[Dietmar Maurer]

> > Hmm, what would you do if more than one of them are plugged in at the same time?
> > Maybe hard to time on a running system, but possible if I connect a hub with
> > devices already plugged in, and can definitively happen (by mistake) on cold boot.
> > 
> 
> my guess is that the udev rules do not trigger simultaneously ?

We detect both devices ...

> and even if, we could lock the mount/unmount so that this
> does not make a problem and we simply mount the first
> triggered one

Why not simply sync to both disks?