[PATCH proxmox{,-backup} 00/11] add GarbageCollection maintenance mode

[PATCH proxmox{,-backup} 00/11] add GarbageCollection maintenance mode

[Robert Obkircher]

Add a maintenance mode that allows reclaiming storage space without
the risk of running out of space because of new backups.

Changes since [RFC]:
* Excluded the space-check patches. I'll send them separately.
* Propagate maintenance mode parse errors.
* Rename Operation::Prune to Operation::WriteNonExpanding.
* Track them in active operations to display GC mode conflicts.
* Replace if-else with match in maintenance mode check.
* Fixed typos and added Bugzilla url trailers.

[RFC] https://lore.proxmox.com/pbs-devel/20260430150607.330413-1-r.obkircher@proxmox.com/


proxmox:

Robert Obkircher (5):
  pbs-api-types: propagate maintenance mode parse errors
  pbs-api-types: use match statement for maintenance mode check
  pbs-api-types: deny non-lookup operations for unknown modes
  pbs-api-types: add WriteNonExpanding datastore operation
  pbs-api-types: add GarbageCollection maintenance mode

 pbs-api-types/src/datastore.rs   | 23 ++++++++++-------
 pbs-api-types/src/maintenance.rs | 44 +++++++++++++++++---------------
 2 files changed, 37 insertions(+), 30 deletions(-)


proxmox-backup:

Robert Obkircher (6):
  datastore: propagate maintenance mode parse errors
  task tracking: use parameter for initial count and refactor updates
  www: access active operation fields by name instead of index
  task tracking: count WriteNonExpanding datastore operations
  datastore: open datastores with WriteNonExpanding instead of Write
  fix #5797: www: display new GarbageCollection maintenance mode

 pbs-datastore/src/datastore.rs      | 17 +++++---
 pbs-datastore/src/task_tracking.rs  | 64 +++++++++++++++++++----------
 src/api2/admin/datastore.rs         | 15 +++----
 src/api2/admin/namespace.rs         |  2 +-
 src/bin/proxmox-backup-proxy.rs     |  2 +-
 src/server/metric_collection/mod.rs |  7 ++--
 src/server/prune_job.rs             |  2 +-
 www/Utils.js                        |  4 ++
 www/datastore/OptionView.js         | 12 +++++-
 www/window/MaintenanceOptions.js    |  1 +
 10 files changed, 83 insertions(+), 43 deletions(-)


Summary over all repositories:
  12 files changed, 120 insertions(+), 73 deletions(-)

-- 
Generated by git-murpp 0.8.1

[PATCH proxmox 1/5] pbs-api-types: propagate maintenance mode parse errors

[Robert Obkircher]

Do not silently ignore errors if the config file is invalid or
contains a new mode from a future version.

Upgrades to the new GarbageCollection maintenance mode should be fine
even without this change, becausue it is unlikely that someone can
enable it before older processes stop initiating new backups or S3
refreshes. There is however a risk that a sync job could unmount the
datastore.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 7643b0de..b770bc03 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -629,18 +629,22 @@ impl DataStoreConfig {
         }
     }
 
-    pub fn get_maintenance_mode(&self) -> Option<MaintenanceMode> {
-        self.maintenance_mode.as_ref().and_then(|str| {
-            MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
-                str,
-                &MaintenanceMode::API_SCHEMA,
-            ))
-            .ok()
-        })
+    pub fn get_maintenance_mode(&self) -> Result<Option<MaintenanceMode>, Error> {
+        self.maintenance_mode
+            .as_ref()
+            .map(|str| {
+                MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
+                    str,
+                    &MaintenanceMode::API_SCHEMA,
+                ))
+                .map_err(|e| format_err!("failed to parse maintenance mode: {e}"))
+            })
+            .transpose()
     }
 
     pub fn set_maintenance_mode(&mut self, new_mode: Option<MaintenanceMode>) -> Result<(), Error> {
-        let current_type = self.get_maintenance_mode().map(|mode| mode.ty);
+        // it is probably best to error out if the current mode can't be parsed:
+        let current_type = self.get_maintenance_mode()?.map(|mode| mode.ty);
         let new_type = new_mode.as_ref().map(|mode| mode.ty);
 
         match current_type {
-- 
2.47.3

[PATCH proxmox 2/5] pbs-api-types: use match statement for maintenance mode check

[Robert Obkircher]

Use an exhaustive match statement to be more explicit and to draw
attention when new variants are added.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-api-types/src/maintenance.rs | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index 2adb5d84..7def498b 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -94,25 +94,22 @@ impl MaintenanceMode {
     }
 
     pub fn check(&self, operation: Operation) -> Result<(), Error> {
-        if self.ty == MaintenanceType::Delete {
-            bail!("datastore is being deleted");
-        }
-
         let message = percent_encoding::percent_decode_str(self.message.as_deref().unwrap_or(""))
             .decode_utf8()
             .unwrap_or(Cow::Borrowed(""));
 
-        if Operation::Lookup == operation {
-            return Ok(());
-        } else if self.ty == MaintenanceType::Unmount {
-            bail!("datastore is being unmounted");
-        } else if self.ty == MaintenanceType::Offline {
-            bail!("offline maintenance mode: {}", message);
-        } else if self.ty == MaintenanceType::S3Refresh {
-            bail!("S3 refresh maintenance mode: {}", message);
-        } else if self.ty == MaintenanceType::ReadOnly && Operation::Write == operation {
-            bail!("read-only maintenance mode: {}", message);
+        match (self.ty, operation) {
+            (MaintenanceType::Delete, _) => bail!("datastore is being deleted"),
+            (_, Operation::Lookup) => Ok(()),
+            (MaintenanceType::Unmount, _) => bail!("datastore is being unmounted"),
+            (MaintenanceType::Offline, _) => bail!("offline maintenance mode: {message}"),
+            (MaintenanceType::S3Refresh, _) => bail!("S3 refresh maintenance mode: {message}"),
+            (MaintenanceType::ReadOnly, Operation::Read) => Ok(()),
+            (MaintenanceType::ReadOnly, Operation::Write) => {
+                bail!("read-only maintenance mode: {message}")
+            }
+            #[cfg(feature = "enum-fallback")]
+            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
         }
-        Ok(())
     }
 }
-- 
2.47.3

[PATCH proxmox 3/5] pbs-api-types: deny non-lookup operations for unknown modes

[Robert Obkircher]

Denying non-lookup operations for unknown modes seems like a safer
default. This change should not affect anything because the backup
server does not enable the enum-fallback feature.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-api-types/src/maintenance.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index 7def498b..7e9599be 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -109,7 +109,7 @@ impl MaintenanceMode {
                 bail!("read-only maintenance mode: {message}")
             }
             #[cfg(feature = "enum-fallback")]
-            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
+            (MaintenanceType::UnknownEnumValue(m), _) => bail!("unknown maintenance mode: {m}"),
         }
     }
 }
-- 
2.47.3

[PATCH proxmox 4/5] pbs-api-types: add WriteNonExpanding datastore operation

[Robert Obkircher]

The new variant will be used for Write operations that should be
allowed in a new GarbageCollection maintenace mode. This name was
chosen over "Prune" because it might also make sense to allow other
administrative tasks that do not actually delete things.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-api-types/src/maintenance.rs | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index 7e9599be..2d11aff2 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -25,14 +25,16 @@ pub const MAINTENANCE_MESSAGE_SCHEMA: Schema =
 pub enum Operation {
     /// for any read operation like backup restore or RRD metric collection
     Read,
-    /// for any write/delete operation, like backup create or GC
+    /// for any read/write/delete operation, like backup create
     Write,
+    /// for read/write/delete operations that reduce or maintain
+    /// storage usage, like GC or prune.
+    WriteNonExpanding,
     /// for any purely logical operation on the in-memory state of the datastore, e.g., to check if
     /// some mutex could be locked (e.g., GC already running?)
     ///
     /// NOTE: one must *not* do any IO operations when only helding this Op state
     Lookup,
-    // GarbageCollect or Delete?
 }
 
 #[api]
@@ -105,7 +107,7 @@ impl MaintenanceMode {
             (MaintenanceType::Offline, _) => bail!("offline maintenance mode: {message}"),
             (MaintenanceType::S3Refresh, _) => bail!("S3 refresh maintenance mode: {message}"),
             (MaintenanceType::ReadOnly, Operation::Read) => Ok(()),
-            (MaintenanceType::ReadOnly, Operation::Write) => {
+            (MaintenanceType::ReadOnly, Operation::Write | Operation::WriteNonExpanding) => {
                 bail!("read-only maintenance mode: {message}")
             }
             #[cfg(feature = "enum-fallback")]
-- 
2.47.3

[PATCH proxmox 5/5] pbs-api-types: add GarbageCollection maintenance mode

[Robert Obkircher]

This mode may be used to safely prune and garbage collect a datastore
without the risk of running out of space due to new backups.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-api-types/src/datastore.rs   |  1 +
 pbs-api-types/src/maintenance.rs | 11 +++++++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index b770bc03..f54af000 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -649,6 +649,7 @@ impl DataStoreConfig {
 
         match current_type {
             Some(MaintenanceType::ReadOnly) => { /* always OK  */ }
+            Some(MaintenanceType::GarbageCollection) => { /* always OK  */ }
             Some(MaintenanceType::Offline) => { /* always OK  */ }
             Some(MaintenanceType::Unmount) => {
                 /* used to reset it after failed unmount, or alternative for aborting unmount task */
diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index 2d11aff2..abbdbf3f 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -42,12 +42,11 @@ pub enum Operation {
 #[serde(rename_all = "kebab-case")]
 /// Maintenance type.
 pub enum MaintenanceType {
-    // TODO:
-    //  - Add "GarbageCollection" or "DeleteOnly" as type and track GC (or all deletes) as separate
-    //    operation, so that one can enable a mode where nothing new can be added but stuff can be
-    //    cleaned
     /// Only read operations are allowed on the datastore.
     ReadOnly,
+    /// Allow reads and non-expanding writes, but disallow writes
+    /// that may increase storage usage.
+    GarbageCollection,
     /// Neither read nor write operations are allowed on the datastore.
     Offline,
     /// The datastore is being deleted.
@@ -110,6 +109,10 @@ impl MaintenanceMode {
             (MaintenanceType::ReadOnly, Operation::Write | Operation::WriteNonExpanding) => {
                 bail!("read-only maintenance mode: {message}")
             }
+            (MaintenanceType::GarbageCollection, Operation::Write) => {
+                bail!("garbage-collection maintenance mode: {message}")
+            }
+            (MaintenanceType::GarbageCollection, _) => Ok(()),
             #[cfg(feature = "enum-fallback")]
             (MaintenanceType::UnknownEnumValue(m), _) => bail!("unknown maintenance mode: {m}"),
         }
-- 
2.47.3

[PATCH proxmox-backup 1/6] datastore: propagate maintenance mode parse errors

[Robert Obkircher]

Handle maintenance mode parse errors since they are no longer silently
mapped to None. That was problematic because during upgrades the older
version would have potentially ignored a newly introduced maintenance
mode.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/datastore.rs      | 17 ++++++++++++-----
 src/api2/admin/datastore.rs         |  2 +-
 src/server/metric_collection/mod.rs |  7 +++----
 3 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e2d1ae67c..da7eba80e 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -303,9 +303,16 @@ impl Drop for DataStore {
                 Ok((section_config, _gen)) => {
                     match section_config.lookup::<DataStoreConfig>("datastore", self.name()) {
                         // second check: check if maintenance mode requires closing FDs
-                        Ok(config) => config
-                            .get_maintenance_mode()
-                            .is_some_and(|m| m.clear_from_cache()),
+                        Ok(config) => match config.get_maintenance_mode() {
+                            Ok(m) => m.is_some_and(|m| m.clear_from_cache()),
+                            Err(err) => {
+                                log::warn!(
+                                    "DataStore::drop: datastore '{}' in unknown maintenance mode; evicting cached instance: {err}",
+                                    self.name()
+                                );
+                                true
+                            }
+                        },
                         Err(err) => {
                             // datastore removed from config; evict cached entry if available (without checking maintenance mode)
                             log::warn!(
@@ -576,7 +583,7 @@ impl DataStore {
         let (section_config, gen_num) = datastore_section_config_cached(true)?;
         let config: DataStoreConfig = section_config.lookup("datastore", lookup.name)?;
 
-        if let Some(maintenance_mode) = config.get_maintenance_mode() {
+        if let Some(maintenance_mode) = config.get_maintenance_mode()? {
             if let Err(error) = maintenance_mode.check(lookup.operation) {
                 bail!("datastore '{}' is unavailable: {error}", lookup.name);
             }
@@ -660,7 +667,7 @@ impl DataStore {
         let datastore: DataStoreConfig = config.lookup("datastore", name)?;
         if datastore
             .get_maintenance_mode()
-            .is_some_and(|m| m.clear_from_cache())
+            .map_or(true, |m| m.is_some_and(|m| m.clear_from_cache()))
         {
             // the datastore drop handler does the checking if tasks are running and clears the
             // cache entry, so we just have to trigger it here
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 64c73ba06..94272c32d 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2695,7 +2695,7 @@ fn expect_maintenance_type(
     let store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
 
     if store_config
-        .get_maintenance_mode()
+        .get_maintenance_mode()?
         .is_none_or(|m| m.ty != maintenance_type)
     {
         bail!("maintenance mode is not '{maintenance_type}'");
diff --git a/src/server/metric_collection/mod.rs b/src/server/metric_collection/mod.rs
index 18625b1a5..299156f8e 100644
--- a/src/server/metric_collection/mod.rs
+++ b/src/server/metric_collection/mod.rs
@@ -286,10 +286,9 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec<DatastoreStats>) {
                 .unwrap_or_default();
 
             for config in datastore_list {
-                if config
-                    .get_maintenance_mode()
-                    .is_some_and(|mode| mode.check(Operation::Read).is_err())
-                {
+                if config.get_maintenance_mode().map_or(true, |m| {
+                    m.is_some_and(|mode| mode.check(Operation::Read).is_err())
+                }) {
                     continue;
                 }
 
-- 
2.47.3

[PATCH proxmox-backup 2/6] task tracking: use parameter for initial count and refactor updates

[Robert Obkircher]

Don't hard-code the initial count to 1 and bail out if it is negative.
Replace the ActiveOperationStats initializers with the update logic
because they would be reformatted to multiple lines after adding a
field and an operation.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/task_tracking.rs | 52 +++++++++++++++++-------------
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs
index d4cc076aa..a67b77221 100644
--- a/pbs-datastore/src/task_tracking.rs
+++ b/pbs-datastore/src/task_tracking.rs
@@ -1,4 +1,4 @@
-use anyhow::Error;
+use anyhow::{Error, bail};
 use libc::pid_t;
 use nix::unistd::Pid;
 use std::iter::Sum;
@@ -15,6 +15,16 @@ pub struct ActiveOperationStats {
     pub write: i64,
 }
 
+impl ActiveOperationStats {
+    fn add(&mut self, count: i64, operation: Operation) {
+        match operation {
+            Operation::Read => self.read += count,
+            Operation::Write => self.write += count,
+            Operation::Lookup => (), // no IO must happen there
+        }
+    }
+}
+
 impl Sum<Self> for ActiveOperationStats {
     fn sum<I>(iter: I) -> Self
     where
@@ -101,14 +111,8 @@ pub fn update_active_operations(
     let (_lock, options) = open_lock_file(name)?;
 
     let pid = std::process::id();
-    let starttime = procfs::PidStat::read_from_pid(Pid::from_raw(pid as pid_t))?.starttime;
 
-    let mut updated_active_operations = match operation {
-        Operation::Read => ActiveOperationStats { read: 1, write: 0 },
-        Operation::Write => ActiveOperationStats { read: 0, write: 1 },
-        Operation::Lookup => ActiveOperationStats { read: 0, write: 0 },
-    };
-    let mut found_entry = false;
+    let mut updated = None;
     let mut updated_tasks: Vec<TaskOperations> = match file_read_optional_string(&path)? {
         Some(data) => serde_json::from_str::<Vec<TaskOperations>>(&data)?
             .iter_mut()
@@ -117,13 +121,8 @@ pub fn update_active_operations(
                     Some(stat) if pid == task.pid && stat.starttime != task.starttime => None,
                     Some(_) => {
                         if pid == task.pid {
-                            found_entry = true;
-                            match operation {
-                                Operation::Read => task.active_operations.read += count,
-                                Operation::Write => task.active_operations.write += count,
-                                Operation::Lookup => (), // no IO must happen there
-                            };
-                            updated_active_operations = task.active_operations;
+                            task.active_operations.add(count, operation);
+                            updated = Some(task.active_operations);
                         }
                         Some(task.clone())
                     }
@@ -134,18 +133,27 @@ pub fn update_active_operations(
         None => Vec::new(),
     };
 
-    if !found_entry {
+    let result = if let Some(updated) = updated {
+        updated
+    } else {
+        if count < 0 {
+            bail!("unexpected initial active operation count: {count} {operation:?}")
+        }
+        let mut new = ActiveOperationStats::default();
+        new.add(count, operation);
         updated_tasks.push(TaskOperations {
             pid,
-            starttime,
-            active_operations: updated_active_operations,
-        })
-    }
+            starttime: procfs::PidStat::read_from_pid(Pid::from_raw(pid as pid_t))?.starttime,
+            active_operations: new,
+        });
+        new
+    };
+
     replace_file(
         &path,
         serde_json::to_string(&updated_tasks)?.as_bytes(),
         options,
         false,
-    )
-    .map(|_| updated_active_operations)
+    )?;
+    Ok(result)
 }
-- 
2.47.3

[PATCH proxmox-backup 3/6] www: access active operation fields by name instead of index

[Robert Obkircher]

Avoid relying on (presumably sorted) field order so new fields could
be added to the API response in the future.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 www/datastore/OptionView.js | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 42a6104dc..64e47aa57 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -122,8 +122,13 @@ Ext.define('PBS.Datastore.Options', {
 
             view.mon(me.activeOperationsRstore, 'load', (store, data, success) => {
                 let activeTasks = me.getView().maintenanceActiveTasks;
-                activeTasks.read = data?.[0]?.data.value ?? 0;
-                activeTasks.write = data?.[1]?.data.value ?? 0;
+                if (success) {
+                  activeTasks.read = store.getById('read')?.data.value ?? 0;
+                  activeTasks.write = store.getById('write')?.data.value ?? 0;
+                } else {
+                  activeTasks.read = 0;
+                  activeTasks.write = 0;
+                }
             });
         },
 
-- 
2.47.3

[PATCH proxmox-backup 4/6] task tracking: count WriteNonExpanding datastore operations

[Robert Obkircher]

Count WriteNonExpanding as writes to ensure that switching over from
Write does not break forward compatibility when older versions read
the counters.

Also add a new counter to display the GarbageCollection maintenance
mode conflicts. It may not be accurate while older processes are
running but it should be good enough for the UI.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/task_tracking.rs | 12 ++++++++++++
 src/api2/admin/datastore.rs        |  1 +
 2 files changed, 13 insertions(+)

diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs
index a67b77221..87ce4f5de 100644
--- a/pbs-datastore/src/task_tracking.rs
+++ b/pbs-datastore/src/task_tracking.rs
@@ -13,6 +13,12 @@ use serde::{Deserialize, Serialize};
 pub struct ActiveOperationStats {
     pub read: i64,
     pub write: i64,
+    /// The number of writers that do not significantly increase disk usage.
+    ///
+    /// The count is unreliable because older processes without this field
+    /// will delete it on every update!
+    #[serde(default)]
+    pub write_non_expanding: i64,
 }
 
 impl ActiveOperationStats {
@@ -20,6 +26,11 @@ impl ActiveOperationStats {
         match operation {
             Operation::Read => self.read += count,
             Operation::Write => self.write += count,
+            Operation::WriteNonExpanding => {
+                self.write += count;
+                // prevent underflow if an older process deleted the field
+                self.write_non_expanding = (self.write_non_expanding + count).max(0);
+            }
             Operation::Lookup => (), // no IO must happen there
         }
     }
@@ -33,6 +44,7 @@ impl Sum<Self> for ActiveOperationStats {
         iter.fold(Self::default(), |a, b| Self {
             read: a.read + b.read,
             write: a.write + b.write,
+            write_non_expanding: a.write_non_expanding + b.write_non_expanding,
         })
     }
 }
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 94272c32d..3cbdd8922 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -2024,6 +2024,7 @@ pub fn get_active_operations(store: String, _param: Value) -> Result<Value, Erro
     Ok(json!({
         "read": active_operations.read,
         "write": active_operations.write,
+        "writeNonExpanding": active_operations.write_non_expanding,
     }))
 }
 
-- 
2.47.3

[PATCH proxmox-backup 5/6] datastore: open datastores with WriteNonExpanding instead of Write

[Robert Obkircher]

Allow reclaim and garbage-collection jobs when the GarbageCollection
maintenance mode is active. Only include the most important operations
for now. Others, such as set_backup_owner and group/namespace moves,
could be reasonably supported as well.

Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 src/api2/admin/datastore.rs     | 12 ++++++------
 src/api2/admin/namespace.rs     |  2 +-
 src/bin/proxmox-backup-proxy.rs |  2 +-
 src/server/prune_job.rs         |  2 +-
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 3cbdd8922..a08531e41 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -253,7 +253,7 @@ pub async fn delete_group(
             &auth_id,
             PRIV_DATASTORE_MODIFY,
             PRIV_DATASTORE_PRUNE,
-            Operation::Write,
+            Operation::WriteNonExpanding,
             &group,
         )?;
 
@@ -463,7 +463,7 @@ pub async fn delete_snapshot(
             &auth_id,
             PRIV_DATASTORE_MODIFY,
             PRIV_DATASTORE_PRUNE,
-            Operation::Write,
+            Operation::WriteNonExpanding,
             &backup_dir.group,
         )?;
 
@@ -1002,7 +1002,7 @@ pub fn prune(
         &auth_id,
         PRIV_DATASTORE_MODIFY,
         PRIV_DATASTORE_PRUNE,
-        Operation::Write,
+        Operation::WriteNonExpanding,
         &group,
     )?;
 
@@ -1174,7 +1174,7 @@ pub fn prune_datastore(
         true,
     )?;
 
-    let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::Write))?;
+    let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::WriteNonExpanding))?;
     let ns = prune_options.ns.clone().unwrap_or_default();
     let worker_id = format!("{store}:{ns}");
 
@@ -1212,7 +1212,7 @@ pub fn start_garbage_collection(
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
-    let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::Write))?;
+    let datastore = DataStore::lookup_datastore(lookup_with(&store, Operation::WriteNonExpanding))?;
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     let job = Job::new("garbage_collection", &store)
@@ -2307,7 +2307,7 @@ pub async fn set_protection(
             &auth_id,
             PRIV_DATASTORE_MODIFY,
             PRIV_DATASTORE_BACKUP,
-            Operation::Write,
+            Operation::WriteNonExpanding,
             &backup_dir.group,
         )?;
 
diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
index 19e1e8cd0..10ac69259 100644
--- a/src/api2/admin/namespace.rs
+++ b/src/api2/admin/namespace.rs
@@ -168,7 +168,7 @@ pub fn delete_namespace(
 
     check_ns_modification_privs(&store, &ns, &auth_id)?;
 
-    let lookup = crate::tools::lookup_with(&store, Operation::Write);
+    let lookup = crate::tools::lookup_with(&store, Operation::WriteNonExpanding);
     let datastore = DataStore::lookup_datastore(lookup)?;
 
     let (removed_all, stats) = datastore.remove_namespace_recursive(&ns, delete_groups)?;
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index b372f779e..ec5f24efb 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -598,7 +598,7 @@ async fn schedule_datastore_garbage_collection() {
             Err(_) => continue, // could not get lock
         };
 
-        let lookup = lookup_with(&store, Operation::Write);
+        let lookup = lookup_with(&store, Operation::WriteNonExpanding);
         let datastore = match DataStore::lookup_datastore(lookup) {
             Ok(datastore) => datastore,
             Err(err) => {
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index 03661127e..aa6e4e73e 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -133,7 +133,7 @@ pub fn do_prune_job(
     auth_id: &Authid,
     schedule: Option<String>,
 ) -> Result<String, Error> {
-    let lookup = crate::tools::lookup_with(&store, Operation::Write);
+    let lookup = crate::tools::lookup_with(&store, Operation::WriteNonExpanding);
     let datastore = DataStore::lookup_datastore(lookup)?;
 
     let worker_type = job.jobtype().to_string();
-- 
2.47.3

[PATCH proxmox-backup 6/6] fix #5797: www: display new GarbageCollection maintenance mode

[Robert Obkircher]

Display the new mode and make it selectable. Subtract non-expanding
writes from the number of conflicts, since they are allowed in this
mode.

Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5797
Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 www/Utils.js                     | 4 ++++
 www/datastore/OptionView.js      | 3 +++
 www/window/MaintenanceOptions.js | 1 +
 3 files changed, 8 insertions(+)

diff --git a/www/Utils.js b/www/Utils.js
index bf4b025c7..fa49fcbf8 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -824,6 +824,7 @@ Ext.define('PBS.Utils', {
         if (activeTasks !== undefined) {
             const conflictingTasks =
                 activeTasks.write +
+                (type === 'garbage-collection' ? -activeTasks.writeNonExpanding : 0) +
                 (type === 'offline' || type === 'unmount' ? activeTasks.read : 0);
 
             if (conflictingTasks > 0) {
@@ -846,6 +847,9 @@ Ext.define('PBS.Utils', {
             case 'read-only':
                 modeText = gettext('Read-only');
                 break;
+            case 'garbage-collection':
+                modeText = gettext('Garbage Collection');
+                break;
             case 'offline':
                 modeText = gettext('Offline');
                 break;
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 64e47aa57..683770738 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -93,6 +93,7 @@ Ext.define('PBS.Datastore.Options', {
         me.maintenanceActiveTasks = {
             read: 0,
             write: 0,
+            writeNonExpanding: 0,
         };
         me.datastore = encodeURIComponent(me.datastore);
         me.url = `/api2/json/config/datastore/${me.datastore}`;
@@ -125,9 +126,11 @@ Ext.define('PBS.Datastore.Options', {
                 if (success) {
                   activeTasks.read = store.getById('read')?.data.value ?? 0;
                   activeTasks.write = store.getById('write')?.data.value ?? 0;
+                  activeTasks.writeNonExpanding = store.getById('writeNonExpanding')?.data.value ?? 0;
                 } else {
                   activeTasks.read = 0;
                   activeTasks.write = 0;
+                  activeTasks.writeNonExpanding = 0;
                 }
             });
         },
diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
index 9a735e5e8..9228f4ee9 100644
--- a/www/window/MaintenanceOptions.js
+++ b/www/window/MaintenanceOptions.js
@@ -5,6 +5,7 @@ Ext.define('PBS.form.maintenanceType', {
     comboItems: [
         ['__default__', gettext('None')],
         ['read-only', gettext('Read only')],
+        ['garbage-collection', gettext('Garbage Collection')],
         ['offline', gettext('Offline')],
     ],
 });
-- 
2.47.3

Re: [PATCH proxmox 1/5] pbs-api-types: propagate maintenance mode parse errors

[Christian Ebner]

a few minor comments only

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Do not silently ignore errors if the config file is invalid or
> contains a new mode from a future version.

The commit message should also include an explicit statement that 
set_maintenance_mode() now fails when mode cannot be parsed and that 
this does not interfere with the current behavior when `enum-fallback` 
is enabled.

> Upgrades to the new GarbageCollection maintenance mode should be fine
> even without this change, becausue it is unlikely that someone can
> enable it before older processes stop initiating new backups or S3
> refreshes. There is however a risk that a sync job could unmount the
> datastore.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-api-types/src/datastore.rs | 22 +++++++++++++---------
>   1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 7643b0de..b770bc03 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -629,18 +629,22 @@ impl DataStoreConfig {
>           }
>       }
>   
> -    pub fn get_maintenance_mode(&self) -> Option<MaintenanceMode> {
> -        self.maintenance_mode.as_ref().and_then(|str| {
> -            MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
> -                str,
> -                &MaintenanceMode::API_SCHEMA,
> -            ))
> -            .ok()
> -        })
> +    pub fn get_maintenance_mode(&self) -> Result<Option<MaintenanceMode>, Error> {
> +        self.maintenance_mode
> +            .as_ref()
> +            .map(|str| {
> +                MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
> +                    str,
> +                    &MaintenanceMode::API_SCHEMA,
> +                ))
> +                .map_err(|e| format_err!("failed to parse maintenance mode: {e}"))
> +            })
> +            .transpose()
>       }
>   
>       pub fn set_maintenance_mode(&mut self, new_mode: Option<MaintenanceMode>) -> Result<(), Error> {
> -        let current_type = self.get_maintenance_mode().map(|mode| mode.ty);
> +        // it is probably best to error out if the current mode can't be parsed:

This comment would benefit from including a bit more reasoning:
e.g. never allow setting of maintenance mode on parsing errors for 
current mode, possible new maintenance type variant.

> +        let current_type = self.get_maintenance_mode()?.map(|mode| mode.ty);
>           let new_type = new_mode.as_ref().map(|mode| mode.ty);
>   
>           match current_type {

Slightly unrelated, but since noted during review and and this fits to 
the patch series: the docstrings for `ty` in MaintenanceMode and 
`maintenance_mode` in DataStoreConfig are outdated and need to be updated.


With that addressed:

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

Re: [PATCH proxmox 2/5] pbs-api-types: use match statement for maintenance mode check

[Christian Ebner]

only one question

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Use an exhaustive match statement to be more explicit and to draw
> attention when new variants are added.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   pbs-api-types/src/maintenance.rs | 27 ++++++++++++---------------
>   1 file changed, 12 insertions(+), 15 deletions(-)
> 
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index 2adb5d84..7def498b 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -94,25 +94,22 @@ impl MaintenanceMode {
>       }
>   
>       pub fn check(&self, operation: Operation) -> Result<(), Error> {
> -        if self.ty == MaintenanceType::Delete {
> -            bail!("datastore is being deleted");
> -        }
> -
>           let message = percent_encoding::percent_decode_str(self.message.as_deref().unwrap_or(""))
>               .decode_utf8()
>               .unwrap_or(Cow::Borrowed(""));

question: should we also avoid decoding the message if not using it? 
Maybe by defining a closure for it?

>   
> -        if Operation::Lookup == operation {
> -            return Ok(());
> -        } else if self.ty == MaintenanceType::Unmount {
> -            bail!("datastore is being unmounted");
> -        } else if self.ty == MaintenanceType::Offline {
> -            bail!("offline maintenance mode: {}", message);
> -        } else if self.ty == MaintenanceType::S3Refresh {
> -            bail!("S3 refresh maintenance mode: {}", message);
> -        } else if self.ty == MaintenanceType::ReadOnly && Operation::Write == operation {
> -            bail!("read-only maintenance mode: {}", message);
> +        match (self.ty, operation) {
> +            (MaintenanceType::Delete, _) => bail!("datastore is being deleted"),
> +            (_, Operation::Lookup) => Ok(()),
> +            (MaintenanceType::Unmount, _) => bail!("datastore is being unmounted"),
> +            (MaintenanceType::Offline, _) => bail!("offline maintenance mode: {message}"),
> +            (MaintenanceType::S3Refresh, _) => bail!("S3 refresh maintenance mode: {message}"),
> +            (MaintenanceType::ReadOnly, Operation::Read) => Ok(()),
> +            (MaintenanceType::ReadOnly, Operation::Write) => {
> +                bail!("read-only maintenance mode: {message}")
> +            }
> +            #[cfg(feature = "enum-fallback")]
> +            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
>           }
> -        Ok(())
>       }
>   }

Re: [PATCH proxmox 3/5] pbs-api-types: deny non-lookup operations for unknown modes

[Christian Ebner]

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Denying non-lookup operations for unknown modes seems like a safer

s/seems like/is/

> default. This change should not affect anything because the backup
> server does not enable the enum-fallback feature.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   pbs-api-types/src/maintenance.rs | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index 7def498b..7e9599be 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -109,7 +109,7 @@ impl MaintenanceMode {
>                   bail!("read-only maintenance mode: {message}")
>               }
>               #[cfg(feature = "enum-fallback")]
> -            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
> +            (MaintenanceType::UnknownEnumValue(m), _) => bail!("unknown maintenance mode: {m}"),

What about PDM? I didn't recompile PDM with your changes on top, but 
code wise the DatastorePanelComp currently only checks for offline 
stores. Probably contents should not be shown if the maintenance mode is 
unknown there (although pre-existing).

>           }
>       }
>   }

Re: [PATCH proxmox 4/5] pbs-api-types: add WriteNonExpanding datastore operation

[Christian Ebner]

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> The new variant will be used for Write operations that should be
> allowed in a new GarbageCollection maintenace mode. This name was
> chosen over "Prune" because it might also make sense to allow other
> administrative tasks that do not actually delete things.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   pbs-api-types/src/maintenance.rs | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index 7e9599be..2d11aff2 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -25,14 +25,16 @@ pub const MAINTENANCE_MESSAGE_SCHEMA: Schema =
>   pub enum Operation {
>       /// for any read operation like backup restore or RRD metric collection
>       Read,
> -    /// for any write/delete operation, like backup create or GC
> +    /// for any read/write/delete operation, like backup create
>       Write,
> +    /// for read/write/delete operations that reduce or maintain
> +    /// storage usage, like GC or prune.
> +    WriteNonExpanding,
>       /// for any purely logical operation on the in-memory state of the datastore, e.g., to check if
>       /// some mutex could be locked (e.g., GC already running?)
>       ///
>       /// NOTE: one must *not* do any IO operations when only helding this Op state
>       Lookup,
> -    // GarbageCollect or Delete?
>   }
>   
>   #[api]
> @@ -105,7 +107,7 @@ impl MaintenanceMode {
>               (MaintenanceType::Offline, _) => bail!("offline maintenance mode: {message}"),
>               (MaintenanceType::S3Refresh, _) => bail!("S3 refresh maintenance mode: {message}"),
>               (MaintenanceType::ReadOnly, Operation::Read) => Ok(()),
> -            (MaintenanceType::ReadOnly, Operation::Write) => {
> +            (MaintenanceType::ReadOnly, Operation::Write | Operation::WriteNonExpanding) => {
>                   bail!("read-only maintenance mode: {message}")
>               }
>               #[cfg(feature = "enum-fallback")]

Re: [PATCH proxmox 5/5] pbs-api-types: add GarbageCollection maintenance mode

[Christian Ebner]

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> This mode may be used to safely prune and garbage collect a datastore
> without the risk of running out of space due to new backups.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   pbs-api-types/src/datastore.rs   |  1 +
>   pbs-api-types/src/maintenance.rs | 11 +++++++----
>   2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index b770bc03..f54af000 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -649,6 +649,7 @@ impl DataStoreConfig {
>   
>           match current_type {
>               Some(MaintenanceType::ReadOnly) => { /* always OK  */ }
> +            Some(MaintenanceType::GarbageCollection) => { /* always OK  */ }
>               Some(MaintenanceType::Offline) => { /* always OK  */ }
>               Some(MaintenanceType::Unmount) => {
>                   /* used to reset it after failed unmount, or alternative for aborting unmount task */
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> index 2d11aff2..abbdbf3f 100644
> --- a/pbs-api-types/src/maintenance.rs
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -42,12 +42,11 @@ pub enum Operation {
>   #[serde(rename_all = "kebab-case")]
>   /// Maintenance type.
>   pub enum MaintenanceType {
> -    // TODO:
> -    //  - Add "GarbageCollection" or "DeleteOnly" as type and track GC (or all deletes) as separate
> -    //    operation, so that one can enable a mode where nothing new can be added but stuff can be
> -    //    cleaned
>       /// Only read operations are allowed on the datastore.
>       ReadOnly,
> +    /// Allow reads and non-expanding writes, but disallow writes
> +    /// that may increase storage usage.
> +    GarbageCollection,
>       /// Neither read nor write operations are allowed on the datastore.
>       Offline,
>       /// The datastore is being deleted.
> @@ -110,6 +109,10 @@ impl MaintenanceMode {
>               (MaintenanceType::ReadOnly, Operation::Write | Operation::WriteNonExpanding) => {
>                   bail!("read-only maintenance mode: {message}")
>               }
> +            (MaintenanceType::GarbageCollection, Operation::Write) => {
> +                bail!("garbage-collection maintenance mode: {message}")
> +            }
> +            (MaintenanceType::GarbageCollection, _) => Ok(()),
>               #[cfg(feature = "enum-fallback")]
>               (MaintenanceType::UnknownEnumValue(m), _) => bail!("unknown maintenance mode: {m}"),
>           }

Re: [PATCH proxmox 2/5] pbs-api-types: use match statement for maintenance mode check

[Robert Obkircher]

On 03.06.26 12:35, Christian Ebner wrote:
> only one question
>
> On 6/2/26 3:00 PM, Robert Obkircher wrote:
>> Use an exhaustive match statement to be more explicit and to draw
>> attention when new variants are added.
>>
>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>
> Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
>
>> ---
>>   pbs-api-types/src/maintenance.rs | 27 ++++++++++++---------------
>>   1 file changed, 12 insertions(+), 15 deletions(-)
>>
>> diff --git a/pbs-api-types/src/maintenance.rs
>> b/pbs-api-types/src/maintenance.rs
>> index 2adb5d84..7def498b 100644
>> --- a/pbs-api-types/src/maintenance.rs
>> +++ b/pbs-api-types/src/maintenance.rs
>> @@ -94,25 +94,22 @@ impl MaintenanceMode {
>>       }
>>         pub fn check(&self, operation: Operation) -> Result<(),
>> Error> {
>> -        if self.ty == MaintenanceType::Delete {
>> -            bail!("datastore is being deleted");
>> -        }
>> -
>>           let message =
>> percent_encoding::percent_decode_str(self.message.as_deref().unwrap_or(""))
>>               .decode_utf8()
>>               .unwrap_or(Cow::Borrowed(""));
>
> question: should we also avoid decoding the message if not using it?
> Maybe by defining a closure for it? 

This should be fairly cheap in the common case where self.message is
None.

The bigger issue is that the message is never percent-encoded in the
first place. I'd also avoid adding ": " if there is no message. I was
going to address this in a v2 for [1] but I can also add a patch to
this series.

[1]
https://lore.proxmox.com/pbs-devel/50fe57b6-d7d4-40a1-9e17-bc904ea30cdd@proxmox.com/

>
>>   -        if Operation::Lookup == operation {
>> -            return Ok(());
>> -        } else if self.ty == MaintenanceType::Unmount {
>> -            bail!("datastore is being unmounted");
>> -        } else if self.ty == MaintenanceType::Offline {
>> -            bail!("offline maintenance mode: {}", message);
>> -        } else if self.ty == MaintenanceType::S3Refresh {
>> -            bail!("S3 refresh maintenance mode: {}", message);
>> -        } else if self.ty == MaintenanceType::ReadOnly &&
>> Operation::Write == operation {
>> -            bail!("read-only maintenance mode: {}", message);
>> +        match (self.ty, operation) {
>> +            (MaintenanceType::Delete, _) => bail!("datastore is
>> being deleted"),
>> +            (_, Operation::Lookup) => Ok(()),
>> +            (MaintenanceType::Unmount, _) => bail!("datastore is
>> being unmounted"),
>> +            (MaintenanceType::Offline, _) => bail!("offline
>> maintenance mode: {message}"),
>> +            (MaintenanceType::S3Refresh, _) => bail!("S3 refresh
>> maintenance mode: {message}"),
>> +            (MaintenanceType::ReadOnly, Operation::Read) => Ok(()),
>> +            (MaintenanceType::ReadOnly, Operation::Write) => {
>> +                bail!("read-only maintenance mode: {message}")
>> +            }
>> +            #[cfg(feature = "enum-fallback")]
>> +            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
>>           }
>> -        Ok(())
>>       }
>>   }
>

Re: [PATCH proxmox 3/5] pbs-api-types: deny non-lookup operations for unknown modes

[Robert Obkircher]

On 03.06.26 12:38, Christian Ebner wrote:
> On 6/2/26 3:00 PM, Robert Obkircher wrote:
>> Denying non-lookup operations for unknown modes seems like a safer
>
> s/seems like/is/
>
>> default. This change should not affect anything because the backup
>> server does not enable the enum-fallback feature.
>>
>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>> ---
>>   pbs-api-types/src/maintenance.rs | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pbs-api-types/src/maintenance.rs
>> b/pbs-api-types/src/maintenance.rs
>> index 7def498b..7e9599be 100644
>> --- a/pbs-api-types/src/maintenance.rs
>> +++ b/pbs-api-types/src/maintenance.rs
>> @@ -109,7 +109,7 @@ impl MaintenanceMode {
>>                   bail!("read-only maintenance mode: {message}")
>>               }
>>               #[cfg(feature = "enum-fallback")]
>> -            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
>> +            (MaintenanceType::UnknownEnumValue(m), _) =>
>> bail!("unknown maintenance mode: {m}"),
>
> What about PDM? I didn't recompile PDM with your changes on top, but
> code wise the DatastorePanelComp currently only checks for offline
> stores. Probably contents should not be shown if the maintenance
> mode is unknown there (although pre-existing). 
Does it ever call that function? I couldn't find anything but my lsp
is not always reliable.

This comparison [1] seems wrong if the mode has a message.

[1]
https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=ui/src/pbs/datastore.rs;h=53be35bcb747561ef8855b1efc9d6c131f516117;hb=HEAD#l52

>
>>           }
>>       }
>>   }
>

Re: [PATCH proxmox-backup 1/6] datastore: propagate maintenance mode parse errors

[Christian Ebner]

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Handle maintenance mode parse errors since they are no longer silently
> mapped to None. That was problematic because during upgrades the older
> version would have potentially ignored a newly introduced maintenance
> mode.

LGTM. Dropping from caches is local to process, so fine and s3-refresh 
and unmount run in a maintenance-locked state anyways. They might fail 
if the maintenance mode was not in the expected one or parsing errors 
prior to entering the mode, but that is intended behavior.

> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   pbs-datastore/src/datastore.rs      | 17 ++++++++++++-----
>   src/api2/admin/datastore.rs         |  2 +-
>   src/server/metric_collection/mod.rs |  7 +++----
>   3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index e2d1ae67c..da7eba80e 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -303,9 +303,16 @@ impl Drop for DataStore {
>                   Ok((section_config, _gen)) => {
>                       match section_config.lookup::<DataStoreConfig>("datastore", self.name()) {
>                           // second check: check if maintenance mode requires closing FDs
> -                        Ok(config) => config
> -                            .get_maintenance_mode()
> -                            .is_some_and(|m| m.clear_from_cache()),
> +                        Ok(config) => match config.get_maintenance_mode() {
> +                            Ok(m) => m.is_some_and(|m| m.clear_from_cache()),
> +                            Err(err) => {
> +                                log::warn!(
> +                                    "DataStore::drop: datastore '{}' in unknown maintenance mode; evicting cached instance: {err}",
> +                                    self.name()
> +                                );
> +                                true
> +                            }
> +                        },
>                           Err(err) => {
>                               // datastore removed from config; evict cached entry if available (without checking maintenance mode)
>                               log::warn!(
> @@ -576,7 +583,7 @@ impl DataStore {
>           let (section_config, gen_num) = datastore_section_config_cached(true)?;
>           let config: DataStoreConfig = section_config.lookup("datastore", lookup.name)?;
>   
> -        if let Some(maintenance_mode) = config.get_maintenance_mode() {
> +        if let Some(maintenance_mode) = config.get_maintenance_mode()? {
>               if let Err(error) = maintenance_mode.check(lookup.operation) {
>                   bail!("datastore '{}' is unavailable: {error}", lookup.name);
>               }
> @@ -660,7 +667,7 @@ impl DataStore {
>           let datastore: DataStoreConfig = config.lookup("datastore", name)?;
>           if datastore
>               .get_maintenance_mode()
> -            .is_some_and(|m| m.clear_from_cache())
> +            .map_or(true, |m| m.is_some_and(|m| m.clear_from_cache()))
>           {
>               // the datastore drop handler does the checking if tasks are running and clears the
>               // cache entry, so we just have to trigger it here
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 64c73ba06..94272c32d 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2695,7 +2695,7 @@ fn expect_maintenance_type(
>       let store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
>   
>       if store_config
> -        .get_maintenance_mode()
> +        .get_maintenance_mode()?
>           .is_none_or(|m| m.ty != maintenance_type)
>       {
>           bail!("maintenance mode is not '{maintenance_type}'");
> diff --git a/src/server/metric_collection/mod.rs b/src/server/metric_collection/mod.rs
> index 18625b1a5..299156f8e 100644
> --- a/src/server/metric_collection/mod.rs
> +++ b/src/server/metric_collection/mod.rs
> @@ -286,10 +286,9 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec<DatastoreStats>) {
>                   .unwrap_or_default();
>   
>               for config in datastore_list {
> -                if config
> -                    .get_maintenance_mode()
> -                    .is_some_and(|mode| mode.check(Operation::Read).is_err())
> -                {
> +                if config.get_maintenance_mode().map_or(true, |m| {
> +                    m.is_some_and(|mode| mode.check(Operation::Read).is_err())
> +                }) {
>                       continue;
>                   }
>   

Re: [PATCH proxmox 2/5] pbs-api-types: use match statement for maintenance mode check

[Christian Ebner]

On 6/3/26 1:03 PM, Robert Obkircher wrote:
> 
> On 03.06.26 12:35, Christian Ebner wrote:
>> only one question
>>
>> On 6/2/26 3:00 PM, Robert Obkircher wrote:
>>> Use an exhaustive match statement to be more explicit and to draw
>>> attention when new variants are added.
>>>
>>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>>
>> Reviewed-by: Christian Ebner <c.ebner@proxmox.com>
>>
>>> ---
>>>    pbs-api-types/src/maintenance.rs | 27 ++++++++++++---------------
>>>    1 file changed, 12 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/pbs-api-types/src/maintenance.rs
>>> b/pbs-api-types/src/maintenance.rs
>>> index 2adb5d84..7def498b 100644
>>> --- a/pbs-api-types/src/maintenance.rs
>>> +++ b/pbs-api-types/src/maintenance.rs
>>> @@ -94,25 +94,22 @@ impl MaintenanceMode {
>>>        }
>>>          pub fn check(&self, operation: Operation) -> Result<(),
>>> Error> {
>>> -        if self.ty == MaintenanceType::Delete {
>>> -            bail!("datastore is being deleted");
>>> -        }
>>> -
>>>            let message =
>>> percent_encoding::percent_decode_str(self.message.as_deref().unwrap_or(""))
>>>                .decode_utf8()
>>>                .unwrap_or(Cow::Borrowed(""));
>>
>> question: should we also avoid decoding the message if not using it?
>> Maybe by defining a closure for it?
> 
> This should be fairly cheap in the common case where self.message is
> None.
> 
> The bigger issue is that the message is never percent-encoded in the
> first place. I'd also avoid adding ": " if there is no message. I was
> going to address this in a v2 for [1] but I can also add a patch to
> this series.
> 
> [1]
> https://lore.proxmox.com/pbs-devel/50fe57b6-d7d4-40a1-9e17-bc904ea30cdd@proxmox.com/

Yes, question was rather if it is cheaper to bypass it altogether. But 
not that relevant here, can remain as is here (and the rest addressed in 
you other series).

Re: [PATCH proxmox 3/5] pbs-api-types: deny non-lookup operations for unknown modes

[Christian Ebner]

On 6/3/26 1:17 PM, Robert Obkircher wrote:
> 
> On 03.06.26 12:38, Christian Ebner wrote:
>> On 6/2/26 3:00 PM, Robert Obkircher wrote:
>>> Denying non-lookup operations for unknown modes seems like a safer
>>
>> s/seems like/is/
>>
>>> default. This change should not affect anything because the backup
>>> server does not enable the enum-fallback feature.
>>>
>>> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
>>> ---
>>>    pbs-api-types/src/maintenance.rs | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/pbs-api-types/src/maintenance.rs
>>> b/pbs-api-types/src/maintenance.rs
>>> index 7def498b..7e9599be 100644
>>> --- a/pbs-api-types/src/maintenance.rs
>>> +++ b/pbs-api-types/src/maintenance.rs
>>> @@ -109,7 +109,7 @@ impl MaintenanceMode {
>>>                    bail!("read-only maintenance mode: {message}")
>>>                }
>>>                #[cfg(feature = "enum-fallback")]
>>> -            (MaintenanceType::UnknownEnumValue(_), _) => Ok(()),
>>> +            (MaintenanceType::UnknownEnumValue(m), _) =>
>>> bail!("unknown maintenance mode: {m}"),
>>
>> What about PDM? I didn't recompile PDM with your changes on top, but
>> code wise the DatastorePanelComp currently only checks for offline
>> stores. Probably contents should not be shown if the maintenance
>> mode is unknown there (although pre-existing).
> Does it ever call that function? I couldn't find anything but my lsp
> is not always reliable.

No, but since the type is used there and we do handle parsing errors 
differently, we should aim for it to be used correctly there as well. 
Contents should not be shown if the maintenance-mode cannot be parsed IMO.

> This comparison [1] seems wrong if the mode has a message.
> 
> [1]
> https://git.proxmox.com/?p=proxmox-datacenter-manager.git;a=blob;f=ui/src/pbs/datastore.rs;h=53be35bcb747561ef8855b1efc9d6c131f516117;hb=HEAD#l52

Yes, it this should parse the maintenance-mode property string instead 
and handle it accordingly.

Re: [PATCH proxmox-backup 2/6] task tracking: use parameter for initial count and refactor updates

[Christian Ebner]

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Don't hard-code the initial count to 1 and bail out if it is negative.
> Replace the ActiveOperationStats initializers with the update logic
> because they would be reformatted to multiple lines after adding a
> field and an operation.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   pbs-datastore/src/task_tracking.rs | 52 +++++++++++++++++-------------
>   1 file changed, 30 insertions(+), 22 deletions(-)
> 
> diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs
> index d4cc076aa..a67b77221 100644
> --- a/pbs-datastore/src/task_tracking.rs
> +++ b/pbs-datastore/src/task_tracking.rs
> @@ -1,4 +1,4 @@
> -use anyhow::Error;
> +use anyhow::{Error, bail};
>   use libc::pid_t;
>   use nix::unistd::Pid;
>   use std::iter::Sum;
> @@ -15,6 +15,16 @@ pub struct ActiveOperationStats {
>       pub write: i64,
>   }
>   
> +impl ActiveOperationStats {
> +    fn add(&mut self, count: i64, operation: Operation) {
> +        match operation {
> +            Operation::Read => self.read += count,
> +            Operation::Write => self.write += count,
> +            Operation::Lookup => (), // no IO must happen there
> +        }
> +    }
> +}
> +
>   impl Sum<Self> for ActiveOperationStats {
>       fn sum<I>(iter: I) -> Self
>       where
> @@ -101,14 +111,8 @@ pub fn update_active_operations(
>       let (_lock, options) = open_lock_file(name)?;
>   
>       let pid = std::process::id();
> -    let starttime = procfs::PidStat::read_from_pid(Pid::from_raw(pid as pid_t))?.starttime;

tiny nit: this change is somewhat unrelated to the rest of the patch, 
could live in its own patch or remain as is... but no hard opinion, just 
distracted by it initially while looking at the changes.

>   
> -    let mut updated_active_operations = match operation {
> -        Operation::Read => ActiveOperationStats { read: 1, write: 0 },
> -        Operation::Write => ActiveOperationStats { read: 0, write: 1 },
> -        Operation::Lookup => ActiveOperationStats { read: 0, write: 0 },
> -    };
> -    let mut found_entry = false;
> +    let mut updated = None;
>       let mut updated_tasks: Vec<TaskOperations> = match file_read_optional_string(&path)? {
>           Some(data) => serde_json::from_str::<Vec<TaskOperations>>(&data)?
>               .iter_mut()
> @@ -117,13 +121,8 @@ pub fn update_active_operations(
>                       Some(stat) if pid == task.pid && stat.starttime != task.starttime => None,
>                       Some(_) => {
>                           if pid == task.pid {
> -                            found_entry = true;
> -                            match operation {
> -                                Operation::Read => task.active_operations.read += count,
> -                                Operation::Write => task.active_operations.write += count,
> -                                Operation::Lookup => (), // no IO must happen there
> -                            };
> -                            updated_active_operations = task.active_operations;
> +                            task.active_operations.add(count, operation);
> +                            updated = Some(task.active_operations);
>                           }
>                           Some(task.clone())
>                       }
> @@ -134,18 +133,27 @@ pub fn update_active_operations(
>           None => Vec::new(),
>       };
>   
> -    if !found_entry {
> +    let result = if let Some(updated) = updated {
> +        updated
> +    } else {
> +        if count < 0 {
> +            bail!("unexpected initial active operation count: {count} {operation:?}")
> +        }
> +        let mut new = ActiveOperationStats::default();
> +        new.add(count, operation);
>           updated_tasks.push(TaskOperations {
>               pid,
> -            starttime,
> -            active_operations: updated_active_operations,
> -        })
> -    }
> +            starttime: procfs::PidStat::read_from_pid(Pid::from_raw(pid as pid_t))?.starttime,
> +            active_operations: new,
> +        });
> +        new
> +    };
> +
>       replace_file(
>           &path,
>           serde_json::to_string(&updated_tasks)?.as_bytes(),
>           options,
>           false,
> -    )
> -    .map(|_| updated_active_operations)
> +    )?;
> +    Ok(result)
>   }

Re: [PATCH proxmox-backup 3/6] www: access active operation fields by name instead of index

[Christian Ebner]

On 6/2/26 3:01 PM, Robert Obkircher wrote:
> Avoid relying on (presumably sorted) field order so new fields could
> be added to the API response in the future.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>
> ---
>   www/datastore/OptionView.js | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
> index 42a6104dc..64e47aa57 100644
> --- a/www/datastore/OptionView.js
> +++ b/www/datastore/OptionView.js
> @@ -122,8 +122,13 @@ Ext.define('PBS.Datastore.Options', {
>   
>               view.mon(me.activeOperationsRstore, 'load', (store, data, success) => {
>                   let activeTasks = me.getView().maintenanceActiveTasks;
> -                activeTasks.read = data?.[0]?.data.value ?? 0;
> -                activeTasks.write = data?.[1]?.data.value ?? 0;
> +                if (success) {
> +                  activeTasks.read = store.getById('read')?.data.value ?? 0;
> +                  activeTasks.write = store.getById('write')?.data.value ?? 0;
> +                } else {
> +                  activeTasks.read = 0;
> +                  activeTasks.write = 0;
> +                }
>               });
>           },
>   

This introduces formatting issues as reported by `proxmox-biome check www`.

Re: [PATCH proxmox-backup 4/6] task tracking: count WriteNonExpanding datastore operations

[Christian Ebner]

Code wise this looks good to me, but I was pondering a bit if we should 
take the occasion here and define a `format-version` field in order to 
have some way of checking this in case of future extension. The older 
process might then only update values it knows about, but write out all 
the contents known to the newer versions.

Other opinions on that?

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Count WriteNonExpanding as writes to ensure that switching over from
> Write does not break forward compatibility when older versions read
> the counters.
> 
> Also add a new counter to display the GarbageCollection maintenance
> mode conflicts. It may not be accurate while older processes are
> running but it should be good enough for the UI.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   pbs-datastore/src/task_tracking.rs | 12 ++++++++++++
>   src/api2/admin/datastore.rs        |  1 +
>   2 files changed, 13 insertions(+)
> 
> diff --git a/pbs-datastore/src/task_tracking.rs b/pbs-datastore/src/task_tracking.rs
> index a67b77221..87ce4f5de 100644
> --- a/pbs-datastore/src/task_tracking.rs
> +++ b/pbs-datastore/src/task_tracking.rs
> @@ -13,6 +13,12 @@ use serde::{Deserialize, Serialize};
>   pub struct ActiveOperationStats {
>       pub read: i64,
>       pub write: i64,
> +    /// The number of writers that do not significantly increase disk usage.
> +    ///
> +    /// The count is unreliable because older processes without this field
> +    /// will delete it on every update!
> +    #[serde(default)]
> +    pub write_non_expanding: i64,
>   }
>   
>   impl ActiveOperationStats {
> @@ -20,6 +26,11 @@ impl ActiveOperationStats {
>           match operation {
>               Operation::Read => self.read += count,
>               Operation::Write => self.write += count,
> +            Operation::WriteNonExpanding => {
> +                self.write += count;
> +                // prevent underflow if an older process deleted the field
> +                self.write_non_expanding = (self.write_non_expanding + count).max(0);
> +            }
>               Operation::Lookup => (), // no IO must happen there
>           }
>       }
> @@ -33,6 +44,7 @@ impl Sum<Self> for ActiveOperationStats {
>           iter.fold(Self::default(), |a, b| Self {
>               read: a.read + b.read,
>               write: a.write + b.write,
> +            write_non_expanding: a.write_non_expanding + b.write_non_expanding,
>           })
>       }
>   }
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 94272c32d..3cbdd8922 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2024,6 +2024,7 @@ pub fn get_active_operations(store: String, _param: Value) -> Result<Value, Erro
>       Ok(json!({
>           "read": active_operations.read,
>           "write": active_operations.write,
> +        "writeNonExpanding": active_operations.write_non_expanding,
>       }))
>   }
>   

Re: [PATCH proxmox-backup 5/6] datastore: open datastores with WriteNonExpanding instead of Write

[Christian Ebner]

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Allow reclaim and garbage-collection jobs when the GarbageCollection
> maintenance mode is active. Only include the most important operations
> for now. Others, such as set_backup_owner and group/namespace moves,
> could be reasonably supported as well.
> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

Re: [PATCH proxmox-backup 6/6] fix #5797: www: display new GarbageCollection maintenance mode

[Christian Ebner]

On 6/2/26 3:01 PM, Robert Obkircher wrote:
> Display the new mode and make it selectable. Subtract non-expanding
> writes from the number of conflicts, since they are allowed in this
> mode.
> 
> Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=5797
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

Re: [PATCH proxmox{,-backup} 00/11] add GarbageCollection maintenance mode

[Christian Ebner]

On 6/2/26 2:59 PM, Robert Obkircher wrote:
> Add a maintenance mode that allows reclaiming storage space without
> the risk of running out of space because of new backups.
> 
> Changes since [RFC]:
> * Excluded the space-check patches. I'll send them separately.
> * Propagate maintenance mode parse errors.
> * Rename Operation::Prune to Operation::WriteNonExpanding.
> * Track them in active operations to display GC mode conflicts.
> * Replace if-else with match in maintenance mode check.
> * Fixed typos and added Bugzilla url trailers.
> 
> [RFC] https://lore.proxmox.com/pbs-devel/20260430150607.330413-1-r.obkircher@proxmox.com/


Tested:
- Maintenance mode GC cannot be set while active backup or other write
   operation.
- Mode is entered when conflicting active operations are finished.
- Restore/singe file restore works while mode is active.
- Verify, prune and GC can be executed while maintenance mode GC is set.
- Protected mode can be set/cleared while maintenance mode active.
- Group and snapshot notes cannot be changed while mode is active.
- Push sync works while mode is active, pull does not.
- Moves fail if mode is set, namespaces can be deleted (inclusive all
   groups) but not created
- Manually editing of maintenance mode to an unknown variant leads to
   errors, showing the corresponding error masks in the ui (noted the 1
   min cache timeout required for config changes to take effect).

Only noticed that the datastore summary panel it might be worth to show 
the maintenance type as well, maybe above or below the estimated full 
and de-duplication factor. Currently only offline datastores are easily 
identified as there they are masked accordingly.

Since the mode can be actively set by users, it should also be added to 
the documentation with a concise description of what operations are 
being blocked in this maintenance mode.

Consider:

Tested-by: Christian Ebner <c.ebner@proxmox.com>