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

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

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."`