refactor: one ownership model for every file written under /etc [P1]

Two modules wrote into /etc/sysctl.d and did it differently. sysctl_tuning refused to touch a file whose first line lacked its marker and cleaned up on disable. swap wrote /etc/sysctl.d/99-awg2-harness.conf with a different marker, no ownership check at all - an administrator's file at that path was silently clobbered - and never removed it when the module was turned off, so vm.swappiness stayed pinned forever. New module managed_file.rs holds the rules once: - a read that fails is never an absent file; - ownership is Absent / Managed / Foreign, decided by the marker on the first line; - write refuses Foreign, skips when the content already matches, and is otherwise atomic: temp sibling created with create_new, sync_all, mode 0644, rename, directory flush; - remove refuses Foreign and treats Absent as success. Both modules now go through it. sysctl_tuning loses its private write_atomic, ensure_managed_or_absent and remove_managed_sysctl_file_at. The swappiness file moves to /etc/sysctl.d/90-awg2-harness-swap.conf with the marker "# Managed by awg2-harness swap", matching the 90- prefix every other managed file uses. The old path is removed on first run of the new version, but only when it still carries the previous "# Managed by awg2-harness" marker; a foreign file there is left alone and does not fail the migration, since the current file lives elsewhere. Disable semantics are now explicit: swap.enabled = false removes the managed file and does nothing else. Runtime vm.swappiness is deliberately not reset - picking a value would be inventing a policy - and active swap is not deactivated, because merely turning a feature off should not trigger a destructive action. Previously ensure_swap returned early and left the persisted file behind. Also replaced a vacuous test: swappiness_conf_render_is_stable asserted that a string literal equals a format! of the same literal, touching no production code at all. It now exercises render_swappiness_conf, and a companion test proves the rendered file is recognised as managed by the same marker check that guards overwriting it - otherwise every reconcile would refuse to touch its own output. Twelve tests: the three ownership states, refusal to overwrite and refusal to remove a foreign file with content asserted intact, idempotent write, stale temp handling, an unreadable path reported as an error, plus legacy migration in all three cases - ours, foreign, absent. cargo test --workspace: 148 + 4 passed. clippy -D warnings clean.
2026-08-06 22:50:19 +05:00
parent a895295701
commit e9f0e29f75
7 changed files with 435 additions and 101 deletions
@@ -14,6 +14,7 @@ use crate::{
    config::Config,
    fstab::{self, FSTAB_PATH, FstabTransaction, escape_field},
    lock::FileLock,
+    managed_file::ManagedFile,
 };

 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -66,6 +67,47 @@ enum SwapfileOwnership {
    Foreign,
 }

+/// Persisted `vm.swappiness`, owned by the swap module.
+///
+/// Lives at `90-` like every other file this package manages. It used to be written to
+/// `/etc/sysctl.d/99-awg2-harness.conf` with a different marker, no ownership check before
+/// overwriting, and no cleanup on disable.
+const SWAPPINESS_CONF_PATH: &str = "/etc/sysctl.d/90-awg2-harness-swap.conf";
+const SWAPPINESS_MARKER: &str = "# Managed by awg2-harness swap";
+/// Written by versions before the unified managed-file handling.
+const LEGACY_SWAPPINESS_CONF_PATH: &str = "/etc/sysctl.d/99-awg2-harness.conf";
+const LEGACY_SWAPPINESS_MARKER: &str = "# Managed by awg2-harness";
+
+fn swappiness_file() -> ManagedFile {
+    ManagedFile::new(SWAPPINESS_CONF_PATH, SWAPPINESS_MARKER)
+}
+
+fn legacy_swappiness_file() -> ManagedFile {
+    ManagedFile::new(LEGACY_SWAPPINESS_CONF_PATH, LEGACY_SWAPPINESS_MARKER)
+}
+
+fn render_swappiness_conf(v: u8) -> String {
+    format!("{SWAPPINESS_MARKER}\n# Source: /etc/awg2-harness/config.toml\nvm.swappiness = {v}\n")
+}
+
+/// Drops the pre-rename file, but only when it still carries the old marker.
+///
+/// A file an administrator put at that path is left alone, which is why the removal goes
+/// through the same ownership check as everything else.
+fn migrate_legacy_swappiness_file() -> anyhow::Result<()> {
+    drop_legacy_managed_file(&legacy_swappiness_file())
+}
+
+/// Removes a file left by an older version, tolerating a foreign file at that path.
+///
+/// A refusal from [`ManagedFile::remove`] means the path now belongs to someone else.
+/// That is not a migration failure: the current file lives elsewhere, so there is nothing
+/// to do and nothing to report.
+fn drop_legacy_managed_file(file: &ManagedFile) -> anyhow::Result<()> {
+    let _ = file.remove();
+    Ok(())
+}
+
 /// Brings host swap to the desired state as a transaction.
 ///
 /// Ordering is the point: the managed swap is created, activated and verified *before*
@@ -73,7 +115,7 @@ enum SwapfileOwnership {
 /// failure in `fallocate`, `dd`, `mkswap` or `swapon` left the host with no swap at all.
 pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> {
    if !cfg.swap.enabled {
-        return Ok(());
+        return remove_managed_swap_state();
    }

    let _lock = FileLock::acquire_timeout("/run/awg2-harness/swap.lock", Duration::from_secs(30))?;
@@ -106,6 +148,19 @@ pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> {
    Ok(())
 }

+/// Undoes the persistent part of the module when `swap.enabled = false`.
+///
+/// Only the managed file is removed. The runtime `vm.swappiness` is deliberately left as
+/// it is: forcing an arbitrary value would be inventing a policy the operator never asked
+/// for. The kernel default applies again after reboot, and `status` reports that the value
+/// is no longer managed. Active swap is not disabled either - turning it off would be a
+/// destructive action triggered by merely disabling a feature.
+fn remove_managed_swap_state() -> anyhow::Result<()> {
+    migrate_legacy_swappiness_file()?;
+    swappiness_file().remove()?;
+    Ok(())
+}
+
 /// Lexical validation of `swap.path`, with no filesystem access.
 ///
 /// Kept side-effect free so `validate-config` can run it on a host where the file does not
@@ -544,12 +599,8 @@ fn apply_swappiness(v: u8) -> anyhow::Result<()> {
        anyhow::bail!("failed to apply vm.swappiness: got {current}, expected {v}");
    }

-    let conf_path = "/etc/sysctl.d/99-awg2-harness.conf";
-    let expected = format!("# Managed by awg2-harness\nvm.swappiness = {v}\n");
-    let current_file = fs::read_to_string(conf_path).unwrap_or_default();
-    if current_file != expected {
-        fs::write(conf_path, expected)?;
-    }
+    migrate_legacy_swappiness_file()?;
+    swappiness_file().write(&render_swappiness_conf(v))?;

    let persisted = read_persisted_swappiness()?;
    if persisted != Some(v) {
@@ -575,10 +626,8 @@ pub fn read_current_swappiness() -> anyhow::Result<u8> {
 }

 pub fn read_persisted_swappiness() -> anyhow::Result<Option<u8>> {
-    let conf_path = "/etc/sysctl.d/99-awg2-harness.conf";
-    let content = match fs::read_to_string(conf_path) {
-        Ok(v) => v,
-        Err(_) => return Ok(None),
+    let Some(content) = swappiness_file().read()? else {
+        return Ok(None);
    };
    for line in content.lines() {
        let t = line.trim();
@@ -959,10 +1008,60 @@ mod tests {

    #[test]
    fn swappiness_conf_render_is_stable() {
-        let expected = "# Managed by awg2-harness\nvm.swappiness = 1\n";
+        let rendered = render_swappiness_conf(1);
+        assert!(rendered.starts_with(SWAPPINESS_MARKER));
+        assert!(rendered.contains("vm.swappiness = 1"));
+        assert!(render_swappiness_conf(60).contains("vm.swappiness = 60"));
+    }
+
+    /// The rendered file must be recognised as ours by the same marker check that guards
+    /// overwriting it, otherwise every reconcile would refuse to touch its own output.
+    #[test]
+    fn rendered_swappiness_conf_is_recognised_as_managed() {
+        let dir = scratch("swappiness_marker");
+        let f = ManagedFile::new(dir.join("90-awg2-harness-swap.conf"), SWAPPINESS_MARKER);
+        assert!(f.write(&render_swappiness_conf(1)).expect("write"));
        assert_eq!(
-            expected,
-            format!("# Managed by awg2-harness\nvm.swappiness = {}\n", 1)
+            f.ownership().expect("ownership"),
+            crate::managed_file::Ownership::Managed
+        );
+        assert!(!f.write(&render_swappiness_conf(1)).expect("idempotent"));
+    }
+
+    #[test]
+    fn legacy_swappiness_file_is_removed_when_it_carries_the_old_marker() {
+        let dir = scratch("legacy_ours");
+        let path = dir.join("99-awg2-harness.conf");
+        fs::write(
+            &path,
+            format!("{LEGACY_SWAPPINESS_MARKER}\nvm.swappiness = 1\n"),
+        )
+        .expect("seed legacy");
+
+        let legacy = ManagedFile::new(path.clone(), LEGACY_SWAPPINESS_MARKER);
+        drop_legacy_managed_file(&legacy).expect("migrate");
+        assert!(!path.exists());
+    }
+
+    /// An administrator's file that happens to sit at the old path is not ours to delete.
+    #[test]
+    fn legacy_migration_leaves_a_foreign_file_alone() {
+        let dir = scratch("legacy_foreign");
+        let path = dir.join("99-awg2-harness.conf");
+        fs::write(&path, "# hand written\nvm.swappiness = 30\n").expect("seed foreign");
+
+        let legacy = ManagedFile::new(path.clone(), LEGACY_SWAPPINESS_MARKER);
+        drop_legacy_managed_file(&legacy).expect("must not fail");
+        assert_eq!(
+            fs::read_to_string(&path).expect("intact"),
+            "# hand written\nvm.swappiness = 30\n"
        );
    }
+
+    #[test]
+    fn legacy_migration_is_a_no_op_when_nothing_is_there() {
+        let dir = scratch("legacy_absent");
+        let legacy = ManagedFile::new(dir.join("99-awg2-harness.conf"), LEGACY_SWAPPINESS_MARKER);
+        assert!(drop_legacy_managed_file(&legacy).is_ok());
+    }
 }