prod/AWG2_harness
0
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 2 Wiki Activity

fix(swap): roll back partial teardown, resize without a swapless gap, lock the disable path [P1]

Browse Source 
Four defects from the follow-up audit.

1. Legacy migration swallowed every error.

   drop_legacy_managed_file was `let _ = file.remove(); Ok(())`, which tolerated not only
   the expected "that path is someone else's now" but also PermissionDenied, read errors
   and failed unlinks. If /etc/sysctl.d/99-awg2-harness.conf survived, it would keep
   winning over the new 90- file - it sorts later - while the reconcile reported success,
   so vm.swappiness would silently stay at the old value. The three cases are now
   distinguished: Absent is fine, Foreign warns and continues, Managed removes and any
   failure propagates.

2. The disable branches ran outside the lock.

   Both ensure_swap and ensure_sysctl_tuning checked `enabled` before acquiring their
   lock, so the cleanup path - which deletes managed files - was unsynchronised. A manual
   CLI invocation and the daemon's periodic reconcile could write and delete the same file
   concurrently. The lock is now taken first in both.

3. A failed teardown left foreign swaps half-disabled.

   disable_foreign_swaps aborted the loop on the first failure, so a host with two foreign
   swaps could end with the first one off and the second still up - a state nobody asked
   for and the error message did not describe. It now tracks what it disabled and
   re-enables it, in reverse order and with the original priority, before returning the
   error. The restore is best-effort and warns per entry rather than replacing the reported
   failure with a rollback failure.

4. Resizing dropped swap for the whole allocation.

   A live swapfile cannot be resized in place, and the previous code did swapoff, remove,
   fallocate/dd, mkswap, swapon. On a host with no other swap that is a swapless window
   spanning the slowest and most failure-prone part of the operation - on a machine that
   was given swap precisely because memory is tight, with no way back if any step failed.

   The replacement is now built and activated at a staging sibling while the old swap is
   still serving, and only then is the old file released. The remaining gap is
   swapoff/rename/swapon, which allocates nothing. A failure while staging removes the
   staging file and leaves the original swap untouched; a failure at the final rename says
   explicitly that the file is in place and can be activated with swapon.

   Related, same function: changing only swap.priority used to fall through to the full
   recreate path, destroying and rebuilding a correct multi-gigabyte file to change one
   integer. Size and priority are now separate decisions - a priority-only change is a
   reactivation.

The reconciliation is now decided by an explicit match over (size correct, currently
active) rather than a single `geometry_ok` flag, which is what hid the priority case.

Three tests added around the new decision surface: the priority-only case is not a resize,
the staging path is a sibling of the swapfile, and the managed swap is never part of the
foreign teardown set.

cargo test --workspace: 179 + 4 passed. clippy -D warnings clean.
This commit is contained in:
Crimson
2026-08-07 04:15:40 +05:00
parent 322f67f69a
commit 116b1ea91a
3 changed files with 233 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
traffic_core/src/swap.rs
+222 -32
View File
@@ -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<Vec<ActiveSwap>> {
/// 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<PathBuf> {
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");