[pbs-devel] [PATCH v2 proxmox-backup 0/4] close #3071: maintenance mode for datastores

[pbs-devel] [PATCH v2 proxmox-backup 0/4] close #3071: maintenance mode for datastores

[Hannes Laimer]

v2:
 - check for maintenance now directly in lookup_datastore
 - parameter for checking is now the last acceptable maintenance type,
   description in commit msg of 2nd patch
 - ui cleanup 

Adds a maintenance mode for datatsores. If a datastore is in maintenance
mode it has a maintenance-type and a maintenance-msg.
The type specifies what is still allowed on the ds and what is not.
Currently there are two types: read only and offline. 'read only' prevents
everything that would write something to the ds, but allows to read from
it. 'offline' prevents everything on the datastore, neither operation
that write nor operations that read form the datastore are allowed.
The message is optional and is a short string that describes the reason
for the maintenance, it is shown whenever an operation is prevented due
to the maintenance.

Already running jobs and operations are not affected. Neither are the
modification or creation of jobs, since they just modify a config file
that is stored on the host system.

As Dominik Csapak <d.csapak@proxmox.com> suggested, it might make sense
to prevent setting a maintenance mode while jobs run.


Hannes Laimer (4):
  pbs-api-types: add maintenance type and msg to ds config
  pbs-datastore: add check for maintenance to lookup_datastore
  api2: make maintenance type and msg updatable/deletable
  ui: add maintenance to datastore options

 pbs-api-types/src/datastore.rs       | 33 ++++++++++++++++++
 pbs-datastore/src/datastore.rs       | 14 ++++++--
 pbs-datastore/src/snapshot_reader.rs |  6 +++-
 src/api2/admin/datastore.rs          | 44 ++++++++++++------------
 src/api2/backup/mod.rs               |  2 +-
 src/api2/config/datastore.rs         |  8 +++++
 src/api2/pull.rs                     |  4 +--
 src/api2/reader/mod.rs               |  6 ++--
 src/api2/status.rs                   |  6 ++--
 src/api2/tape/backup.rs              |  4 +--
 src/api2/tape/restore.rs             |  6 ++--
 src/bin/proxmox-backup-proxy.rs      |  6 ++--
 src/server/prune_job.rs              |  2 +-
 src/server/verify_job.rs             |  4 +--
 www/Makefile                         |  1 +
 www/Utils.js                         |  7 ++++
 www/datastore/OptionView.js          |  9 +++++
 www/window/MaintenanceOptions.js     | 51 ++++++++++++++++++++++++++++
 18 files changed, 168 insertions(+), 45 deletions(-)
 create mode 100644 www/window/MaintenanceOptions.js

-- 
2.30.2

[pbs-devel] [PATCH v2 proxmox-backup 1/4] pbs-api-types: add maintenance-type and msg to ds config

[Hannes Laimer]

---
 pbs-api-types/src/datastore.rs | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 75f82ea4..d1f3db9a 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -165,6 +165,26 @@ pub struct PruneOptions {
     pub keep_yearly: Option<u64>,
 }
 
+#[api()]
+#[derive(PartialOrd, Debug, Copy, Clone, PartialEq, Serialize, Deserialize)]
+#[serde(rename_all = "lowercase")]
+/// Different maintenance types.
+pub enum MaintenanceType {
+    /// Only reading operations are allowed.
+    ReadOnly,
+    /// Neither reading nor writing is allowed on the datastore. 
+    Offline,
+}
+
+impl std::fmt::Display for MaintenanceType {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+       match *self {
+           MaintenanceType::ReadOnly => write!(f, "read-only"),
+           MaintenanceType::Offline => write!(f, "offline"),
+       }
+    }
+}
+
 #[api(
     properties: {
         name: {
@@ -222,6 +242,15 @@ pub struct PruneOptions {
             optional: true,
             type: bool,
         },
+        "maintenance-type": {
+            optional: true,
+            type: MaintenanceType,
+        },
+        "maintenance-msg": {
+            description: "Text that will be shown as a description for the maintenance.",
+            optional: true,
+            type: String,
+        },
     }
 )]
 #[derive(Serialize,Deserialize,Updater)]
@@ -259,6 +288,10 @@ pub struct DataStoreConfig {
     /// Send notification only for job errors
     #[serde(skip_serializing_if="Option::is_none")]
     pub notify: Option<String>,
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub maintenance_type: Option<MaintenanceType>,
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub maintenance_msg: Option<String>,
 }
 
 #[api(
-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 1/4] pbs-api-types: add maintenance-type and msg to ds config

[Dominik Csapak]

any reason why you did not combine the type+message like i suggested?
in general it's ok to do it this way, but maybe giving a reason
(at least in the patch changelog) would be good

On 10/6/21 17:14, Hannes Laimer wrote:
> ---
>   pbs-api-types/src/datastore.rs | 33 +++++++++++++++++++++++++++++++++
>   1 file changed, 33 insertions(+)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 75f82ea4..d1f3db9a 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -165,6 +165,26 @@ pub struct PruneOptions {
>       pub keep_yearly: Option<u64>,
>   }
>   
> +#[api()]
> +#[derive(PartialOrd, Debug, Copy, Clone, PartialEq, Serialize, Deserialize)]
> +#[serde(rename_all = "lowercase")]
> +/// Different maintenance types.
> +pub enum MaintenanceType {
> +    /// Only reading operations are allowed.
> +    ReadOnly,
> +    /// Neither reading nor writing is allowed on the datastore.
> +    Offline,
> +}
> +
> +impl std::fmt::Display for MaintenanceType {
> +    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
> +       match *self {
> +           MaintenanceType::ReadOnly => write!(f, "read-only"),
> +           MaintenanceType::Offline => write!(f, "offline"),
> +       }
> +    }
> +}
> +
>   #[api(
>       properties: {
>           name: {
> @@ -222,6 +242,15 @@ pub struct PruneOptions {
>               optional: true,
>               type: bool,
>           },
> +        "maintenance-type": {
> +            optional: true,
> +            type: MaintenanceType,
> +        },
> +        "maintenance-msg": {
> +            description: "Text that will be shown as a description for the maintenance.",
> +            optional: true,
> +            type: String,
> +        },
>       }
>   )]
>   #[derive(Serialize,Deserialize,Updater)]
> @@ -259,6 +288,10 @@ pub struct DataStoreConfig {
>       /// Send notification only for job errors
>       #[serde(skip_serializing_if="Option::is_none")]
>       pub notify: Option<String>,
> +    #[serde(skip_serializing_if="Option::is_none")]
> +    pub maintenance_type: Option<MaintenanceType>,
> +    #[serde(skip_serializing_if="Option::is_none")]
> +    pub maintenance_msg: Option<String>,
>   }
>   
>   #[api(
> 

[pbs-devel] [PATCH v2 proxmox-backup 2/4] pbs-datastore: add check for maintenance to lookup_datastore

[Hannes Laimer]

lookup_datastore now takes a second parameter, an option
'acceptable_maintenance'-type.
If None is passed, the datastore has to be in no maintenance at all.
If read-only is passed, the datastore can either be in 'read-only' or
any less restrictive maintenance type.
If offline is passed, the datastore can either be in 'offline' or
any less restrictive maintenance type.
---
 pbs-datastore/src/datastore.rs       | 14 +++++++--
 pbs-datastore/src/snapshot_reader.rs |  6 +++-
 src/api2/admin/datastore.rs          | 44 ++++++++++++++--------------
 src/api2/backup/mod.rs               |  2 +-
 src/api2/pull.rs                     |  4 +--
 src/api2/reader/mod.rs               |  6 ++--
 src/api2/status.rs                   |  6 ++--
 src/api2/tape/backup.rs              |  4 +--
 src/api2/tape/restore.rs             |  6 ++--
 src/bin/proxmox-backup-proxy.rs      |  6 ++--
 src/server/prune_job.rs              |  2 +-
 src/server/verify_job.rs             |  4 +--
 12 files changed, 59 insertions(+), 45 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index b068a9c9..faaff1ca 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -11,7 +11,7 @@ use lazy_static::lazy_static;
 
 use proxmox::tools::fs::{replace_file, file_read_optional_string, CreateOptions};
 
-use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus};
+use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus, MaintenanceType};
 use pbs_tools::format::HumanByte;
 use pbs_tools::fs::{lock_dir_noblock, DirLockGuard};
 use pbs_tools::process_locker::ProcessLockSharedGuard;
@@ -61,12 +61,22 @@ pub struct DataStore {
 
 impl DataStore {
 
-    pub fn lookup_datastore(name: &str) -> Result<Arc<DataStore>, Error> {
+    pub fn lookup_datastore(name: &str, acceptable_maintenance: Option<MaintenanceType>) -> Result<Arc<DataStore>, Error> {
 
         let (config, _digest) = pbs_config::datastore::config()?;
         let config: DataStoreConfig = config.lookup("datastore", name)?;
         let path = PathBuf::from(&config.path);
 
+        if let Some(maintenance_type) = config.maintenance_type {
+            if acceptable_maintenance.map_or(true, |acceptable| maintenance_type > acceptable) {
+                bail!("Datastore '{}' is in maintenance '{}' mode: {}", 
+                    name,
+                    maintenance_type,
+                    config.maintenance_msg.unwrap_or(String::from("no description"))
+                ); 
+            } 
+        }
+
         let mut map = DATASTORE_MAP.lock().unwrap();
 
         if let Some(datastore) = map.get(name) {
diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs
index c49cf16f..5b1f6a0f 100644
--- a/pbs-datastore/src/snapshot_reader.rs
+++ b/pbs-datastore/src/snapshot_reader.rs
@@ -12,6 +12,7 @@ use crate::fixed_index::FixedIndexReader;
 use crate::dynamic_index::DynamicIndexReader;
 use crate::manifest::{archive_type, ArchiveType, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME};
 use crate::DataStore;
+use pbs_api_types::MaintenanceType;
 use pbs_tools::fs::lock_dir_noblock_shared;
 
 /// Helper to access the contents of a datastore backup snapshot
@@ -119,7 +120,10 @@ impl <'a> Iterator for SnapshotChunkIterator<'a> {
                         };
 
                         let datastore =
-                            DataStore::lookup_datastore(self.snapshot_reader.datastore_name())?;
+                            DataStore::lookup_datastore(
+                                self.snapshot_reader.datastore_name(),
+                                Some(MaintenanceType::ReadOnly)
+                            )?;
                         let order = datastore.get_chunks_in_order(&index, |_| false, |_| Ok(()))?;
 
                         self.current_index = Some((Arc::new(index), 0, order));
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 7e9a0ee0..f97e3642 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -28,7 +28,7 @@ use pxar::EntryKind;
 
 use pbs_api_types::{ Authid, BackupContent, Counts, CryptMode,
     DataStoreListItem, GarbageCollectionStatus, GroupListItem,
-    SnapshotListItem, SnapshotVerifyState, PruneOptions,
+    MaintenanceType, SnapshotListItem, SnapshotVerifyState, PruneOptions,
     DataStoreStatus, RRDMode, RRDTimeFrameResolution,
     BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_TIME_SCHEMA,
     BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA,
@@ -171,7 +171,7 @@ pub fn list_groups(
     let user_info = CachedUserInfo::new()?;
     let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
     let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
 
     let backup_groups = BackupInfo::list_backup_groups(&datastore.base_path())?;
@@ -269,7 +269,7 @@ pub fn delete_group(
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     let group = BackupGroup::new(backup_type, backup_id);
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
 
@@ -314,7 +314,7 @@ pub fn list_snapshot_files(
 ) -> Result<Vec<BackupContent>, Error> {
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
 
     let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
 
@@ -364,7 +364,7 @@ pub fn delete_snapshot(
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
 
@@ -413,7 +413,7 @@ pub fn list_snapshots (
 
     let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
 
     let base_path = datastore.base_path();
 
@@ -607,7 +607,7 @@ pub fn status(
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<DataStoreStatus, Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
     let storage = crate::tools::disks::disk_usage(&datastore.base_path())?;
     let (counts, gc_status) = if verbose {
         let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
@@ -685,7 +685,7 @@ pub fn verify(
     outdated_after: Option<i64>,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
     let ignore_verified = ignore_verified.unwrap_or(true);
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
@@ -830,7 +830,7 @@ pub fn prune(
 
     let group = BackupGroup::new(&backup_type, &backup_id);
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
 
@@ -953,7 +953,7 @@ pub fn prune_datastore(
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
 
@@ -997,7 +997,7 @@ pub fn start_garbage_collection(
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     let job =  Job::new("garbage_collection", &store)
@@ -1033,7 +1033,7 @@ pub fn garbage_collection_status(
     _rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<GarbageCollectionStatus, Error> {
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::Offline))?;
 
     let status = datastore.last_gc_status();
 
@@ -1109,7 +1109,7 @@ pub fn download_file(
 
     async move {
         let store = required_string_param(&param, "store")?;
-        let datastore = DataStore::lookup_datastore(store)?;
+        let datastore = DataStore::lookup_datastore(store, Some(MaintenanceType::ReadOnly))?;
 
         let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
@@ -1179,7 +1179,7 @@ pub fn download_file_decoded(
 
     async move {
         let store = required_string_param(&param, "store")?;
-        let datastore = DataStore::lookup_datastore(store)?;
+        let datastore = DataStore::lookup_datastore(store, Some(MaintenanceType::ReadOnly))?;
 
         let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
@@ -1293,7 +1293,7 @@ pub fn upload_backup_log(
 
     async move {
         let store = required_string_param(&param, "store")?;
-        let datastore = DataStore::lookup_datastore(store)?;
+        let datastore = DataStore::lookup_datastore(store, None)?;
 
         let file_name =  CLIENT_LOG_BLOB_NAME;
 
@@ -1370,7 +1370,7 @@ pub fn catalog(
     filepath: String,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Vec<ArchiveEntry>, Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
@@ -1440,7 +1440,7 @@ pub fn pxar_file_download(
 
     async move {
         let store = required_string_param(&param, "store")?;
-        let datastore = DataStore::lookup_datastore(&store)?;
+        let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
 
         let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
@@ -1595,7 +1595,7 @@ pub fn get_group_notes(
     backup_id: String,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_group = BackupGroup::new(backup_type, backup_id);
@@ -1637,7 +1637,7 @@ pub fn set_group_notes(
     notes: String,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<(), Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_group = BackupGroup::new(backup_type, backup_id);
@@ -1679,7 +1679,7 @@ pub fn get_notes(
     backup_time: i64,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
@@ -1730,7 +1730,7 @@ pub fn set_notes(
     notes: String,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<(), Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
@@ -1775,7 +1775,7 @@ pub fn set_backup_owner(
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<(), Error> {
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     let backup_group = BackupGroup::new(backup_type, backup_id);
 
diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
index 4432c8d5..8e45ec1f 100644
--- a/src/api2/backup/mod.rs
+++ b/src/api2/backup/mod.rs
@@ -74,7 +74,7 @@ async move {
     let user_info = CachedUserInfo::new()?;
     user_info.check_privs(&auth_id, &["datastore", &store], PRIV_DATASTORE_BACKUP, false)?;
 
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     let backup_type = required_string_param(&param, "backup-type")?;
     let backup_id = required_string_param(&param, "backup-id")?;
diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index ea8faab8..26b8a3be 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -9,7 +9,7 @@ use proxmox::api::{ApiMethod, Router, RpcEnvironment, Permission};
 
 use pbs_client::{HttpClient, BackupRepository};
 use pbs_api_types::{
-    Remote, Authid, SyncJobConfig,
+    Remote, Authid, MaintenanceType, SyncJobConfig,
     DATASTORE_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
     PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ,
 };
@@ -46,7 +46,7 @@ pub async fn get_pull_parameters(
     remote_store: &str,
 ) -> Result<(HttpClient, BackupRepository, Arc<DataStore>), Error> {
 
-    let tgt_store = DataStore::lookup_datastore(store)?;
+    let tgt_store = DataStore::lookup_datastore(store, Some(MaintenanceType::Offline))?;
 
     let (remote_config, _digest) = pbs_config::remote::config()?;
     let remote: Remote = remote_config.lookup("remote", remote)?;
diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
index 1fd25a5c..f209e50a 100644
--- a/src/api2/reader/mod.rs
+++ b/src/api2/reader/mod.rs
@@ -28,8 +28,8 @@ use proxmox::{
 };
 
 use pbs_api_types::{
-    Authid, DATASTORE_SCHEMA, BACKUP_TYPE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_ID_SCHEMA,
-    CHUNK_DIGEST_SCHEMA, PRIV_DATASTORE_READ, PRIV_DATASTORE_BACKUP,
+    Authid, MaintenanceType, DATASTORE_SCHEMA, BACKUP_TYPE_SCHEMA, BACKUP_TIME_SCHEMA,
+    BACKUP_ID_SCHEMA, CHUNK_DIGEST_SCHEMA, PRIV_DATASTORE_READ, PRIV_DATASTORE_BACKUP,
     BACKUP_ARCHIVE_NAME_SCHEMA,
 };
 use pbs_tools::fs::lock_dir_noblock_shared;
@@ -93,7 +93,7 @@ fn upgrade_to_backup_reader_protocol(
             bail!("no permissions on /datastore/{}", store);
         }
 
-        let datastore = DataStore::lookup_datastore(&store)?;
+        let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
 
         let backup_type = required_string_param(&param, "backup-type")?;
         let backup_id = required_string_param(&param, "backup-id")?;
diff --git a/src/api2/status.rs b/src/api2/status.rs
index 548e5319..3f168f25 100644
--- a/src/api2/status.rs
+++ b/src/api2/status.rs
@@ -15,8 +15,8 @@ use proxmox::api::{
 };
 
 use pbs_api_types::{
-    Authid, DATASTORE_SCHEMA, RRDMode, RRDTimeFrameResolution,
-    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
+    Authid, MaintenanceType, DATASTORE_SCHEMA, RRDMode,
+    RRDTimeFrameResolution, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
 };
 
 use pbs_datastore::DataStore;
@@ -98,7 +98,7 @@ pub fn datastore_status(
             continue;
         }
 
-        let datastore = match DataStore::lookup_datastore(&store) {
+        let datastore = match DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly)) {
             Ok(datastore) => datastore,
             Err(err) => {
                 list.push(json!({
diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
index 7627e35b..05ed9c53 100644
--- a/src/api2/tape/backup.rs
+++ b/src/api2/tape/backup.rs
@@ -174,7 +174,7 @@ pub fn do_tape_backup_job(
 
     let worker_type = job.jobtype().to_string();
 
-    let datastore = DataStore::lookup_datastore(&setup.store)?;
+    let datastore = DataStore::lookup_datastore(&setup.store, None)?;
 
     let (config, _digest) = pbs_config::media_pool::config()?;
     let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
@@ -355,7 +355,7 @@ pub fn backup(
         &setup.drive,
     )?;
 
-    let datastore = DataStore::lookup_datastore(&setup.store)?;
+    let datastore = DataStore::lookup_datastore(&setup.store, None)?;
 
     let (config, _digest) = pbs_config::media_pool::config()?;
     let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
index 61e17d1b..a23c69c7 100644
--- a/src/api2/tape/restore.rs
+++ b/src/api2/tape/restore.rs
@@ -29,7 +29,7 @@ use proxmox::{
 };
 
 use pbs_api_types::{
-    Authid, Userid, CryptMode,
+    Authid, MaintenanceType, Userid, CryptMode,
     DATASTORE_MAP_ARRAY_SCHEMA, DATASTORE_MAP_LIST_SCHEMA, DRIVE_NAME_SCHEMA,
     UPID_SCHEMA, TAPE_RESTORE_SNAPSHOT_SCHEMA,
     PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_TAPE_READ,
@@ -106,10 +106,10 @@ impl TryFrom<String> for DataStoreMap {
             if let Some(index) = store.find('=') {
                 let mut target = store.split_off(index);
                 target.remove(0); // remove '='
-                let datastore = DataStore::lookup_datastore(&target)?;
+                let datastore = DataStore::lookup_datastore(&target, Some(MaintenanceType::ReadOnly))?;
                 map.insert(store, datastore);
             } else if default.is_none() {
-                default = Some(DataStore::lookup_datastore(&store)?);
+                default = Some(DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?);
             } else {
                 bail!("multiple default stores given");
             }
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 9199ebae..f80b9d7c 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -47,8 +47,8 @@ use proxmox_systemd::time::{compute_next_event, parse_calendar_event};
 use pbs_tools::logrotate::LogRotate;
 
 use pbs_api_types::{
-    Authid, TapeBackupJobConfig, VerificationJobConfig, SyncJobConfig, DataStoreConfig,
-    PruneOptions,
+    Authid, MaintenanceType, TapeBackupJobConfig, VerificationJobConfig,
+    SyncJobConfig, DataStoreConfig, PruneOptions,
 };
 
 use proxmox_rest_server::daemon;
@@ -527,7 +527,7 @@ async fn schedule_datastore_garbage_collection() {
     };
 
     for (store, (_, store_config)) in config.sections {
-        let datastore = match DataStore::lookup_datastore(&store) {
+        let datastore = match DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly)) {
             Ok(datastore) => datastore,
             Err(err) => {
                 eprintln!("lookup_datastore failed - {}", err);
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index fc6443e9..00a055f4 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -96,7 +96,7 @@ pub fn do_prune_job(
     auth_id: &Authid,
     schedule: Option<String>,
 ) -> Result<String, Error> {
-    let datastore = DataStore::lookup_datastore(&store)?;
+    let datastore = DataStore::lookup_datastore(&store, None)?;
 
     let worker_type = job.jobtype().to_string();
     let auth_id = auth_id.clone();
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index 6aba97c9..05c43602 100644
--- a/src/server/verify_job.rs
+++ b/src/server/verify_job.rs
@@ -1,7 +1,7 @@
 use anyhow::{format_err, Error};
 
 use pbs_tools::task_log;
-use pbs_api_types::{Authid, VerificationJobConfig};
+use pbs_api_types::{Authid, MaintenanceType, VerificationJobConfig};
 use proxmox_rest_server::WorkerTask;
 use pbs_datastore::DataStore;
 
@@ -22,7 +22,7 @@ pub fn do_verification_job(
     to_stdout: bool,
 ) -> Result<String, Error> {
 
-    let datastore = DataStore::lookup_datastore(&verification_job.store)?;
+    let datastore = DataStore::lookup_datastore(&verification_job.store, Some(MaintenanceType::ReadOnly))?;
 
     let outdated_after = verification_job.outdated_after;
     let ignore_verified_snapshots = verification_job.ignore_verified.unwrap_or(true);
-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 2/4] pbs-datastore: add check for maintenance to lookup_datastore

[Dominik Csapak]

much better, since now we

* do not accidentally lookup a datastore without making the intent clear
* and have the intent as parameter

though maybe it would have been even nicer to give the intended
operation (read/write/none) instead of the maintanence type directly,
so the user does not need to know which maintenance type does which?
(though it is very clear now too, so see this more as an optional 
comment...)

some more comments inline

On 10/6/21 17:14, Hannes Laimer wrote:
> lookup_datastore now takes a second parameter, an option
> 'acceptable_maintenance'-type.
> If None is passed, the datastore has to be in no maintenance at all.
> If read-only is passed, the datastore can either be in 'read-only' or
> any less restrictive maintenance type.
> If offline is passed, the datastore can either be in 'offline' or
> any less restrictive maintenance type.
> ---
>   pbs-datastore/src/datastore.rs       | 14 +++++++--
>   pbs-datastore/src/snapshot_reader.rs |  6 +++-
>   src/api2/admin/datastore.rs          | 44 ++++++++++++++--------------
>   src/api2/backup/mod.rs               |  2 +-
>   src/api2/pull.rs                     |  4 +--
>   src/api2/reader/mod.rs               |  6 ++--
>   src/api2/status.rs                   |  6 ++--
>   src/api2/tape/backup.rs              |  4 +--
>   src/api2/tape/restore.rs             |  6 ++--
>   src/bin/proxmox-backup-proxy.rs      |  6 ++--
>   src/server/prune_job.rs              |  2 +-
>   src/server/verify_job.rs             |  4 +--
>   12 files changed, 59 insertions(+), 45 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index b068a9c9..faaff1ca 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -11,7 +11,7 @@ use lazy_static::lazy_static;
>   
>   use proxmox::tools::fs::{replace_file, file_read_optional_string, CreateOptions};
>   
> -use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus};
> +use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus, MaintenanceType};
>   use pbs_tools::format::HumanByte;
>   use pbs_tools::fs::{lock_dir_noblock, DirLockGuard};
>   use pbs_tools::process_locker::ProcessLockSharedGuard;
> @@ -61,12 +61,22 @@ pub struct DataStore {
>   
>   impl DataStore {
>   
> -    pub fn lookup_datastore(name: &str) -> Result<Arc<DataStore>, Error> {
> +    pub fn lookup_datastore(name: &str, acceptable_maintenance: Option<MaintenanceType>) -> Result<Arc<DataStore>, Error> {
>   
>           let (config, _digest) = pbs_config::datastore::config()?;
>           let config: DataStoreConfig = config.lookup("datastore", name)?;
>           let path = PathBuf::from(&config.path);
>   
> +        if let Some(maintenance_type) = config.maintenance_type {
> +            if acceptable_maintenance.map_or(true, |acceptable| maintenance_type > acceptable) {
> +                bail!("Datastore '{}' is in maintenance '{}' mode: {}",
> +                    name,
> +                    maintenance_type,
> +                    config.maintenance_msg.unwrap_or(String::from("no description"))
> +                );
> +            }
> +        }

imho it would be better to write it as a match like this:

match (config.maintenance_type, acceptable_maintenance) => {
     (Some(a), Some(b)) if a > b | (Some(_), None) => error,
     (None, _) => {},
}

?

> +
>           let mut map = DATASTORE_MAP.lock().unwrap();
>   
>           if let Some(datastore) = map.get(name) {
> diff --git a/pbs-datastore/src/snapshot_reader.rs b/pbs-datastore/src/snapshot_reader.rs
> index c49cf16f..5b1f6a0f 100644
> --- a/pbs-datastore/src/snapshot_reader.rs
> +++ b/pbs-datastore/src/snapshot_reader.rs
> @@ -12,6 +12,7 @@ use crate::fixed_index::FixedIndexReader;
>   use crate::dynamic_index::DynamicIndexReader;
>   use crate::manifest::{archive_type, ArchiveType, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME};
>   use crate::DataStore;
> +use pbs_api_types::MaintenanceType;
>   use pbs_tools::fs::lock_dir_noblock_shared;
>   
>   /// Helper to access the contents of a datastore backup snapshot
> @@ -119,7 +120,10 @@ impl <'a> Iterator for SnapshotChunkIterator<'a> {
>                           };
>   
>                           let datastore =
> -                            DataStore::lookup_datastore(self.snapshot_reader.datastore_name())?;
> +                            DataStore::lookup_datastore(
> +                                self.snapshot_reader.datastore_name(),
> +                                Some(MaintenanceType::ReadOnly)
> +                            )?;
>                           let order = datastore.get_chunks_in_order(&index, |_| false, |_| Ok(()))?;
>   
>                           self.current_index = Some((Arc::new(index), 0, order));
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 7e9a0ee0..f97e3642 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -28,7 +28,7 @@ use pxar::EntryKind;
>   
>   use pbs_api_types::{ Authid, BackupContent, Counts, CryptMode,
>       DataStoreListItem, GarbageCollectionStatus, GroupListItem,
> -    SnapshotListItem, SnapshotVerifyState, PruneOptions,
> +    MaintenanceType, SnapshotListItem, SnapshotVerifyState, PruneOptions,
>       DataStoreStatus, RRDMode, RRDTimeFrameResolution,
>       BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_TIME_SCHEMA,
>       BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA,
> @@ -171,7 +171,7 @@ pub fn list_groups(
>       let user_info = CachedUserInfo::new()?;
>       let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>       let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
>   
>       let backup_groups = BackupInfo::list_backup_groups(&datastore.base_path())?;
> @@ -269,7 +269,7 @@ pub fn delete_group(
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>       let group = BackupGroup::new(backup_type, backup_id);
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -314,7 +314,7 @@ pub fn list_snapshot_files(
>   ) -> Result<Vec<BackupContent>, Error> {
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
>   
> @@ -364,7 +364,7 @@ pub fn delete_snapshot(
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>       let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -413,7 +413,7 @@ pub fn list_snapshots (
>   
>       let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let base_path = datastore.base_path();
>   
> @@ -607,7 +607,7 @@ pub fn status(
>       _info: &ApiMethod,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<DataStoreStatus, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>       let storage = crate::tools::disks::disk_usage(&datastore.base_path())?;
>       let (counts, gc_status) = if verbose {
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> @@ -685,7 +685,7 @@ pub fn verify(
>       outdated_after: Option<i64>,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Value, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;

does the verify not write to the datastore (changes the verify state) ?
or is changing the manifest not considered 'writing to the datastore' ?
either way, we should maybe decide on this and comment it here?

>       let ignore_verified = ignore_verified.unwrap_or(true);
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> @@ -830,7 +830,7 @@ pub fn prune(
>   
>       let group = BackupGroup::new(&backup_type, &backup_id);
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -953,7 +953,7 @@ pub fn prune_datastore(
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>   
> @@ -997,7 +997,7 @@ pub fn start_garbage_collection(
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Value, Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>       let job =  Job::new("garbage_collection", &store)
> @@ -1033,7 +1033,7 @@ pub fn garbage_collection_status(
>       _rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<GarbageCollectionStatus, Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::Offline))?;
>   
>       let status = datastore.last_gc_status();
>   
> @@ -1109,7 +1109,7 @@ pub fn download_file(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(store)?;
> +        let datastore = DataStore::lookup_datastore(store, Some(MaintenanceType::ReadOnly))?;
>   
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1179,7 +1179,7 @@ pub fn download_file_decoded(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(store)?;
> +        let datastore = DataStore::lookup_datastore(store, Some(MaintenanceType::ReadOnly))?;
>   
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1293,7 +1293,7 @@ pub fn upload_backup_log(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(store)?;
> +        let datastore = DataStore::lookup_datastore(store, None)?;
>   
>           let file_name =  CLIENT_LOG_BLOB_NAME;
>   
> @@ -1370,7 +1370,7 @@ pub fn catalog(
>       filepath: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Vec<ArchiveEntry>, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1440,7 +1440,7 @@ pub fn pxar_file_download(
>   
>       async move {
>           let store = required_string_param(&param, "store")?;
> -        let datastore = DataStore::lookup_datastore(&store)?;
> +        let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
> @@ -1595,7 +1595,7 @@ pub fn get_group_notes(
>       backup_id: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<String, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_group = BackupGroup::new(backup_type, backup_id);
> @@ -1637,7 +1637,7 @@ pub fn set_group_notes(
>       notes: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<(), Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_group = BackupGroup::new(backup_type, backup_id);
> @@ -1679,7 +1679,7 @@ pub fn get_notes(
>       backup_time: i64,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<String, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> @@ -1730,7 +1730,7 @@ pub fn set_notes(
>       notes: String,
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<(), Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;

imho if the verify state change does not count as a write, this 
shouldn't either (both modify the manifest)

>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> @@ -1775,7 +1775,7 @@ pub fn set_backup_owner(
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<(), Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let backup_group = BackupGroup::new(backup_type, backup_id);
>   
> diff --git a/src/api2/backup/mod.rs b/src/api2/backup/mod.rs
> index 4432c8d5..8e45ec1f 100644
> --- a/src/api2/backup/mod.rs
> +++ b/src/api2/backup/mod.rs
> @@ -74,7 +74,7 @@ async move {
>       let user_info = CachedUserInfo::new()?;
>       user_info.check_privs(&auth_id, &["datastore", &store], PRIV_DATASTORE_BACKUP, false)?;
>   
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let backup_type = required_string_param(&param, "backup-type")?;
>       let backup_id = required_string_param(&param, "backup-id")?;
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index ea8faab8..26b8a3be 100644
> --- a/src/api2/pull.rs
> +++ b/src/api2/pull.rs
> @@ -9,7 +9,7 @@ use proxmox::api::{ApiMethod, Router, RpcEnvironment, Permission};
>   
>   use pbs_client::{HttpClient, BackupRepository};
>   use pbs_api_types::{
> -    Remote, Authid, SyncJobConfig,
> +    Remote, Authid, MaintenanceType, SyncJobConfig,
>       DATASTORE_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
>       PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ,
>   };
> @@ -46,7 +46,7 @@ pub async fn get_pull_parameters(
>       remote_store: &str,
>   ) -> Result<(HttpClient, BackupRepository, Arc<DataStore>), Error> {
>   
> -    let tgt_store = DataStore::lookup_datastore(store)?;
> +    let tgt_store = DataStore::lookup_datastore(store, Some(MaintenanceType::Offline))?;
>   
>       let (remote_config, _digest) = pbs_config::remote::config()?;
>       let remote: Remote = remote_config.lookup("remote", remote)?;
> diff --git a/src/api2/reader/mod.rs b/src/api2/reader/mod.rs
> index 1fd25a5c..f209e50a 100644
> --- a/src/api2/reader/mod.rs
> +++ b/src/api2/reader/mod.rs
> @@ -28,8 +28,8 @@ use proxmox::{
>   };
>   
>   use pbs_api_types::{
> -    Authid, DATASTORE_SCHEMA, BACKUP_TYPE_SCHEMA, BACKUP_TIME_SCHEMA, BACKUP_ID_SCHEMA,
> -    CHUNK_DIGEST_SCHEMA, PRIV_DATASTORE_READ, PRIV_DATASTORE_BACKUP,
> +    Authid, MaintenanceType, DATASTORE_SCHEMA, BACKUP_TYPE_SCHEMA, BACKUP_TIME_SCHEMA,
> +    BACKUP_ID_SCHEMA, CHUNK_DIGEST_SCHEMA, PRIV_DATASTORE_READ, PRIV_DATASTORE_BACKUP,
>       BACKUP_ARCHIVE_NAME_SCHEMA,
>   };
>   use pbs_tools::fs::lock_dir_noblock_shared;
> @@ -93,7 +93,7 @@ fn upgrade_to_backup_reader_protocol(
>               bail!("no permissions on /datastore/{}", store);
>           }
>   
> -        let datastore = DataStore::lookup_datastore(&store)?;
> +        let datastore = DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?;
>   
>           let backup_type = required_string_param(&param, "backup-type")?;
>           let backup_id = required_string_param(&param, "backup-id")?;
> diff --git a/src/api2/status.rs b/src/api2/status.rs
> index 548e5319..3f168f25 100644
> --- a/src/api2/status.rs
> +++ b/src/api2/status.rs
> @@ -15,8 +15,8 @@ use proxmox::api::{
>   };
>   
>   use pbs_api_types::{
> -    Authid, DATASTORE_SCHEMA, RRDMode, RRDTimeFrameResolution,
> -    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
> +    Authid, MaintenanceType, DATASTORE_SCHEMA, RRDMode,
> +    RRDTimeFrameResolution, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
>   };
>   
>   use pbs_datastore::DataStore;
> @@ -98,7 +98,7 @@ pub fn datastore_status(
>               continue;
>           }
>   
> -        let datastore = match DataStore::lookup_datastore(&store) {
> +        let datastore = match DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly)) {
>               Ok(datastore) => datastore,
>               Err(err) => {
>                   list.push(json!({
> diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
> index 7627e35b..05ed9c53 100644
> --- a/src/api2/tape/backup.rs
> +++ b/src/api2/tape/backup.rs
> @@ -174,7 +174,7 @@ pub fn do_tape_backup_job(
>   
>       let worker_type = job.jobtype().to_string();
>   
> -    let datastore = DataStore::lookup_datastore(&setup.store)?;
> +    let datastore = DataStore::lookup_datastore(&setup.store, None)?;

this and tape restore are reversed:

a tape backup job reads from the datastore and a tape restore job writes
to the datastore

>   
>       let (config, _digest) = pbs_config::media_pool::config()?;
>       let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
> @@ -355,7 +355,7 @@ pub fn backup(
>           &setup.drive,
>       )?;
>   
> -    let datastore = DataStore::lookup_datastore(&setup.store)?;
> +    let datastore = DataStore::lookup_datastore(&setup.store, None)?;

same here

>   
>       let (config, _digest) = pbs_config::media_pool::config()?;
>       let pool_config: MediaPoolConfig = config.lookup("pool", &setup.pool)?;
> diff --git a/src/api2/tape/restore.rs b/src/api2/tape/restore.rs
> index 61e17d1b..a23c69c7 100644
> --- a/src/api2/tape/restore.rs
> +++ b/src/api2/tape/restore.rs
> @@ -29,7 +29,7 @@ use proxmox::{
>   };
>   
>   use pbs_api_types::{
> -    Authid, Userid, CryptMode,
> +    Authid, MaintenanceType, Userid, CryptMode,
>       DATASTORE_MAP_ARRAY_SCHEMA, DATASTORE_MAP_LIST_SCHEMA, DRIVE_NAME_SCHEMA,
>       UPID_SCHEMA, TAPE_RESTORE_SNAPSHOT_SCHEMA,
>       PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_TAPE_READ,
> @@ -106,10 +106,10 @@ impl TryFrom<String> for DataStoreMap {
>               if let Some(index) = store.find('=') {
>                   let mut target = store.split_off(index);
>                   target.remove(0); // remove '='
> -                let datastore = DataStore::lookup_datastore(&target)?;
> +                let datastore = DataStore::lookup_datastore(&target, Some(MaintenanceType::ReadOnly))?;

same here

>                   map.insert(store, datastore);
>               } else if default.is_none() {
> -                default = Some(DataStore::lookup_datastore(&store)?);
> +                default = Some(DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly))?);

same here

>               } else {
>                   bail!("multiple default stores given");
>               }
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 9199ebae..f80b9d7c 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -47,8 +47,8 @@ use proxmox_systemd::time::{compute_next_event, parse_calendar_event};
>   use pbs_tools::logrotate::LogRotate;
>   
>   use pbs_api_types::{
> -    Authid, TapeBackupJobConfig, VerificationJobConfig, SyncJobConfig, DataStoreConfig,
> -    PruneOptions,
> +    Authid, MaintenanceType, TapeBackupJobConfig, VerificationJobConfig,
> +    SyncJobConfig, DataStoreConfig, PruneOptions,
>   };
>   
>   use proxmox_rest_server::daemon;
> @@ -527,7 +527,7 @@ async fn schedule_datastore_garbage_collection() {
>       };
>   
>       for (store, (_, store_config)) in config.sections {
> -        let datastore = match DataStore::lookup_datastore(&store) {
> +        let datastore = match DataStore::lookup_datastore(&store, Some(MaintenanceType::ReadOnly)) {

in this special case, i'd even allow 'offline' for checking the schedule
iff the schedule triggers, we'd have to lookup the datastore again with 
the correct maintenance type (None, since it writes to the datstore, no?)

now it would log 'lookup_datastore failed - {err}' every minute while
the datastore is in offline maintenance and even do gc when it is in
read-only mode

if there is a reason why gc jobs should work in read-only mode,
it'd warrant a comment i think

>               Ok(datastore) => datastore,
>               Err(err) => {
>                   eprintln!("lookup_datastore failed - {}", err);
> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
> index fc6443e9..00a055f4 100644
> --- a/src/server/prune_job.rs
> +++ b/src/server/prune_job.rs
> @@ -96,7 +96,7 @@ pub fn do_prune_job(
>       auth_id: &Authid,
>       schedule: Option<String>,
>   ) -> Result<String, Error> {
> -    let datastore = DataStore::lookup_datastore(&store)?;
> +    let datastore = DataStore::lookup_datastore(&store, None)?;
>   
>       let worker_type = job.jobtype().to_string();
>       let auth_id = auth_id.clone();
> diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
> index 6aba97c9..05c43602 100644
> --- a/src/server/verify_job.rs
> +++ b/src/server/verify_job.rs
> @@ -1,7 +1,7 @@
>   use anyhow::{format_err, Error};
>   
>   use pbs_tools::task_log;
> -use pbs_api_types::{Authid, VerificationJobConfig};
> +use pbs_api_types::{Authid, MaintenanceType, VerificationJobConfig};
>   use proxmox_rest_server::WorkerTask;
>   use pbs_datastore::DataStore;
>   
> @@ -22,7 +22,7 @@ pub fn do_verification_job(
>       to_stdout: bool,
>   ) -> Result<String, Error> {
>   
> -    let datastore = DataStore::lookup_datastore(&verification_job.store)?;
> +    let datastore = DataStore::lookup_datastore(&verification_job.store, Some(MaintenanceType::ReadOnly))?;

same reasoning as above, either writing to the manifest is a 'write' or 
the above code in 'set_notes' is wrong

>   
>       let outdated_after = verification_job.outdated_after;
>       let ignore_verified_snapshots = verification_job.ignore_verified.unwrap_or(true);
> 

[pbs-devel] [PATCH v2 proxmox-backup 3/4] api2: make maintenance type and msg updatable/deletable

[Hannes Laimer]

---
 src/api2/config/datastore.rs | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 0819d5ec..1f1bb9c1 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -183,6 +183,10 @@ pub enum DeletableProperty {
     notify_user,
     /// Delete the notify property
     notify,
+    /// Delete the maintenance_type property
+    maintenance_type,
+    /// Delete the maintenance_msg property
+    maintenance_msg,
 }
 
 #[api(
@@ -249,6 +253,8 @@ pub fn update_datastore(
                 DeletableProperty::verify_new => { data.verify_new = None; },
                 DeletableProperty::notify => { data.notify = None; },
                 DeletableProperty::notify_user => { data.notify_user = None; },
+                DeletableProperty::maintenance_type => { data.maintenance_type = None; },
+                DeletableProperty::maintenance_msg => { data.maintenance_msg = None; },
             }
         }
     }
@@ -291,6 +297,8 @@ pub fn update_datastore(
         }
     }
     if update.verify_new.is_some() { data.verify_new = update.verify_new; }
+    if update.maintenance_type.is_some() { data.maintenance_type = update.maintenance_type; }
+    if update.maintenance_msg.is_some() { data.maintenance_msg = update.maintenance_msg; }
 
     if update.notify_user.is_some() { data.notify_user = update.notify_user; }
 
-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 3/4] api2: make maintenance type and msg updatable/deletable

[Dominik Csapak]

as i said off-list and you wrote in the cover-letter, it'd probably
make sense to avoid changing the maintenance mode when a datastore
is currently held in a mode that conflicts with the given setting...

this would require a refcounted variable for each mode
(None, Read-only, Offline (maybe not that)) and a check here
if it is set

everytime a 'lookup' would happen, increase the count, everytime
the reference is dropped, decrease the count

(though we can always implement that later if we deem it not necessary)

the big drawback of the approach currently, is that the admin might not
immediately see that a task/api-call is running while setting the
mode and thinking he can take the datastore offline for example
(e.g. a large file-download)

comment inline

On 10/6/21 17:14, Hannes Laimer wrote:
> ---
>   src/api2/config/datastore.rs | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
> index 0819d5ec..1f1bb9c1 100644
> --- a/src/api2/config/datastore.rs
> +++ b/src/api2/config/datastore.rs
> @@ -183,6 +183,10 @@ pub enum DeletableProperty {
>       notify_user,
>       /// Delete the notify property
>       notify,
> +    /// Delete the maintenance_type property
> +    maintenance_type,
> +    /// Delete the maintenance_msg property
> +    maintenance_msg,
>   }
>   
>   #[api(
> @@ -249,6 +253,8 @@ pub fn update_datastore(
>                   DeletableProperty::verify_new => { data.verify_new = None; },
>                   DeletableProperty::notify => { data.notify = None; },
>                   DeletableProperty::notify_user => { data.notify_user = None; },
> +                DeletableProperty::maintenance_type => { data.maintenance_type = None; },
> +                DeletableProperty::maintenance_msg => { data.maintenance_msg = None; },

i think it does not make sense to save the msg when the type is None.
(this would be no issue if both would be in the same variable, see my 
comment in 1/4)

>               }
>           }
>       }
> @@ -291,6 +297,8 @@ pub fn update_datastore(
>           }
>       }
>       if update.verify_new.is_some() { data.verify_new = update.verify_new; }
> +    if update.maintenance_type.is_some() { data.maintenance_type = update.maintenance_type; }
> +    if update.maintenance_msg.is_some() { data.maintenance_msg = update.maintenance_msg; }
>   
>       if update.notify_user.is_some() { data.notify_user = update.notify_user; }
>   
> 

[pbs-devel] [PATCH v2 proxmox-backup 4/4] ui: add maintenance to datastore options

[Hannes Laimer]

---
 www/Makefile                     |  1 +
 www/Utils.js                     |  7 +++++
 www/datastore/OptionView.js      |  9 ++++++
 www/window/MaintenanceOptions.js | 51 ++++++++++++++++++++++++++++++++
 4 files changed, 68 insertions(+)
 create mode 100644 www/window/MaintenanceOptions.js

diff --git a/www/Makefile b/www/Makefile
index 4aec6e2c..8f6b17ed 100644
--- a/www/Makefile
+++ b/www/Makefile
@@ -63,6 +63,7 @@ JSSRC=							\
 	window/BackupGroupChangeOwner.js		\
 	window/CreateDirectory.js			\
 	window/DataStoreEdit.js				\
+	window/MaintenanceOptions.js			\
 	window/NotesEdit.js				\
 	window/RemoteEdit.js				\
 	window/NotifyOptions.js				\
diff --git a/www/Utils.js b/www/Utils.js
index 36a94211..a1198c57 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -640,4 +640,11 @@ Ext.define('PBS.Utils', {
 	return `${icon} ${value}`;
     },
 
+    renderMaintenance: function(type) {
+	if (type === 'readonly') {
+	    type = 'read only';
+	}
+	return Ext.String.capitalize(gettext(type)) || gettext('None');
+    },
+
 });
diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
index 5a5e85be..a4e72e76 100644
--- a/www/datastore/OptionView.js
+++ b/www/datastore/OptionView.js
@@ -1,3 +1,4 @@
+
 Ext.define('PBS.Datastore.Options', {
     extend: 'Proxmox.grid.ObjectGrid',
     xtype: 'pbsDatastoreOptionView',
@@ -111,5 +112,13 @@ Ext.define('PBS.Datastore.Options', {
 		},
 	    },
 	},
+	"maintenance-type": {
+	    required: true,
+	    header: gettext('Maintenance mode'),
+	    renderer: Proxmox.Utils.renderMaintenance,
+	    editor: {
+		xtype: 'pbsMaintenanceOptionEdit',
+	    },
+	},
     },
 });
diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
new file mode 100644
index 00000000..85fd3fc9
--- /dev/null
+++ b/www/window/MaintenanceOptions.js
@@ -0,0 +1,51 @@
+Ext.define('PBS.form.MaintenanceType', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: 'widget.pbsMaintenanceType',
+
+    comboItems: [
+	['__default__', gettext('None')],
+	['readonly', gettext('Read only')],
+	['offline', gettext('Offline')],
+    ],
+});
+
+Ext.define('PBS.window.MaintenanceOptions', {
+    extend: 'Proxmox.window.Edit',
+    xtype: 'pbsMaintenanceOptionEdit',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    subject: gettext('Maintenance mode'),
+
+    width: 450,
+    fieldDefaults: {
+	labelWidth: 120,
+    },
+
+    items: {
+	xtype: 'inputpanel',
+	items: [
+	    {
+		xtype: 'pbsMaintenanceType',
+		name: 'maintenance-type',
+		fieldLabel: gettext('Maintenance Type'),
+		value: '__default__',
+		deleteEmpty: true,
+	    },
+	    {
+		xtype: 'proxmoxtextfield',
+		name: 'maintenance-msg',
+		fieldLabel: gettext('Description'),
+		deleteEmpty: true,
+	    },
+	],
+    },
+    setValues: function(values) {
+	let me = this;
+
+	const options = {
+	    'maintenance-type': values['maintenance-type'] || '__default__',
+	    'maintenance-msg': values['maintenance-msg'],
+	};
+	me.callParent([options]);
+    },
+});
-- 
2.30.2

Re: [pbs-devel] [PATCH v2 proxmox-backup 4/4] ui: add maintenance to datastore options

[Dominik Csapak]

comment inline:

On 10/6/21 17:14, Hannes Laimer wrote:
> ---
>   www/Makefile                     |  1 +
>   www/Utils.js                     |  7 +++++
>   www/datastore/OptionView.js      |  9 ++++++
>   www/window/MaintenanceOptions.js | 51 ++++++++++++++++++++++++++++++++
>   4 files changed, 68 insertions(+)
>   create mode 100644 www/window/MaintenanceOptions.js
> 
> diff --git a/www/Makefile b/www/Makefile
> index 4aec6e2c..8f6b17ed 100644
> --- a/www/Makefile
> +++ b/www/Makefile
> @@ -63,6 +63,7 @@ JSSRC=							\
>   	window/BackupGroupChangeOwner.js		\
>   	window/CreateDirectory.js			\
>   	window/DataStoreEdit.js				\
> +	window/MaintenanceOptions.js			\
>   	window/NotesEdit.js				\
>   	window/RemoteEdit.js				\
>   	window/NotifyOptions.js				\
> diff --git a/www/Utils.js b/www/Utils.js
> index 36a94211..a1198c57 100644
> --- a/www/Utils.js
> +++ b/www/Utils.js
> @@ -640,4 +640,11 @@ Ext.define('PBS.Utils', {
>   	return `${icon} ${value}`;
>       },
>   
> +    renderMaintenance: function(type) {
> +	if (type === 'readonly') {
> +	    type = 'read only';
> +	}
> +	return Ext.String.capitalize(gettext(type)) || gettext('None');
> +    },
> +
>   });
> diff --git a/www/datastore/OptionView.js b/www/datastore/OptionView.js
> index 5a5e85be..a4e72e76 100644
> --- a/www/datastore/OptionView.js
> +++ b/www/datastore/OptionView.js
> @@ -1,3 +1,4 @@
> +
>   Ext.define('PBS.Datastore.Options', {
>       extend: 'Proxmox.grid.ObjectGrid',
>       xtype: 'pbsDatastoreOptionView',
> @@ -111,5 +112,13 @@ Ext.define('PBS.Datastore.Options', {
>   		},
>   	    },
>   	},
> +	"maintenance-type": {
> +	    required: true,
> +	    header: gettext('Maintenance mode'),
> +	    renderer: Proxmox.Utils.renderMaintenance,
> +	    editor: {
> +		xtype: 'pbsMaintenanceOptionEdit',
> +	    },
> +	},
>       },
>   });
> diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
> new file mode 100644
> index 00000000..85fd3fc9
> --- /dev/null
> +++ b/www/window/MaintenanceOptions.js
> @@ -0,0 +1,51 @@
> +Ext.define('PBS.form.MaintenanceType', {
> +    extend: 'Proxmox.form.KVComboBox',
> +    alias: 'widget.pbsMaintenanceType',
> +
> +    comboItems: [
> +	['__default__', gettext('None')],
> +	['readonly', gettext('Read only')],
> +	['offline', gettext('Offline')],
> +    ],
> +});
> +
> +Ext.define('PBS.window.MaintenanceOptions', {
> +    extend: 'Proxmox.window.Edit',
> +    xtype: 'pbsMaintenanceOptionEdit',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    subject: gettext('Maintenance mode'),
> +
> +    width: 450,
> +    fieldDefaults: {
> +	labelWidth: 120,
> +    },
> +
> +    items: {
> +	xtype: 'inputpanel',
> +	items: [
> +	    {
> +		xtype: 'pbsMaintenanceType',
> +		name: 'maintenance-type',
> +		fieldLabel: gettext('Maintenance Type'),
> +		value: '__default__',
> +		deleteEmpty: true,
> +	    },
> +	    {
> +		xtype: 'proxmoxtextfield',
> +		name: 'maintenance-msg',
> +		fieldLabel: gettext('Description'),
> +		deleteEmpty: true,
> +	    },

i am still convinced that we should disable that field if the 
maintenance type is 'None'. It does not make sense to let the user
set a maintenance message when he disables maintenance mode...
(it will never be seen aside from this config?)

if there is a strong reason why this is important, it warrants
a comment in the git patch change log at least

> +	],
> +    },
> +    setValues: function(values) {
> +	let me = this;
> +
> +	const options = {
> +	    'maintenance-type': values['maintenance-type'] || '__default__',
> +	    'maintenance-msg': values['maintenance-msg'],
> +	};
> +	me.callParent([options]);
> +    },
> +});
>