Re: [pbs-devel] [PATCH proxmox-backup v7 3/8] pbs-config: add metrics config class

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

On Thu, Jun 02, 2022 at 02:18:06PM +0200, Dominik Csapak wrote:
(...)
> @@ -0,0 +1,115 @@
> +use std::collections::HashMap;
> +
> +use anyhow::Error;
> +use lazy_static::lazy_static;
> +
> +use proxmox_metrics::{influxdb_http, influxdb_udp, Metrics};
> +use proxmox_schema::*;
> +use proxmox_section_config::{SectionConfig, SectionConfigData, SectionConfigPlugin};
> +
> +use pbs_api_types::{InfluxDbHttp, InfluxDbUdp, METRIC_SERVER_ID_SCHEMA};
> +
> +use crate::{open_backup_lockfile, BackupLockGuard};
> +
> +lazy_static! {
> +    pub static ref CONFIG: SectionConfig = init();
> +}
> +
> +fn init() -> SectionConfig {
> +    let mut config = SectionConfig::new(&METRIC_SERVER_ID_SCHEMA);
> +
> +    let udp_schema = match InfluxDbUdp::API_SCHEMA {
> +        Schema::Object(ref object_schema) => object_schema,
> +        _ => unreachable!(),
> +    };

New code should prefer using the `unwrap_<type>_schema()` const fns,
defining a const for the schema, in order to get some compile time
checks there.

    const UDP_SCHEMA: &ObjectSchema = InfluxDbUdp::API_SCHEMA.unwrap_object_schema();

Eg. if you ever `#[serde(flatten)]` some struct into the type the schema
type will change to `AllOfSchema`. See prune.rs as an example for this.
(We should also switch the remaining files probably.)

> +
> +    let udp_plugin = SectionConfigPlugin::new(
> +        "influxdb-udp".to_string(),
> +        Some("name".to_string()),
> +        udp_schema,
> +    );
> +    config.register_plugin(udp_plugin);
> +
> +    let http_schema = match InfluxDbHttp::API_SCHEMA {
> +        Schema::Object(ref object_schema) => object_schema,
> +        _ => unreachable!(),
> +    };
> +
> +    let http_plugin = SectionConfigPlugin::new(
> +        "influxdb-http".to_string(),
> +        Some("name".to_string()),
> +        http_schema,
> +    );
> +
> +    config.register_plugin(http_plugin);
> +
> +    config
> +}
> +
> +pub const METRIC_SERVER_CFG_FILENAME: &str = "/etc/proxmox-backup/metricserver.cfg";
> +pub const METRIC_SERVER_CFG_LOCKFILE: &str = "/etc/proxmox-backup/.metricserver.lck";
> +
> +/// Get exclusive lock
> +pub fn lock_config() -> Result<BackupLockGuard, Error> {
> +    open_backup_lockfile(METRIC_SERVER_CFG_LOCKFILE, None, true)
> +}
> +
> +pub fn config() -> Result<(SectionConfigData, [u8; 32]), Error> {
> +    let content = proxmox_sys::fs::file_read_optional_string(METRIC_SERVER_CFG_FILENAME)?
> +        .unwrap_or_else(|| "".to_string());

Easier on the compiler (and a `const fn`):

    .unwrap_or_else(String::new)

or even

    .unwrap_or_default()

> +
> +    let digest = openssl::sha::sha256(content.as_bytes());
> +    let data = CONFIG.parse(METRIC_SERVER_CFG_FILENAME, &content)?;
> +    Ok((data, digest))
> +}
> +
> +pub fn save_config(config: &SectionConfigData) -> Result<(), Error> {
> +    let raw = CONFIG.write(METRIC_SERVER_CFG_FILENAME, config)?;
> +    crate::replace_backup_config(METRIC_SERVER_CFG_FILENAME, raw.as_bytes())
> +}
> +
> +// shell completion helper
> +pub fn complete_remote_name(_arg: &str, _param: &HashMap<String, String>) -> Vec<String> {
> +    match config() {
> +        Ok((data, _digest)) => data.sections.iter().map(|(id, _)| id.to_string()).collect(),

I think you could simplify this to `.keys().cloned().collect()`