[PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag

[PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag

[Hannes Laimer]

So GC can run before unmount, without this its somewhat complicated to
run gc on a datastore that is configured to auto unmount after jobs
finish.


Hannes Laimer (2):
  api: datastore: add option to run garbage collection before unmount
  ui: datastore: expose gc-on-unmount setting

 src/api2/admin/datastore.rs  | 33 +++++++++++++++++++++++++++------
 src/api2/config/datastore.rs |  9 +++++++++
 www/datastore/OptionView.js  | 23 +++++++++++++++++++++++
 3 files changed, 59 insertions(+), 6 deletions(-)

-- 
2.47.3

[PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount

[Hannes Laimer]

Removable datastores set up for auto-unmount have no natural point at
which to run garbage collection, since the drive is unmounted right
after jobs finish. Expose a gc-on-unmount option so GC can be triggered
as part of the unmount for those setups.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/admin/datastore.rs  | 33 +++++++++++++++++++++++++++------
 src/api2/config/datastore.rs |  9 +++++++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 757b3114..5ce30c57 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2641,15 +2641,36 @@ fn do_unmount_device(
 }
 
 async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<Value, Error> {
-    let _lock = pbs_config::datastore::lock_config()?;
-    let (mut section_config, _digest) = pbs_config::datastore::config()?;
-    let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
+    let gc_on_unmount = {
+        let _lock = pbs_config::datastore::lock_config()?;
+        let (section_config, _digest) = pbs_config::datastore::config()?;
+        let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
 
-    if datastore.backing_device.is_none() {
-        bail!("datastore '{store}' is not removable");
+        if datastore.backing_device.is_none() {
+            bail!("datastore '{store}' is not removable");
+        }
+
+        ensure_datastore_is_mounted(&datastore)?;
+
+        datastore.gc_on_unmount.unwrap_or(false)
+    };
+
+    // Must happen outside the config lock, the GC handler acquires it too.
+    if gc_on_unmount {
+        let client = crate::client_helpers::connect_to_localhost()
+            .context("failed to connect to localhost for starting GC")?;
+        match client
+            .post(&format!("api2/json/admin/datastore/{store}/gc"), None)
+            .await
+        {
+            Ok(_) => info!("started garbage collection, unmount will wait for it to finish"),
+            Err(err) => warn!("unable to start garbage collection before unmount: {err}"),
+        }
     }
 
-    ensure_datastore_is_mounted(&datastore)?;
+    let _lock = pbs_config::datastore::lock_config()?;
+    let (mut section_config, _digest) = pbs_config::datastore::config()?;
+    let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
 
     datastore.set_maintenance_mode(Some(MaintenanceMode {
         ty: MaintenanceType::Unmount,
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index c44d50d1..c0be0296 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -409,6 +409,8 @@ pub enum DeletableProperty {
     Comment,
     /// Delete the garbage collection schedule.
     GcSchedule,
+    /// Delete the gc-on-unmount property.
+    GcOnUnmount,
     /// Delete the prune job schedule.
     PruneSchedule,
     /// Delete the keep-last property
@@ -495,6 +497,9 @@ pub fn update_datastore(
                 DeletableProperty::GcSchedule => {
                     data.gc_schedule = None;
                 }
+                DeletableProperty::GcOnUnmount => {
+                    data.gc_on_unmount = None;
+                }
                 DeletableProperty::PruneSchedule => {
                     data.prune_schedule = None;
                 }
@@ -560,6 +565,10 @@ pub fn update_datastore(
         data.gc_schedule = update.gc_schedule;
     }
 
+    if update.gc_on_unmount.is_some() {
+        data.gc_on_unmount = update.gc_on_unmount;
+    }
+
     macro_rules! prune_disabled {
         ($(($param:literal, $($member:tt)+)),+) => {
             $(
-- 
2.47.3

[PATCH proxmox-backup 2/2] ui: datastore: expose gc-on-unmount setting

[Hannes Laimer]

Shows the gc-on-unmount option in the datastore options and allows
editing for removable datastores.

Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/datastore/OptionView.js | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 160edd44..2b35355a 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -104,6 +104,7 @@ Ext.define('PBS.Datastore.Options', {
         let record = listStore.findRecord('store', me.datastore, 0, false, true, true);
         return {
             s3Visible: record?.get('backend-type') === 's3' ? undefined : false,
+            removableVisible: record?.get('mount-status') !== 'nonremovable' ? undefined : false,
         };
     },
 
@@ -226,6 +227,28 @@ Ext.define('PBS.Datastore.Options', {
                 },
             },
         },
+        'gc-on-unmount': {
+            required: true,
+            header: gettext('GC on Unmount'),
+            defaultValue: false,
+            renderer: Proxmox.Utils.format_boolean,
+            cbind: {
+                visible: '{removableVisible}',
+            },
+            editor: {
+                xtype: 'proxmoxWindowEdit',
+                title: gettext('GC on Unmount'),
+                width: 350,
+                items: {
+                    xtype: 'proxmoxcheckbox',
+                    name: 'gc-on-unmount',
+                    boxLabel: gettext('Run garbage collection before unmounting'),
+                    defaultValue: false,
+                    deleteDefaultValue: true,
+                    deleteEmpty: true,
+                },
+            },
+        },
         'maintenance-mode': {
             required: true,
             header: gettext('Maintenance mode'),
-- 
2.47.3

Re: [PATCH proxmox-backup 1/2] api: datastore: add option to run garbage collection before unmount

[Christian Ebner]

On 4/17/26 1:26 PM, Hannes Laimer wrote:
> Removable datastores set up for auto-unmount have no natural point at
> which to run garbage collection, since the drive is unmounted right
> after jobs finish. Expose a gc-on-unmount option so GC can be triggered
> as part of the unmount for those setups.
> 
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/admin/datastore.rs  | 33 +++++++++++++++++++++++++++------
>   src/api2/config/datastore.rs |  9 +++++++++
>   2 files changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 757b3114..5ce30c57 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2641,15 +2641,36 @@ fn do_unmount_device(
>   }
>   
>   async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<Value, Error> {
> -    let _lock = pbs_config::datastore::lock_config()?;
> -    let (mut section_config, _digest) = pbs_config::datastore::config()?;
> -    let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
> +    let gc_on_unmount = {
> +        let _lock = pbs_config::datastore::lock_config()?;

question: Is the exclusive lock on the datastore config really required 
here? After all this only reads the config and checks it's parameters.

> +        let (section_config, _digest) = pbs_config::datastore::config()?;
> +        let datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
>   
> -    if datastore.backing_device.is_none() {
> -        bail!("datastore '{store}' is not removable");
> +        if datastore.backing_device.is_none() {
> +            bail!("datastore '{store}' is not removable");
> +        }
> +
> +        ensure_datastore_is_mounted(&datastore)?;
> +
> +        datastore.gc_on_unmount.unwrap_or(false)
> +    };
> +
> +    // Must happen outside the config lock, the GC handler acquires it too.
> +    if gc_on_unmount {
> +        let client = crate::client_helpers::connect_to_localhost()
> +            .context("failed to connect to localhost for starting GC")?;
> +        match client
> +            .post(&format!("api2/json/admin/datastore/{store}/gc"), None)

question: What about permissions here? Running garbage collection 
normally requires Datastore.Modify, but since a datastore config edit 
requires Datastore.Modify or creation via Datastore.Allocate I guess 
this is okay. Might however warrant a comment.

> +            .await
> +        {
> +            Ok(_) => info!("started garbage collection, unmount will wait for it to finish"),

comment: Why this works should be outlined in the commit message IMO. 
Maybe something along the lines of:
```
Relies on the active write operation by garbage collection to block the 
unmount task until GC completes (among possible other active 
operations). Further, no other active operation can occur in the mean 
time, since the datastore remains in maintenance mode `unmount`.
```

> +            Err(err) => warn!("unable to start garbage collection before unmount: {err}"),
> +        }
>       }
>   
> -    ensure_datastore_is_mounted(&datastore)?;
> +    let _lock = pbs_config::datastore::lock_config()?;
> +    let (mut section_config, _digest) = pbs_config::datastore::config()?;
> +    let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
>   
>       datastore.set_maintenance_mode(Some(MaintenanceMode {
>           ty: MaintenanceType::Unmount,
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index c44d50d1..c0be0296 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -409,6 +409,8 @@ pub enum DeletableProperty {
>       Comment,
>       /// Delete the garbage collection schedule.
>       GcSchedule,
> +    /// Delete the gc-on-unmount property.
> +    GcOnUnmount,
>       /// Delete the prune job schedule.
>       PruneSchedule,
>       /// Delete the keep-last property
> @@ -495,6 +497,9 @@ pub fn update_datastore(
>                   DeletableProperty::GcSchedule => {
>                       data.gc_schedule = None;
>                   }
> +                DeletableProperty::GcOnUnmount => {
> +                    data.gc_on_unmount = None;
> +                }
>                   DeletableProperty::PruneSchedule => {
>                       data.prune_schedule = None;
>                   }
> @@ -560,6 +565,10 @@ pub fn update_datastore(
>           data.gc_schedule = update.gc_schedule;
>       }
>   
> +    if update.gc_on_unmount.is_some() {
> +        data.gc_on_unmount = update.gc_on_unmount;
> +    }
> +
>       macro_rules! prune_disabled {
>           ($(($param:literal, $($member:tt)+)),+) => {
>               $(

Re: [PATCH proxmox-backup 0/2] fixes #7465: add gc-on-unmount flag

[Christian Ebner]

On 4/17/26 1:26 PM, Hannes Laimer wrote:
> So GC can run before unmount, without this its somewhat complicated to
> run gc on a datastore that is configured to auto unmount after jobs
> finish.
> 
> 
> Hannes Laimer (2):
>    api: datastore: add option to run garbage collection before unmount
>    ui: datastore: expose gc-on-unmount setting
> 
>   src/api2/admin/datastore.rs  | 33 +++++++++++++++++++++++++++------
>   src/api2/config/datastore.rs |  9 +++++++++
>   www/datastore/OptionView.js  | 23 +++++++++++++++++++++++
>   3 files changed, 59 insertions(+), 6 deletions(-)
> 


Looks good apart from a few smaller comments and questions.

I'm missing the patch for pbs-api-types though? (Adapted myself for 
review since it is a trivial adaption).