[pbs-devel] [PATCH proxmox-backup 0/2] add default prune job and gc-schedule

[pbs-devel] [PATCH proxmox-backup 0/2] add default prune job and gc-schedule

[Christian Ebner]

These patches add a default prune job configuration and a default
garbage collection schedule to datastores created as part of disk/zpool
creation transactions.

This is to be consistent with the conventional creation of a datastore,
where these also are created so the user might not forget about it.

E.g.
`proxmox-backup-manager disk fs create <storage-name> --disk <block-device>
--add-datastore true` creates the datastore with daily gc-schedule and a
daily prune job with default keep options.

Currently, patch 0001 depends on this patch being applied:
https://lists.proxmox.com/pipermail/pbs-devel/2023-November/007336.html

I can send a v2 without this interdependecy if desired and/or required.

Christian Ebner (2):
  api: create default prune job on datastore creation
  api: create default garbage collection schedule

 src/api2/node/disks/directory.rs | 30 ++++++++++++++++++++++++++----
 src/api2/node/disks/zfs.rs       | 32 +++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 9 deletions(-)

-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 1/2] api: create default prune job on datastore creation

[Christian Ebner]

Create a default prune job when a datastore is created as part of a
disk/zpool creation transaction via e.g.:
`proxmox-backup-manager disk fs create <storage-name> --disk <block-device>
--add-datastore true`

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
Currently depends on this patch to apply:
https://lists.proxmox.com/pipermail/pbs-devel/2023-November/007336.html

I can send a v2 without this interdependecy if requested.

 src/api2/node/disks/directory.rs | 25 +++++++++++++++++++++++--
 src/api2/node/disks/zfs.rs       | 27 ++++++++++++++++++++++++---
 2 files changed, 47 insertions(+), 5 deletions(-)

diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index af2e1a14..89260b8e 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -6,10 +6,11 @@ use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
 use proxmox_schema::api;
 use proxmox_section_config::SectionConfigData;
 use proxmox_sys::task_log;
+use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
-    DataStoreConfig, BLOCKDEVICE_NAME_SCHEMA, STORAGE_NAME_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT,
-    PRIV_SYS_MODIFY, UPID_SCHEMA,
+    DataStoreConfig, KeepOptions, PruneJobConfig, PruneJobOptions, BLOCKDEVICE_NAME_SCHEMA,
+    NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, STORAGE_NAME_SCHEMA, UPID_SCHEMA,
 };
 
 use crate::tools::disks::{
@@ -210,6 +211,26 @@ pub fn create_datastore_disk(
                     datastore,
                     Some(&worker),
                 )?;
+
+                crate::api2::config::prune::do_create_prune_job(
+                    PruneJobConfig {
+                        id: {
+                            let mut id = format!("default-{}-{}", name, Uuid::generate());
+                            id.truncate(32);
+                            id
+                        },
+                        store: name.clone(),
+                        comment: None,
+                        disable: false,
+                        schedule: String::from("daily"),
+                        options: PruneJobOptions {
+                            keep: KeepOptions::default(),
+                            max_depth: None,
+                            ns: None,
+                        },
+                    },
+                    Some(&worker),
+                )?;
             }
 
             Ok(())
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index e225b9a7..7bc46986 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -4,11 +4,12 @@ use serde_json::{json, Value};
 use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
 use proxmox_schema::api;
 use proxmox_sys::{task_error, task_log};
+use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
-    DataStoreConfig, ZfsCompressionType, ZfsRaidLevel, ZpoolListItem, STORAGE_NAME_SCHEMA,
-    DISK_ARRAY_SCHEMA, DISK_LIST_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
-    ZFS_ASHIFT_SCHEMA, ZPOOL_NAME_SCHEMA,
+    DataStoreConfig, KeepOptions, PruneJobConfig, PruneJobOptions, ZfsCompressionType,
+    ZfsRaidLevel, ZpoolListItem, DISK_ARRAY_SCHEMA, DISK_LIST_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT,
+    PRIV_SYS_MODIFY, STORAGE_NAME_SCHEMA, UPID_SCHEMA, ZFS_ASHIFT_SCHEMA, ZPOOL_NAME_SCHEMA,
 };
 
 use crate::tools::disks::{
@@ -325,6 +326,26 @@ pub fn create_zpool(
                     datastore,
                     Some(&worker),
                 )?;
+
+                crate::api2::config::prune::do_create_prune_job(
+                    PruneJobConfig {
+                        id: {
+                            let mut id = format!("default-{}-{}", name, Uuid::generate());
+                            id.truncate(32);
+                            id
+                        },
+                        store: name.clone(),
+                        comment: None,
+                        disable: false,
+                        schedule: String::from("daily"),
+                        options: PruneJobOptions {
+                            keep: KeepOptions::default(),
+                            max_depth: None,
+                            ns: None,
+                        },
+                    },
+                    Some(&worker),
+                )?;
             }
 
             Ok(())
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 2/2] api: create default garbage collection schedule

[Christian Ebner]

Create a default garbage collection schedule when a datastore is created
as part of a disk/zpool creation transaction via e.g.
`proxmox-backup-manager disk fs create <storage-name> --disk <block-device>
--add-datastore true`

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/node/disks/directory.rs | 5 +++--
 src/api2/node/disks/zfs.rs       | 5 +++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
index 89260b8e..a3e1ba71 100644
--- a/src/api2/node/disks/directory.rs
+++ b/src/api2/node/disks/directory.rs
@@ -196,8 +196,9 @@ pub fn create_datastore_disk(
 
             if add_datastore {
                 let lock = pbs_config::datastore::lock_config()?;
-                let datastore: DataStoreConfig =
-                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
+                let datastore: DataStoreConfig = serde_json::from_value(
+                    json!({ "name": name, "path": mount_point, "gc-schedule": String::from("daily") }),
+                )?;
 
                 let (config, _digest) = pbs_config::datastore::config()?;
 
diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
index 7bc46986..7b8a3891 100644
--- a/src/api2/node/disks/zfs.rs
+++ b/src/api2/node/disks/zfs.rs
@@ -311,8 +311,9 @@ 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 datastore: DataStoreConfig = serde_json::from_value(
+                    json!({ "name": name, "path": mount_point, "gc-scheudle": String::from("daily") }),
+                )?;
 
                 let (config, _digest) = pbs_config::datastore::config()?;
 
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api: create default prune job on datastore creation

[Fabian Grünbichler]

On November 29, 2023 12:36 pm, Christian Ebner wrote:
> Create a default prune job when a datastore is created as part of a
> disk/zpool creation transaction via e.g.:
> `proxmox-backup-manager disk fs create <storage-name> --disk <block-device>
> --add-datastore true`

is this documented anywhere?

`proxmox-backup-manager datastore create ..` does not add any
schedules/jobs by default, it's just the web UI that does (but that also
tells you about this, and allows you to override it!)

I agree that it probably makes sense, but it should be consistent
- web UI does it, but then it should display it (and it would need to
  become a new optional parameter here, maybe by extending
  "add_datastore", else overriding would not be possible)
- proxmox-backup-manager does it, but then it should also do it for a
  "plain" datastore creation by default?

> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> Currently depends on this patch to apply:
> https://lists.proxmox.com/pipermail/pbs-devel/2023-November/007336.html
> 
> I can send a v2 without this interdependecy if requested.
> 
>  src/api2/node/disks/directory.rs | 25 +++++++++++++++++++++++--
>  src/api2/node/disks/zfs.rs       | 27 ++++++++++++++++++++++++---
>  2 files changed, 47 insertions(+), 5 deletions(-)
> 
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index af2e1a14..89260b8e 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -6,10 +6,11 @@ use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
>  use proxmox_schema::api;
>  use proxmox_section_config::SectionConfigData;
>  use proxmox_sys::task_log;
> +use proxmox_uuid::Uuid;
>  
>  use pbs_api_types::{
> -    DataStoreConfig, BLOCKDEVICE_NAME_SCHEMA, STORAGE_NAME_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT,
> -    PRIV_SYS_MODIFY, UPID_SCHEMA,
> +    DataStoreConfig, KeepOptions, PruneJobConfig, PruneJobOptions, BLOCKDEVICE_NAME_SCHEMA,
> +    NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, STORAGE_NAME_SCHEMA, UPID_SCHEMA,
>  };
>  
>  use crate::tools::disks::{
> @@ -210,6 +211,26 @@ pub fn create_datastore_disk(
>                      datastore,
>                      Some(&worker),
>                  )?;
> +
> +                crate::api2::config::prune::do_create_prune_job(
> +                    PruneJobConfig {
> +                        id: {
> +                            let mut id = format!("default-{}-{}", name, Uuid::generate());
> +                            id.truncate(32);
> +                            id
> +                        },
> +                        store: name.clone(),
> +                        comment: None,
> +                        disable: false,
> +                        schedule: String::from("daily"),
> +                        options: PruneJobOptions {
> +                            keep: KeepOptions::default(),
> +                            max_depth: None,
> +                            ns: None,
> +                        },
> +                    },
> +                    Some(&worker),
> +                )?;
>              }
>  
>              Ok(())
> diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
> index e225b9a7..7bc46986 100644
> --- a/src/api2/node/disks/zfs.rs
> +++ b/src/api2/node/disks/zfs.rs
> @@ -4,11 +4,12 @@ use serde_json::{json, Value};
>  use proxmox_router::{Permission, Router, RpcEnvironment, RpcEnvironmentType};
>  use proxmox_schema::api;
>  use proxmox_sys::{task_error, task_log};
> +use proxmox_uuid::Uuid;
>  
>  use pbs_api_types::{
> -    DataStoreConfig, ZfsCompressionType, ZfsRaidLevel, ZpoolListItem, STORAGE_NAME_SCHEMA,
> -    DISK_ARRAY_SCHEMA, DISK_LIST_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT, PRIV_SYS_MODIFY, UPID_SCHEMA,
> -    ZFS_ASHIFT_SCHEMA, ZPOOL_NAME_SCHEMA,
> +    DataStoreConfig, KeepOptions, PruneJobConfig, PruneJobOptions, ZfsCompressionType,
> +    ZfsRaidLevel, ZpoolListItem, DISK_ARRAY_SCHEMA, DISK_LIST_SCHEMA, NODE_SCHEMA, PRIV_SYS_AUDIT,
> +    PRIV_SYS_MODIFY, STORAGE_NAME_SCHEMA, UPID_SCHEMA, ZFS_ASHIFT_SCHEMA, ZPOOL_NAME_SCHEMA,
>  };
>  
>  use crate::tools::disks::{
> @@ -325,6 +326,26 @@ pub fn create_zpool(
>                      datastore,
>                      Some(&worker),
>                  )?;
> +
> +                crate::api2::config::prune::do_create_prune_job(
> +                    PruneJobConfig {
> +                        id: {
> +                            let mut id = format!("default-{}-{}", name, Uuid::generate());
> +                            id.truncate(32);
> +                            id
> +                        },
> +                        store: name.clone(),
> +                        comment: None,
> +                        disable: false,
> +                        schedule: String::from("daily"),
> +                        options: PruneJobOptions {
> +                            keep: KeepOptions::default(),
> +                            max_depth: None,
> +                            ns: None,
> +                        },
> +                    },
> +                    Some(&worker),
> +                )?;
>              }
>  
>              Ok(())
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [PATCH proxmox-backup 2/2] api: create default garbage collection schedule

[Fabian Grünbichler]

one nit below, but the same general remark as with patch #1 applies here
as well..

On November 29, 2023 12:36 pm, Christian Ebner wrote:
> Create a default garbage collection schedule when a datastore is created
> as part of a disk/zpool creation transaction via e.g.
> `proxmox-backup-manager disk fs create <storage-name> --disk <block-device>
> --add-datastore true`
> 
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
>  src/api2/node/disks/directory.rs | 5 +++--
>  src/api2/node/disks/zfs.rs       | 5 +++--
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/src/api2/node/disks/directory.rs b/src/api2/node/disks/directory.rs
> index 89260b8e..a3e1ba71 100644
> --- a/src/api2/node/disks/directory.rs
> +++ b/src/api2/node/disks/directory.rs
> @@ -196,8 +196,9 @@ pub fn create_datastore_disk(
>  
>              if add_datastore {
>                  let lock = pbs_config::datastore::lock_config()?;
> -                let datastore: DataStoreConfig =
> -                    serde_json::from_value(json!({ "name": name, "path": mount_point }))?;
> +                let datastore: DataStoreConfig = serde_json::from_value(
> +                    json!({ "name": name, "path": mount_point, "gc-schedule": String::from("daily") }),
> +                )?;
>  
>                  let (config, _digest) = pbs_config::datastore::config()?;
>  
> diff --git a/src/api2/node/disks/zfs.rs b/src/api2/node/disks/zfs.rs
> index 7bc46986..7b8a3891 100644
> --- a/src/api2/node/disks/zfs.rs
> +++ b/src/api2/node/disks/zfs.rs
> @@ -311,8 +311,9 @@ 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 datastore: DataStoreConfig = serde_json::from_value(
> +                    json!({ "name": name, "path": mount_point, "gc-scheudle": String::from("daily") }),

typo: s/scheudle/schedule

> +                )?;
>  
>                  let (config, _digest) = pbs_config::datastore::config()?;
>  
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api: create default prune job on datastore creation

[Thomas Lamprecht]

Am 29/11/2023 um 14:27 schrieb Fabian Grünbichler:
> On November 29, 2023 12:36 pm, Christian Ebner wrote:
>> Create a default prune job when a datastore is created as part of a
>> disk/zpool creation transaction via e.g.:
>> `proxmox-backup-manager disk fs create <storage-name> --disk <block-device>
>> --add-datastore true`
> is this documented anywhere?
> 
> `proxmox-backup-manager datastore create ..` does not add any
> schedules/jobs by default, it's just the web UI that does (but that also
> tells you about this, and allows you to override it!)
> 
> I agree that it probably makes sense, but it should be consistent
> - web UI does it, but then it should display it (and it would need to
>   become a new optional parameter here, maybe by extending
>   "add_datastore", else overriding would not be possible)
> - proxmox-backup-manager does it, but then it should also do it for a
>   "plain" datastore creation by default?

yeah, API might be indeed odd, especially if it happens unconditionally.

It might be better in general to add a hint to the UI (e.g., the datastore
summary) if there's no prune job and/or no garbage-collection schedule.

Re: [pbs-devel] [PATCH proxmox-backup 1/2] api: create default prune job on datastore creation

[Christian Ebner]

> On 29.11.2023 14:27 CET Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
> is this documented anywhere?

No, there is no documentation on this. I will have to include that as well in a new
version of the patches.

> `proxmox-backup-manager datastore create ..` does not add any
> schedules/jobs by default, it's just the web UI that does (but that also
> tells you about this, and allows you to override it!)

Yes, that is indeed true. I would argue for an increased usability by a default
creation of prune job and garbage collection schedules with well documented defaults
for all possible datastore creation routes, and allow to override these defaults via
the optional parameters.
Are there objections to such a change in behavior? This should further
be considered a breaking change for the API, I suppose.
 
> 
> I agree that it probably makes sense, but it should be consistent
> - web UI does it, but then it should display it (and it would need to
>   become a new optional parameter here, maybe by extending
>   "add_datastore", else overriding would not be possible)
> - proxmox-backup-manager does it, but then it should also do it for a
>   "plain" datastore creation by default?

You are right in that these patches are not sufficient to create consistent behavior
for all possible cases. I will have to refine and extend this.

Thank you for your feedback!

Cheers,
Chris