[pbs-devel] [PATCH proxmox-backup v2 0/4] improve datastore removal/creation

[pbs-devel] [PATCH proxmox-backup v2 0/4] improve datastore removal/creation

[Dominik Csapak]

by fixing the leftover datastore references
changing the api call that creates the datastore to use a worker
and shows a removal button in the gui

changes from v1:
* renamed one 'refresh_datstores' to 'notify_datastore_removed'
  (and the relevant command in the socket)
* changed the create api call to always use a worker,
  this makes it necessary to factor the creation code out, so that
  we can reuse it in /api2/node/disks/*.rs
* move the remove button from the datastore list to the
  datastore option view

Dominik Csapak (4):
  proxmox-backup-proxy: fix leftover references on datastore removal
  api2/config/datastore: change create datastore api call to a worker
  backup/chunk_store: optionally log progress on creation
  ui: DataStoreList: add remove button

 src/api2/config/datastore.rs     | 56 ++++++++++++++++++++++----------
 src/api2/node/disks/directory.rs | 15 +++++++--
 src/api2/node/disks/zfs.rs       | 14 +++++++-
 src/backup/chunk_store.rs        | 11 ++++---
 src/backup/datastore.rs          | 16 +++++++++
 src/bin/proxmox-backup-proxy.rs  | 11 +++++++
 src/server.rs                    |  8 +++++
 www/Utils.js                     |  1 +
 www/datastore/OptionView.js      | 30 +++++++++++++++++
 www/window/DataStoreEdit.js      |  1 +
 10 files changed, 139 insertions(+), 24 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup v2 1/4] proxmox-backup-proxy: fix leftover references on datastore removal

[Dominik Csapak]

when we remove a datastore via api/cli, the proxy
has sometimes leftover references to that datastore in its
DATASTORE_MAP which includes an open filehandle on the
'.lock' file

this prevents unmounting/exporting the datastore even after removal,
only a reload/restart of the proxy did help

add a command to our command socket, which removes all non
configured datastores from the map, dropping the open filehandle

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/config/datastore.rs    |  4 +++-
 src/backup/datastore.rs         | 16 ++++++++++++++++
 src/bin/proxmox-backup-proxy.rs | 11 +++++++++++
 src/server.rs                   |  8 ++++++++
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 6ca98b53..7299c91d 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -403,7 +403,7 @@ pub fn update_datastore(
     },
 )]
 /// Remove a datastore configuration.
-pub fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Error> {
+pub async fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Error> {
 
     let _lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
@@ -425,6 +425,8 @@ pub fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Erro
     let _ = jobstate::remove_state_file("prune", &name);
     let _ = jobstate::remove_state_file("garbage_collection", &name);
 
+    crate::server::notify_datastore_removed().await?;
+
     Ok(())
 }
 
diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 584b2020..6989313d 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -69,6 +69,22 @@ impl DataStore {
         Ok(datastore)
     }
 
+    /// removes all datastores that are not configured anymore
+    pub fn refresh_datastores() -> Result<(), Error>{
+        let (config, _digest) = datastore::config()?;
+        let mut stores = HashSet::new();
+        for (store, _) in config.sections {
+            stores.insert(store);
+        }
+
+        let mut map = DATASTORE_MAP.lock().unwrap();
+        // removes all elements that are not in the config
+        map.retain(|key, _| {
+            stores.contains(key)
+        });
+        Ok(())
+    }
+
     fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
         let chunk_store = ChunkStore::open(store_name, path)?;
 
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index a53f554a..75d40e21 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -136,6 +136,17 @@ async fn run() -> Result<(), Error> {
         },
     )?;
 
+    // to remove references for not configured datastores
+    commando_sock.register_command(
+        "datastore-removed".to_string(),
+        |_value| {
+            if let Err(err) = proxmox_backup::backup::DataStore::refresh_datastores() {
+                log::error!("could not refresh datastores: {}", err);
+            }
+            Ok(Value::Null)
+        }
+    )?;
+
     let server = daemon::create_daemon(
         ([0,0,0,0,0,0,0,0], 8007).into(),
         move |listener, ready| {
diff --git a/src/server.rs b/src/server.rs
index ba25617d..c4a36967 100644
--- a/src/server.rs
+++ b/src/server.rs
@@ -100,3 +100,11 @@ pub(crate) async fn reload_proxy_certificate() -> Result<(), Error> {
         .await?;
     Ok(())
 }
+
+pub(crate) async fn notify_datastore_removed() -> Result<(), Error> {
+    let proxy_pid = crate::server::read_pid(buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)?;
+    let sock = crate::server::ctrl_sock_from_pid(proxy_pid);
+    let _: Value = crate::server::send_raw_command(sock, "{\"command\":\"datastore-removed\"}\n")
+        .await?;
+    Ok(())
+}
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup v2 1/4] proxmox-backup-proxy: fix leftover references on datastore removal

[Dietmar Maurer]

applied with small modifications

- simplify refresh_datastores (as suggested by Thomas)

- rename  refresh_datastores to remove_unused_datastores

On 6/2/21 1:27 PM, Dominik Csapak wrote:
> when we remove a datastore via api/cli, the proxy
> has sometimes leftover references to that datastore in its
> DATASTORE_MAP which includes an open filehandle on the
> '.lock' file
>
> this prevents unmounting/exporting the datastore even after removal,
> only a reload/restart of the proxy did help
>
> add a command to our command socket, which removes all non
> configured datastores from the map, dropping the open filehandle
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/api2/config/datastore.rs    |  4 +++-
>   src/backup/datastore.rs         | 16 ++++++++++++++++
>   src/bin/proxmox-backup-proxy.rs | 11 +++++++++++
>   src/server.rs                   |  8 ++++++++
>   4 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 6ca98b53..7299c91d 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -403,7 +403,7 @@ pub fn update_datastore(
>       },
>   )]
>   /// Remove a datastore configuration.
> -pub fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Error> {
> +pub async fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Error> {
>   
>       let _lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
>   
> @@ -425,6 +425,8 @@ pub fn delete_datastore(name: String, digest: Option<String>) -> Result<(), Erro
>       let _ = jobstate::remove_state_file("prune", &name);
>       let _ = jobstate::remove_state_file("garbage_collection", &name);
>   
> +    crate::server::notify_datastore_removed().await?;
> +
>       Ok(())
>   }
>   
> diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
> index 584b2020..6989313d 100644
> --- a/src/backup/datastore.rs
> +++ b/src/backup/datastore.rs
> @@ -69,6 +69,22 @@ impl DataStore {
>           Ok(datastore)
>       }
>   
> +    /// removes all datastores that are not configured anymore
> +    pub fn refresh_datastores() -> Result<(), Error>{
> +        let (config, _digest) = datastore::config()?;
> +        let mut stores = HashSet::new();
> +        for (store, _) in config.sections {
> +            stores.insert(store);
> +        }
> +
> +        let mut map = DATASTORE_MAP.lock().unwrap();
> +        // removes all elements that are not in the config
> +        map.retain(|key, _| {
> +            stores.contains(key)
> +        });
> +        Ok(())
> +    }
> +
>       fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
>           let chunk_store = ChunkStore::open(store_name, path)?;
>   
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index a53f554a..75d40e21 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -136,6 +136,17 @@ async fn run() -> Result<(), Error> {
>           },
>       )?;
>   
> +    // to remove references for not configured datastores
> +    commando_sock.register_command(
> +        "datastore-removed".to_string(),
> +        |_value| {
> +            if let Err(err) = proxmox_backup::backup::DataStore::refresh_datastores() {
> +                log::error!("could not refresh datastores: {}", err);
> +            }
> +            Ok(Value::Null)
> +        }
> +    )?;
> +
>       let server = daemon::create_daemon(
>           ([0,0,0,0,0,0,0,0], 8007).into(),
>           move |listener, ready| {
> diff --git a/src/server.rs b/src/server.rs
> index ba25617d..c4a36967 100644
> --- a/src/server.rs
> +++ b/src/server.rs
> @@ -100,3 +100,11 @@ pub(crate) async fn reload_proxy_certificate() -> Result<(), Error> {
>           .await?;
>       Ok(())
>   }
> +
> +pub(crate) async fn notify_datastore_removed() -> Result<(), Error> {
> +    let proxy_pid = crate::server::read_pid(buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)?;
> +    let sock = crate::server::ctrl_sock_from_pid(proxy_pid);
> +    let _: Value = crate::server::send_raw_command(sock, "{\"command\":\"datastore-removed\"}\n")
> +        .await?;
> +    Ok(())
> +}

[pbs-devel] [PATCH proxmox-backup v2 2/4] api2/config/datastore: change create datastore api call to a worker

[Dominik Csapak]

so that longer running creates (e.g. a slow storage), does not
run in a timeout and we can follow its creation

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/config/datastore.rs     | 51 ++++++++++++++++++++++----------
 src/api2/node/disks/directory.rs | 15 ++++++++--
 src/api2/node/disks/zfs.rs       | 14 ++++++++-
 www/window/DataStoreEdit.js      |  1 +
 4 files changed, 62 insertions(+), 19 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 7299c91d..4fa76b40 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -5,6 +5,7 @@ use serde_json::Value;
 use ::serde::{Deserialize, Serialize};
 
 use proxmox::api::{api, Router, RpcEnvironment, Permission};
+use proxmox::api::section_config::SectionConfigData;
 use proxmox::api::schema::parse_property_string;
 use proxmox::tools::fs::open_file_locked;
 
@@ -13,7 +14,7 @@ use crate::backup::*;
 use crate::config::cached_user_info::CachedUserInfo;
 use crate::config::datastore::{self, DataStoreConfig, DIR_NAME_SCHEMA};
 use crate::config::acl::{PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY};
-use crate::server::jobstate;
+use crate::server::{jobstate, WorkerTask};
 
 #[api(
     input: {
@@ -50,6 +51,25 @@ pub fn list_datastores(
     Ok(list.into_iter().filter(filter_by_privs).collect())
 }
 
+pub(crate) fn create_datastore_impl(
+    _lock: std::fs::File,
+    mut config: SectionConfigData,
+    datastore: DataStoreConfig,
+) -> Result<(), Error> {
+    let path: PathBuf = datastore.path.clone().into();
+
+    let backup_user = crate::backup::backup_user()?;
+    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid)?;
+
+    config.set_data(&datastore.name, "datastore", &datastore)?;
+
+    datastore::save_config(&config)?;
+
+    jobstate::create_state_file("prune", &datastore.name)?;
+    jobstate::create_state_file("garbage_collection", &datastore.name)?;
+
+    Ok(())
+}
 
 // fixme: impl. const fn get_object_schema(datastore::DataStoreConfig::API_SCHEMA),
 // but this need support for match inside const fn
@@ -116,31 +136,30 @@ pub fn list_datastores(
     },
 )]
 /// Create new datastore config.
-pub fn create_datastore(param: Value) -> Result<(), Error> {
+pub fn create_datastore(
+    param: Value,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<String, Error> {
 
-    let _lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
+    let lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
 
     let datastore: datastore::DataStoreConfig = serde_json::from_value(param)?;
 
-    let (mut config, _digest) = datastore::config()?;
+    let (config, _digest) = datastore::config()?;
 
     if config.sections.get(&datastore.name).is_some() {
         bail!("datastore '{}' already exists.", datastore.name);
     }
 
-    let path: PathBuf = datastore.path.clone().into();
-
-    let backup_user = crate::backup::backup_user()?;
-    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid)?;
-
-    config.set_data(&datastore.name, "datastore", &datastore)?;
-
-    datastore::save_config(&config)?;
-
-    jobstate::create_state_file("prune", &datastore.name)?;
-    jobstate::create_state_file("garbage_collection", &datastore.name)?;
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
-    Ok(())
+    WorkerTask::new_thread(
+        "create-datastore",
+        Some(datastore.name.to_string()),
+        auth_id,
+        false,
+        move |_worker| create_datastore_impl(lock, config, datastore),
+    )
 }
 
 #[api(
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index c968b333..006e9522 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -5,6 +5,7 @@ use ::serde::{Deserialize, Serialize};
 use proxmox::api::{api, Permission, RpcEnvironment, RpcEnvironmentType};
 use proxmox::api::section_config::SectionConfigData;
 use proxmox::api::router::Router;
+use proxmox::tools::fs::open_file_locked;
 
 use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
 use crate::tools::disks::{
@@ -16,7 +17,7 @@ use crate::tools::systemd::{self, types::*};
 use crate::server::WorkerTask;
 
 use crate::api2::types::*;
-use crate::config::datastore::DataStoreConfig;
+use crate::config::datastore::{self, DataStoreConfig};
 
 #[api(
     properties: {
@@ -179,7 +180,17 @@ pub fn create_datastore_disk(
             systemd::start_unit(&mount_unit_name)?;
 
             if add_datastore {
-                crate::api2::config::datastore::create_datastore(json!({ "name": name, "path": mount_point }))?
+                let lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
+                let datastore: DataStoreConfig =
+                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
+
+                let (config, _digest) = datastore::config()?;
+
+                if config.sections.get(&datastore.name).is_some() {
+                    bail!("datastore '{}' already exists.", datastore.name);
+                }
+
+                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
             }
 
             Ok(())
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 0f93f110..ee8037e2 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -14,12 +14,14 @@ use proxmox::api::{
     },
 };
 use proxmox::api::router::Router;
+use proxmox::tools::fs::open_file_locked;
 
 use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
 use crate::tools::disks::{
     zpool_list, zpool_status, parse_zpool_status_config_tree, vdev_list_to_tree,
     DiskUsageType,
 };
+use crate::config::datastore::{self, DataStoreConfig};
 
 use crate::server::WorkerTask;
 
@@ -372,7 +374,17 @@ pub fn create_zpool(
             }
 
             if add_datastore {
-                crate::api2::config::datastore::create_datastore(json!({ "name": name, "path": mount_point }))?
+                let lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
+                let datastore: DataStoreConfig =
+                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
+
+                let (config, _digest) = datastore::config()?;
+
+                if config.sections.get(&datastore.name).is_some() {
+                    bail!("datastore '{}' already exists.", datastore.name);
+                }
+
+                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
             }
 
             Ok(())
diff --git a/www/window/DataStoreEdit.js b/www/window/DataStoreEdit.js
index c2b2809f..2f67db76 100644
--- a/www/window/DataStoreEdit.js
+++ b/www/window/DataStoreEdit.js
@@ -75,6 +75,7 @@ Ext.define('PBS.DataStoreEdit', {
     isAdd: true,
 
     bodyPadding: 0,
+    showProgress: true,
 
     cbindData: function(initialConfig) {
 	var me = this;
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup v2 2/4] api2/config/datastore: change create datastore api call to a worker

[Dietmar Maurer]

applied with additional cleanups:

- rename create_datastore_impl to do_create_datastore

- factor out the datastore config lock function

On 6/2/21 1:27 PM, Dominik Csapak wrote:
> so that longer running creates (e.g. a slow storage), does not
> run in a timeout and we can follow its creation
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/api2/config/datastore.rs     | 51 ++++++++++++++++++++++----------
>   src/api2/node/disks/directory.rs | 15 ++++++++--
>   src/api2/node/disks/zfs.rs       | 14 ++++++++-
>   www/window/DataStoreEdit.js      |  1 +
>   4 files changed, 62 insertions(+), 19 deletions(-)
>
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 7299c91d..4fa76b40 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -5,6 +5,7 @@ use serde_json::Value;
>   use ::serde::{Deserialize, Serialize};
>   
>   use proxmox::api::{api, Router, RpcEnvironment, Permission};
> +use proxmox::api::section_config::SectionConfigData;
>   use proxmox::api::schema::parse_property_string;
>   use proxmox::tools::fs::open_file_locked;
>   
> @@ -13,7 +14,7 @@ use crate::backup::*;
>   use crate::config::cached_user_info::CachedUserInfo;
>   use crate::config::datastore::{self, DataStoreConfig, DIR_NAME_SCHEMA};
>   use crate::config::acl::{PRIV_DATASTORE_ALLOCATE, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY};
> -use crate::server::jobstate;
> +use crate::server::{jobstate, WorkerTask};
>   
>   #[api(
>       input: {
> @@ -50,6 +51,25 @@ pub fn list_datastores(
>       Ok(list.into_iter().filter(filter_by_privs).collect())
>   }
>   
> +pub(crate) fn create_datastore_impl(
> +    _lock: std::fs::File,
> +    mut config: SectionConfigData,
> +    datastore: DataStoreConfig,
> +) -> Result<(), Error> {
> +    let path: PathBuf = datastore.path.clone().into();
> +
> +    let backup_user = crate::backup::backup_user()?;
> +    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid)?;
> +
> +    config.set_data(&datastore.name, "datastore", &datastore)?;
> +
> +    datastore::save_config(&config)?;
> +
> +    jobstate::create_state_file("prune", &datastore.name)?;
> +    jobstate::create_state_file("garbage_collection", &datastore.name)?;
> +
> +    Ok(())
> +}
>   
>   // fixme: impl. const fn get_object_schema(datastore::DataStoreConfig::API_SCHEMA),
>   // but this need support for match inside const fn
> @@ -116,31 +136,30 @@ pub fn list_datastores(
>       },
>   )]
>   /// Create new datastore config.
> -pub fn create_datastore(param: Value) -> Result<(), Error> {
> +pub fn create_datastore(
> +    param: Value,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<String, Error> {
>   
> -    let _lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +    let lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
>   
>       let datastore: datastore::DataStoreConfig = serde_json::from_value(param)?;
>   
> -    let (mut config, _digest) = datastore::config()?;
> +    let (config, _digest) = datastore::config()?;
>   
>       if config.sections.get(&datastore.name).is_some() {
>           bail!("datastore '{}' already exists.", datastore.name);
>       }
>   
> -    let path: PathBuf = datastore.path.clone().into();
> -
> -    let backup_user = crate::backup::backup_user()?;
> -    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid)?;
> -
> -    config.set_data(&datastore.name, "datastore", &datastore)?;
> -
> -    datastore::save_config(&config)?;
> -
> -    jobstate::create_state_file("prune", &datastore.name)?;
> -    jobstate::create_state_file("garbage_collection", &datastore.name)?;
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> -    Ok(())
> +    WorkerTask::new_thread(
> +        "create-datastore",
> +        Some(datastore.name.to_string()),
> +        auth_id,
> +        false,
> +        move |_worker| create_datastore_impl(lock, config, datastore),
> +    )
>   }
>   
>   #[api(
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index c968b333..006e9522 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -5,6 +5,7 @@ use ::serde::{Deserialize, Serialize};
>   use proxmox::api::{api, Permission, RpcEnvironment, RpcEnvironmentType};
>   use proxmox::api::section_config::SectionConfigData;
>   use proxmox::api::router::Router;
> +use proxmox::tools::fs::open_file_locked;
>   
>   use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
>   use crate::tools::disks::{
> @@ -16,7 +17,7 @@ use crate::tools::systemd::{self, types::*};
>   use crate::server::WorkerTask;
>   
>   use crate::api2::types::*;
> -use crate::config::datastore::DataStoreConfig;
> +use crate::config::datastore::{self, DataStoreConfig};
>   
>   #[api(
>       properties: {
> @@ -179,7 +180,17 @@ pub fn create_datastore_disk(
>               systemd::start_unit(&mount_unit_name)?;
>   
>               if add_datastore {
> -                crate::api2::config::datastore::create_datastore(json!({ "name": name, "path": mount_point }))?
> +                let lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +                let datastore: DataStoreConfig =
> +                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
> +
> +                let (config, _digest) = datastore::config()?;
> +
> +                if config.sections.get(&datastore.name).is_some() {
> +                    bail!("datastore '{}' already exists.", datastore.name);
> +                }
> +
> +                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
>               }
>   
>               Ok(())
> diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
> index 0f93f110..ee8037e2 100644
> --- a/src/api2/node/disks/zfs.rs
> +++ b/src/api2/node/disks/zfs.rs
> @@ -14,12 +14,14 @@ use proxmox::api::{
>       },
>   };
>   use proxmox::api::router::Router;
> +use proxmox::tools::fs::open_file_locked;
>   
>   use crate::config::acl::{PRIV_SYS_AUDIT, PRIV_SYS_MODIFY};
>   use crate::tools::disks::{
>       zpool_list, zpool_status, parse_zpool_status_config_tree, vdev_list_to_tree,
>       DiskUsageType,
>   };
> +use crate::config::datastore::{self, DataStoreConfig};
>   
>   use crate::server::WorkerTask;
>   
> @@ -372,7 +374,17 @@ pub fn create_zpool(
>               }
>   
>               if add_datastore {
> -                crate::api2::config::datastore::create_datastore(json!({ "name": name, "path": mount_point }))?
> +                let lock = open_file_locked(datastore::DATASTORE_CFG_LOCKFILE, std::time::Duration::new(10, 0), true)?;
> +                let datastore: DataStoreConfig =
> +                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
> +
> +                let (config, _digest) = datastore::config()?;
> +
> +                if config.sections.get(&datastore.name).is_some() {
> +                    bail!("datastore '{}' already exists.", datastore.name);
> +                }
> +
> +                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
>               }
>   
>               Ok(())
> diff --git a/www/window/DataStoreEdit.js b/www/window/DataStoreEdit.js
> index c2b2809f..2f67db76 100644
> --- a/www/window/DataStoreEdit.js
> +++ b/www/window/DataStoreEdit.js
> @@ -75,6 +75,7 @@ Ext.define('PBS.DataStoreEdit', {
>       isAdd: true,
>   
>       bodyPadding: 0,
> +    showProgress: true,
>   
>       cbindData: function(initialConfig) {
>   	var me = this;

[pbs-devel] [PATCH proxmox-backup v2 3/4] backup/chunk_store: optionally log progress on creation

[Dominik Csapak]

and enable it for the worker variants

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/config/datastore.rs     |  5 +++--
 src/api2/node/disks/directory.rs |  2 +-
 src/api2/node/disks/zfs.rs       |  2 +-
 src/backup/chunk_store.rs        | 11 +++++++----
 4 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 4fa76b40..4e079cd5 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -55,11 +55,12 @@ pub(crate) fn create_datastore_impl(
     _lock: std::fs::File,
     mut config: SectionConfigData,
     datastore: DataStoreConfig,
+    worker: Option<&dyn crate::task::TaskState>,
 ) -> Result<(), Error> {
     let path: PathBuf = datastore.path.clone().into();
 
     let backup_user = crate::backup::backup_user()?;
-    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid)?;
+    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid, worker)?;
 
     config.set_data(&datastore.name, "datastore", &datastore)?;
 
@@ -158,7 +159,7 @@ pub fn create_datastore(
         Some(datastore.name.to_string()),
         auth_id,
         false,
-        move |_worker| create_datastore_impl(lock, config, datastore),
+        move |worker| create_datastore_impl(lock, config, datastore, Some(&worker)),
     )
 }
 
diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 006e9522..4eadf867 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -190,7 +190,7 @@ pub fn create_datastore_disk(
                     bail!("datastore '{}' already exists.", datastore.name);
                 }
 
-                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
+                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore, Some(&worker))?;
             }
 
             Ok(())
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index ee8037e2..ebdc3688 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -384,7 +384,7 @@ pub fn create_zpool(
                     bail!("datastore '{}' already exists.", datastore.name);
                 }
 
-                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
+                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore, Some(&worker))?;
             }
 
             Ok(())
diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs
index 31e8307c..e9cc3897 100644
--- a/src/backup/chunk_store.rs
+++ b/src/backup/chunk_store.rs
@@ -7,6 +7,7 @@ use std::os::unix::io::AsRawFd;
 
 use proxmox::tools::fs::{CreateOptions, create_path, create_dir};
 
+use crate::task_log;
 use crate::tools;
 use crate::api2::types::GarbageCollectionStatus;
 
@@ -61,7 +62,7 @@ impl ChunkStore {
         chunk_dir
     }
 
-    pub fn create<P>(name: &str, path: P, uid: nix::unistd::Uid, gid: nix::unistd::Gid) -> Result<Self, Error>
+    pub fn create<P>(name: &str, path: P, uid: nix::unistd::Uid, gid: nix::unistd::Gid, worker: Option<&dyn TaskState>) -> Result<Self, Error>
     where
         P: Into<PathBuf>,
     {
@@ -104,7 +105,9 @@ impl ChunkStore {
             }
             let percentage = (i*100)/(64*1024);
             if percentage != last_percentage {
-                // eprintln!("ChunkStore::create {}%", percentage);
+                if let Some(worker) = worker {
+                    task_log!(worker, "Chunkstore create: {}%", percentage)
+                }
                 last_percentage = percentage;
             }
         }
@@ -461,7 +464,7 @@ fn test_chunk_store1() {
     assert!(chunk_store.is_err());
 
     let user = nix::unistd::User::from_uid(nix::unistd::Uid::current()).unwrap().unwrap();
-    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid).unwrap();
+    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid, None).unwrap();
 
     let (chunk, digest) = super::DataChunkBuilder::new(&[0u8, 1u8]).build().unwrap();
 
@@ -472,7 +475,7 @@ fn test_chunk_store1() {
     assert!(exists);
 
 
-    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid);
+    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid, None);
     assert!(chunk_store.is_err());
 
     if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup v2 3/4] backup/chunk_store: optionally log progress on creation

[Dietmar Maurer]

applied

On 6/2/21 1:27 PM, Dominik Csapak wrote:
> and enable it for the worker variants
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/api2/config/datastore.rs     |  5 +++--
>   src/api2/node/disks/directory.rs |  2 +-
>   src/api2/node/disks/zfs.rs       |  2 +-
>   src/backup/chunk_store.rs        | 11 +++++++----
>   4 files changed, 12 insertions(+), 8 deletions(-)
>
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 4fa76b40..4e079cd5 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -55,11 +55,12 @@ pub(crate) fn create_datastore_impl(
>       _lock: std::fs::File,
>       mut config: SectionConfigData,
>       datastore: DataStoreConfig,
> +    worker: Option<&dyn crate::task::TaskState>,
>   ) -> Result<(), Error> {
>       let path: PathBuf = datastore.path.clone().into();
>   
>       let backup_user = crate::backup::backup_user()?;
> -    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid)?;
> +    let _store = ChunkStore::create(&datastore.name, path, backup_user.uid, backup_user.gid, worker)?;
>   
>       config.set_data(&datastore.name, "datastore", &datastore)?;
>   
> @@ -158,7 +159,7 @@ pub fn create_datastore(
>           Some(datastore.name.to_string()),
>           auth_id,
>           false,
> -        move |_worker| create_datastore_impl(lock, config, datastore),
> +        move |worker| create_datastore_impl(lock, config, datastore, Some(&worker)),
>       )
>   }
>   
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index 006e9522..4eadf867 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -190,7 +190,7 @@ pub fn create_datastore_disk(
>                       bail!("datastore '{}' already exists.", datastore.name);
>                   }
>   
> -                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
> +                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore, Some(&worker))?;
>               }
>   
>               Ok(())
> diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
> index ee8037e2..ebdc3688 100644
> --- a/src/api2/node/disks/zfs.rs
> +++ b/src/api2/node/disks/zfs.rs
> @@ -384,7 +384,7 @@ pub fn create_zpool(
>                       bail!("datastore '{}' already exists.", datastore.name);
>                   }
>   
> -                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore)?;
> +                crate::api2::config::datastore::create_datastore_impl(lock, config, datastore, Some(&worker))?;
>               }
>   
>               Ok(())
> diff --git a/src/backup/chunk_store.rs b/src/backup/chunk_store.rs
> index 31e8307c..e9cc3897 100644
> --- a/src/backup/chunk_store.rs
> +++ b/src/backup/chunk_store.rs
> @@ -7,6 +7,7 @@ use std::os::unix::io::AsRawFd;
>   
>   use proxmox::tools::fs::{CreateOptions, create_path, create_dir};
>   
> +use crate::task_log;
>   use crate::tools;
>   use crate::api2::types::GarbageCollectionStatus;
>   
> @@ -61,7 +62,7 @@ impl ChunkStore {
>           chunk_dir
>       }
>   
> -    pub fn create<P>(name: &str, path: P, uid: nix::unistd::Uid, gid: nix::unistd::Gid) -> Result<Self, Error>
> +    pub fn create<P>(name: &str, path: P, uid: nix::unistd::Uid, gid: nix::unistd::Gid, worker: Option<&dyn TaskState>) -> Result<Self, Error>
>       where
>           P: Into<PathBuf>,
>       {
> @@ -104,7 +105,9 @@ impl ChunkStore {
>               }
>               let percentage = (i*100)/(64*1024);
>               if percentage != last_percentage {
> -                // eprintln!("ChunkStore::create {}%", percentage);
> +                if let Some(worker) = worker {
> +                    task_log!(worker, "Chunkstore create: {}%", percentage)
> +                }
>                   last_percentage = percentage;
>               }
>           }
> @@ -461,7 +464,7 @@ fn test_chunk_store1() {
>       assert!(chunk_store.is_err());
>   
>       let user = nix::unistd::User::from_uid(nix::unistd::Uid::current()).unwrap().unwrap();
> -    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid).unwrap();
> +    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid, None).unwrap();
>   
>       let (chunk, digest) = super::DataChunkBuilder::new(&[0u8, 1u8]).build().unwrap();
>   
> @@ -472,7 +475,7 @@ fn test_chunk_store1() {
>       assert!(exists);
>   
>   
> -    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid);
> +    let chunk_store = ChunkStore::create("test", &path, user.uid, user.gid, None);
>       assert!(chunk_store.is_err());
>   
>       if let Err(_e) = std::fs::remove_dir_all(".testdir") { /* ignore */ }

[pbs-devel] [PATCH proxmox-backup v2 4/4] ui: DataStoreList: add remove button

[Dominik Csapak]

so that a user can remove a datastore from the gui,
though no data is deleted, this has to be done elsewhere (for now)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/Utils.js                |  1 +
 www/datastore/OptionView.js | 30 ++++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/www/Utils.js b/www/Utils.js
index f614d77e..6b378355 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -380,6 +380,7 @@ Ext.define('PBS.Utils', {
 	    backup: (type, id) => PBS.Utils.render_datastore_worker_id(id, gettext('Backup')),
 	    'barcode-label-media': [gettext('Drive'), gettext('Barcode-Label Media')],
 	    'catalog-media': [gettext('Drive'), gettext('Catalog Media')],
+	    'delete-datastore': [gettext('Datastore'), gettext('Remove Datastore')],
 	    dircreate: [gettext('Directory Storage'), gettext('Create')],
 	    dirremove: [gettext('Directory'), gettext('Remove')],
 	    'eject-media': [gettext('Drive'), gettext('Eject Media')],
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 723730fd..98152dce 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -21,6 +21,28 @@ Ext.define('PBS.Datastore.Options', {
 	edit: function() {
 	    this.getView().run_editor();
 	},
+
+	removeDatastore: function() {
+	    let me = this;
+	    let datastore = me.getView().datastore;
+	    Ext.create('Proxmox.window.SafeDestroy', {
+		url: `/config/datastore/${datastore}`,
+		item: {
+		    id: datastore,
+		},
+		note: gettext('Configuration change only, no data will be deleted.'),
+		autoShow: true,
+		taskName: 'delete-datastore',
+		listeners: {
+		    destroy: () => {
+			let navtree = Ext.ComponentQuery.query('navigationtree')[0];
+			navtree.rstore.load();
+			let mainview = me.getView().up('mainview');
+			mainview.getController().redirectTo('pbsDataStores');
+		    },
+		},
+	    });
+	},
     },
 
     tbar: [
@@ -30,6 +52,14 @@ Ext.define('PBS.Datastore.Options', {
 	    disabled: true,
 	    handler: 'edit',
 	},
+	'->',
+	{
+	    xtype: 'proxmoxButton',
+	    selModel: null,
+	    iconCls: 'fa fa-trash-o',
+	    text: gettext('Remove Datastore'),
+	    handler: 'removeDatastore',
+	},
     ],
 
     listeners: {
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup v2 4/4] ui: DataStoreList: add remove button

[Dietmar Maurer]

applied

On 6/2/21 1:27 PM, Dominik Csapak wrote:
> so that a user can remove a datastore from the gui,
> though no data is deleted, this has to be done elsewhere (for now)
>
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   www/Utils.js                |  1 +
>   www/datastore/OptionView.js | 30 ++++++++++++++++++++++++++++++
>   2 files changed, 31 insertions(+)
>
> diff --git a/www/Utils.js b/www/Utils.js
> index f614d77e..6b378355 100644
> --- a/www/Utils.js
> +++ b/www/Utils.js
> @@ -380,6 +380,7 @@ Ext.define('PBS.Utils', {
>   	    backup: (type, id) => PBS.Utils.render_datastore_worker_id(id, gettext('Backup')),
>   	    'barcode-label-media': [gettext('Drive'), gettext('Barcode-Label Media')],
>   	    'catalog-media': [gettext('Drive'), gettext('Catalog Media')],
> +	    'delete-datastore': [gettext('Datastore'), gettext('Remove Datastore')],
>   	    dircreate: [gettext('Directory Storage'), gettext('Create')],
>   	    dirremove: [gettext('Directory'), gettext('Remove')],
>   	    'eject-media': [gettext('Drive'), gettext('Eject Media')],
> diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
> index 723730fd..98152dce 100644
> --- a/www/datastore/OptionView.js
> +++ b/www/datastore/OptionView.js
> @@ -21,6 +21,28 @@ Ext.define('PBS.Datastore.Options', {
>   	edit: function() {
>   	    this.getView().run_editor();
>   	},
> +
> +	removeDatastore: function() {
> +	    let me = this;
> +	    let datastore = me.getView().datastore;
> +	    Ext.create('Proxmox.window.SafeDestroy', {
> +		url: `/config/datastore/${datastore}`,
> +		item: {
> +		    id: datastore,
> +		},
> +		note: gettext('Configuration change only, no data will be deleted.'),
> +		autoShow: true,
> +		taskName: 'delete-datastore',
> +		listeners: {
> +		    destroy: () => {
> +			let navtree = Ext.ComponentQuery.query('navigationtree')[0];
> +			navtree.rstore.load();
> +			let mainview = me.getView().up('mainview');
> +			mainview.getController().redirectTo('pbsDataStores');
> +		    },
> +		},
> +	    });
> +	},
>       },
>   
>       tbar: [
> @@ -30,6 +52,14 @@ Ext.define('PBS.Datastore.Options', {
>   	    disabled: true,
>   	    handler: 'edit',
>   	},
> +	'->',
> +	{
> +	    xtype: 'proxmoxButton',
> +	    selModel: null,
> +	    iconCls: 'fa fa-trash-o',
> +	    text: gettext('Remove Datastore'),
> +	    handler: 'removeDatastore',
> +	},
>       ],
>   
>       listeners: {