[pbs-devel] [PATCH proxmox-backup v2 0/3] pbs-api-types: new MaintenanceType::Unmount, implement and use set_maintenance_mode

[pbs-devel] [PATCH proxmox-backup v2 0/3] pbs-api-types: new MaintenanceType::Unmount, implement and use set_maintenance_mode

[Dietmar Maurer]

Same functionality as v1, but

- split patch into smaller ones
- cleanup commit messages

Dietmar Maurer (3):
  pbs-api-types: use SchemaDeserializer for maintenance mode
  maintenance: add 'Unmount' maintenance type
  pbs-api-types: add check for allowed maintenmance mode changes

 pbs-api-types/src/datastore.rs   | 49 ++++++++++++++++++++++++++++----
 pbs-api-types/src/maintenance.rs | 11 ++++---
 pbs-datastore/src/datastore.rs   |  8 ++++--
 src/api2/config/datastore.rs     | 13 +++++++--
 4 files changed, 66 insertions(+), 15 deletions(-)

-- 
2.39.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 1/3] pbs-api-types: use SchemaDeserializer for maintenance mode

[Dietmar Maurer]

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 5e13c157..483cdef4 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -11,8 +11,8 @@ use proxmox_schema::{
 };
 
 use crate::{
-    Authid, CryptMode, Fingerprint, GroupFilter, MaintenanceMode, Userid, BACKUP_ID_RE,
-    BACKUP_NS_RE, BACKUP_TIME_RE, BACKUP_TYPE_RE, DATASTORE_NOTIFY_STRING_SCHEMA,
+    Authid, CryptMode, Fingerprint, GroupFilter, MaintenanceMode, MaintenanceType, Userid,
+    BACKUP_ID_RE, BACKUP_NS_RE, BACKUP_TIME_RE, BACKUP_TYPE_RE, DATASTORE_NOTIFY_STRING_SCHEMA,
     GC_SCHEDULE_SCHEMA, GROUP_OR_SNAPSHOT_PATH_REGEX_STR, PROXMOX_SAFE_ID_FORMAT,
     PROXMOX_SAFE_ID_REGEX_STR, PRUNE_SCHEDULE_SCHEMA, SHA256_HEX_REGEX, SINGLE_LINE_COMMENT_SCHEMA,
     SNAPSHOT_PATH_REGEX_STR, UPID,
@@ -336,10 +336,13 @@ impl DataStoreConfig {
     }
 
     pub fn get_maintenance_mode(&self) -> Option<MaintenanceMode> {
-        self.maintenance_mode
-            .as_ref()
-            .and_then(|str| MaintenanceMode::API_SCHEMA.parse_property_string(str).ok())
-            .and_then(|value| MaintenanceMode::deserialize(value).ok())
+        self.maintenance_mode.as_ref().and_then(|str| {
+            MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(
+                str,
+                &MaintenanceMode::API_SCHEMA,
+            ))
+            .ok()
+        })
     }
 }
 
-- 
2.39.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2 1/3] pbs-api-types: use SchemaDeserializer for maintenance mode

[Christian Ebner]

On 4/19/24 09:37, Dietmar Maurer wrote:
> Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   pbs-api-types/src/datastore.rs | 15 +++++++++------
>   1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 5e13c157..483cdef4 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -11,8 +11,8 @@ use proxmox_schema::{
>   };
>   
>   use crate::{
> -    Authid, CryptMode, Fingerprint, GroupFilter, MaintenanceMode, Userid, BACKUP_ID_RE,
> -    BACKUP_NS_RE, BACKUP_TIME_RE, BACKUP_TYPE_RE, DATASTORE_NOTIFY_STRING_SCHEMA,
> +    Authid, CryptMode, Fingerprint, GroupFilter, MaintenanceMode, MaintenanceType, Userid,
> +    BACKUP_ID_RE, BACKUP_NS_RE, BACKUP_TIME_RE, BACKUP_TYPE_RE, DATASTORE_NOTIFY_STRING_SCHEMA,
>       GC_SCHEDULE_SCHEMA, GROUP_OR_SNAPSHOT_PATH_REGEX_STR, PROXMOX_SAFE_ID_FORMAT,
>       PROXMOX_SAFE_ID_REGEX_STR, PRUNE_SCHEDULE_SCHEMA, SHA256_HEX_REGEX, SINGLE_LINE_COMMENT_SCHEMA,
>       SNAPSHOT_PATH_REGEX_STR, UPID,
> @@ -336,10 +336,13 @@ impl DataStoreConfig {
>       }
>   
>       pub fn get_maintenance_mode(&self) -> Option<MaintenanceMode> {
> -        self.maintenance_mode
> -            .as_ref()
> -            .and_then(|str| MaintenanceMode::API_SCHEMA.parse_property_string(str).ok())
> -            .and_then(|value| MaintenanceMode::deserialize(value).ok())
> +        self.maintenance_mode.as_ref().and_then(|str| {
> +            MaintenanceMode::deserialize(proxmox_schema::de::SchemaDeserializer::new(

Should `SchemaDeserializer` be brought into scope by adding it to the 
already present `proxmox_schema` use statement?

> +                str,
> +                &MaintenanceMode::API_SCHEMA,
> +            ))
> +            .ok()
> +        })
>       }
>   }
>   



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 2/3] maintenance: add 'Unmount' maintenance type

[Dietmar Maurer]

- and derive Copy and Clone for maintenance type
- make maintenance mode fields public, because it is a public api type

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 pbs-api-types/src/maintenance.rs | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
index a605cc17..fd4d3416 100644
--- a/pbs-api-types/src/maintenance.rs
+++ b/pbs-api-types/src/maintenance.rs
@@ -33,12 +33,11 @@ pub enum Operation {
 }
 
 #[api]
-#[derive(Deserialize, Serialize, PartialEq, Eq)]
+#[derive(Copy, Clone, Deserialize, Serialize, PartialEq, Eq)]
 #[serde(rename_all = "kebab-case")]
 /// Maintenance type.
 pub enum MaintenanceType {
     // TODO:
-    //  - Add "unmounting" once we got pluggable datastores
     //  - Add "GarbageCollection" or "DeleteOnly" as type and track GC (or all deletes) as separate
     //    operation, so that one can enable a mode where nothing new can be added but stuff can be
     //    cleaned
@@ -48,6 +47,8 @@ pub enum MaintenanceType {
     Offline,
     /// The datastore is being deleted.
     Delete,
+    /// The (removable) datastore is being unmounted.
+    Unmount,
 }
 serde_plain::derive_display_from_serialize!(MaintenanceType);
 serde_plain::derive_fromstr_from_deserialize!(MaintenanceType);
@@ -69,11 +70,11 @@ serde_plain::derive_fromstr_from_deserialize!(MaintenanceType);
 pub struct MaintenanceMode {
     /// Type of maintenance ("read-only" or "offline").
     #[serde(rename = "type")]
-    ty: MaintenanceType,
+    pub ty: MaintenanceType,
 
     /// Reason for maintenance.
     #[serde(skip_serializing_if = "Option::is_none")]
-    message: Option<String>,
+    pub message: Option<String>,
 }
 
 impl MaintenanceMode {
@@ -94,6 +95,8 @@ impl MaintenanceMode {
 
         if let Some(Operation::Lookup) = operation {
             return Ok(());
+        } else if self.ty == MaintenanceType::Unmount {
+            bail!("datastore is being unmounted");
         } else if self.ty == MaintenanceType::Offline {
             bail!("offline maintenance mode: {}", message);
         } else if self.ty == MaintenanceType::ReadOnly {
-- 
2.39.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

[pbs-devel] [PATCH proxmox-backup v2 3/3] pbs-api-types: add check for allowed maintenmance mode changes

[Dietmar Maurer]

Signed-off-by: Dietmar Maurer <dietmar@proxmox.com>
---
 pbs-api-types/src/datastore.rs | 34 ++++++++++++++++++++++++++++++++++
 pbs-datastore/src/datastore.rs |  8 ++++++--
 src/api2/config/datastore.rs   | 13 ++++++++++---
 3 files changed, 50 insertions(+), 5 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 483cdef4..9fe45bc0 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -344,6 +344,40 @@ impl DataStoreConfig {
             .ok()
         })
     }
+
+    pub fn set_maintenance_mode(&mut self, new_mode: Option<MaintenanceMode>) -> Result<(), Error> {
+        let current_type = self.get_maintenance_mode().map(|mode| mode.ty);
+        let new_type = new_mode.as_ref().map(|mode| mode.ty);
+
+        match current_type {
+            Some(MaintenanceType::ReadOnly) => { /* always OK  */ }
+            Some(MaintenanceType::Offline) => { /* always OK  */ }
+            Some(MaintenanceType::Unmount) => {
+                bail!("datastore is being unmounted");
+            }
+            Some(MaintenanceType::Delete) => {
+                match new_type {
+                    Some(MaintenanceType::Delete) => { /* allow to delete a deleted storage */ }
+                    _ => {
+                        bail!("datastore is being deleted")
+                    }
+                }
+            }
+            None => { /* always OK  */ }
+        }
+
+        let new_mode = match new_mode {
+            Some(new_mode) => Some(
+                proxmox_schema::property_string::PropertyString::new(new_mode)
+                    .to_property_string()?,
+            ),
+            None => None,
+        };
+
+        self.maintenance_mode = new_mode;
+
+        Ok(())
+    }
 }
 
 #[api(
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 0685cc84..f95da761 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -20,7 +20,7 @@ use proxmox_sys::{task_log, task_warn};
 
 use pbs_api_types::{
     Authid, BackupNamespace, BackupType, ChunkOrder, DataStoreConfig, DatastoreFSyncLevel,
-    DatastoreTuning, GarbageCollectionStatus, Operation, UPID,
+    DatastoreTuning, GarbageCollectionStatus, MaintenanceMode, MaintenanceType, Operation, UPID,
 };
 
 use crate::backup_info::{BackupDir, BackupGroup, BackupGroupDeleteStats};
@@ -1390,7 +1390,11 @@ impl DataStore {
         let (mut config, _digest) = pbs_config::datastore::config()?;
         let mut datastore_config: DataStoreConfig = config.lookup("datastore", name)?;
 
-        datastore_config.maintenance_mode = Some("type=delete".to_string());
+        datastore_config.set_maintenance_mode(Some(MaintenanceMode {
+            ty: MaintenanceType::Delete,
+            message: None,
+        }))?;
+
         config.set_data(name, "datastore", &datastore_config)?;
         pbs_config::datastore::save_config(&config)?;
         drop(config_lock);
diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs
index 3081e1f4..87425ff5 100644
--- a/src/api2/config/datastore.rs
+++ b/src/api2/config/datastore.rs
@@ -13,7 +13,7 @@ use proxmox_uuid::Uuid;
 
 use pbs_api_types::{
     Authid, DataStoreConfig, DataStoreConfigUpdater, DatastoreNotify, DatastoreTuning, KeepOptions,
-    PruneJobConfig, PruneJobOptions, DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE,
+    MaintenanceMode, PruneJobConfig, PruneJobOptions, DATASTORE_SCHEMA, PRIV_DATASTORE_ALLOCATE,
     PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_MODIFY, PROXMOX_CONFIG_DIGEST_SCHEMA, UPID_SCHEMA,
 };
 use pbs_config::BackupLockGuard;
@@ -319,7 +319,7 @@ pub fn update_datastore(
                     data.tuning = None;
                 }
                 DeletableProperty::MaintenanceMode => {
-                    data.maintenance_mode = None;
+                    data.set_maintenance_mode(None)?;
                 }
             }
         }
@@ -392,7 +392,14 @@ pub fn update_datastore(
     let mut maintenance_mode_changed = false;
     if update.maintenance_mode.is_some() {
         maintenance_mode_changed = data.maintenance_mode != update.maintenance_mode;
-        data.maintenance_mode = update.maintenance_mode;
+
+        let maintenance_mode = match update.maintenance_mode {
+            Some(mode_str) => Some(MaintenanceMode::deserialize(
+                proxmox_schema::de::SchemaDeserializer::new(mode_str, &MaintenanceMode::API_SCHEMA),
+            )?),
+            None => None,
+        };
+        data.set_maintenance_mode(maintenance_mode)?;
     }
 
     config.set_data(&name, "datastore", &data)?;
-- 
2.39.2


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup v2 3/3] pbs-api-types: add check for allowed maintenmance mode changes

[Thomas Lamprecht]

This is a semantic change where the fact that code is touched that's inside a
crate named pbs-api-types is rather an implementation detail, I'd rather use
something like the following as subject:

api: assert that maintenance mode transitions are valid


And having some commit message with some basic reasoning would be also great..

If this was ordered as second patch, i.e., without the new maintenance mode,
we could already apply that and Hannes could add the patch that adds the
maintenance mode to his series, which would be nicer to review and apply
(and IMO a slightly better commit order)


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel