Re: [pve-devel] [PATCH pve-cluster 02/15] pmxcfs-rs: add pmxcfs-config crate

[Samuel Rufinatscha <s.rufinatscha@proxmox.com>]

comments inline

On 1/6/26 3:25 PM, Kefu Chai wrote:
> Add configuration management crate that provides:
> - Config struct for runtime configuration
> - Node hostname, IP, and group ID tracking
> - Debug and local mode flags
> - Thread-safe configuration access via parking_lot Mutex
> 
> This is a foundational crate with no internal dependencies, only
> requiring parking_lot for synchronization. Other crates will use
> this for accessing runtime configuration.
> 
> Signed-off-by: Kefu Chai <k.chai@proxmox.com>
> ---
>   src/pmxcfs-rs/Cargo.toml               |   3 +-
>   src/pmxcfs-rs/pmxcfs-config/Cargo.toml |  16 +
>   src/pmxcfs-rs/pmxcfs-config/README.md  | 127 +++++++
>   src/pmxcfs-rs/pmxcfs-config/src/lib.rs | 471 +++++++++++++++++++++++++
>   4 files changed, 616 insertions(+), 1 deletion(-)
>   create mode 100644 src/pmxcfs-rs/pmxcfs-config/Cargo.toml
>   create mode 100644 src/pmxcfs-rs/pmxcfs-config/README.md
>   create mode 100644 src/pmxcfs-rs/pmxcfs-config/src/lib.rs
> 
> diff --git a/src/pmxcfs-rs/Cargo.toml b/src/pmxcfs-rs/Cargo.toml
> index 15d88f52..28e20bb7 100644
> --- a/src/pmxcfs-rs/Cargo.toml
> +++ b/src/pmxcfs-rs/Cargo.toml
> @@ -1,7 +1,8 @@
>   # Workspace root for pmxcfs Rust implementation
>   [workspace]
>   members = [
> -    "pmxcfs-api-types", # Shared types and error definitions
> +    "pmxcfs-api-types",  # Shared types and error definitions
> +    "pmxcfs-config",     # Configuration management
>   ]
>   resolver = "2"
>   
> diff --git a/src/pmxcfs-rs/pmxcfs-config/Cargo.toml b/src/pmxcfs-rs/pmxcfs-config/Cargo.toml
> new file mode 100644
> index 00000000..f5a60995
> --- /dev/null
> +++ b/src/pmxcfs-rs/pmxcfs-config/Cargo.toml
> @@ -0,0 +1,16 @@
> +[package]
> +name = "pmxcfs-config"
> +description = "Configuration management for pmxcfs"
> +
> +version.workspace = true
> +edition.workspace = true
> +authors.workspace = true
> +license.workspace = true
> +repository.workspace = true
> +
> +[lints]
> +workspace = true
> +
> +[dependencies]
> +# Concurrency primitives
> +parking_lot.workspace = true
> diff --git a/src/pmxcfs-rs/pmxcfs-config/README.md b/src/pmxcfs-rs/pmxcfs-config/README.md
> new file mode 100644
> index 00000000..c06b2170
> --- /dev/null
> +++ b/src/pmxcfs-rs/pmxcfs-config/README.md
> @@ -0,0 +1,127 @@
> +# pmxcfs-config
> +
> +**Configuration Management** and **Cluster Services** for pmxcfs.
> +
> +This crate provides configuration structures and cluster integration services including quorum tracking and cluster configuration monitoring via Corosync APIs.
> +
> +## Overview
> +
> +This crate contains:
> +1. **Config struct**: Runtime configuration (node name, IPs, flags)
> +2. Integration with Corosync services (tracked in main pmxcfs crate):
> +   - **QuorumService** (`pmxcfs/src/quorum_service.rs`) - Quorum monitoring
> +   - **ClusterConfigService** (`pmxcfs/src/cluster_config_service.rs`) - Config tracking

This patch only contains the Config struct, but not Cluster Services
or QuorumService, please revist commit message and README.

> +
> +## Config Struct
> +
> +The `Config` struct holds daemon-wide configuration including node hostname, IP address, www-data group ID, debug flag, local mode flag, and cluster name.
> +
> +## Cluster Services
> +
> +The following services are implemented in the main pmxcfs crate but documented here for completeness.
> +
> +### QuorumService
> +
> +**C Equivalent:** `src/pmxcfs/quorum.c` - `service_quorum_new()`
> +**Rust Location:** `src/pmxcfs-rs/pmxcfs/src/quorum_service.rs`
> +
> +Monitors cluster quorum status via Corosync quorum API.
> +
> +#### Features
> +- Tracks quorum state (quorate/inquorate)
> +- Monitors member list changes
> +- Automatic reconnection on Corosync restart
> +- Updates `Status` quorum flag
> +
> +#### C to Rust Mapping
> +
> +| C Function | Rust Equivalent | Location |
> +|-----------|-----------------|----------|
> +| `service_quorum_new()` | `QuorumService::new()` | quorum_service.rs |
> +| `service_quorum_destroy()` | (Drop trait / finalize) | Automatic |
> +| `quorum_notification_fn` | quorum_notification closure | quorum_service.rs |
> +| `nodelist_notification_fn` | nodelist_notification closure | quorum_service.rs |
> +
> +#### Quorum Notifications
> +
> +The service monitors quorum state changes and member list changes, updating the Status accordingly.
> +
> +### ClusterConfigService
> +
> +**C Equivalent:** `src/pmxcfs/confdb.c` - `service_confdb_new()`
> +**Rust Location:** `src/pmxcfs-rs/pmxcfs/src/cluster_config_service.rs`
> +
> +Monitors Corosync cluster configuration (cmap) and tracks node membership.
> +
> +#### Features
> +- Monitors cluster membership via Corosync cmap API
> +- Tracks node additions/removals
> +- Registers nodes in Status
> +- Automatic reconnection on Corosync restart
> +
> +#### C to Rust Mapping
> +
> +| C Function | Rust Equivalent | Location |
> +|-----------|-----------------|----------|
> +| `service_confdb_new()` | `ClusterConfigService::new()` | cluster_config_service.rs |
> +| `service_confdb_destroy()` | (Drop trait / finalize) | Automatic |
> +| `confdb_track_fn` | (direct cmap queries) | Different approach |
> +
> +#### Configuration Tracking
> +
> +The service monitors:
> +- `nodelist.node.*.nodeid` - Node IDs
> +- `nodelist.node.*.name` - Node names
> +- `nodelist.node.*.ring*_addr` - Node IP addresses
> +
> +Updates `Status` with current cluster membership.
> +
> +## Key Differences from C Implementation
> +
> +### Cluster Config Service API
> +
> +**C Version (confdb.c):**
> +- Uses deprecated confdb API
> +- Track changes via confdb notifications
> +
> +**Rust Version:**
> +- Uses modern cmap API
> +- Direct cmap queries
> +
> +Both read the same data, but Rust uses the modern Corosync API.
> +
> +### Service Integration
> +
> +**C Version:**
> +- qb_loop manages lifecycle
> +
> +**Rust Version:**
> +- Service trait abstracts lifecycle
> +- ServiceManager handles retry
> +- Tokio async dispatch
> +
> +## Known Issues / TODOs
> +
> +### Compatibility
> +- **Quorum tracking**: Compatible with C implementation
> +- **Node registration**: Equivalent behavior
> +- **cmap vs confdb**: Rust uses modern cmap API (C uses deprecated confdb)
> +
> +### Missing Features
> +- None identified
> +
> +### Behavioral Differences (Benign)
> +- **API choice**: Rust uses cmap, C uses confdb (both read same data)
> +- **Lifecycle**: Rust uses Service trait, C uses manual lifecycle
> +
> +## References
> +
> +### C Implementation
> +- `src/pmxcfs/quorum.c` / `quorum.h` - Quorum service
> +- `src/pmxcfs/confdb.c` / `confdb.h` - Cluster config service
> +
> +### Related Crates
> +- **pmxcfs**: Main daemon with QuorumService and ClusterConfigService
> +- **pmxcfs-status**: Status tracking updated by these services
> +- **pmxcfs-services**: Service framework used by both services
> +- **rust-corosync**: Corosync FFI bindings
> diff --git a/src/pmxcfs-rs/pmxcfs-config/src/lib.rs b/src/pmxcfs-rs/pmxcfs-config/src/lib.rs
> new file mode 100644
> index 00000000..5e1ee1b2
> --- /dev/null
> +++ b/src/pmxcfs-rs/pmxcfs-config/src/lib.rs
> @@ -0,0 +1,471 @@
> +use parking_lot::RwLock;
> +use std::sync::Arc;
> +
> +/// Global configuration for pmxcfs
> +pub struct Config {
> +    /// Node name (hostname without domain)
> +    pub nodename: String,
> +
> +    /// Node IP address
> +    pub node_ip: String,

Consider using std::net::IpAddr (or SocketAddr if a port is part of the
value). Tests currently mix IP vs IP:PORT, so it’s unclear what node_ip
is supposed to represent.

> +
> +    /// www-data group ID for file permissions
> +    pub www_data_gid: u32,
> +
> +    /// Debug mode enabled
> +    pub debug: bool,
> +
> +    /// Force local mode (no clustering)
> +    pub local_mode: bool,
> +
> +    /// Cluster name (CPG group name)
> +    pub cluster_name: String,
> +
> +    /// Debug level (0 = normal, 1+ = debug) - mutable at runtime
> +    debug_level: RwLock<u8>,

in the crate docs it says: “The Config struct uses Arc<AtomicU8> for
debug_level” but the implementation uses parking_lot::RwLock<u8>.
Unless we need lock coupling with other fields, AtomicU8 would likely
be sufficient (and cheaper) for debug_level. Also please re-check the
commit message, which mentions parking_lot::Mutex.

> +}
> +
> +impl Clone for Config {
> +    fn clone(&self) -> Self {
> +        Self {
> +            nodename: self.nodename.clone(),
> +            node_ip: self.node_ip.clone(),
> +            www_data_gid: self.www_data_gid,
> +            debug: self.debug,
> +            local_mode: self.local_mode,
> +            cluster_name: self.cluster_name.clone(),
> +            debug_level: RwLock::new(*self.debug_level.read()),
> +        }
> +    }
> +}
> +
> +impl std::fmt::Debug for Config {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        f.debug_struct("Config")
> +            .field("nodename", &self.nodename)
> +            .field("node_ip", &self.node_ip)
> +            .field("www_data_gid", &self.www_data_gid)
> +            .field("debug", &self.debug)
> +            .field("local_mode", &self.local_mode)
> +            .field("cluster_name", &self.cluster_name)
> +            .field("debug_level", &*self.debug_level.read())
> +            .finish()
> +    }
> +}
> +
> +impl Config {
> +    pub fn new(
> +        nodename: String,
> +        node_ip: String,
> +        www_data_gid: u32,
> +        debug: bool,
> +        local_mode: bool,
> +        cluster_name: String,
> +    ) -> Arc<Self> {

The constructor returns Arc<Config>
I think we could keep new() -> Self, and provide convenience
constructor shared() -> Arc<Self>.
This would allow local usage (e.g. for tests) without heap allocation
of the struct

> +        let debug_level = if debug { 1 } else { 0 };

debug_level is derived from debug at creation time, but thereafter:
set_debug_level() does not update debug and is_debug() would continue
to reflect the initial flag, not the effective debug level
is_debug() should just be a helper that returns self.debug_level() > 0.
The debug field should probably be removed entirely.

> +        Arc::new(Self {
> +            nodename,
> +            node_ip,
> +            www_data_gid,
> +            debug,
> +            local_mode,
> +            cluster_name,
> +            debug_level: RwLock::new(debug_level),
> +        })
> +    }
> +
> +    pub fn cluster_name(&self) -> &str {
> +        &self.cluster_name
> +    }
> +
> +    pub fn nodename(&self) -> &str {
> +        &self.nodename
> +    }
> +
> +    pub fn node_ip(&self) -> &str {
> +        &self.node_ip
> +    }
> +
> +    pub fn www_data_gid(&self) -> u32 {
> +        self.www_data_gid
> +    }
> +
> +    pub fn is_debug(&self) -> bool {
> +        self.debug
> +    }
> +
> +    pub fn is_local_mode(&self) -> bool {
> +        self.local_mode
> +    }
> +
> +    /// Get current debug level (0 = normal, 1+ = debug)
> +    pub fn debug_level(&self) -> u8 {
> +        *self.debug_level.read()
> +    }
> +
> +    /// Set debug level (0 = normal, 1+ = debug)
> +    pub fn set_debug_level(&self, level: u8) {
> +        *self.debug_level.write() = level;
> +    }

Right now most fields are pub but also getters are exposed. This will
make it harder to enforce invariants.
I would suggest to make fields private and keep getters, or keep fields
public and drop the getters.

> +}
> +
> +#[cfg(test)]
> +mod tests {
> +    //! Unit tests for Config struct
> +    //!
> +    //! This test module provides comprehensive coverage for:
> +    //! - Configuration creation and initialization
> +    //! - Getter methods for all configuration fields
> +    //! - Debug level mutation and thread safety
> +    //! - Concurrent access patterns (reads and writes)
> +    //! - Clone independence
> +    //! - Debug formatting
> +    //! - Edge cases (empty strings, long strings, special characters, unicode)
> +    //!
> +    //! ## Thread Safety
> +    //!
> +    //! The Config struct uses `Arc<AtomicU8>` for debug_level to allow
> +    //! safe concurrent reads and writes. Tests verify:
> +    //! - 10 threads × 100 operations (concurrent modifications)
> +    //! - 20 threads × 1000 operations (concurrent reads)
> +    //!
> +    //! ## Edge Cases
> +    //!
> +    //! Tests cover various edge cases including:
> +    //! - Empty strings for node/cluster names
> +    //! - Long strings (1000+ characters)
> +    //! - Special characters in strings
> +    //! - Unicode support (emoji, non-ASCII characters)
> +
> +    use super::*;
> +    use std::thread;
> +
> +    // ===== Basic Construction Tests =====
> +
> +    #[test]
> +    fn test_config_creation() {
> +        let config = Config::new(
> +            "node1".to_string(),
> +            "192.168.1.10".to_string(),
> +            33,
> +            false,
> +            false,
> +            "pmxcfs".to_string(),
> +        );
> +
> +        assert_eq!(config.nodename(), "node1");
> +        assert_eq!(config.node_ip(), "192.168.1.10");
> +        assert_eq!(config.www_data_gid(), 33);
> +        assert!(!config.is_debug());
> +        assert!(!config.is_local_mode());
> +        assert_eq!(config.cluster_name(), "pmxcfs");
> +        assert_eq!(
> +            config.debug_level(),
> +            0,
> +            "Debug level should be 0 when debug is false"
> +        );
> +    }
> +
> +    #[test]
> +    fn test_config_creation_with_debug() {
> +        let config = Config::new(
> +            "node2".to_string(),
> +            "10.0.0.5".to_string(),
> +            1000,
> +            true,
> +            false,
> +            "test-cluster".to_string(),
> +        );
> +
> +        assert!(config.is_debug());
> +        assert_eq!(
> +            config.debug_level(),
> +            1,
> +            "Debug level should be 1 when debug is true"
> +        );
> +    }
> +
> +    #[test]
> +    fn test_config_creation_local_mode() {
> +        let config = Config::new(
> +            "localhost".to_string(),
> +            "127.0.0.1".to_string(),
> +            33,
> +            false,
> +            true,
> +            "local".to_string(),
> +        );
> +
> +        assert!(config.is_local_mode());
> +        assert!(!config.is_debug());
> +    }
> +
> +    // ===== Getter Tests =====
> +
> +    #[test]
> +    fn test_all_getters() {
> +        let config = Config::new(
> +            "testnode".to_string(),
> +            "172.16.0.1".to_string(),
> +            999,
> +            true,
> +            true,
> +            "my-cluster".to_string(),
> +        );
> +
> +        // Test all getter methods
> +        assert_eq!(config.nodename(), "testnode");
> +        assert_eq!(config.node_ip(), "172.16.0.1");
> +        assert_eq!(config.www_data_gid(), 999);
> +        assert!(config.is_debug());
> +        assert!(config.is_local_mode());
> +        assert_eq!(config.cluster_name(), "my-cluster");
> +        assert_eq!(config.debug_level(), 1);
> +    }
> +
> +    // ===== Debug Level Mutation Tests =====
> +
> +    #[test]
> +    fn test_debug_level_mutation() {
> +        let config = Config::new(
> +            "node1".to_string(),
> +            "192.168.1.1".to_string(),
> +            33,
> +            false,
> +            false,
> +            "pmxcfs".to_string(),
> +        );
> +
> +        assert_eq!(config.debug_level(), 0);
> +
> +        config.set_debug_level(1);
> +        assert_eq!(config.debug_level(), 1);
> +
> +        config.set_debug_level(5);
> +        assert_eq!(config.debug_level(), 5);
> +
> +        config.set_debug_level(0);
> +        assert_eq!(config.debug_level(), 0);
> +    }
> +
> +    #[test]
> +    fn test_debug_level_max_value() {
> +        let config = Config::new(
> +            "node1".to_string(),
> +            "192.168.1.1".to_string(),
> +            33,
> +            false,
> +            false,
> +            "pmxcfs".to_string(),
> +        );
> +
> +        config.set_debug_level(255);
> +        assert_eq!(config.debug_level(), 255);
> +
> +        config.set_debug_level(0);
> +        assert_eq!(config.debug_level(), 0);
> +    }
> +
> +    // ===== Thread Safety Tests =====
> +
> +    #[test]
> +    fn test_debug_level_thread_safety() {
> +        let config = Config::new(
> +            "node1".to_string(),
> +            "192.168.1.1".to_string(),
> +            33,
> +            false,
> +            false,
> +            "pmxcfs".to_string(),
> +        );
> +
> +        let config_clone = Arc::clone(&config);
> +
> +        // Spawn multiple threads that concurrently modify debug level
> +        let handles: Vec<_> = (0..10)
> +            .map(|i| {
> +                let cfg = Arc::clone(&config);
> +                thread::spawn(move || {
> +                    for _ in 0..100 {
> +                        cfg.set_debug_level(i);
> +                        let _ = cfg.debug_level();
> +                    }
> +                })
> +            })
> +            .collect();
> +
> +        // All threads should complete without panicking
> +        for handle in handles {
> +            handle.join().unwrap();
> +        }
> +
> +        // Final value should be one of the values set by threads
> +        let final_level = config_clone.debug_level();
> +        assert!(
> +            final_level < 10,
> +            "Debug level should be < 10, got {final_level}"
> +        );
> +    }
> +
> +    #[test]
> +    fn test_concurrent_reads() {
> +        let config = Config::new(
> +            "node1".to_string(),
> +            "192.168.1.1".to_string(),
> +            33,
> +            true,
> +            false,
> +            "pmxcfs".to_string(),
> +        );
> +
> +        // Spawn multiple threads that concurrently read config
> +        let handles: Vec<_> = (0..20)
> +            .map(|_| {
> +                let cfg = Arc::clone(&config);
> +                thread::spawn(move || {
> +                    for _ in 0..1000 {
> +                        assert_eq!(cfg.nodename(), "node1");
> +                        assert_eq!(cfg.node_ip(), "192.168.1.1");
> +                        assert_eq!(cfg.www_data_gid(), 33);
> +                        assert!(cfg.is_debug());
> +                        assert!(!cfg.is_local_mode());
> +                        assert_eq!(cfg.cluster_name(), "pmxcfs");
> +                    }
> +                })
> +            })
> +            .collect();
> +
> +        for handle in handles {
> +            handle.join().unwrap();
> +        }
> +    }
> +
> +    // ===== Clone Tests =====
> +
> +    #[test]
> +    fn test_config_clone() {
> +        let config1 = Config::new(
> +            "node1".to_string(),
> +            "192.168.1.1".to_string(),
> +            33,
> +            true,
> +            false,
> +            "pmxcfs".to_string(),
> +        );
> +
> +        config1.set_debug_level(5);
> +
> +        let config2 = (*config1).clone();
> +
> +        // Cloned config should have same values
> +        assert_eq!(config2.nodename(), config1.nodename());
> +        assert_eq!(config2.node_ip(), config1.node_ip());
> +        assert_eq!(config2.www_data_gid(), config1.www_data_gid());
> +        assert_eq!(config2.is_debug(), config1.is_debug());
> +        assert_eq!(config2.is_local_mode(), config1.is_local_mode());
> +        assert_eq!(config2.cluster_name(), config1.cluster_name());
> +        assert_eq!(config2.debug_level(), 5);
> +
> +        // Modifying one should not affect the other
> +        config2.set_debug_level(10);
> +        assert_eq!(config1.debug_level(), 5);
> +        assert_eq!(config2.debug_level(), 10);
> +    }
> +
> +    // ===== Debug Formatting Tests =====
> +
> +    #[test]
> +    fn test_debug_format() {
> +        let config = Config::new(
> +            "node1".to_string(),
> +            "192.168.1.1".to_string(),
> +            33,
> +            true,
> +            false,
> +            "pmxcfs".to_string(),
> +        );
> +
> +        let debug_str = format!("{config:?}");
> +
> +        // Check that debug output contains all fields
> +        assert!(debug_str.contains("Config"));
> +        assert!(debug_str.contains("nodename"));
> +        assert!(debug_str.contains("node1"));
> +        assert!(debug_str.contains("node_ip"));
> +        assert!(debug_str.contains("192.168.1.1"));
> +        assert!(debug_str.contains("www_data_gid"));
> +        assert!(debug_str.contains("33"));
> +        assert!(debug_str.contains("debug"));
> +        assert!(debug_str.contains("true"));
> +        assert!(debug_str.contains("local_mode"));
> +        assert!(debug_str.contains("false"));
> +        assert!(debug_str.contains("cluster_name"));
> +        assert!(debug_str.contains("pmxcfs"));
> +        assert!(debug_str.contains("debug_level"));
> +    }
> +
> +    // ===== Edge Cases and Boundary Tests =====
> +
> +    #[test]
> +    fn test_empty_strings() {
> +        let config = Config::new(String::new(), String::new(), 0, false, false, String::new());
> +
> +        assert_eq!(config.nodename(), "");
> +        assert_eq!(config.node_ip(), "");
> +        assert_eq!(config.cluster_name(), "");
> +        assert_eq!(config.www_data_gid(), 0);
> +    }
> +
> +    #[test]
> +    fn test_long_strings() {
> +        let long_name = "a".repeat(1000);
> +        let long_ip = "192.168.1.".to_string() + &"1".repeat(100);
> +        let long_cluster = "cluster-".to_string() + &"x".repeat(500);
> +
> +        let config = Config::new(
> +            long_name.clone(),
> +            long_ip.clone(),
> +            u32::MAX,
> +            true,
> +            true,
> +            long_cluster.clone(),
> +        );
> +
> +        assert_eq!(config.nodename(), long_name);
> +        assert_eq!(config.node_ip(), long_ip);
> +        assert_eq!(config.cluster_name(), long_cluster);
> +        assert_eq!(config.www_data_gid(), u32::MAX);
> +    }
> +
> +    #[test]
> +    fn test_special_characters_in_strings() {
> +        let config = Config::new(
> +            "node-1_test.local".to_string(),
> +            "192.168.1.10:8006".to_string(),
> +            33,
> +            false,
> +            false,
> +            "my-cluster_v2.0".to_string(),
> +        );
> +
> +        assert_eq!(config.nodename(), "node-1_test.local");
> +        assert_eq!(config.node_ip(), "192.168.1.10:8006");
> +        assert_eq!(config.cluster_name(), "my-cluster_v2.0");
> +    }
> +
> +    #[test]
> +    fn test_unicode_in_strings() {
> +        let config = Config::new(
> +            "ノード1".to_string(),
> +            "::1".to_string(),
> +            33,
> +            false,
> +            false,
> +            "集群".to_string(),
> +        );
> +
> +        assert_eq!(config.nodename(), "ノード1");
> +        assert_eq!(config.node_ip(), "::1");
> +        assert_eq!(config.cluster_name(), "集群");
> +    }
> +}



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel