[pbs-devel] [PATCH proxmox-backup 0/5] maintenance mode for datastore

[pbs-devel] [PATCH proxmox-backup 0/5] maintenance mode for datastore

[Hannes Laimer]

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.

Here 'read only' is less restrictive than 'offline'. In order to check
if some operation is allowed in the current maintenance mode a function
has to be called before every operation that would be effected by a
maintenance. This function recieves an upper bound for
'restrictiveness', so if we want to get the list of available snapshots
we have to read from the ds, everything that is less
restrictive than 'offline' is fine. Here that means maintenance has to
either be off or 'read only'.

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.

(It might make sense to add a possibility to shut down running jobs when
maintenance is turned on.)

Hannes Laimer (5):
  pbs-api-types: add maintenance type and msg to ds config
  pbs-datastore: add check_maintenence function
  api2: make maintenance type and msg updatable/deletable
  jobs/api2: add checks for maintenance
  ui: add maintenance to datastore options

 pbs-api-types/src/datastore.rs   | 33 +++++++++++++++++
 pbs-datastore/src/datastore.rs   | 18 ++++++++-
 src/api2/admin/datastore.rs      | 48 +++++++++++++++++++++---
 src/api2/config/datastore.rs     |  8 ++++
 src/api2/pull.rs                 |  5 ++-
 src/server/gc_job.rs             |  5 ++-
 src/server/prune_job.rs          |  4 +-
 src/server/verify_job.rs         |  5 ++-
 www/Makefile                     |  1 +
 www/datastore/OptionView.js      | 10 +++++
 www/window/MaintenanceOptions.js | 63 ++++++++++++++++++++++++++++++++
 11 files changed, 188 insertions(+), 12 deletions(-)
 create mode 100644 www/window/MaintenanceOptions.js

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 1/5] 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..80ae77f2 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 proxmox-backup 1/5] pbs-api-types: add maintenance type and msg to ds config

[Dominik Csapak]

On 9/28/21 12:05, 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..80ae77f2 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"),

since that goes into the config and api (AFAICS), i'd not use spaces in 
the value. we could use 'read-only' for example, but maybe check other
parts of the api/configs where we may have values similar to this

> +           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,
> +        },

i think we could combine the message + type in the enum, we just have
to manually to the deserialization (look for example how we
do the media-location for tapes, there we can have a named 'vault'
which gets (de)serialized as/from "vault-NAME"

we could to the same here with "read-only-foo' and 'offline-foo'

this would make for a nicer api maybe? (any other opinions @thomas 
@wolfgang @dietmar?)

also i think the way it is now, i could update the maintenance message 
without setting a mode...

>       }
>   )]
>   #[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(
> 

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

[Wolfgang Bumiller]

On Fri, Oct 01, 2021 at 10:23:08AM +0200, Dominik Csapak wrote:
> On 9/28/21 12:05, 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..80ae77f2 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"),
> 
> since that goes into the config and api (AFAICS), i'd not use spaces in the
> value. we could use 'read-only' for example, but maybe check other
> parts of the api/configs where we may have values similar to this

I'm actually kind of leaning towards using
`serde_plain::forward_display_to_serde!()` for such cases. We have some
of serde_plain's functionality int he proxmox crate and since the plan
is to split things up further for the sake of compile time,
`serde_plain` seems like a perfectly fine additional dependency (it's
tiny and only uses serde)

> 
> > +           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,
> > +        },
> 
> i think we could combine the message + type in the enum, we just have
> to manually to the deserialization (look for example how we
> do the media-location for tapes, there we can have a named 'vault'
> which gets (de)serialized as/from "vault-NAME"
> 
> we could to the same here with "read-only-foo' and 'offline-foo'
> 
> this would make for a nicer api maybe? (any other opinions @thomas @wolfgang
> @dietmar?)
> 
> also i think the way it is now, i could update the maintenance message
> without setting a mode...

I suppose it's a tuple type, which, unfortunately, the schema cannot
represent.
On a side note, OpenAPI 3.1 & JSON Schema 2020-12 seem to have settled
on a way to represent this though, in case we want to try and add
support for that...

Or you know, a string with custom deserialization works fine ;-)
If there's supposed to be a text though and the enum uses dashes, maybe
use a colon to separate the text?
`"offline: Holding your data hostage until you pay up."`
`"read-only: Raid resilvering, stop slowing it down to a crawl."`

[pbs-devel] [PATCH proxmox-backup 2/5] pbs-datastore: add check_maintenence function

[Hannes Laimer]

This function checks if something is allowed in a specific maintenance
type, it has to be called whenever something depends on maintenance to
be off or less rescrictive than a specific type.
---
 pbs-datastore/src/datastore.rs | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index b068a9c9..1014b357 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;
@@ -98,6 +98,22 @@ impl DataStore {
         Ok(())
     }
 
+    /// checks if the current maintenace mode of the datastore allows something
+    pub fn check_maintenance(&self, need_less_than: MaintenanceType) -> Result<(), Error> {
+        let (config, _digest) = pbs_config::datastore::config()?;
+        let config: DataStoreConfig = config.lookup("datastore", &self.name())?;
+        if let Some(maintenance_type) = config.maintenance_type {
+            if maintenance_type >= need_less_than {
+                bail!("Datastore '{}' is in maintenance {} mode: {}", 
+                    &self.name(),
+                    maintenance_type,
+                    config.maintenance_msg.unwrap_or(String::from("no description"))
+                ); 
+            }
+        }
+        Ok(())
+    }
+
     fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
         let chunk_store = ChunkStore::open(store_name, path)?;
 
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 2/5] pbs-datastore: add check_maintenence function

[Wolfgang Bumiller]

On Tue, Sep 28, 2021 at 12:05:45PM +0200, Hannes Laimer wrote:
> This function checks if something is allowed in a specific maintenance
> type, it has to be called whenever something depends on maintenance to
> be off or less rescrictive than a specific type.
> ---
>  pbs-datastore/src/datastore.rs | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index b068a9c9..1014b357 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;
> @@ -98,6 +98,22 @@ impl DataStore {
>          Ok(())
>      }
>  
> +    /// checks if the current maintenace mode of the datastore allows something
> +    pub fn check_maintenance(&self, need_less_than: MaintenanceType) -> Result<(), Error> {

I'd very much like the method name itself to suggest the direction of
such a check, rather than have only the parameter name tell you this,
not even the doc comment ;-)
When you read `datastore.check_maintenance(foo)` the parameter name
isn't actually visible in code, but the code should speak for itself.

And like Dominik said, it's a confusing interface. Plus, I can't think
if a really good name for the method that fixes my remark above.

So maybe the public facing interface should just be:

    pub fn can_read(&self) -> bool;
    pub fn can_write(&self) -> bool;

and/or

    pub fn check_read(&self) -> Result<(), Error>;
    pub fn check_write(&self) -> Result<(), Error>;

with the error including the maintenance message

> +        let (config, _digest) = pbs_config::datastore::config()?;
> +        let config: DataStoreConfig = config.lookup("datastore", &self.name())?;
> +        if let Some(maintenance_type) = config.maintenance_type {
> +            if maintenance_type >= need_less_than {
> +                bail!("Datastore '{}' is in maintenance {} mode: {}", 
> +                    &self.name(),
> +                    maintenance_type,
> +                    config.maintenance_msg.unwrap_or(String::from("no description"))
> +                ); 
> +            }
> +        }
> +        Ok(())
> +    }
> +
>      fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
>          let chunk_store = ChunkStore::open(store_name, path)?;
>  
> -- 
> 2.30.2

[pbs-devel] [PATCH proxmox-backup 3/5] 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

[pbs-devel] [PATCH proxmox-backup 4/5] jobs/api2: add checks for maintenance

[Hannes Laimer]

---
 src/api2/admin/datastore.rs | 48 +++++++++++++++++++++++++++++++++----
 src/api2/pull.rs            |  5 +++-
 src/server/gc_job.rs        |  5 ++--
 src/server/prune_job.rs     |  4 +++-
 src/server/verify_job.rs    |  5 ++--
 5 files changed, 56 insertions(+), 11 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 7e9a0ee0..f5ed6603 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -29,7 +29,7 @@ use pxar::EntryKind;
 use pbs_api_types::{ Authid, BackupContent, Counts, CryptMode,
     DataStoreListItem, GarbageCollectionStatus, GroupListItem,
     SnapshotListItem, SnapshotVerifyState, PruneOptions,
-    DataStoreStatus, RRDMode, RRDTimeFrameResolution,
+    DataStoreStatus, RRDMode, RRDTimeFrameResolution, MaintenanceType,
     BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_TIME_SCHEMA,
     BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA,
     IGNORE_VERIFIED_BACKUPS_SCHEMA, UPID_SCHEMA,
@@ -83,6 +83,7 @@ fn check_priv_or_backup_owner(
     auth_id: &Authid,
     required_privs: u64,
 ) -> Result<(), Error> {
+    store.check_maintenance(MaintenanceType::Offline)?;
     let user_info = CachedUserInfo::new()?;
     let privs = user_info.lookup_privs(&auth_id, &["datastore", store.name()]);
 
@@ -97,7 +98,7 @@ fn read_backup_index(
     store: &DataStore,
     backup_dir: &BackupDir,
 ) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
-
+    store.check_maintenance(MaintenanceType::Offline)?;
     let (manifest, index_size) = store.load_manifest(backup_dir)?;
 
     let mut result = Vec::new();
@@ -125,7 +126,7 @@ fn get_all_snapshot_files(
     store: &DataStore,
     info: &BackupInfo,
 ) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
-
+    store.check_maintenance(MaintenanceType::Offline)?;
     let (manifest, mut files) = read_backup_index(&store, &info.backup_dir)?;
 
     let file_set = files.iter().fold(HashSet::new(), |mut acc, item| {
@@ -172,6 +173,9 @@ pub fn list_groups(
     let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
 
     let datastore = DataStore::lookup_datastore(&store)?;
+
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+
     let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
 
     let backup_groups = BackupInfo::list_backup_groups(&datastore.base_path())?;
@@ -267,9 +271,11 @@ pub fn delete_group(
 ) -> Result<Value, Error> {
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let datastore = DataStore::lookup_datastore(&store)?;
+
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
 
     let group = BackupGroup::new(backup_type, backup_id);
-    let datastore = DataStore::lookup_datastore(&store)?;
 
     check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
 
@@ -316,6 +322,8 @@ pub fn list_snapshot_files(
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+ 
     let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
 
     check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ)?;
@@ -362,9 +370,12 @@ pub fn delete_snapshot(
 ) -> Result<Value, Error> {
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    
+    let datastore = DataStore::lookup_datastore(&store)?;
+    
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
 
     let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
-    let datastore = DataStore::lookup_datastore(&store)?;
 
     check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
 
@@ -415,6 +426,8 @@ pub fn list_snapshots (
 
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+
     let base_path = datastore.base_path();
 
     let groups = match (backup_type, backup_id) {
@@ -686,6 +699,9 @@ pub fn verify(
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
+
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+
     let ignore_verified = ignore_verified.unwrap_or(true);
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
@@ -832,6 +848,8 @@ pub fn prune(
 
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
+
     check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
 
     let worker_id = format!("{}:{}/{}", store, &backup_type, &backup_id);
@@ -1035,6 +1053,8 @@ pub fn garbage_collection_status(
 
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+
     let status = datastore.last_gc_status();
 
     Ok(status)
@@ -1111,6 +1131,8 @@ pub fn download_file(
         let store = required_string_param(&param, "store")?;
         let datastore = DataStore::lookup_datastore(store)?;
 
+        datastore.check_maintenance(MaintenanceType::Offline)?;
+
         let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
         let file_name = required_string_param(&param, "file-name")?.to_owned();
@@ -1181,6 +1203,8 @@ pub fn download_file_decoded(
         let store = required_string_param(&param, "store")?;
         let datastore = DataStore::lookup_datastore(store)?;
 
+        datastore.check_maintenance(MaintenanceType::Offline)?;
+
         let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
         let file_name = required_string_param(&param, "file-name")?.to_owned();
@@ -1372,6 +1396,8 @@ pub fn catalog(
 ) -> Result<Vec<ArchiveEntry>, Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
     let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
@@ -1442,6 +1468,8 @@ pub fn pxar_file_download(
         let store = required_string_param(&param, "store")?;
         let datastore = DataStore::lookup_datastore(&store)?;
 
+        datastore.check_maintenance(MaintenanceType::Offline)?;
+
         let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
 
         let filepath = required_string_param(&param, "filepath")?.to_owned();
@@ -1597,6 +1625,8 @@ pub fn get_group_notes(
 ) -> Result<String, Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_group = BackupGroup::new(backup_type, backup_id);
 
@@ -1639,6 +1669,8 @@ pub fn set_group_notes(
 ) -> Result<(), Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_group = BackupGroup::new(backup_type, backup_id);
 
@@ -1681,6 +1713,8 @@ pub fn get_notes(
 ) -> Result<String, Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::Offline)?;
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
@@ -1732,6 +1766,8 @@ pub fn set_notes(
 ) -> Result<(), Error> {
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
+
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
     let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
 
@@ -1777,6 +1813,8 @@ pub fn set_backup_owner(
 
     let datastore = DataStore::lookup_datastore(&store)?;
 
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
+
     let backup_group = BackupGroup::new(backup_type, backup_id);
 
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index ea8faab8..b1669288 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, SyncJobConfig, MaintenanceType,
     DATASTORE_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
     PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ,
 };
@@ -71,6 +71,9 @@ pub fn do_sync_job(
     to_stdout: bool,
 ) -> Result<String, Error> {
 
+    let datastore = DataStore::lookup_datastore(&sync_job.store)?;
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
+    
     let job_id = format!("{}:{}:{}:{}",
                          sync_job.remote,
                          sync_job.remote_store,
diff --git a/src/server/gc_job.rs b/src/server/gc_job.rs
index 794fe146..be04b635 100644
--- a/src/server/gc_job.rs
+++ b/src/server/gc_job.rs
@@ -1,7 +1,7 @@
 use std::sync::Arc;
 use anyhow::Error;
 
-use pbs_api_types::Authid;
+use pbs_api_types::{Authid, MaintenanceType};
 use pbs_tools::task_log;
 use pbs_datastore::DataStore;
 use proxmox_rest_server::WorkerTask;
@@ -16,7 +16,8 @@ pub fn do_garbage_collection_job(
     schedule: Option<String>,
     to_stdout: bool,
 ) -> Result<String, Error> {
-
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
+    
     let store = datastore.name().to_string();
 
     let (email, notify) = crate::server::lookup_datastore_notify_settings(&store);
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index fc6443e9..84131634 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -5,7 +5,7 @@ use anyhow::Error;
 use pbs_datastore::backup_info::BackupInfo;
 use pbs_datastore::prune::compute_prune_info;
 use pbs_datastore::DataStore;
-use pbs_api_types::{Authid, PRIV_DATASTORE_MODIFY, PruneOptions};
+use pbs_api_types::{Authid, MaintenanceType, PRIV_DATASTORE_MODIFY, PruneOptions};
 use pbs_config::CachedUserInfo;
 use pbs_tools::{task_log, task_warn};
 use proxmox_rest_server::WorkerTask;
@@ -20,6 +20,8 @@ pub fn prune_datastore(
     datastore: Arc<DataStore>,
     dry_run: bool,
 ) -> Result<(), Error> {
+    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
+
     task_log!(worker, "Starting datastore prune on store \"{}\"", store);
 
     if dry_run {
diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
index 6aba97c9..3b2b1472 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, VerificationJobConfig, MaintenanceType};
 use proxmox_rest_server::WorkerTask;
 use pbs_datastore::DataStore;
 
@@ -21,9 +21,10 @@ pub fn do_verification_job(
     schedule: Option<String>,
     to_stdout: bool,
 ) -> Result<String, Error> {
-
     let datastore = DataStore::lookup_datastore(&verification_job.store)?;
 
+    datastore.check_maintenance(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 proxmox-backup 4/5] jobs/api2: add checks for maintenance

[Dominik Csapak]

On 9/28/21 12:05, Hannes Laimer wrote:
> ---
>   src/api2/admin/datastore.rs | 48 +++++++++++++++++++++++++++++++++----
>   src/api2/pull.rs            |  5 +++-
>   src/server/gc_job.rs        |  5 ++--
>   src/server/prune_job.rs     |  4 +++-
>   src/server/verify_job.rs    |  5 ++--
>   5 files changed, 56 insertions(+), 11 deletions(-)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 7e9a0ee0..f5ed6603 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -29,7 +29,7 @@ use pxar::EntryKind;
>   use pbs_api_types::{ Authid, BackupContent, Counts, CryptMode,
>       DataStoreListItem, GarbageCollectionStatus, GroupListItem,
>       SnapshotListItem, SnapshotVerifyState, PruneOptions,
> -    DataStoreStatus, RRDMode, RRDTimeFrameResolution,
> +    DataStoreStatus, RRDMode, RRDTimeFrameResolution, MaintenanceType,
>       BACKUP_ARCHIVE_NAME_SCHEMA, BACKUP_ID_SCHEMA, BACKUP_TIME_SCHEMA,
>       BACKUP_TYPE_SCHEMA, DATASTORE_SCHEMA,
>       IGNORE_VERIFIED_BACKUPS_SCHEMA, UPID_SCHEMA,
> @@ -83,6 +83,7 @@ fn check_priv_or_backup_owner(
>       auth_id: &Authid,
>       required_privs: u64,
>   ) -> Result<(), Error> {
> +    store.check_maintenance(MaintenanceType::Offline)?;
>       let user_info = CachedUserInfo::new()?;
>       let privs = user_info.lookup_privs(&auth_id, &["datastore", store.name()]);
>   
> @@ -97,7 +98,7 @@ fn read_backup_index(
>       store: &DataStore,
>       backup_dir: &BackupDir,
>   ) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
> -
> +    store.check_maintenance(MaintenanceType::Offline)?;
>       let (manifest, index_size) = store.load_manifest(backup_dir)?;
>   
>       let mut result = Vec::new();
> @@ -125,7 +126,7 @@ fn get_all_snapshot_files(
>       store: &DataStore,
>       info: &BackupInfo,
>   ) -> Result<(BackupManifest, Vec<BackupContent>), Error> {
> -
> +    store.check_maintenance(MaintenanceType::Offline)?;
>       let (manifest, mut files) = read_backup_index(&store, &info.backup_dir)?;
>   
>       let file_set = files.iter().fold(HashSet::new(), |mut acc, item| {
> @@ -172,6 +173,9 @@ pub fn list_groups(
>       let user_privs = user_info.lookup_privs(&auth_id, &["datastore", &store]);
>   
>       let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>       let list_all = (user_privs & PRIV_DATASTORE_AUDIT) != 0;
>   
>       let backup_groups = BackupInfo::list_backup_groups(&datastore.base_path())?;
> @@ -267,9 +271,11 @@ pub fn delete_group(
>   ) -> Result<Value, Error> {
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
>   
>       let group = BackupGroup::new(backup_type, backup_id);
> -    let datastore = DataStore::lookup_datastore(&store)?;
>   
>       check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -316,6 +322,8 @@ pub fn list_snapshot_files(
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>       let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
>   
>       check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_READ)?;
> @@ -362,9 +370,12 @@ pub fn delete_snapshot(
>   ) -> Result<Value, Error> {
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +
> +    let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
>   
>       let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
> -    let datastore = DataStore::lookup_datastore(&store)?;
>   
>       check_priv_or_backup_owner(&datastore, snapshot.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
> @@ -415,6 +426,8 @@ pub fn list_snapshots (
>   
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>       let base_path = datastore.base_path();
>   
>       let groups = match (backup_type, backup_id) {
> @@ -686,6 +699,9 @@ pub fn verify(
>       rpcenv: &mut dyn RpcEnvironment,
>   ) -> Result<Value, Error> {
>       let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>       let ignore_verified = ignore_verified.unwrap_or(true);
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> @@ -832,6 +848,8 @@ pub fn prune(
>   
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +
>       check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
>   
>       let worker_id = format!("{}:{}/{}", store, &backup_type, &backup_id);
> @@ -1035,6 +1053,8 @@ pub fn garbage_collection_status(
>   
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +

i was confused here at first, why garbage collect why garbage collect
needs only read access, then noticed it's the status. but the status
is not really a 'read' operation on the datastore? its more like
the config or a task log since it only shows the status of the last gc

>       let status = datastore.last_gc_status();
>   
>       Ok(status)
> @@ -1111,6 +1131,8 @@ pub fn download_file(
>           let store = required_string_param(&param, "store")?;
>           let datastore = DataStore::lookup_datastore(store)?;
>   
> +        datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>           let file_name = required_string_param(&param, "file-name")?.to_owned();
> @@ -1181,6 +1203,8 @@ pub fn download_file_decoded(
>           let store = required_string_param(&param, "store")?;
>           let datastore = DataStore::lookup_datastore(store)?;
>   
> +        datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>           let file_name = required_string_param(&param, "file-name")?.to_owned();
> @@ -1372,6 +1396,8 @@ pub fn catalog(
>   ) -> Result<Vec<ArchiveEntry>, Error> {
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>       let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> @@ -1442,6 +1468,8 @@ pub fn pxar_file_download(
>           let store = required_string_param(&param, "store")?;
>           let datastore = DataStore::lookup_datastore(&store)?;
>   
> +        datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>           let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>   
>           let filepath = required_string_param(&param, "filepath")?.to_owned();
> @@ -1597,6 +1625,8 @@ pub fn get_group_notes(
>   ) -> Result<String, Error> {
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_group = BackupGroup::new(backup_type, backup_id);
>   
> @@ -1639,6 +1669,8 @@ pub fn set_group_notes(
>   ) -> Result<(), Error> {
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_group = BackupGroup::new(backup_type, backup_id);
>   
> @@ -1681,6 +1713,8 @@ pub fn get_notes(
>   ) -> Result<String, Error> {
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::Offline)?;
> +
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
>   
> @@ -1732,6 +1766,8 @@ pub fn set_notes(
>   ) -> Result<(), Error> {
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>       let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
>   
> @@ -1777,6 +1813,8 @@ pub fn set_backup_owner(
>   
>       let datastore = DataStore::lookup_datastore(&store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +
>       let backup_group = BackupGroup::new(backup_type, backup_id);
>   
>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
> index ea8faab8..b1669288 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, SyncJobConfig, MaintenanceType,
>       DATASTORE_SCHEMA, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
>       PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ,
>   };
> @@ -71,6 +71,9 @@ pub fn do_sync_job(
>       to_stdout: bool,
>   ) -> Result<String, Error> {
>   
> +    let datastore = DataStore::lookup_datastore(&sync_job.store)?;
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +
>       let job_id = format!("{}:{}:{}:{}",
>                            sync_job.remote,
>                            sync_job.remote_store,
> diff --git a/src/server/gc_job.rs b/src/server/gc_job.rs
> index 794fe146..be04b635 100644
> --- a/src/server/gc_job.rs
> +++ b/src/server/gc_job.rs
> @@ -1,7 +1,7 @@
>   use std::sync::Arc;
>   use anyhow::Error;
>   
> -use pbs_api_types::Authid;
> +use pbs_api_types::{Authid, MaintenanceType};
>   use pbs_tools::task_log;
>   use pbs_datastore::DataStore;
>   use proxmox_rest_server::WorkerTask;
> @@ -16,7 +16,8 @@ pub fn do_garbage_collection_job(
>       schedule: Option<String>,
>       to_stdout: bool,
>   ) -> Result<String, Error> {
> -
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +
>       let store = datastore.name().to_string();
>   
>       let (email, notify) = crate::server::lookup_datastore_notify_settings(&store);
> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
> index fc6443e9..84131634 100644
> --- a/src/server/prune_job.rs
> +++ b/src/server/prune_job.rs
> @@ -5,7 +5,7 @@ use anyhow::Error;
>   use pbs_datastore::backup_info::BackupInfo;
>   use pbs_datastore::prune::compute_prune_info;
>   use pbs_datastore::DataStore;
> -use pbs_api_types::{Authid, PRIV_DATASTORE_MODIFY, PruneOptions};
> +use pbs_api_types::{Authid, MaintenanceType, PRIV_DATASTORE_MODIFY, PruneOptions};
>   use pbs_config::CachedUserInfo;
>   use pbs_tools::{task_log, task_warn};
>   use proxmox_rest_server::WorkerTask;
> @@ -20,6 +20,8 @@ pub fn prune_datastore(
>       datastore: Arc<DataStore>,
>       dry_run: bool,
>   ) -> Result<(), Error> {
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +
>       task_log!(worker, "Starting datastore prune on store \"{}\"", store);
>   
>       if dry_run {
> diff --git a/src/server/verify_job.rs b/src/server/verify_job.rs
> index 6aba97c9..3b2b1472 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, VerificationJobConfig, MaintenanceType};
>   use proxmox_rest_server::WorkerTask;
>   use pbs_datastore::DataStore;
>   
> @@ -21,9 +21,10 @@ pub fn do_verification_job(
>       schedule: Option<String>,
>       to_stdout: bool,
>   ) -> Result<String, Error> {
> -
>       let datastore = DataStore::lookup_datastore(&verification_job.store)?;
>   
> +    datastore.check_maintenance(MaintenanceType::ReadOnly)?;
> +

why does a verification write access? because of the verify status in 
the manifest?
imho that reason would be good to have in a comment

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

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

[Hannes Laimer]

---
 www/Makefile                     |  1 +
 www/datastore/OptionView.js      | 10 +++++
 www/window/MaintenanceOptions.js | 63 ++++++++++++++++++++++++++++++++
 3 files changed, 74 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/datastore/OptionView.js b/www/datastore/OptionView.js
index 5a5e85be..1e0c3357 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,14 @@ Ext.define('PBS.Datastore.Options', {
 		},
 	    },
 	},
+	"maintenance-type": {
+	    required: true,
+	    header: gettext('Maintenance mode'),
+	    defaultValue: 'Off',
+	    renderer: (value) => Ext.String.capitalize(value) || 'Off',
+	    editor: {
+		xtype: 'pbsMaintenanceOptionEdit',
+	    },
+	},
     },
 });
diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
new file mode 100644
index 00000000..8ff5ccf9
--- /dev/null
+++ b/www/window/MaintenanceOptions.js
@@ -0,0 +1,63 @@
+Ext.define('PBS.form.MaintenanceType', {
+    extend: 'Proxmox.form.KVComboBox',
+    alias: 'widget.pbsMaintenanceType',
+
+    comboItems: [
+	['__default__', gettext('Off')],
+	['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',
+	onGetValues: function(values) {
+	    PBS.Utils.delete_if_default(values, 'maintenance-type', '');
+	    PBS.Utils.delete_if_default(values, 'maintenance-msg', '');
+	    if (values.delete) {
+		values.delete = values.delete.split(',');
+	    }
+
+	    return values;
+	},
+	items: [
+	    {
+		xtype: 'pbsMaintenanceType',
+		name: 'maintenance-type',
+		fieldLabel: gettext('Maintenance Type'),
+		value: '__default__',
+		deleteEmpty: true,
+	    },
+	    {
+		xtype: 'textfield',
+		emptyText: 'none',
+		name: 'maintenance-msg',
+		fieldLabel: gettext('Description'),
+		value: '__default__',
+		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 proxmox-backup 5/5] ui: add maintenance to datastore options

[Dominik Csapak]

On 9/28/21 12:05, Hannes Laimer wrote:
> ---
>   www/Makefile                     |  1 +
>   www/datastore/OptionView.js      | 10 +++++
>   www/window/MaintenanceOptions.js | 63 ++++++++++++++++++++++++++++++++
>   3 files changed, 74 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/datastore/OptionView.js b/www/datastore/OptionView.js
> index 5a5e85be..1e0c3357 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,14 @@ Ext.define('PBS.Datastore.Options', {
>   		},
>   	    },
>   	},
> +	"maintenance-type": {
> +	    required: true,
> +	    header: gettext('Maintenance mode'),
> +	    defaultValue: 'Off',
the default should probably be empty, no? Off should be handled
by the renderer anyway

> +	    renderer: (value) => Ext.String.capitalize(value) || 'Off',

i would use that, but refactor the gettexts into e.g. Utils and
write a renderer that maps it. that way we show the localized
values here too

> +	    editor: {
> +		xtype: 'pbsMaintenanceOptionEdit',
> +	    },
> +	},
>       },
>   });
> diff --git a/www/window/MaintenanceOptions.js b/www/window/MaintenanceOptions.js
> new file mode 100644
> index 00000000..8ff5ccf9
> --- /dev/null
> +++ b/www/window/MaintenanceOptions.js
> @@ -0,0 +1,63 @@
> +Ext.define('PBS.form.MaintenanceType', {
> +    extend: 'Proxmox.form.KVComboBox',
> +    alias: 'widget.pbsMaintenanceType',
> +
> +    comboItems: [
> +	['__default__', gettext('Off')],

i think having 'Off' as no maintenance mode while also having 'Offline'
will be confusing to some users. instead i'd use something
like 'no maintenance' (at least as rendered value)

> +	['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',
> +	onGetValues: function(values) {
> +	    PBS.Utils.delete_if_default(values, 'maintenance-type', '');
> +	    PBS.Utils.delete_if_default(values, 'maintenance-msg', '');
> +	    if (values.delete) {
> +		values.delete = values.delete.split(',');
> +	    }

this is unnecessary, since the 'delete_if_default' should already make
this an array if necessary

it even blocked me from submitting the form, since 'split' is not
a function of an array...

> +
> +	    return values;
> +	},
> +	items: [
> +	    {
> +		xtype: 'pbsMaintenanceType',
> +		name: 'maintenance-type',
> +		fieldLabel: gettext('Maintenance Type'),
> +		value: '__default__',
> +		deleteEmpty: true,
> +	    },
> +	    {
> +		xtype: 'textfield',
> +		emptyText: 'none',

i'd leave that out, since we don't do it with comments either

> +		name: 'maintenance-msg',
> +		fieldLabel: gettext('Description'),
> +		value: '__default__',

that should be empty?
then you also don't need the 'delete_if_default' for the message
since you set deleteEmpty

also i think we should disable that field for the 'Off' variant
since it does not make sense to have a maintenance message
for no maintenance mode...

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

Re: [pbs-devel] [PATCH proxmox-backup 0/5] maintenance mode for datastore

[Dominik Csapak]

high level comment here, rest in the individual patches following

i like the idea, but i find some things a bit clunky:

* what gets saved is the maintenance mode (which is okay) but the
interface to check it is confusing since the user has to give the mode
that is *not* enough. IMHO it would be better if the check would require
the least mode it requires instead. this could either be a "none" (or
"writable") mode of the enum, or making the parameter an Option where
None represents needing write access.

* the check requiring an addition call to the storage seems like a thing
one can easily forget. instead we could make our intent clear on the
lookup part, which could do the check for us. if we then need elevated
access, we can still have the check function. this way we cannot even
request a datastore if its in offline mode anywhere in the code.

i know this would require more code changes possibly, but i think
this would provide a safer interface for this feature

Re: [pbs-devel] [PATCH proxmox-backup 0/5] maintenance mode for datastore

[Dominik Csapak]

i just noticed something that is a good example why the
current approach regarding checking the mode is not optimal.

setting the mode to 'offline' still lets me backup to tape
and even restore from tape to it ;)