Decouple sysctl runtime validation and modprobe dependencies

2026-05-28 12:12:42 +05:00
parent 10b2d0aefb
commit 4f384bcff0
4 changed files with 100 additions and 23 deletions
@@ -287,6 +287,25 @@ impl Config {
        self.check_host_tools()
    }

+    pub fn validate_sysctl_tuning_runtime(&self) -> anyhow::Result<()> {
+        self.validate_config()?;
+
+        if !self.sysctl_tuning.enabled {
+            return Ok(());
+        }
+
+        if !tool_exists("sysctl") {
+            anyhow::bail!("required host tool not found in PATH: sysctl");
+        }
+
+        let tcp_bbr_mode = self.sysctl_tuning.tcp_bbr.trim().to_ascii_lowercase();
+        if tcp_bbr_mode == "require" && !tool_exists("modprobe") {
+            anyhow::bail!("required host tool not found in PATH: modprobe");
+        }
+
+        Ok(())
+    }
+
    pub fn validate_all(&self) -> anyhow::Result<()> {
        self.validate_config()?;
        self.validate_runtime()?;
@@ -299,12 +318,15 @@ impl Config {
            anyhow::bail!("required host tool not found in PATH: nsenter");
        }
        required.push("nsenter");
+        let tcp_bbr_mode = self.sysctl_tuning.tcp_bbr.trim().to_ascii_lowercase();
        if self.sysctl_tuning.enabled {
            required.push("sysctl");
        }
-        if self.sysctl_tuning.enabled
-            && self.sysctl_tuning.tcp_bbr.trim().to_ascii_lowercase() == "require"
-        {
+        if requires_modprobe(
+            self.traffic.enabled,
+            self.sysctl_tuning.enabled,
+            &tcp_bbr_mode,
+        ) {
            required.push("modprobe");
        }
        if self.anti_torrent.enabled {
@@ -328,6 +350,9 @@ impl Config {
            ]);
        }

+        required.sort_unstable();
+        required.dedup();
+
        for tool in required {
            if !tool_exists(tool) {
                anyhow::bail!("required host tool not found in PATH: {tool}");
@@ -383,6 +408,10 @@ impl Config {
    }
 }

+fn requires_modprobe(traffic_enabled: bool, sysctl_enabled: bool, tcp_bbr_mode: &str) -> bool {
+    traffic_enabled || (sysctl_enabled && tcp_bbr_mode == "require")
+}
+
 fn parse_hhmm(v: &str) -> anyhow::Result<()> {
    let mut s = v.split(':');
    let h = s
@@ -707,4 +736,25 @@ swappiness = 1
        assert!(cfg.sysctl_tuning.apply_on_daemon_start);
        assert_eq!(cfg.sysctl_tuning.tcp_bbr, "auto");
    }
+
+    #[test]
+    fn validate_sysctl_tuning_runtime_skips_modprobe_for_auto_when_disabled() {
+        let mut cfg = base_config();
+        cfg.sysctl_tuning.enabled = false;
+        cfg.sysctl_tuning.tcp_bbr = "auto".into();
+        assert!(cfg.validate_sysctl_tuning_runtime().is_ok());
+    }
+
+    #[test]
+    fn requires_modprobe_when_traffic_enabled() {
+        assert!(requires_modprobe(true, true, "auto"));
+        assert!(requires_modprobe(true, false, "off"));
+    }
+
+    #[test]
+    fn requires_modprobe_for_sysctl_require_mode() {
+        assert!(requires_modprobe(false, true, "require"));
+        assert!(!requires_modprobe(false, true, "auto"));
+        assert!(!requires_modprobe(false, true, "off"));
+    }
 }
@@ -52,7 +52,7 @@ fn main() -> anyhow::Result<()> {
        }
        Command::ApplyNetworkTuning => {
            let cfg = Config::load(&config_path)?;
-            cfg.validate_all()?;
+            cfg.validate_sysctl_tuning_runtime()?;
            sysctl_tuning::ensure_sysctl_tuning(&cfg)?;
            println!("network tuning reconciled");
        }
@@ -19,6 +19,7 @@ pub struct SysctlTuningStatus {
    pub bbr_available: Option<bool>,
    pub bbr_active: Option<bool>,
    pub current: Vec<SysctlValue>,
+    #[allow(dead_code)]
    pub desired: Vec<SysctlValue>,
    pub drift_reason: Option<String>,
    pub warnings: Vec<String>,
@@ -29,6 +30,7 @@ pub struct SysctlValue {
    pub key: String,
    pub desired: Option<String>,
    pub current: Option<String>,
+    #[allow(dead_code)]
    pub managed: bool,
 }

@@ -220,7 +222,10 @@ fn remove_managed_sysctl_file_at(path: &Path) -> anyhow::Result<()> {
    let raw = fs::read_to_string(path)?;
    let first = raw.lines().next().unwrap_or("");
    if !first.contains(MANAGED_MARKER) {
-        anyhow::bail!("refusing to remove unmanaged sysctl file: {}", path.display());
+        anyhow::bail!(
+            "refusing to remove unmanaged sysctl file: {}",
+            path.display()
+        );
    }
    fs::remove_file(path)?;
    Ok(())
@@ -258,22 +263,23 @@ fn build_render_plan(cfg: &Config, allow_modprobe: bool) -> anyhow::Result<Rende
    }

    let mode = cfg.sysctl_tuning.tcp_bbr.trim().to_ascii_lowercase();
-    let mut bbr_avail = bbr_available()?;
-    if !bbr_avail && mode != "off" && allow_modprobe {
-        let _ = Command::new("modprobe").args(["tcp_bbr"]).output();
-        bbr_avail = bbr_available()?;
-    }
-
-    let bbr_active = if mode == "off" {
-        None
-    } else if bbr_avail {
-        Some(true)
+    let (bbr_available_opt, bbr_active) = if mode == "off" {
+        (None, None)
    } else {
-        if mode == "require" {
-            anyhow::bail!("tcp_bbr=require but bbr is unavailable");
+        let mut bbr_avail = bbr_available()?;
+        if !bbr_avail && allow_modprobe {
+            let _ = Command::new("modprobe").args(["tcp_bbr"]).output();
+            bbr_avail = bbr_available()?;
+        }
+
+        if bbr_avail {
+            (Some(true), Some(true))
+        } else if mode == "require" {
+            anyhow::bail!("tcp_bbr=require but bbr is unavailable");
+        } else {
+            warnings.push("bbr unavailable; tcp tuning skipped in auto mode".to_string());
+            (Some(false), Some(false))
        }
-        warnings.push("bbr unavailable; tcp tuning skipped in auto mode".to_string());
-        Some(false)
    };

    if cfg.sysctl_tuning.ip_forward {
@@ -315,7 +321,7 @@ fn build_render_plan(cfg: &Config, allow_modprobe: bool) -> anyhow::Result<Rende
    Ok(RenderPlan {
        desired,
        persisted,
-        bbr_available: Some(bbr_avail),
+        bbr_available: bbr_available_opt,
        bbr_active,
        warnings,
    })
@@ -367,7 +373,9 @@ fn normalize(v: &str) -> String {
 }

 fn write_atomic(path: &str, content: &str) -> anyhow::Result<()> {
+    #[cfg(unix)]
    let p = Path::new(path);
+    #[cfg(unix)]
    let dir = p
        .parent()
        .ok_or_else(|| anyhow::anyhow!("no parent directory for path: {path}"))?;
@@ -517,11 +525,26 @@ mod tests {
            "awg2_harness_managed_overwrite_test_{}.conf",
            std::process::id()
        ));
-        fs::write(&managed, format!("{MANAGED_MARKER}\nnet.core.rmem_max = 1\n"))
-            .expect("write managed");
+        fs::write(
+            &managed,
+            format!("{MANAGED_MARKER}\nnet.core.rmem_max = 1\n"),
+        )
+        .expect("write managed");
        let managed_str = managed.to_string_lossy().to_string();
        ensure_managed_or_absent(&managed_str).expect("allow managed");

        let _ = fs::remove_file(&managed);
    }
+
+    #[test]
+    fn build_render_plan_off_mode_skips_bbr_desired_keys() {
+        let mut c = cfg();
+        c.sysctl_tuning.tcp_bbr = "off".into();
+
+        let plan = build_render_plan(&c, false).expect("render plan");
+        assert!(!plan.desired.contains_key("net.ipv4.tcp_congestion_control"));
+        assert!(!plan.desired.contains_key("net.core.default_qdisc"));
+        assert_eq!(plan.bbr_available, None);
+        assert_eq!(plan.bbr_active, None);
+    }
 }