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

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

[Dominik Csapak]

by fixing the leftover datastore references
adding an api call that creates the datastore as a worker
and shows a removal butto in the gui

i am not sure if adding another api call for creation is a good idea,
or if we want to use the chance of a major release to do a breaking
api change and simply change the existing api call to a worker?

also i'd like to know if anybody thinks it would make sense to add another remove
button to the datastore panel itself? (e.g. summary?, or in the panel at
the top?). it *may* be possible to add one in the list itself, though
this seems more complicated (extjs treelists are not that flexible)

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

 src/api2/config/datastore.rs    | 113 +++++++++++++++++++++++++++++++-
 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/DataStoreList.js  |  39 +++++++++++
 www/window/DataStoreEdit.js     |   3 +-
 8 files changed, 194 insertions(+), 8 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 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..33e76b27 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::refresh_datastores().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..e2953417 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(
+        "refresh-datastores".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..aa068a07 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 refresh_datastores() -> 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\":\"refresh-datastores\"}\n")
+        .await?;
+    Ok(())
+}
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/4] api2/confif/datastore: add create datastore api call in 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 | 107 ++++++++++++++++++++++++++++++++++-
 www/window/DataStoreEdit.js  |   3 +-
 2 files changed, 108 insertions(+), 2 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 33e76b27..16ac030f 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -13,7 +13,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: {
@@ -143,6 +143,110 @@ pub fn create_datastore(param: Value) -> Result<(), Error> {
     Ok(())
 }
 
+#[api(
+    protected: true,
+    input: {
+        properties: {
+            name: {
+                schema: DATASTORE_SCHEMA,
+            },
+            path: {
+                schema: DIR_NAME_SCHEMA,
+            },
+            comment: {
+                optional: true,
+                schema: SINGLE_LINE_COMMENT_SCHEMA,
+            },
+            "notify-user": {
+                optional: true,
+                type: Userid,
+            },
+            "notify": {
+                optional: true,
+                schema: DATASTORE_NOTIFY_STRING_SCHEMA,
+            },
+            "gc-schedule": {
+                optional: true,
+                schema: GC_SCHEDULE_SCHEMA,
+            },
+            "prune-schedule": {
+                optional: true,
+                schema: PRUNE_SCHEDULE_SCHEMA,
+            },
+            "keep-last": {
+                optional: true,
+                schema: PRUNE_SCHEMA_KEEP_LAST,
+            },
+            "keep-hourly": {
+                optional: true,
+                schema: PRUNE_SCHEMA_KEEP_HOURLY,
+            },
+            "keep-daily": {
+                optional: true,
+                schema: PRUNE_SCHEMA_KEEP_DAILY,
+            },
+            "keep-weekly": {
+                optional: true,
+                schema: PRUNE_SCHEMA_KEEP_WEEKLY,
+            },
+            "keep-monthly": {
+                optional: true,
+                schema: PRUNE_SCHEMA_KEEP_MONTHLY,
+            },
+            "keep-yearly": {
+                optional: true,
+                schema: PRUNE_SCHEMA_KEEP_YEARLY,
+            },
+        },
+    },
+    access: {
+        permission: &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
+    },
+)]
+/// Create new datastore config.
+pub fn create_datastore_worker(
+    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 datastore: datastore::DataStoreConfig = serde_json::from_value(param)?;
+
+    let (mut config, _digest) = datastore::config()?;
+
+    if config.sections.get(&datastore.name).is_some() {
+        bail!("datastore '{}' already exists.", datastore.name);
+    }
+
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+
+    let upid_str = WorkerTask::new_thread(
+        "create-datastore",
+        Some(datastore.name.to_string()),
+        auth_id,
+        false,
+        move |_worker| {
+            let _lock = lock; // keep the lock
+            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(())
+        },
+    )?;
+
+    Ok(upid_str)
+}
+
 #[api(
    input: {
         properties: {
@@ -438,4 +542,5 @@ const ITEM_ROUTER: Router = Router::new()
 pub const ROUTER: Router = Router::new()
     .get(&API_METHOD_LIST_DATASTORES)
     .post(&API_METHOD_CREATE_DATASTORE)
+    .put(&API_METHOD_CREATE_DATASTORE_WORKER)
     .match_all("name", &ITEM_ROUTER);
diff --git a/www/window/DataStoreEdit.js b/www/window/DataStoreEdit.js
index c2b2809f..34ab74b9 100644
--- a/www/window/DataStoreEdit.js
+++ b/www/window/DataStoreEdit.js
@@ -75,6 +75,8 @@ Ext.define('PBS.DataStoreEdit', {
     isAdd: true,
 
     bodyPadding: 0,
+    method: 'PUT',
+    showProgress: true,
 
     cbindData: function(initialConfig) {
 	var me = this;
@@ -87,7 +89,6 @@ Ext.define('PBS.DataStoreEdit', {
 	    me.defaultFocus = 'textfield[name=comment]';
 	}
 	me.url = name ? baseurl + '/' + name : baseurl;
-	me.method = name ? 'PUT' : 'POST';
 	me.scheduleValue = name ? null : 'daily';
 	me.autoLoad = !!name;
 	return {};
-- 
2.20.1

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

[Dominik Csapak]

and enable it for the worker variant

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/config/datastore.rs |  6 +++---
 src/backup/chunk_store.rs    | 11 +++++++----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 16ac030f..5c2ad050 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -131,7 +131,7 @@ pub fn create_datastore(param: Value) -> 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, None)?;
 
     config.set_data(&datastore.name, "datastore", &datastore)?;
 
@@ -226,12 +226,12 @@ pub fn create_datastore_worker(
         Some(datastore.name.to_string()),
         auth_id,
         false,
-        move |_worker| {
+        move |worker| {
             let _lock = lock; // keep the lock
             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, Some(&worker))?;
 
             config.set_data(&datastore.name, "datastore", &datastore)?;
 
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] [PATCH proxmox-backup 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/DataStoreList.js | 39 ++++++++++++++++++++++++++++++++++
 2 files changed, 40 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/DataStoreList.js b/www/datastore/DataStoreList.js
index 353709d3..f81b25b7 100644
--- a/www/datastore/DataStoreList.js
+++ b/www/datastore/DataStoreList.js
@@ -113,11 +113,50 @@ Ext.define('PBS.datastore.DataStoreList', {
 
 	me.datastores[data.store] = me.insert(i, {
 	    datastore: data.store,
+	    header: {
+		padding: '6 6 7 9',
+	    },
+	    tools: [
+		{
+		    xtype: 'button',
+		    text: gettext('Remove'),
+		    iconCls: 'fa fa-trash-o',
+		    handler: () => me.removeDatastore(data.store),
+		    padding: 2,
+		},
+	    ],
 	});
 	me.datastores[data.store].setStatus(data);
 	me.datastores[data.store].setTasks(me.tasks[data.store], me.since);
     },
 
+    removeDatastore: function(datastore) {
+	let me = this;
+	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: () => {
+		    me.reload();
+		    let navtree = Ext.ComponentQuery.query('navigationtree')[0];
+		    navtree.rstore.load();
+		},
+	    },
+	});
+    },
+
+    reload: function() {
+	let me = this;
+	me.mask();
+	me.usageStore.load();
+	me.taskStore.load();
+    },
+
     initComponent: function() {
 	let me = this;
 	me.items = [
-- 
2.20.1

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

[Thomas Lamprecht]

On 01.06.21 14:13, 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..33e76b27 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::refresh_datastores().await?;

two methods named exactly the same, one async the other not is a bit weird, even
if in different modules. 

I'd, Either follow the name scheme from the cert command for the send command over
socket helper, e.g., refresh_proxy_datastores or notify_datastore_removed (in
theory the refresh of the internal datastore map may not be the only thing we
want to do (in the future)); just to name two examples for improving on the
duplicate function names

> +
>      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);
> +        }

above feels a bit odd as the section config effectively is a hashmap already
and it's loaded anyway

> +
> +        let mut map = DATASTORE_MAP.lock().unwrap();
> +        // removes all elements that are not in the config
> +        map.retain(|key, _| {
> +            stores.contains(key)
> +        });

Single expression closures do not need { }

map.retain(|key| stores.contains(key)); // drop removed datastores

> +        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..e2953417 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(
> +        "refresh-datastores".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..aa068a07 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 refresh_datastores() -> 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\":\"refresh-datastores\"}\n")
> +        .await?;
> +    Ok(())
> +}
> 

Re: [pbs-devel] [PATCH proxmox-backup 2/4] api2/confif/datastore: add create datastore api call in a worker

[Thomas Lamprecht]

On 01.06.21 14:13, 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
> 

In PVE we did not treated a changed in return value as breaking change, I mean not
in general, as it is a bit depended on what was returned previously, but we didn't
considered it if there was nothing returned before.

I'd rather do one either:
* wait for the 2.0 branch and just move over the existing one to always return a task
  UPID, least amount of work and as 2.0 is not that far away it would be quite reasonable
  IMO

* just switch now, the manager CLI can be adapted in one go, and the gui can switch too
  as both are shipped by the exact same binary package, also OK IMO, but others may be
  have objections (?)

* return the upid as option and add an opt-in switch for the in worker, and if not set
  then poll in sync for it being finished, could be removed with 2.0 or just the default
  switched

mostly as I'd like to avoid duplicate entries of endpoints where we plan to use the new
one only anyway (as quite some metadata IO is involved it's effectively the only sensible
one).

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/api2/config/datastore.rs | 107 ++++++++++++++++++++++++++++++++++-
>  www/window/DataStoreEdit.js  |   3 +-
>  2 files changed, 108 insertions(+), 2 deletions(-)
> 
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 33e76b27..16ac030f 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -13,7 +13,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: {
> @@ -143,6 +143,110 @@ pub fn create_datastore(param: Value) -> Result<(), Error> {
>      Ok(())
>  }
>  
> +#[api(
> +    protected: true,
> +    input: {
> +        properties: {
> +            name: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            path: {
> +                schema: DIR_NAME_SCHEMA,
> +            },
> +            comment: {
> +                optional: true,
> +                schema: SINGLE_LINE_COMMENT_SCHEMA,
> +            },
> +            "notify-user": {
> +                optional: true,
> +                type: Userid,
> +            },
> +            "notify": {
> +                optional: true,
> +                schema: DATASTORE_NOTIFY_STRING_SCHEMA,
> +            },
> +            "gc-schedule": {
> +                optional: true,
> +                schema: GC_SCHEDULE_SCHEMA,
> +            },
> +            "prune-schedule": {
> +                optional: true,
> +                schema: PRUNE_SCHEDULE_SCHEMA,
> +            },
> +            "keep-last": {
> +                optional: true,
> +                schema: PRUNE_SCHEMA_KEEP_LAST,
> +            },
> +            "keep-hourly": {
> +                optional: true,
> +                schema: PRUNE_SCHEMA_KEEP_HOURLY,
> +            },
> +            "keep-daily": {
> +                optional: true,
> +                schema: PRUNE_SCHEMA_KEEP_DAILY,
> +            },
> +            "keep-weekly": {
> +                optional: true,
> +                schema: PRUNE_SCHEMA_KEEP_WEEKLY,
> +            },
> +            "keep-monthly": {
> +                optional: true,
> +                schema: PRUNE_SCHEMA_KEEP_MONTHLY,
> +            },
> +            "keep-yearly": {
> +                optional: true,
> +                schema: PRUNE_SCHEMA_KEEP_YEARLY,
> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore"], PRIV_DATASTORE_ALLOCATE, false),
> +    },
> +)]
> +/// Create new datastore config.
> +pub fn create_datastore_worker(
> +    param: Value,

for new API endpoints, even if copied, it'd be great to avoid `param: Value`

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

[Thomas Lamprecht]

On 01.06.21 14:13, 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)
> 

we could move that one to options, it's not a frequent operation and moving out of
the probably more frequented views would seem reasonable.
It'd put it there at the right of the tbar 

And we may want to consider adding a "purge data" opt-in checkbox in the long run
(with some bells and whistles to ensure the user has feedback about the difference),
as I do not really see the use-case for just removing it from the config happen that
often in practice.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/Utils.js                   |  1 +
>  www/datastore/DataStoreList.js | 39 ++++++++++++++++++++++++++++++++++
>  2 files changed, 40 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/DataStoreList.js b/www/datastore/DataStoreList.js
> index 353709d3..f81b25b7 100644
> --- a/www/datastore/DataStoreList.js
> +++ b/www/datastore/DataStoreList.js
> @@ -113,11 +113,50 @@ Ext.define('PBS.datastore.DataStoreList', {
>  
>  	me.datastores[data.store] = me.insert(i, {
>  	    datastore: data.store,
> +	    header: {
> +		padding: '6 6 7 9',
> +	    },
> +	    tools: [
> +		{
> +		    xtype: 'button',
> +		    text: gettext('Remove'),
> +		    iconCls: 'fa fa-trash-o',
> +		    handler: () => me.removeDatastore(data.store),
> +		    padding: 2,
> +		},
> +	    ],
>  	});
>  	me.datastores[data.store].setStatus(data);
>  	me.datastores[data.store].setTasks(me.tasks[data.store], me.since);
>      },
>  
> +    removeDatastore: function(datastore) {
> +	let me = this;
> +	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: () => {
> +		    me.reload();
> +		    let navtree = Ext.ComponentQuery.query('navigationtree')[0];
> +		    navtree.rstore.load();
> +		},
> +	    },
> +	});
> +    },
> +
> +    reload: function() {
> +	let me = this;
> +	me.mask();
> +	me.usageStore.load();
> +	me.taskStore.load();
> +    },
> +
>      initComponent: function() {
>  	let me = this;
>  	me.items = [
> 

Re: [pbs-devel] [PATCH proxmox-backup 2/4] api2/confif/datastore: add create datastore api call in a worker

[Dietmar Maurer]

On 6/1/21 4:33 PM, Thomas Lamprecht wrote:
> On 01.06.21 14:13, 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
>>
> In PVE we did not treated a changed in return value as breaking change, I mean not
> in general, as it is a bit depended on what was returned previously, but we didn't
> considered it if there was nothing returned before.
>
> I'd rather do one either:
> * wait for the 2.0 branch and just move over the existing one to always return a task
>    UPID, least amount of work and as 2.0 is not that far away it would be quite reasonable
>    IMO
>
> * just switch now, the manager CLI can be adapted in one go, and the gui can switch too
>    as both are shipped by the exact same binary package, also OK IMO, but others may be
>    have objections (?)

I prefer above option (just switch now).

>
> * return the upid as option and add an opt-in switch for the in worker, and if not set
>    then poll in sync for it being finished, could be removed with 2.0 or just the default
>    switched
>
> mostly as I'd like to avoid duplicate entries of endpoints where we plan to use the new
> one only anyway (as quite some metadata IO is involved it's effectively the only sensible
> one).
>
>