public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function
@ 2024-05-03 14:29 Gabriel Goller
  2024-05-03 14:29 ` [pbs-devel] [PATCH proxmox-backup 2/2] disks: unify default datastore creation Gabriel Goller
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Gabriel Goller @ 2024-05-03 14:29 UTC (permalink / raw)
  To: pbs-devel

Add a function to create a default datastore. This datastore will have a
default prune-job with a daily schedule and a gc-job with a daily
schedule as well.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/api2/config/datastore.rs | 92 ++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 42 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 6b742acb..6cb3b21a 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -1,4 +1,5 @@
 use std::path::PathBuf;
+use std::sync::Arc;
 
 use ::serde::{Deserialize, Serialize};
 use anyhow::Error;
@@ -66,6 +67,54 @@ pub fn list_datastores(
     Ok(list.into_iter().filter(filter_by_privs).collect())
 }
 
+/// Creates a default datastore with the provided config.
+/// Also adds a gc schedule (daily) and the default prune-job (daily).
+pub(crate) fn create_default_datastore(
+    config: DataStoreConfig,
+    worker: Arc<WorkerTask>,
+) -> Result<(), Error> {
+    let lock = pbs_config::datastore::lock_config()?;
+
+    let (section_config, _digest) = pbs_config::datastore::config()?;
+
+    if section_config.sections.get(&config.name).is_some() {
+        param_bail!("name", "datastore '{}' already exists.", config.name);
+    }
+
+    let prune_job_config = config.prune_schedule.as_ref().map(|schedule| {
+        let mut id = format!("default-{}-{}", config.name, Uuid::generate());
+        id.truncate(32);
+
+        PruneJobConfig {
+            id,
+            store: config.name.clone(),
+            comment: None,
+            disable: false,
+            schedule: schedule.clone(),
+            options: PruneJobOptions {
+                keep: config.keep.clone(),
+                max_depth: None,
+                ns: None,
+            },
+        }
+    });
+
+    // clearing prune settings in the datastore config, as they are now handled by prune jobs
+    let config = DataStoreConfig {
+        prune_schedule: None,
+        keep: KeepOptions::default(),
+        ..config
+    };
+
+    do_create_datastore(lock, section_config, config, Some(&worker))?;
+
+    if let Some(prune_job_config) = prune_job_config {
+        do_create_prune_job(prune_job_config, Some(&worker))
+    } else {
+        Ok(())
+    }
+}
+
 pub(crate) fn do_create_datastore(
     _lock: BackupLockGuard,
     mut config: SectionConfigData,
@@ -114,56 +163,15 @@ pub fn create_datastore(
     config: DataStoreConfig,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
-    let lock = pbs_config::datastore::lock_config()?;
-
-    let (section_config, _digest) = pbs_config::datastore::config()?;
-
-    if section_config.sections.get(&config.name).is_some() {
-        param_bail!("name", "datastore '{}' already exists.", config.name);
-    }
-
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
 
-    let prune_job_config = config.prune_schedule.as_ref().map(|schedule| {
-        let mut id = format!("default-{}-{}", config.name, Uuid::generate());
-        id.truncate(32);
-
-        PruneJobConfig {
-            id,
-            store: config.name.clone(),
-            comment: None,
-            disable: false,
-            schedule: schedule.clone(),
-            options: PruneJobOptions {
-                keep: config.keep.clone(),
-                max_depth: None,
-                ns: None,
-            },
-        }
-    });
-
-    // clearing prune settings in the datastore config, as they are now handled by prune jobs
-    let config = DataStoreConfig {
-        prune_schedule: None,
-        keep: KeepOptions::default(),
-        ..config
-    };
-
     WorkerTask::new_thread(
         "create-datastore",
         Some(config.name.to_string()),
         auth_id.to_string(),
         to_stdout,
-        move |worker| {
-            do_create_datastore(lock, section_config, config, Some(&worker))?;
-
-            if let Some(prune_job_config) = prune_job_config {
-                do_create_prune_job(prune_job_config, Some(&worker))
-            } else {
-                Ok(())
-            }
-        },
+        move |worker| create_default_datastore(config, worker),
     )
 }
 
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 2/2] disks: unify default datastore creation
  2024-05-03 14:29 [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
@ 2024-05-03 14:29 ` Gabriel Goller
  2024-05-03 14:32 ` [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
  2024-05-13  8:52 ` Christian Ebner
  2 siblings, 0 replies; 7+ messages in thread
From: Gabriel Goller @ 2024-05-03 14:29 UTC (permalink / raw)
  To: pbs-devel

A datastore created with the "Add: ZFS" and "Add: Directory" button is
not the same as when created manually. The difference is that the
manually created one includes a default prune-job and a gc-job with a
daily schedule.
Use the create_default_datatore function so that every
datastore created will look the same.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 src/api2/node/disks/directory.rs | 23 ++++++-----------------
 src/api2/node/disks/zfs.rs       | 28 ++++++++++------------------
 2 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 9f1112a9..de5df953 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -1,6 +1,5 @@
 use ::serde::{Deserialize, Serialize};
 use anyhow::{bail, Error};
-use serde_json::json;
 use std::os::linux::fs::MetadataExt;
 
 use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
@@ -13,6 +12,7 @@ use pbs_api_types::{
     PRIV_SYS_MODIFY, UPID_SCHEMA,
 };
 
+use crate::api2::config::datastore::create_default_datastore;
 use crate::tools::disks::{
     create_file_system, create_single_linux_partition, get_fs_uuid, DiskManage, DiskUsageQuery,
     DiskUsageType, FileSystemType,
@@ -203,22 +203,11 @@ pub fn create_datastore_disk(
             crate::tools::systemd::start_unit(&mount_unit_name)?;
 
             if add_datastore {
-                let lock = pbs_config::datastore::lock_config()?;
-                let datastore: DataStoreConfig =
-                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
-
-                let (config, _digest) = pbs_config::datastore::config()?;
-
-                if config.sections.get(&datastore.name).is_some() {
-                    bail!("datastore '{}' already exists.", datastore.name);
-                }
-
-                crate::api2::config::datastore::do_create_datastore(
-                    lock,
-                    config,
-                    datastore,
-                    Some(&worker),
-                )?;
+                let mut config = DataStoreConfig::new(name, mount_point);
+                config.gc_schedule = Some("daily".to_string());
+                config.prune_schedule = Some("daily".to_string());
+
+                create_default_datastore(config, worker)?;
             }
 
             Ok(())
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 673dc1bf..6422f6fc 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -1,5 +1,5 @@
 use anyhow::{bail, Error};
-use serde_json::{json, Value};
+use serde_json::Value;
 
 use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
 use proxmox_schema::api;
@@ -11,8 +11,11 @@ use pbs_api_types::{
     ZFS_ASHIFT_SCHEMA, ZPOOL_NAME_SCHEMA,
 };
 
-use crate::tools::disks::{
-    parse_zpool_status_config_tree, vdev_list_to_tree, zpool_list, zpool_status, DiskUsageType,
+use crate::{
+    api2::config::datastore::create_default_datastore,
+    tools::disks::{
+        parse_zpool_status_config_tree, vdev_list_to_tree, zpool_list, zpool_status, DiskUsageType,
+    },
 };
 
 use proxmox_rest_server::WorkerTask;
@@ -309,22 +312,11 @@ pub fn create_zpool(
             };
 
             if add_datastore {
-                let lock = pbs_config::datastore::lock_config()?;
-                let datastore: DataStoreConfig =
-                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
-
-                let (config, _digest) = pbs_config::datastore::config()?;
-
-                if config.sections.get(&datastore.name).is_some() {
-                    bail!("datastore '{}' already exists.", datastore.name);
-                }
+                let mut config = DataStoreConfig::new(name, mount_point);
+                config.gc_schedule = Some("daily".to_string());
+                config.prune_schedule = Some("daily".to_string());
 
-                crate::api2::config::datastore::do_create_datastore(
-                    lock,
-                    config,
-                    datastore,
-                    Some(&worker),
-                )?;
+                create_default_datastore(config, worker)?;
             }
 
             Ok(())
-- 
2.43.0



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function
  2024-05-03 14:29 [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
  2024-05-03 14:29 ` [pbs-devel] [PATCH proxmox-backup 2/2] disks: unify default datastore creation Gabriel Goller
@ 2024-05-03 14:32 ` Gabriel Goller
  2024-05-03 14:49   ` Christian Ebner
  2024-05-13  8:52 ` Christian Ebner
  2 siblings, 1 reply; 7+ messages in thread
From: Gabriel Goller @ 2024-05-03 14:32 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion

I am sure this has been discussed before, but couldn't find anything on
the mailing list: Why don't we create a default verify job?

It seems sensible to me to add a default verify job that runs once a
week or once a month to every datastore (at least those created through
the UI).



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function
  2024-05-03 14:32 ` [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
@ 2024-05-03 14:49   ` Christian Ebner
  2024-05-03 15:31     ` Christian Ebner
  0 siblings, 1 reply; 7+ messages in thread
From: Christian Ebner @ 2024-05-03 14:49 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Gabriel Goller

> On 03.05.2024 16:32 CEST Gabriel Goller <g.goller@proxmox.com> wrote:
> 
>  
> I am sure this has been discussed before, but couldn't find anything on
> the mailing list: Why don't we create a default verify job?
> 
> It seems sensible to me to add a default verify job that runs once a
> week or once a month to every datastore (at least those created through
> the UI).

Without having looked at your patches yet, there has been some patches and discussion on this which you can find here https://lists.proxmox.com/pipermail/pbs-devel/2023-November/007342.html


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function
  2024-05-03 14:49   ` Christian Ebner
@ 2024-05-03 15:31     ` Christian Ebner
  0 siblings, 0 replies; 7+ messages in thread
From: Christian Ebner @ 2024-05-03 15:31 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Gabriel Goller

On 5/3/24 16:49, Christian Ebner wrote:
>> On 03.05.2024 16:32 CEST Gabriel Goller <g.goller@proxmox.com> wrote:
>>
>>   
>> I am sure this has been discussed before, but couldn't find anything on
>> the mailing list: Why don't we create a default verify job?
>>
>> It seems sensible to me to add a default verify job that runs once a
>> week or once a month to every datastore (at least those created through
>> the UI).
> 
> Without having looked at your patches yet, there has been some patches and discussion on this which you can find here https://lists.proxmox.com/pipermail/pbs-devel/2023-November/007342.html

Forgot to answer your question: verify jobs can be very IO intensive and 
the user might even not want to have one, e.g. if data integrity is 
provided by other means. So this is better set up manually.




_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function
  2024-05-03 14:29 [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
  2024-05-03 14:29 ` [pbs-devel] [PATCH proxmox-backup 2/2] disks: unify default datastore creation Gabriel Goller
  2024-05-03 14:32 ` [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
@ 2024-05-13  8:52 ` Christian Ebner
  2024-05-13  9:31   ` Gabriel Goller
  2 siblings, 1 reply; 7+ messages in thread
From: Christian Ebner @ 2024-05-13  8:52 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Gabriel Goller

On 5/3/24 16:29, Gabriel Goller wrote:
> Add a function to create a default datastore. This datastore will have a
> default prune-job with a daily schedule and a gc-job with a daily
> schedule as well.
> 

Tested this and found that while the default prune and 
garbage-collection jobs are created when creating the datastore via the 
WebUI or as part of a disk create transaction, this is not true if done 
via the CLI or API for the datastore create itself, by e.g. 
`proxmox-backup-manager datastore create <name> <path>`.
Also, this default behavior should be documented.

So similar to what Fabian already commented to patches from my side:
https://lists.proxmox.com/pipermail/pbs-devel/2023-November/007361.html



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function
  2024-05-13  8:52 ` Christian Ebner
@ 2024-05-13  9:31   ` Gabriel Goller
  0 siblings, 0 replies; 7+ messages in thread
From: Gabriel Goller @ 2024-05-13  9:31 UTC (permalink / raw)
  To: Christian Ebner, Proxmox Backup Server development discussion

On Mon May 13, 2024 at 10:52 AM CEST, Christian Ebner wrote:
> On 5/3/24 16:29, Gabriel Goller wrote:
> > Add a function to create a default datastore. This datastore will have a
> > default prune-job with a daily schedule and a gc-job with a daily
> > schedule as well.
> > 
>
> Tested this and found that while the default prune and 
> garbage-collection jobs are created when creating the datastore via the 
> WebUI or as part of a disk create transaction, this is not true if done 
> via the CLI or API for the datastore create itself, by e.g. 
> `proxmox-backup-manager datastore create <name> <path>`.

This was intended. I talked about this with Stefan Lendl as well when he
wrote the patches.

I think the idea was to create the default jobs when creating a
datastore through the ui (thus my patches above), but create a bare
datastore (without any jobs) when using the cli.

Obviously this is not so consistent.

> Also, this default behavior should be documented.

Absolutely.

>
> So similar to what Fabian already commented to patches from my side:
> https://lists.proxmox.com/pipermail/pbs-devel/2023-November/007361.html

Yes, fabian also mentioned the crucial point: when creating a datastore
through the ui, you have the option to *not* create the default prune/gc
jobs.

In my patch series above you cannot do that because it's done on the
api side.

IMO I would discard my series here... 

I don't even know if adding a button to the ui 'create default jobs' or
something like that would even be beneficial or just clutter the
interface.



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2024-05-13  9:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-05-03 14:29 [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
2024-05-03 14:29 ` [pbs-devel] [PATCH proxmox-backup 2/2] disks: unify default datastore creation Gabriel Goller
2024-05-03 14:32 ` [pbs-devel] [PATCH proxmox-backup 1/2] config: add default datastore creation function Gabriel Goller
2024-05-03 14:49   ` Christian Ebner
2024-05-03 15:31     ` Christian Ebner
2024-05-13  8:52 ` Christian Ebner
2024-05-13  9:31   ` Gabriel Goller

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal