* [pbs-devel] [PATCH proxmox-backup v2 1/4] proxmox-backup-proxy: fix leftover references on datastore removal
2021-06-02 11:27 [pbs-devel] [PATCH proxmox-backup v2 0/4] improve datastore removal/creation Dominik Csapak
@ 2021-06-02 11:27 ` Dominik Csapak
2021-06-04 6:24 ` Dietmar Maurer
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 2/4] api2/config/datastore: change create datastore api call to a worker Dominik Csapak
` (2 subsequent siblings)
3 siblings, 1 reply; 9+ messages in thread
From: Dominik Csapak @ 2021-06-02 11:27 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup v2 1/4] proxmox-backup-proxy: fix leftover references on datastore removal
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 1/4] proxmox-backup-proxy: fix leftover references on datastore removal Dominik Csapak
@ 2021-06-04 6:24 ` Dietmar Maurer
0 siblings, 0 replies; 9+ messages in thread
From: Dietmar Maurer @ 2021-06-04 6:24 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dominik Csapak
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(())
> +}
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 2/4] api2/config/datastore: change create datastore api call to a worker
2021-06-02 11:27 [pbs-devel] [PATCH proxmox-backup v2 0/4] improve datastore removal/creation Dominik Csapak
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 1/4] proxmox-backup-proxy: fix leftover references on datastore removal Dominik Csapak
@ 2021-06-02 11:27 ` Dominik Csapak
2021-06-04 7:07 ` [pbs-devel] applied: " Dietmar Maurer
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] backup/chunk_store: optionally log progress on creation Dominik Csapak
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 4/4] ui: DataStoreList: add remove button Dominik Csapak
3 siblings, 1 reply; 9+ messages in thread
From: Dominik Csapak @ 2021-06-02 11:27 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup v2 2/4] api2/config/datastore: change create datastore api call to a worker
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 2/4] api2/config/datastore: change create datastore api call to a worker Dominik Csapak
@ 2021-06-04 7:07 ` Dietmar Maurer
0 siblings, 0 replies; 9+ messages in thread
From: Dietmar Maurer @ 2021-06-04 7:07 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dominik Csapak
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;
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 3/4] backup/chunk_store: optionally log progress on creation
2021-06-02 11:27 [pbs-devel] [PATCH proxmox-backup v2 0/4] improve datastore removal/creation Dominik Csapak
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 1/4] proxmox-backup-proxy: fix leftover references on datastore removal Dominik Csapak
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 2/4] api2/config/datastore: change create datastore api call to a worker Dominik Csapak
@ 2021-06-02 11:27 ` Dominik Csapak
2021-06-04 7:40 ` [pbs-devel] applied: " Dietmar Maurer
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 4/4] ui: DataStoreList: add remove button Dominik Csapak
3 siblings, 1 reply; 9+ messages in thread
From: Dominik Csapak @ 2021-06-02 11:27 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup v2 3/4] backup/chunk_store: optionally log progress on creation
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] backup/chunk_store: optionally log progress on creation Dominik Csapak
@ 2021-06-04 7:40 ` Dietmar Maurer
0 siblings, 0 replies; 9+ messages in thread
From: Dietmar Maurer @ 2021-06-04 7:40 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dominik Csapak
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 */ }
^ permalink raw reply [flat|nested] 9+ messages in thread
* [pbs-devel] [PATCH proxmox-backup v2 4/4] ui: DataStoreList: add remove button
2021-06-02 11:27 [pbs-devel] [PATCH proxmox-backup v2 0/4] improve datastore removal/creation Dominik Csapak
` (2 preceding siblings ...)
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 3/4] backup/chunk_store: optionally log progress on creation Dominik Csapak
@ 2021-06-02 11:27 ` Dominik Csapak
2021-06-04 7:52 ` [pbs-devel] applied: " Dietmar Maurer
3 siblings, 1 reply; 9+ messages in thread
From: Dominik Csapak @ 2021-06-02 11:27 UTC (permalink / raw)
To: pbs-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [pbs-devel] applied: [PATCH proxmox-backup v2 4/4] ui: DataStoreList: add remove button
2021-06-02 11:27 ` [pbs-devel] [PATCH proxmox-backup v2 4/4] ui: DataStoreList: add remove button Dominik Csapak
@ 2021-06-04 7:52 ` Dietmar Maurer
0 siblings, 0 replies; 9+ messages in thread
From: Dietmar Maurer @ 2021-06-04 7:52 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Dominik Csapak
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: {
^ permalink raw reply [flat|nested] 9+ messages in thread