[pbs-devel] [PATCH proxmox-backup] reuse-datastore: avoid creating another default prune job

[pbs-devel] [PATCH proxmox-backup] reuse-datastore: avoid creating another default prune job

[Gabriel Goller]

If a datastore with a default prune job is removed, the prune job is
preserverd as it is stored in /etc/proxmox-backup/prune.cfg. We also
create a default prune job for every datastore – this means that when
reusing a datastore that previously existed, you end up with duplicate
prune jobs.

Reported-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---

This is a bit janky, because we rely on the default datastore being
named `default-{datastore}`, but that shouldn't be an issue.

 src/api2/config/datastore.rs | 41 ++++++++++++++++++++----------------
 src/api2/config/prune.rs     | 15 +++++++++++++
 2 files changed, 38 insertions(+), 18 deletions(-)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 37d1528c70fb..cbe67cfc6ac5 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -23,7 +23,9 @@ use pbs_datastore::chunk_store::ChunkStore;
 use crate::api2::admin::{
     prune::list_prune_jobs, sync::list_config_sync_jobs, verify::list_verification_jobs,
 };
-use crate::api2::config::prune::{delete_prune_job, do_create_prune_job};
+use crate::api2::config::prune::{
+    default_prune_job_existing, delete_prune_job, do_create_prune_job,
+};
 use crate::api2::config::sync::delete_sync_job;
 use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_backup_jobs};
 use crate::api2::config::verify::delete_verification_job;
@@ -150,23 +152,26 @@ pub fn create_datastore(
     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,
-            },
-        }
-    });
+    let mut prune_job_config = None;
+    if !default_prune_job_existing(&config.name)? {
+        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 {
diff --git a/src/api2/config/prune.rs b/src/api2/config/prune.rs
index ce7b8ce565ce..dafb97e2f1e5 100644
--- a/src/api2/config/prune.rs
+++ b/src/api2/config/prune.rs
@@ -77,6 +77,21 @@ pub fn do_create_prune_job(config: PruneJobConfig) -> Result<(), Error> {
     Ok(())
 }
 
+pub fn default_prune_job_existing(datastore: &str) -> Result<bool, Error> {
+    let (section_config, _digest) = prune::config()?;
+    if section_config
+        .sections
+        .keys()
+        .filter(|s| s.starts_with(&format!("default-{datastore}")))
+        .count()
+        > 0
+    {
+        Ok(true)
+    } else {
+        Ok(false)
+    }
+}
+
 #[api(
     protected: true,
     input: {
-- 
2.39.5



_______________________________________________
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] reuse-datastore: avoid creating another default prune job

[Christian Ebner]

On 11/22/24 12:11, Gabriel Goller wrote:
> If a datastore with a default prune job is removed, the prune job is
> preserverd as it is stored in /etc/proxmox-backup/prune.cfg. We also
> create a default prune job for every datastore – this means that when
> reusing a datastore that previously existed, you end up with duplicate
> prune jobs.
> 
> Reported-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
> 
> This is a bit janky, because we rely on the default datastore being
> named `default-{datastore}`, but that shouldn't be an issue.
> 
>   src/api2/config/datastore.rs | 41 ++++++++++++++++++++----------------
>   src/api2/config/prune.rs     | 15 +++++++++++++
>   2 files changed, 38 insertions(+), 18 deletions(-)
> 
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 37d1528c70fb..cbe67cfc6ac5 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -23,7 +23,9 @@ use pbs_datastore::chunk_store::ChunkStore;
>   use crate::api2::admin::{
>       prune::list_prune_jobs, sync::list_config_sync_jobs, verify::list_verification_jobs,
>   };
> -use crate::api2::config::prune::{delete_prune_job, do_create_prune_job};
> +use crate::api2::config::prune::{
> +    default_prune_job_existing, delete_prune_job, do_create_prune_job,
> +};
>   use crate::api2::config::sync::delete_sync_job;
>   use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_backup_jobs};
>   use crate::api2::config::verify::delete_verification_job;
> @@ -150,23 +152,26 @@ pub fn create_datastore(
>       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,
> -            },
> -        }
> -    });
> +    let mut prune_job_config = None;
> +    if !default_prune_job_existing(&config.name)? {
> +        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 {
> diff --git a/src/api2/config/prune.rs b/src/api2/config/prune.rs
> index ce7b8ce565ce..dafb97e2f1e5 100644
> --- a/src/api2/config/prune.rs
> +++ b/src/api2/config/prune.rs
> @@ -77,6 +77,21 @@ pub fn do_create_prune_job(config: PruneJobConfig) -> Result<(), Error> {
>       Ok(())
>   }
>   
> +pub fn default_prune_job_existing(datastore: &str) -> Result<bool, Error> {
> +    let (section_config, _digest) = prune::config()?;
> +    if section_config
> +        .sections
> +        .keys()
> +        .filter(|s| s.starts_with(&format!("default-{datastore}")))
> +        .count()
> +        > 0
> +    {
> +        Ok(true)
> +    } else {
> +        Ok(false)
> +    }

could be more compact? no need for the if block:

let has_default = section_config.sections.keys().filter(...).count() >
0;
Ok(has_default)

> +}
> +
>   #[api(
>       protected: true,
>       input: {



_______________________________________________
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] reuse-datastore: avoid creating another default prune job

[Gabriel Goller]

On 22.11.2024 12:18, Christian Ebner wrote:
>On 11/22/24 12:11, Gabriel Goller wrote:
>>diff --git a/src/api2/config/prune.rs b/src/api2/config/prune.rs
>>index ce7b8ce565ce..dafb97e2f1e5 100644
>>--- a/src/api2/config/prune.rs
>>+++ b/src/api2/config/prune.rs
>>@@ -77,6 +77,21 @@ pub fn do_create_prune_job(config: PruneJobConfig) -> Result<(), Error> {
>>      Ok(())
>>  }
>>+pub fn default_prune_job_existing(datastore: &str) -> Result<bool, Error> {
>>+    let (section_config, _digest) = prune::config()?;
>>+    if section_config
>>+        .sections
>>+        .keys()
>>+        .filter(|s| s.starts_with(&format!("default-{datastore}")))
>>+        .count()
>>+        > 0
>>+    {
>>+        Ok(true)
>>+    } else {
>>+        Ok(false)
>>+    }
>
>could be more compact? no need for the if block:
>
>let has_default = section_config.sections.keys().filter(...).count() >
>0;
>Ok(has_default)

Yep, corrected it in v2!
Thanks for looking at it!

>>+}
>>+
>>  #[api(
>>      protected: true,
>>      input: {
>


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