feat(#3139): bound the agent container start limit and back off between retries
container@.service sets Restart=on-failure and no start limit, so systemd's defaults applied: 5 starts per 10s with RestartSec 100ms. That made the bound a function of how fast a container dies -- one failing instantly tripped the limit in under a second, one taking longer than ~2s never tripped it and restarted forever. The per-container drop-in already carried both sections this needs, so the bound goes there rather than into a second file. StartLimit* are [Unit] settings; systemd silently ignores them under [Service], which the pinned body test now guards. The values are hive-wide constants rather than wire fields: the bound is policy, identical for every agent, and threading it through the socket as a per-agent parameter would be plumbing for a value nobody varies. Giving up is safe to make tight because it is not terminal -- reconcile retries later, and the reset-failed already done before each start clears the latch first.
This commit is contained in:
parent
1e9b924bc4
commit
ec476dfaae
1 changed files with 78 additions and 9 deletions
|
|
@ -741,14 +741,44 @@ fn write_resource_limits(
|
|||
Ok((String::new(), String::new()))
|
||||
}
|
||||
|
||||
/// How long a window the start-limit counts over, and how many starts it
|
||||
/// allows inside it.
|
||||
///
|
||||
/// `container@.service` sets `Restart=on-failure` and **no** start limit, so
|
||||
/// systemd's defaults apply: 5 starts per 10s, `RestartSec` 100ms. That
|
||||
/// makes the bound depend on *how fast* a container dies — one that fails
|
||||
/// instantly trips the limit in under a second, one that takes longer than
|
||||
/// ~2s never trips it and restarts forever. Whether an agent gets bounded
|
||||
/// is not meant to be a function of its failure speed.
|
||||
///
|
||||
/// The window has to exceed the worst-case time to burn the burst, or the
|
||||
/// counter ages out between attempts and the limit is again unreachable:
|
||||
/// `TimeoutStartSec` is 1min, so `BURST` slow failures plus their backoff
|
||||
/// can span several minutes. 10min covers that with room.
|
||||
///
|
||||
/// Giving up is cheap here **because it is not terminal** — hive-c0re's
|
||||
/// reconcile sweep retries later, and `reset-failed` (see `StartContainer`)
|
||||
/// clears the latch first. That is what makes a tight burst safe.
|
||||
const START_LIMIT_INTERVAL_SEC: u32 = 600;
|
||||
/// One start plus two retries — the operator's ruling was "retry once or
|
||||
/// twice", with the reconcile sweep as the slow path after that.
|
||||
const START_LIMIT_BURST: u32 = 3;
|
||||
/// Backoff between those retries. The 100ms default is for processes that
|
||||
/// respawn instantly; a container that just failed to boot gains nothing
|
||||
/// from being retried a tenth of a second later.
|
||||
const RESTART_SEC: u32 = 5;
|
||||
|
||||
/// Render the body of `hyperhive-limits.conf`.
|
||||
///
|
||||
/// `[Unit]`: the condition is checked at start time — it skips (not fails)
|
||||
/// the unit when the MCP socket dir is absent, avoiding restart loops.
|
||||
/// `[Service]`: the hard caps first, then the relative weights. A weight of
|
||||
/// `None` means "not configured" and omits its line entirely, so a request
|
||||
/// from a hive-c0re built before the weights existed — or one whose nix
|
||||
/// option is `null` — reproduces the old two-setting drop-in byte for byte.
|
||||
/// the unit when the MCP socket dir is absent, avoiding restart loops —
|
||||
/// plus the bounded start limit (see the constants above; `StartLimit*` are
|
||||
/// `[Unit]` settings since systemd 229 and are silently ignored under
|
||||
/// `[Service]`).
|
||||
/// `[Service]`: the restart backoff, the hard caps, then the relative
|
||||
/// weights. A weight of `None` means "not configured" and omits its line
|
||||
/// entirely, so a request from a hive-c0re built before the weights
|
||||
/// existed — or one whose nix option is `null` — renders no weight lines.
|
||||
fn limits_dropin_body(
|
||||
runtime_dir: &str,
|
||||
memory_max: &str,
|
||||
|
|
@ -764,8 +794,11 @@ fn limits_dropin_body(
|
|||
format!(
|
||||
"[Unit]\n\
|
||||
ConditionPathIsDirectory={runtime_dir}\n\
|
||||
StartLimitIntervalSec={START_LIMIT_INTERVAL_SEC}\n\
|
||||
StartLimitBurst={START_LIMIT_BURST}\n\
|
||||
\n\
|
||||
[Service]\n\
|
||||
RestartSec={RESTART_SEC}\n\
|
||||
MemoryMax={memory_max}\n\
|
||||
CPUQuota={cpu_quota}\n\
|
||||
{cpu_weight_line}{io_weight_line}"
|
||||
|
|
@ -2600,22 +2633,58 @@ mod tests {
|
|||
.expect("every ordinary request arrives without a descriptor");
|
||||
}
|
||||
|
||||
/// An unset weight is "not configured": the drop-in must come out
|
||||
/// byte-identical to the pre-weights two-setting body, so a hive-c0re
|
||||
/// older than this field can't change what lands on disk.
|
||||
/// The whole rendered body, pinned literally — including the values,
|
||||
/// so changing the restart policy is a visible test edit rather than a
|
||||
/// silent one.
|
||||
///
|
||||
/// Placement is the part most worth pinning: `StartLimit*` are `[Unit]`
|
||||
/// settings and systemd **silently ignores** them under `[Service]`, so
|
||||
/// a bound that moved sections would look configured and do nothing.
|
||||
/// An unset weight still emits no line at all, which is what keeps a
|
||||
/// hive-c0re older than that field from changing what lands on disk.
|
||||
#[test]
|
||||
fn unset_weights_reproduce_the_pre_weights_dropin() {
|
||||
fn dropin_body_is_pinned_exactly() {
|
||||
assert_eq!(
|
||||
limits_dropin_body("/run/hyperhive/agents/iris", "4G", "200%", None, None),
|
||||
"[Unit]\n\
|
||||
ConditionPathIsDirectory=/run/hyperhive/agents/iris\n\
|
||||
StartLimitIntervalSec=600\n\
|
||||
StartLimitBurst=3\n\
|
||||
\n\
|
||||
[Service]\n\
|
||||
RestartSec=5\n\
|
||||
MemoryMax=4G\n\
|
||||
CPUQuota=200%\n"
|
||||
);
|
||||
}
|
||||
|
||||
/// The bound is hive-wide **policy**, not a per-agent parameter: it is
|
||||
/// rendered from constants and no caller-supplied value can omit or
|
||||
/// alter it. This is the property that justifies keeping it out of the
|
||||
/// wire protocol — if it ever varies by request, that argument is gone.
|
||||
#[test]
|
||||
fn the_start_limit_is_present_whatever_the_caller_passes() {
|
||||
for (mem, cpu, cw, iw) in [
|
||||
("4G", "200%", None, None),
|
||||
("512M", "50%", Some(10), Some(10)),
|
||||
("infinity", "infinity", Some(10_000), None),
|
||||
] {
|
||||
let body = limits_dropin_body("/rt/x", mem, cpu, cw, iw);
|
||||
let unit = body
|
||||
.split("[Service]")
|
||||
.next()
|
||||
.expect("the body always has a [Unit] section before [Service]");
|
||||
assert!(
|
||||
unit.contains("StartLimitIntervalSec=600") && unit.contains("StartLimitBurst=3"),
|
||||
"start limit missing from [Unit] for ({mem}, {cpu}): {body}"
|
||||
);
|
||||
assert!(
|
||||
body.contains("\nRestartSec=5\n"),
|
||||
"restart backoff missing for ({mem}, {cpu}): {body}"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Weights are appended to the `[Service]` section, each omitted
|
||||
/// independently when `None`.
|
||||
#[test]
|
||||
|
|
|
|||
Loading…
Reference in a new issue