[PATCH proxmox-backup 4/4] api/datastore: use maintenance-mode lock to protect against changes

[Christian Ebner <c.ebner@proxmox.com>]

Instead of locking the whole config, which causes also unrelated
datastores to not be available, use a per-datastore maintenance mode
lock to protect against changes during potentially longer running
operations such as s3-refresh or removable datastore mount/unmount.

However keep the current semantic of first setting the maintenance
mode, allow to still opt out from the mode while waiting for active
operations to finish, and only then enforce the maintenance mode
during the s3 refresh or removable datastore mount operation.

unset_maintenance() is adapted to either reacquire the datastore
config lock or use the optional lock guard.

Fixes: https://forum.proxmox.com/threads/183244/
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/datastore.rs |  2 ++
 pbs-datastore/src/lib.rs       | 11 ++++++++++-
 src/api2/admin/datastore.rs    | 26 ++++++++++++++++++++------
 src/api2/config/datastore.rs   |  6 +++++-
 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 34efcd398..aa372cd73 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -3036,6 +3036,7 @@ impl DataStore {
         let (mut config, _digest) = pbs_config::datastore::config()?;
         let mut datastore_config: DataStoreConfig = config.lookup("datastore", name)?;
 
+        let maintenance_mode_lock = crate::maintenance_mode_lock(name)?;
         datastore_config.set_maintenance_mode(Some(MaintenanceMode {
             ty: MaintenanceType::Delete,
             message: None,
@@ -3043,6 +3044,7 @@ impl DataStore {
 
         config.set_data(name, "datastore", &datastore_config)?;
         pbs_config::datastore::save_config(&config)?;
+        drop(maintenance_mode_lock);
         drop(config_lock);
 
         let (operations, _lock) = task_tracking::get_active_operations_locked(name)?;
diff --git a/pbs-datastore/src/lib.rs b/pbs-datastore/src/lib.rs
index 48acba4c8..823f3bf2f 100644
--- a/pbs-datastore/src/lib.rs
+++ b/pbs-datastore/src/lib.rs
@@ -159,8 +159,9 @@
 
 use std::os::unix::io::AsRawFd;
 use std::path::Path;
+use std::time::Duration;
 
-use anyhow::{bail, Error};
+use anyhow::{bail, Context, Error};
 
 use pbs_config::BackupLockGuard;
 
@@ -271,3 +272,11 @@ where
 
     Ok(lock)
 }
+
+/// Acquire an exclusive lock for the datastore's maintenance-mode
+pub fn maintenance_mode_lock(store: &str) -> Result<BackupLockGuard, Error> {
+    lock_helper(store, Path::new("maintenance-mode.lck"), |p| {
+        pbs_config::open_backup_lockfile(p, Some(Duration::from_secs(0)), true)
+            .context("unable to acquire exclusive datastore's maintenance-mode lock")
+    })
+}
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 1e0a78f3a..883091e6f 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -61,8 +61,8 @@ use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::BackupManifest;
 use pbs_datastore::prune::compute_prune_info;
 use pbs_datastore::{
-    check_backup_owner, ensure_datastore_is_mounted, task_tracking, BackupDir, DataStore,
-    LocalChunkReader, StoreProgress,
+    check_backup_owner, ensure_datastore_is_mounted, maintenance_mode_lock, task_tracking,
+    BackupDir, DataStore, LocalChunkReader, StoreProgress,
 };
 use pbs_tools::json::required_string_param;
 use proxmox_rest_server::{formatter, worker_is_active, WorkerTask};
@@ -2705,9 +2705,13 @@ fn expect_maintenance_type(
 }
 
 fn unset_maintenance(
-    _lock: pbs_config::BackupLockGuard,
+    lock: Option<pbs_config::BackupLockGuard>,
     mut config: DataStoreConfig,
 ) -> Result<(), Error> {
+    let _lock = match lock {
+        Some(lock) => lock,
+        None => pbs_config::datastore::lock_config()?,
+    };
     let (mut section_config, _digest) = pbs_config::datastore::config()?;
     config.maintenance_mode = None;
     section_config.set_data(&config.name, "datastore", &config)?;
@@ -2763,6 +2767,7 @@ async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<V
     let (mut section_config, _digest) = pbs_config::datastore::config()?;
     let mut datastore: DataStoreConfig = section_config.lookup("datastore", &store)?;
 
+    let maintenance_mode_lock = maintenance_mode_lock(&store)?;
     datastore.set_maintenance_mode(Some(MaintenanceMode {
         ty: MaintenanceType::Unmount,
         message: None,
@@ -2770,6 +2775,7 @@ async fn do_unmount(store: String, auth_id: Authid, to_stdout: bool) -> Result<V
     section_config.set_data(&store, "datastore", &datastore)?;
     pbs_config::datastore::save_config(&section_config)?;
 
+    drop(maintenance_mode_lock);
     drop(_lock);
 
     if let Ok(proxy_pid) = proxmox_rest_server::read_pid(pbs_buildcfg::PROXMOX_BACKUP_PROXY_PID_FN)
@@ -2845,6 +2851,7 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
     let (mut section_config, _digest) = pbs_config::datastore::config()?;
     let mut datastore_config: DataStoreConfig = section_config.lookup("datastore", &store)?;
 
+    let maintenance_mode_lock = maintenance_mode_lock(&store)?;
     datastore_config.set_maintenance_mode(Some(MaintenanceMode {
         ty: MaintenanceType::S3Refresh,
         message: None,
@@ -2852,6 +2859,7 @@ pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Valu
 
     section_config.set_data(&store, "datastore", &datastore_config)?;
     pbs_config::datastore::save_config(&section_config)?;
+    drop(maintenance_mode_lock);
     drop(_lock);
 
     let upid = WorkerTask::new_thread(
@@ -2875,7 +2883,8 @@ pub(crate) fn do_s3_refresh(store: &str, worker: &dyn WorkerTaskContext) -> Resu
 }
 
 /// Wait for no more active operations on the given datastore and run the provided callback in
-/// a datastore locked context, protecting against maintenance mode changes.
+/// a maintenance mode locked context, protecting against leaving the mode for the functions
+/// lifetime.
 fn run_maintenance_locked(
     store: &str,
     maintenance_expected: MaintenanceType,
@@ -2908,15 +2917,20 @@ fn run_maintenance_locked(
         let _ = expect_maintenance_type(store, maintenance_expected)
             .inspect_err(|e| warn!("maintenance mode was not as expected: {e}"))
             .and_then(|(lock, config)| {
-                unset_maintenance(lock, config)
+                unset_maintenance(Some(lock), config)
                     .inspect_err(|e| warn!("could not reset maintenance mode: {e}"))
             });
         bail!("aborted, due to user request");
     }
 
     let (lock, config) = expect_maintenance_type(store, maintenance_expected)?;
+    let _maintenance_mode_lock = maintenance_mode_lock(&config.name)?;
+    // avoid blocking config access/updates, but keep maintenance-mode locked
+    // until done.
+    drop(lock);
+
     callback()?;
-    unset_maintenance(lock, config)
+    unset_maintenance(None, config)
         .map_err(|e| format_err!("could not reset maintenance mode: {e}"))?;
 
     Ok(())
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 16e85a636..b07e7fc7e 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -31,7 +31,9 @@ use crate::api2::config::tape_backup_job::{delete_tape_backup_job, list_tape_bac
 use crate::api2::config::verify::delete_verification_job;
 use pbs_config::CachedUserInfo;
 
-use pbs_datastore::{get_datastore_mount_status, DataStore, S3_DATASTORE_IN_USE_MARKER};
+use pbs_datastore::{
+    get_datastore_mount_status, maintenance_mode_lock, DataStore, S3_DATASTORE_IN_USE_MARKER,
+};
 use proxmox_rest_server::WorkerTask;
 use proxmox_s3_client::{S3ObjectKey, S3_HTTP_REQUEST_TIMEOUT};
 
@@ -540,6 +542,7 @@ pub fn update_datastore(
                     data.tuning = None;
                 }
                 DeletableProperty::MaintenanceMode => {
+                    let _maintenance_mode_lock = maintenance_mode_lock(&name)?;
                     data.set_maintenance_mode(None)?;
                 }
                 DeletableProperty::NotificationThresholds => {
@@ -638,6 +641,7 @@ pub fn update_datastore(
             )?),
             None => None,
         };
+        let _maintenance_mode_lock = maintenance_mode_lock(&name)?;
         data.set_maintenance_mode(maintenance_mode)?;
     }
 
-- 
2.47.3