From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 1D4F960F6A for ; Tue, 8 Feb 2022 10:40:23 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0F2762C8F0 for ; Tue, 8 Feb 2022 10:40:23 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 48B962C8E7 for ; Tue, 8 Feb 2022 10:40:22 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D2BBE44D95 for ; Tue, 8 Feb 2022 10:40:21 +0100 (CET) Date: Tue, 8 Feb 2022 10:40:20 +0100 From: Wolfgang Bumiller To: Hannes Laimer Cc: pbs-devel@lists.proxmox.com Message-ID: <20220208094020.gmfyxgb3dkfcnol5@olga.proxmox.com> References: <20220204111729.22107-1-h.laimer@proxmox.com> <20220204111729.22107-2-h.laimer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220204111729.22107-2-h.laimer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.396 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lib.rs, datastore.rs, maintenance.rs] Subject: Re: [pbs-devel] [PATCH proxmox-backup v7 1/6] api-types: add maintenance type X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 08 Feb 2022 09:40:23 -0000 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 > --- > 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, > + #[serde(skip_serializing_if="Option::is_none")] > + pub maintenance_type: Option, > } > > #[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, } 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, > +}