Re: [pbs-devel] [PATCH proxmox-backup v7 1/6] api-types: add maintenance type

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

I'm a little late in the series, but there are a few things I'd like to
get cleaned up here, so here it goes:

On Fri, Feb 04, 2022 at 11:17:24AM +0000, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs   |  8 +++-
>  pbs-api-types/src/lib.rs         |  3 ++
>  pbs-api-types/src/maintenance.rs | 82 ++++++++++++++++++++++++++++++++
>  3 files changed, 92 insertions(+), 1 deletion(-)
>  create mode 100644 pbs-api-types/src/maintenance.rs
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 36279b3a..e2491a92 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -7,7 +7,7 @@ use proxmox_schema::{
>  
>  use crate::{
>      PROXMOX_SAFE_ID_FORMAT, SHA256_HEX_REGEX, SINGLE_LINE_COMMENT_SCHEMA, CryptMode, UPID,
> -    Fingerprint, Userid, Authid,
> +    Fingerprint, Userid, Authid, MaintenanceType,
>      GC_SCHEDULE_SCHEMA, DATASTORE_NOTIFY_STRING_SCHEMA, PRUNE_SCHEDULE_SCHEMA,
>  
>  };
> @@ -224,6 +224,10 @@ pub struct PruneOptions {
>              optional: true,
>              type: bool,
>          },
> +        "maintenance-type": {

So we mostl talk of "maintenance mode" and use "type" and "mode"
interchangeably quite a bit.
While I don't have *strong* feelings about this, I do prefer `mode` a
little for reasons apparent below...

> +            optional: true,
> +            type: MaintenanceType,
> +        },
>      }
>  )]
>  #[derive(Serialize,Deserialize,Updater)]
> @@ -261,6 +265,8 @@ 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>,
>  }
>  
>  #[api(
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 754e7b22..efb01c3e 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -49,6 +49,9 @@ pub use jobs::*;
>  mod key_derivation;
>  pub use key_derivation::{Kdf, KeyInfo};
>  
> +mod maintenance;
> +pub use maintenance::*;
> +
>  mod network;
>  pub use network::*;
>  
> diff --git a/pbs-api-types/src/maintenance.rs b/pbs-api-types/src/maintenance.rs
> new file mode 100644
> index 00000000..4d3ccb3b
> --- /dev/null
> +++ b/pbs-api-types/src/maintenance.rs
> @@ -0,0 +1,82 @@
> +use anyhow::{bail, Error};
> +
> +use proxmox_schema::{ApiStringFormat, Schema, StringSchema, UpdaterType};
> +
> +use crate::{PROXMOX_SAFE_ID_FORMAT, SINGLE_LINE_COMMENT_FORMAT};
> +
> +pub const MAINTENANCE_MODE_SCHEMA: Schema = StringSchema::new("Maintenance mode.")

This doesn't seem to be used.

> +    .format(&PROXMOX_SAFE_ID_FORMAT)
> +    .min_length(3)
> +    .max_length(32)
> +    .schema();
> +
> +pub const MAINTENANCE_MESSAGE_SCHEMA: Schema =
> +    StringSchema::new("Message describing the reason for the maintenance.")
> +        .format(&SINGLE_LINE_COMMENT_FORMAT)
> +        .max_length(32)
> +        .schema();
> +
> +#[derive(UpdaterType)]
> +/// Maintenance type and message.
> +pub enum MaintenanceType {
> +    /// Only reading operations are allowed on the datastore.
> +    ReadOnly(String),
> +    /// Neither reading nor writing operations are allowed on the datastore.
> +    Offline(String),
> +}

While this seems nice at first, we now have the same data with the same
meaning in two different enum values which I find rather weird.

I'd really prefer a _property string_ here. The `type` can be a
`default_key` which makes it look like:
    read-only,message=Something
    offline,message=Something

We wouldn't need an extra parser and the value wouldn't look "weird" if
the message is a free form string, like:
`"read-only-Here is a message with CAPS, spaces and 💥 emoji"`

Iow.
    #[api]
    enum MaintenanceType { // actually just its "type" (or maybe "kind")
        ReadOnly,
        Offline,
    }

    #[api(
        default_key: "type",
        ...
    )]
    struct MaintenanceMode { //
        #[serde(rename = "type")]
        ty: MaintenanceType,

        /// Reason for the maintenance mode.
        #[serde(skip_serializing_if = "Option::is_none")]
        message: Option<String>,
    }

Unfortunatley for now the `DataStoreConfig` would have to have this as
a `String` with a getter that uses `parse_property_string` on it, with
`format: ApiStringFormat::PropertyString(&MaintenanceMode::API_SCHEMA)`

> +
> +/// Operation requirments, used when checking for maintenance mode.

+ #[derive(Clone, Copy, Debug)]

> +pub enum Operation {
> +    Read,
> +    Write,
> +}