feat(admin): restore config admin compatibility (#3133)

* feat(admin): restore config admin compatibility Co-authored-by: weisd <im@weisd.in> * fix(admin): align config admin clean rebuild Co-authored-by: weisd <im@weisd.in> * fix(admin): align config history and peer signals * fix(admin): harden config admin mutations * fix(admin): tighten config review follow-ups * perf(admin): reuse env snapshot in config render * fix(ecstore): clean up config admin and listing error handling Remove redundant is_all_volume_not_found check in list_merged, add storage class encode/decode roundtrip tests, fresh boot integration test, and config admin clean rebuild improvements. Co-authored-by: hehutu <heihutu@gmail.com> * fix(admin): sync global server config on mutation and reload Change GLOBAL_SERVER_CONFIG from OnceLock to RwLock so config mutations (set/del/restore/reload) are visible to readers without restart. Call set_global_server_config after every store save and on snapshot reload. Register storage_class as a dynamic config subsystem. Co-authored-by: hehutu <heihutu@gmail.com> * style: apply rustfmt to config and admin tests Co-authored-by: hehutu <heihutu@gmail.com> * fix(test): update signal_service test for storage_class dynamic subsystem storage_class is now a valid dynamic config subsystem, so the "requires object layer" test should expect "storage layer not initialized" instead of "unsupported dynamic config subsystem". Co-authored-by: hehutu <heihutu@gmail.com> * fix(config): publish storage_class runtime config on dynamic reload Change GLOBAL_STORAGE_CLASS from OnceLock to RwLock so runtime updates are possible. apply_storage_class_runtime_config now actually publishes the parsed config via set_global_storage_class instead of dropping it. Addresses review feedback: storage_class was marked as dynamically applied but the parsed result was discarded, so mc admin config set returned config_applied=true while the runtime kept using stale parity settings until restart. Co-authored-by: hehutu <heihutu@gmail.com> * fix(admin): harden config init, history ordering, and env redaction - Change GLOBAL_SERVER_CONFIG from RwLock<Config> to RwLock<Option<Config>> initialized with None, preserving "not initialized" detection via None - Move save_server_config_history before save_server_config_to_store in SetConfigKVHandler, DelConfigKVHandler, and SetConfigHandler so a restore point exists before mutations are persisted - Redact sensitive env override values with *redacted* instead of silently omitting the line, improving admin visibility - Add code comment explaining VolumeNotFound removal rationale in list_merged for listing paths Co-authored-by: hehutu <heihutu@gmail.com> * fix(config): keep in-memory config in sync after set/restore/reload GLOBAL_SERVER_CONFIG was a OnceLock set once at startup and never updated. After mc admin config set writes to the store, any fallback to get_global_server_config() returned stale init-time data. Similarly, reload_runtime_config_snapshot read from the store but discarded the result. - Replace OnceLock with RwLock for GLOBAL_SERVER_CONFIG and GLOBAL_STORAGE_CLASS so they can be updated at runtime - Add set_global_server_config / set_global_storage_class setters - Call set_global_server_config after every config save (set-kv, del-kv, set-config, restore-history) - Re-apply dynamic subsystems (storage_class, audit_webhook, audit_mqtt) and signal peers in reload_runtime_config_snapshot and full-config operations - Fix render_selected_config scope boundary check: track per-scope line count instead of checking global lines.is_empty() - Include STORAGE_CLASS_SUB_SYS in is_dynamic_config_subsystem so apply_storage_class_runtime_config is reachable Co-authored-by: hehutu <heihutu@gmail.com> * fix(storageclass): use CLASS_RRS key in lookup_config for RRS parity lookup_config used kvs.get(RRS) where RRS="REDUCED_REDUNDANCY", but the admin config path writes the key as CLASS_RRS="rrs". This caused RRS values to never be read back, always falling back to default parity. - Changed kvs.get(RRS) to kvs.get(CLASS_RRS) in lookup_config - Added regression tests verifying RRS read/write consistency Co-authored-by: hehutu <heihutu@gmail.com> * fix(config): add peer-side logging and don't swallow apply errors - Add tracing::warn! in reload_dynamic_config_runtime_state and reload_runtime_config_snapshot when config read or subsystem apply fails, so on-host diagnostics show which signal failed and why - Change `let _ = apply_dynamic_config_for_subsystem(...)` to `if let Err(err) = ... { warn!(...) }` in reload_runtime_config_snapshot so per-subsystem failures are logged instead of silently swallowed - Remove weak test global_server_config_returns_none_before_init that had no meaningful assertion due to shared global state Co-authored-by: hehutu <heihutu@gmail.com> * style: apply rustfmt to config and storageclass tests Co-authored-by: hehutu <heihutu@gmail.com> --------- Co-authored-by: weisd <im@weisd.in> Co-authored-by: hehutu <heihutu@gmail.com>
2026-08-27 15:37:02 +00:00 · 2026-05-31 19:50:13 +08:00
parent 92104cb354
commit 8577bd825e
19 changed files with 3027 additions and 39 deletions
@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.

-use crate::config::{Config, GLOBAL_STORAGE_CLASS, KVS, audit, notify, oidc, storageclass};
+use crate::config::{Config, KVS, audit, notify, oidc, set_global_storage_class, storageclass};
 use crate::disk::{MIGRATING_META_BUCKET, RUSTFS_META_BUCKET};
 use crate::error::{Error, Result};
 use crate::global::is_first_cluster_node_local;
@@ -1164,11 +1164,8 @@ async fn apply_dynamic_config_for_sub_sys<S: StorageAPI>(cfg: &mut Config, api:
        for (i, count) in set_drive_counts.iter().enumerate() {
            match storageclass::lookup_config(&kvs, *count) {
                Ok(res) => {
-                    if i == 0
-                        && GLOBAL_STORAGE_CLASS.get().is_none()
-                        && let Err(r) = GLOBAL_STORAGE_CLASS.set(res)
-                    {
-                        error!("GLOBAL_STORAGE_CLASS.set failed {:?}", r);
+                    if i == 0 {
+                        set_global_storage_class(res);
                    }
                }
                Err(err) => {
@@ -2638,4 +2635,87 @@ mod tests {
        assert_eq!(object_info.bucket, crate::disk::RUSTFS_META_BUCKET);
        assert_eq!(object_info.name, "config/test.json");
    }
+
+    #[test]
+    fn test_encode_decode_roundtrip_preserves_storage_class() {
+        use crate::config::STORAGE_CLASS_SUB_SYS;
+
+        // Create a config with custom storage class values
+        let mut cfg = Config::new();
+        let kvs = storage_class_kvs_mut(&mut cfg);
+        kvs.insert("standard".to_string(), "EC:4".to_string());
+        kvs.insert("rrs".to_string(), "EC:2".to_string());
+        kvs.insert("optimize".to_string(), "availability".to_string());
+
+        // Encode to external format (what save_server_config produces)
+        let encoded = encode_server_config_blob(&cfg, None).expect("encode should succeed");
+
+        // Verify the encoded data uses "storageclass" (no underscore)
+        let v: serde_json::Value = serde_json::from_slice(&encoded).expect("should be valid json");
+        assert!(v.get("storageclass").is_some(), "encoded should have 'storageclass' key");
+        assert!(v.get("storage_class").is_none(), "encoded should NOT have 'storage_class' key");
+
+        // Decode back (what load_server_config_from_store produces)
+        let decoded = decode_server_config_blob(&encoded).expect("decode should succeed");
+
+        // Verify the decoded config has "storage_class" (with underscore) subsystem
+        let kvs = decoded
+            .get_value(STORAGE_CLASS_SUB_SYS, crate::config::DEFAULT_DELIMITER)
+            .expect("decoded config should have storage_class subsystem");
+        assert_eq!(kvs.get("standard"), "EC:4", "standard should be EC:4");
+        assert_eq!(kvs.get("rrs"), "EC:2", "rrs should be EC:2");
+        assert_eq!(kvs.get("optimize"), "availability", "optimize should be availability");
+    }
+
+    #[test]
+    fn test_set_then_load_preserves_storage_class_values() {
+        use crate::config::STORAGE_CLASS_SUB_SYS;
+
+        // Step 1: Start with a fresh config (simulates initial server state)
+        let mut cfg = Config::new();
+
+        // Step 2: Apply set directives (simulates "mc admin config set storage_class standard=EC:4 rrs=EC:2")
+        let kvs = cfg
+            .0
+            .entry(STORAGE_CLASS_SUB_SYS.to_string())
+            .or_default()
+            .entry(DEFAULT_DELIMITER.to_string())
+            .or_default();
+        kvs.insert("standard".to_string(), "EC:4".to_string());
+        kvs.insert("rrs".to_string(), "EC:2".to_string());
+        cfg.set_defaults();
+
+        // Step 3: Save (encode to external format)
+        let encoded = encode_server_config_blob(&cfg, None).expect("encode should succeed");
+
+        // Step 4: Load (decode from external format)
+        let loaded = decode_server_config_blob(&encoded).expect("decode should succeed");
+
+        // Step 5: Verify the values are preserved
+        let loaded_kvs = loaded
+            .get_value(STORAGE_CLASS_SUB_SYS, DEFAULT_DELIMITER)
+            .expect("should have storage_class");
+        assert_eq!(loaded_kvs.get("standard"), "EC:4");
+        assert_eq!(loaded_kvs.get("rrs"), "EC:2");
+
+        // Step 6: Verify the subsystem is accessible by name (what render_selected_config does)
+        let targets = loaded
+            .0
+            .get(STORAGE_CLASS_SUB_SYS)
+            .expect("storage_class subsystem should exist");
+        let target_kvs = targets.get(DEFAULT_DELIMITER).expect("default target should exist");
+        assert_eq!(target_kvs.get("standard"), "EC:4");
+        assert_eq!(target_kvs.get("rrs"), "EC:2");
+    }
+
+    #[test]
+    fn test_config_unmarshal_fails_on_external_format() {
+        // This verifies that the external "storageclass" format cannot be
+        // mistakenly parsed by Config::unmarshal (which expects internal format).
+        // If unmarshal succeeds, the config would have "storageclass" instead of
+        // "storage_class", causing render_selected_config to fail.
+        let external_json = r#"{"version":"33","storageclass":{"standard":"EC:4","rrs":"EC:2"}}"#;
+        let result = Config::unmarshal(external_json.as_bytes());
+        assert!(result.is_err(), "Config::unmarshal should fail on external format, but got: {:?}", result);
+    }
 }
@@ -37,11 +37,12 @@ use rustfs_config::oidc::IDENTITY_OPENID_SUB_SYS;
 use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
 use std::sync::LazyLock;
-use std::sync::{Arc, OnceLock};
+use std::sync::{Arc, OnceLock, RwLock};

-pub static GLOBAL_STORAGE_CLASS: LazyLock<OnceLock<storageclass::Config>> = LazyLock::new(OnceLock::new);
+pub static GLOBAL_STORAGE_CLASS: LazyLock<RwLock<storageclass::Config>> =
+    LazyLock::new(|| RwLock::new(storageclass::Config::default()));
 pub static DEFAULT_KVS: LazyLock<OnceLock<HashMap<String, KVS>>> = LazyLock::new(OnceLock::new);
-pub static GLOBAL_SERVER_CONFIG: LazyLock<OnceLock<Config>> = LazyLock::new(OnceLock::new);
+pub static GLOBAL_SERVER_CONFIG: LazyLock<RwLock<Option<Config>>> = LazyLock::new(|| RwLock::new(None));
 pub static GLOBAL_CONFIG_SYS: LazyLock<ConfigSys> = LazyLock::new(ConfigSys::new);

 pub static RUSTFS_CONFIG_PREFIX: &str = "config";
@@ -63,14 +64,30 @@ impl ConfigSys {

        lookup_configs(&mut cfg, api).await;

-        let _ = GLOBAL_SERVER_CONFIG.set(cfg);
+        set_global_server_config(cfg);

        Ok(())
    }
 }

 pub fn get_global_server_config() -> Option<Config> {
-    GLOBAL_SERVER_CONFIG.get().cloned()
+    GLOBAL_SERVER_CONFIG.read().ok().and_then(|guard| (*guard).clone())
+}
+
+pub fn set_global_server_config(cfg: Config) {
+    if let Ok(mut guard) = GLOBAL_SERVER_CONFIG.write() {
+        *guard = Some(cfg);
+    }
+}
+
+pub fn get_global_storage_class() -> Option<storageclass::Config> {
+    GLOBAL_STORAGE_CLASS.read().ok().map(|guard| (*guard).clone())
+}
+
+pub fn set_global_storage_class(cfg: storageclass::Config) {
+    if let Ok(mut guard) = GLOBAL_STORAGE_CLASS.write() {
+        *guard = cfg;
+    }
 }

 pub async fn init_global_config_sys(api: Arc<ECStore>) -> Result<()> {
@@ -262,3 +279,25 @@ pub fn init() {
    // Register all default configurations
    register_default_kvs(kvs)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn global_server_config_set_and_get_roundtrip() {
+        init();
+        let mut cfg = Config::new();
+        let mut kvs = KVS::new();
+        kvs.insert("standard".to_string(), "EC:4".to_string());
+        cfg.0
+            .insert(STORAGE_CLASS_SUB_SYS.to_string(), HashMap::from([("_".to_string(), kvs)]));
+
+        set_global_server_config(cfg.clone());
+        let loaded = get_global_server_config().expect("global config should be set");
+        let sc_kvs = loaded
+            .get_value(STORAGE_CLASS_SUB_SYS, "_")
+            .expect("storage_class should exist");
+        assert_eq!(sc_kvs.get("standard"), "EC:4");
+    }
+}
@@ -101,13 +101,13 @@ pub static DEFAULT_KVS: LazyLock<KVS> = LazyLock::new(|| {
 });

 // StorageClass - holds storage class information
-#[derive(Serialize, Deserialize, Debug, Default)]
+#[derive(Serialize, Deserialize, Debug, Default, Clone)]
 pub struct StorageClass {
    parity: usize,
 }

 // Config storage class configuration
-#[derive(Serialize, Deserialize, Debug, Default)]
+#[derive(Serialize, Deserialize, Debug, Default, Clone)]
 pub struct Config {
    standard: StorageClass,
    rrs: StorageClass,
@@ -205,7 +205,7 @@ pub fn lookup_config(kvs: &KVS, set_drive_count: usize) -> Result<Config> {
            if let Ok(ssc_str) = env::var(RRS_ENV) {
                ssc_str
            } else {
-                kvs.get(RRS)
+                kvs.get(CLASS_RRS)
            }
        };

@@ -336,3 +336,36 @@ pub fn validate_parity_inner(ss_parity: usize, rrs_parity: usize, set_drive_coun
    }
    Ok(())
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn lookup_config_reads_rrs_from_class_rrs_key() {
+        // Regression: kvs.get(RRS) used RRS="REDUCED_REDUNDANCY" instead of
+        // CLASS_RRS="rrs", so admin-written RRS values were never read back.
+        let mut kvs = KVS::new();
+        kvs.insert(CLASS_STANDARD.to_string(), "EC:4".to_string());
+        kvs.insert(CLASS_RRS.to_string(), "EC:2".to_string());
+
+        let cfg = lookup_config(&kvs, 8).expect("lookup should succeed");
+        assert_eq!(cfg.standard.parity, 4, "standard parity should be 4");
+        assert_eq!(cfg.rrs.parity, 2, "rrs parity should be 2");
+    }
+
+    #[test]
+    fn lookup_config_ignores_redundancy_key_name() {
+        // Ensure the old key name "REDUCED_REDUNDANCY" is NOT read.
+        let mut kvs = KVS::new();
+        kvs.insert(CLASS_STANDARD.to_string(), "EC:4".to_string());
+        kvs.insert(RRS.to_string(), "EC:2".to_string());
+
+        let cfg = lookup_config(&kvs, 8).expect("lookup should succeed");
+        assert_eq!(cfg.standard.parity, 4);
+        assert_eq!(
+            cfg.rrs.parity, DEFAULT_RRS_PARITY,
+            "rrs should fall back to default when CLASS_RRS key is absent"
+        );
+    }
+}