Re: [pbs-devel] [PATCH proxmox-backup v8 2/7] pbs-config: add metrics config class

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

On Wed, Jun 08, 2022 at 02:22:33PM +0200, Dominik Csapak wrote:
> a section config like in pve
> 
> also adds a helper to get Metrics structs for all configured servers
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  Cargo.toml                |   1 +
>  pbs-config/Cargo.toml     |   1 +
>  pbs-config/src/lib.rs     |   1 +
>  pbs-config/src/metrics.rs | 108 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 111 insertions(+)
>  create mode 100644 pbs-config/src/metrics.rs
> 
> diff --git a/Cargo.toml b/Cargo.toml
> index 234f2bee..caa91429 100644
> --- a/Cargo.toml
> +++ b/Cargo.toml
> @@ -96,6 +96,7 @@ pxar = { version = "0.10.1", features = [ "tokio-io" ] }
>  proxmox-http = { version = "0.6.1", features = [ "client", "http-helpers", "websocket" ] }
>  proxmox-io = "1"
>  proxmox-lang = "1.1"
> +proxmox-metrics = "0.1"
>  proxmox-router = { version = "1.2.2", features = [ "cli" ] }
>  proxmox-schema = { version = "1.3.1", features = [ "api-macro" ] }
>  proxmox-section-config = "1"
> diff --git a/pbs-config/Cargo.toml b/pbs-config/Cargo.toml
> index 9af1144c..37f7e263 100644
> --- a/pbs-config/Cargo.toml
> +++ b/pbs-config/Cargo.toml
> @@ -25,6 +25,7 @@ proxmox-time = "1"
>  proxmox-serde = "0.1"
>  proxmox-shared-memory = "0.2"
>  proxmox-sys = "0.3"
> +proxmox-metrics = "0.1"
>  
>  pbs-api-types = { path = "../pbs-api-types" }
>  pbs-buildcfg = { path = "../pbs-buildcfg" }
> diff --git a/pbs-config/src/lib.rs b/pbs-config/src/lib.rs
> index a6627caa..a83db4e1 100644
> --- a/pbs-config/src/lib.rs
> +++ b/pbs-config/src/lib.rs
> @@ -6,6 +6,7 @@ pub mod domains;
>  pub mod drive;
>  pub mod key_config;
>  pub mod media_pool;
> +pub mod metrics;
>  pub mod network;
>  pub mod prune;
>  pub mod remote;
> diff --git a/pbs-config/src/metrics.rs b/pbs-config/src/metrics.rs
> new file mode 100644
> index 00000000..cc248623
> --- /dev/null
> +++ b/pbs-config/src/metrics.rs
> @@ -0,0 +1,108 @@
> +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 = InfluxDbUdp::API_SCHEMA.unwrap_object_schema();

Please use `const UDP_SCHEMA` for this please to make sure this is
checked at compile time. (see datastore.rs or prune.rs)

> +    let udp_plugin = SectionConfigPlugin::new(
> +        "influxdb-udp".to_string(),
> +        Some("name".to_string()),
> +        udp_schema,
> +    );
> +    config.register_plugin(udp_plugin);
> +
> +    let http_schema = InfluxDbHttp::API_SCHEMA.unwrap_object_schema();

same here

> +
> +    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_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.keys().cloned().collect(),
> +        Err(_) => return vec![],
> +    }
> +}
> +
> +/// Get the metric server connections from a config
> +pub fn get_metric_server_connections(
> +    metric_config: SectionConfigData,
> +) -> Result<(Vec<Metrics>, Vec<String>), Error> {

Bad API. A `Vec<(Metrics, String)>` or `HashMap` would make much more
sense.
This could more easily avoided if proxmox-metrics'
`send_data_to_channels` took an `IntoIterator<Item = &Metrics>` instead
of a slice :-S