diff --git a/traffic_core/src/managed_file.rs b/traffic_core/src/managed_file.rs index f7ce203..1cf17e0 100644 --- a/traffic_core/src/managed_file.rs +++ b/traffic_core/src/managed_file.rs @@ -40,6 +40,10 @@ impl ManagedFile { } } + pub fn path(&self) -> &std::path::Path { + &self.path + } + /// `Ok(None)` only when the file is genuinely absent. Any other failure propagates. pub fn read(&self) -> anyhow::Result> { match fs::read_to_string(&self.path) { diff --git a/traffic_core/src/swap.rs b/traffic_core/src/swap.rs index 1eca4cb..b8a3ccb 100644 --- a/traffic_core/src/swap.rs +++ b/traffic_core/src/swap.rs @@ -14,7 +14,7 @@ use crate::{ config::Config, fstab::{self, FSTAB_PATH, FstabTransaction, escape_field}, lock::FileLock, - managed_file::ManagedFile, + managed_file::{ManagedFile, Ownership}, }; #[derive(Debug, Clone, PartialEq, Eq)] @@ -131,14 +131,26 @@ 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. +/// Removes a file left by an older version. /// -/// 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. +/// Only a foreign file is tolerated - that path belongs to someone else now and the +/// current file lives elsewhere, so there is nothing to migrate. Everything else is +/// reported: swallowing `PermissionDenied` or a failed unlink would leave +/// `/etc/sysctl.d/99-awg2-harness.conf` in place, and it sorts *after* the new `90-` file, +/// so the stale value would keep winning while the reconcile reported success. fn drop_legacy_managed_file(file: &ManagedFile) -> anyhow::Result<()> { - let _ = file.remove(); - Ok(()) + match file.ownership()? { + Ownership::Absent => Ok(()), + Ownership::Foreign => { + tracing::warn!( + target: "swap", + "legacy path {} holds a file we do not own; leaving it alone", + file.path().display() + ); + Ok(()) + } + Ownership::Managed => file.remove().map(|_| ()), + } } /// Brings host swap to the desired state as a transaction. @@ -147,12 +159,15 @@ fn drop_legacy_managed_file(file: &ManagedFile) -> anyhow::Result<()> { /// any foreign swap is disabled. The previous order disabled foreign swaps first, so a /// failure in `fallocate`, `dd`, `mkswap` or `swapon` left the host with no swap at all. pub fn ensure_swap(cfg: &Config) -> anyhow::Result<()> { + // Taken before the enabled check: the disabled branch removes managed files, which is + // a mutation like any other. Leaving it outside the lock let a manual CLI run and the + // daemon's reconcile write and delete the same file at the same time. + let _lock = FileLock::acquire_timeout("/run/awg2-harness/swap.lock", Duration::from_secs(30))?; + if !cfg.swap.enabled { return remove_managed_swap_state(); } - let _lock = FileLock::acquire_timeout("/run/awg2-harness/swap.lock", Duration::from_secs(30))?; - // 1. Preflight: nothing is modified here. let desired = DesiredSwap::from_config(cfg)?; assert_swap_path_is_safe(&desired.path)?; @@ -614,16 +629,50 @@ fn parse_swapon_output(raw: &str) -> anyhow::Result> { /// Destroying a swapfile an administrator created is not this package's call: `swapoff` /// plus removal from `/etc/fstab` already achieves the exclusive runtime state, and the /// file can be removed by hand once the operator agrees. +/// Takes foreign swap offline, restoring what it already disabled if a later one fails. +/// +/// Without the rollback a host with two foreign swaps could end up with the first one +/// disabled and the second still up, because the error aborted the loop and nothing put +/// the first back. That is a state neither the operator nor the next reconcile asked for. fn disable_foreign_swaps(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyhow::Result<()> { + let mut disabled: Vec<&ActiveSwap> = Vec::new(); for entry in active { if entry.name == desired.path { continue; } - run("swapoff", &[entry.name.to_string_lossy().as_ref()])?; + if let Err(e) = run("swapoff", &[entry.name.to_string_lossy().as_ref()]) { + reenable_swaps(&disabled); + return Err(anyhow::anyhow!( + "failed to disable foreign swap {}: {e}", + entry.name.display() + )); + } + disabled.push(entry); } Ok(()) } +/// Best-effort restore after a failed teardown: report what could not be brought back +/// rather than masking it, but never turn the rollback itself into the reported error. +fn reenable_swaps(entries: &[&ActiveSwap]) { + for entry in entries.iter().rev() { + if let Err(e) = run( + "swapon", + &[ + "-p", + &entry.priority.to_string(), + entry.name.to_string_lossy().as_ref(), + ], + ) { + tracing::warn!( + target: "swap", + "could not re-enable {} while rolling back: {e}", + entry.name.display() + ); + } + } +} + /// Creates or validates the managed swapfile without ever touching an unowned one. /// /// `active` is needed because a live swapfile cannot be resized in place: if the managed @@ -648,39 +697,119 @@ fn ensure_desired_swapfile(desired: &DesiredSwap, active: &[ActiveSwap]) -> anyh _ => {} } - let desired_active = active.iter().find(|s| s.name == *p); - let geometry_ok = desired_active - .map(|s| { - active_swap_size_matches(s.size_bytes, desired.size_bytes) - && s.priority == desired.priority - }) - .unwrap_or(false); + let active_entry = active.iter().find(|s| s.name == *p); + let size_ok = ownership == SwapfileOwnership::ManagedValid; - if ownership == SwapfileOwnership::ManagedValid && (desired_active.is_none() || geometry_ok) { - set_swapfile_mode(p)?; - drop_legacy_ownership_sidecar(p); - return Ok(()); + match (size_ok, active_entry) { + // Right file, right size, already active with the right priority: nothing to do. + (true, Some(entry)) + if active_swap_size_matches(entry.size_bytes, desired.size_bytes) + && entry.priority == desired.priority => + { + set_swapfile_mode(p)?; + drop_legacy_ownership_sidecar(p); + Ok(()) + } + // Right file, right size, but the active priority differs. Re-activating is + // enough; the previous code fell through to a full recreate, so changing + // swap.priority destroyed and rebuilt a perfectly good multi-gigabyte file. + (true, Some(_)) => { + run("swapoff", &[p.to_string_lossy().as_ref()])?; + set_swapfile_mode(p)?; + drop_legacy_ownership_sidecar(p); + Ok(()) + } + // Right file, not active: the caller activates it next. + (true, None) => { + set_swapfile_mode(p)?; + drop_legacy_ownership_sidecar(p); + Ok(()) + } + // Wrong size or no file at all, and nothing of ours is active: build in place. + (false, None) => { + if ownership.is_managed() { + fs::remove_file(p) + .map_err(|e| anyhow::anyhow!("failed to remove {}: {e}", p.display()))?; + } + build_swapfile(p, desired.size_bytes)?; + record_ownership(p)?; + drop_legacy_ownership_sidecar(p); + Ok(()) + } + // Wrong size while our swap is live: replace it through a staging file. + (false, Some(_)) => replace_active_swapfile(desired), + } +} + +/// Resizes the managed swapfile while it is active, without a swapless gap during the +/// slow part. +/// +/// A live swapfile cannot be resized in place, so it has to be replaced. Doing that as +/// swapoff-then-create left the host with no swap for the whole duration of +/// `fallocate`/`dd`/`mkswap` - on a machine that was given swap precisely because memory +/// is tight, and with no way back if any of those steps failed. The allocation now happens +/// while the old swap is still serving, and the only gap left is the swapoff/rename/swapon +/// triple, which allocates nothing. +fn replace_active_swapfile(desired: &DesiredSwap) -> anyhow::Result<()> { + let p = &desired.path; + let staging = staging_path(p)?; + + match fs::remove_file(&staging) { + Ok(()) => {} + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => anyhow::bail!("failed to clear stale {}: {e}", staging.display()), } - if desired_active.is_some() { - run("swapoff", &[p.to_string_lossy().as_ref()])?; + // Built and activated while the old swap is still up. + build_swapfile(&staging, desired.size_bytes)?; + if let Err(e) = run( + "swapon", + &[ + "-p", + &desired.priority.to_string(), + staging.to_string_lossy().as_ref(), + ], + ) { + let _ = fs::remove_file(&staging); + return Err(e); } - if ownership.is_managed() { - fs::remove_file(p).map_err(|e| anyhow::anyhow!("failed to remove {}: {e}", p.display()))?; - } + // The old file is only released once the replacement is carrying load. + run("swapoff", &[p.to_string_lossy().as_ref()])?; + fs::remove_file(p).map_err(|e| anyhow::anyhow!("failed to remove {}: {e}", p.display()))?; - create_swapfile(p, desired.size_bytes)?; - // Restrictive mode before mkswap, so the file is never briefly world-readable while - // it already contains swap structure. - set_swapfile_mode(p)?; - run("mkswap", &[p.to_string_lossy().as_ref()])?; - set_swapfile_mode(p)?; + // Short gap: no allocation happens between here and the swapon below. + run("swapoff", &[staging.to_string_lossy().as_ref()])?; + fs::rename(&staging, p).map_err(|e| { + anyhow::anyhow!( + "failed to move {} into place at {}: {e}; activate it manually with swapon", + staging.display(), + p.display() + ) + })?; record_ownership(p)?; drop_legacy_ownership_sidecar(p); Ok(()) } +fn staging_path(path: &Path) -> anyhow::Result { + let name = path + .file_name() + .ok_or_else(|| anyhow::anyhow!("invalid swap.path: {}", path.display()))?; + let mut staging = name.to_os_string(); + staging.push(".awg2-harness.new"); + Ok(path.with_file_name(staging)) +} + +/// Allocates a swapfile and formats it, with the restrictive mode applied before `mkswap` +/// so the file is never briefly world-readable while it already holds swap structure. +fn build_swapfile(path: &Path, size_bytes: u64) -> anyhow::Result<()> { + create_swapfile(path, size_bytes)?; + set_swapfile_mode(path)?; + run("mkswap", &[path.to_string_lossy().as_ref()])?; + set_swapfile_mode(path) +} + #[cfg(unix)] fn set_swapfile_mode(path: &Path) -> anyhow::Result<()> { fs::set_permissions(path, fs::Permissions::from_mode(0o600)) @@ -1170,6 +1299,67 @@ mod tests { } } + fn active(name: &str, size_bytes: u64, priority: i32) -> ActiveSwap { + ActiveSwap { + name: PathBuf::from(name), + kind: "file".to_string(), + size_bytes, + priority, + } + } + + /// Changing only `swap.priority` used to fall through to a full recreate, destroying + /// and rebuilding a correct multi-gigabyte file. It is a reactivation, nothing more. + #[test] + fn priority_only_change_does_not_need_a_new_file() { + let desired = DesiredSwap { + path: PathBuf::from("/swapfile"), + size_bytes: 2048 * 1024 * 1024, + priority: 5, + swappiness: 1, + }; + let entry = active("/swapfile", desired.size_bytes, 1); + + // Same size, different priority: the file is fine, only the activation is wrong. + assert!(active_swap_size_matches( + entry.size_bytes, + desired.size_bytes + )); + assert_ne!(entry.priority, desired.priority); + } + + #[test] + fn staging_path_is_a_sibling_of_the_swapfile() { + let staging = staging_path(Path::new("/var/lib/awg2-harness/swapfile")).expect("staging"); + assert_eq!( + staging, + PathBuf::from("/var/lib/awg2-harness/swapfile.awg2-harness.new") + ); + } + + /// Foreign swaps are identified by not being the managed path; the managed one is + /// never included in the teardown set. + #[test] + fn managed_swap_is_never_in_the_foreign_set() { + let desired = DesiredSwap { + path: PathBuf::from("/swapfile"), + size_bytes: 1024, + priority: 1, + swappiness: 1, + }; + let entries = [ + active("/swapfile", 1024, 1), + active("/dev/sda2", 4096, -2), + active("/other/swap", 2048, 0), + ]; + let foreign: Vec<_> = entries + .iter() + .filter(|e| e.name != desired.path) + .map(|e| e.name.display().to_string()) + .collect(); + assert_eq!(foreign, vec!["/dev/sda2", "/other/swap"]); + } + #[test] fn ownership_rejects_non_regular_file() { let dir = scratch("own_dir"); diff --git a/traffic_core/src/sysctl_tuning.rs b/traffic_core/src/sysctl_tuning.rs index 0477201..cb32147 100644 --- a/traffic_core/src/sysctl_tuning.rs +++ b/traffic_core/src/sysctl_tuning.rs @@ -44,16 +44,18 @@ struct RenderPlan { } pub fn ensure_sysctl_tuning(cfg: &Config) -> anyhow::Result<()> { - if !cfg.sysctl_tuning.enabled { - remove_managed_sysctl_files_if_disabled(cfg)?; - return Ok(()); - } - + // Taken before the enabled check: removing the managed files is a mutation and must + // not race a concurrent apply. let _lock = FileLock::acquire_timeout( "/run/awg2-harness/sysctl_tuning.lock", Duration::from_secs(30), )?; + if !cfg.sysctl_tuning.enabled { + remove_managed_sysctl_files_if_disabled(cfg)?; + return Ok(()); + } + let plan = build_render_plan(cfg, true)?; for w in &plan.warnings { tracing::warn!(target: "sysctl_tuning", "{w}");