hive-c0re: accept friendly MemoryMax size spellings, normalize for systemd
mara hit this directly: setting an agent's memory cap to "16GB" fails with 'invalid MemoryMax "16GB": expected a size such as "8G"...'. Confirmed directly against a running systemd 260 (systemd-run -p MemoryMax=<value>): systemd's own parser accepts a bare byte count, <digits>B, or <digits> plus exactly one uppercase K/M/G/T, and rejects both "16GB" (redundant B after a multiplier) and "16g" (lowercase) with "Invalid argument". So the prior rejection of "16GB" matched systemd, but is bad UX for input a human reasonably expects to work. validate_memory_max now returns the value to store (not just an ok/err verdict): it accepts 8gb/8Gb/8GB/8g/8G interchangeably and normalizes all of them to systemd's own 8G form before it is ever persisted or passed downstream. Also fixes the adjacent bug the same investigation turned up: the old validator incorrectly accepted lowercase (8g) even though systemd itself rejects it. Updated both call sites (server.rs, dashboard/lifecycle_ops.rs) to use the normalized return value. New test friendly_size_spellings_normalize_to_systemds_own_form; flipped the old "8GB should be rejected" assertion, which encoded the human-hostile behavior this fixes.
This commit is contained in:
parent
a704a844d1
commit
27bed74d42
3 changed files with 85 additions and 23 deletions
|
|
@ -260,18 +260,32 @@ pub fn validate_cpu_quota(value: &str) -> Result<(), String> {
|
||||||
))
|
))
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Validate a systemd `MemoryMax=` value: a byte count with an optional
|
/// Validate a systemd `MemoryMax=` value — a byte count with an optional
|
||||||
/// `K`/`M`/`G`/`T` suffix, a percentage of physical memory, or the
|
/// `K`/`M`/`G`/`T` suffix, a percentage of physical memory, or the literal
|
||||||
/// literal `infinity`.
|
/// `infinity` — and return the exact string to store and pass to systemd.
|
||||||
|
///
|
||||||
|
/// That's not always `value` itself: a size is accepted case-insensitively
|
||||||
|
/// and with an optional redundant trailing `B` (`8gb`, `8Gb`, `8GB`, `8g`,
|
||||||
|
/// `8G` are all the same size to a human), but only `8G` is what systemd's
|
||||||
|
/// own parser actually takes — confirmed directly against a running
|
||||||
|
/// systemd 260: `MemoryMax=8GB` and `MemoryMax=8g` both fail unit
|
||||||
|
/// activation with "Invalid argument", while `MemoryMax=8G` starts fine.
|
||||||
|
/// Accepting the friendly spellings and normalizing them here means every
|
||||||
|
/// caller downstream only ever sees the one form systemd accepts, instead
|
||||||
|
/// of everyone re-deriving that systemd is this particular about case and
|
||||||
|
/// the redundant `B`.
|
||||||
///
|
///
|
||||||
/// # Errors
|
/// # Errors
|
||||||
///
|
///
|
||||||
/// Returns a human-readable message naming the offending value and the
|
/// Returns a human-readable message naming the offending value and the
|
||||||
/// three accepted shapes. Same operator-facing contract as
|
/// three accepted shapes. Same operator-facing contract as
|
||||||
/// [`validate_cpu_quota`].
|
/// [`validate_cpu_quota`].
|
||||||
pub fn validate_memory_max(value: &str) -> Result<(), String> {
|
pub fn validate_memory_max(value: &str) -> Result<String, String> {
|
||||||
if value == "infinity" || is_percentage(value) || is_byte_size(value) {
|
if value == "infinity" || is_percentage(value) {
|
||||||
return Ok(());
|
return Ok(value.to_owned());
|
||||||
|
}
|
||||||
|
if let Some(normalized) = normalized_byte_size(value) {
|
||||||
|
return Ok(normalized);
|
||||||
}
|
}
|
||||||
Err(format!(
|
Err(format!(
|
||||||
"invalid MemoryMax {value:?}: expected a size such as \"8G\", a percentage \
|
"invalid MemoryMax {value:?}: expected a size such as \"8G\", a percentage \
|
||||||
|
|
@ -284,12 +298,32 @@ fn is_percentage(value: &str) -> bool {
|
||||||
value.strip_suffix('%').is_some_and(is_plain_number)
|
value.strip_suffix('%').is_some_and(is_plain_number)
|
||||||
}
|
}
|
||||||
|
|
||||||
/// A decimal number with an optional single-letter binary suffix.
|
/// A decimal number with an optional `K`/`M`/`G`/`T` suffix — accepted
|
||||||
fn is_byte_size(value: &str) -> bool {
|
/// case-insensitively and with an optional trailing `B` (`gb`/`Gb`/`GB`/`g`
|
||||||
let mantissa = value
|
/// are all read as `G`) — returned in the single uppercase-letter-no-
|
||||||
.strip_suffix(['K', 'M', 'G', 'T', 'k', 'm', 'g', 't'])
|
/// redundant-`B` form systemd itself accepts. See [`validate_memory_max`]
|
||||||
.unwrap_or(value);
|
/// for why. A trailing `B` with no multiplier before it (`1048576B`) is a
|
||||||
is_plain_number(mantissa)
|
/// bare byte count and kept as-is — systemd accepts `B` as a unit on its
|
||||||
|
/// own, unlike the redundant `KB`/`MB`/`GB`/`TB` combinations.
|
||||||
|
fn normalized_byte_size(value: &str) -> Option<String> {
|
||||||
|
if let Some(before_b) = value.strip_suffix(['B', 'b']) {
|
||||||
|
return match before_b.strip_suffix(['K', 'M', 'G', 'T', 'k', 'm', 'g', 't']) {
|
||||||
|
Some(mantissa) if is_plain_number(mantissa) => {
|
||||||
|
let unit = before_b[mantissa.len()..].to_ascii_uppercase();
|
||||||
|
Some(format!("{mantissa}{unit}"))
|
||||||
|
}
|
||||||
|
Some(_) => None,
|
||||||
|
None => is_plain_number(before_b).then(|| format!("{before_b}B")),
|
||||||
|
};
|
||||||
|
}
|
||||||
|
match value.strip_suffix(['K', 'M', 'G', 'T', 'k', 'm', 'g', 't']) {
|
||||||
|
Some(mantissa) if is_plain_number(mantissa) => {
|
||||||
|
let unit = value[mantissa.len()..].to_ascii_uppercase();
|
||||||
|
Some(format!("{mantissa}{unit}"))
|
||||||
|
}
|
||||||
|
Some(_) => None,
|
||||||
|
None => is_plain_number(value).then(|| value.to_owned()),
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Digits, optionally followed by a single `.` and more digits. Hand
|
/// Digits, optionally followed by a single `.` and more digits. Hand
|
||||||
|
|
@ -402,9 +436,31 @@ mod tests {
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// mara hit this directly: "16GB" reads as an obviously
|
||||||
|
/// valid size to a human, but systemd's own parser takes only the
|
||||||
|
/// single-letter form — confirmed against a running systemd 260,
|
||||||
|
/// `MemoryMax=16GB` and `MemoryMax=16g` both fail unit activation,
|
||||||
|
/// `MemoryMax=16G` doesn't. Accept the friendly spellings and normalize
|
||||||
|
/// them to what systemd actually wants, rather than rejecting input a
|
||||||
|
/// human reasonably expects to work.
|
||||||
|
#[test]
|
||||||
|
fn friendly_size_spellings_normalize_to_systemds_own_form() {
|
||||||
|
for (input, want) in [
|
||||||
|
("8GB", "8G"),
|
||||||
|
("8Gb", "8G"),
|
||||||
|
("8gb", "8G"),
|
||||||
|
("8g", "8G"),
|
||||||
|
("512mb", "512M"),
|
||||||
|
("1048576b", "1048576B"),
|
||||||
|
("1.5gb", "1.5G"),
|
||||||
|
] {
|
||||||
|
assert_eq!(validate_memory_max(input).as_deref(), Ok(want));
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn rejects_invalid_memory_maxes() {
|
fn rejects_invalid_memory_maxes() {
|
||||||
for v in ["", "8GB", "G", "abc", "-8G", "8 G", "Infinity", "8Gi"] {
|
for v in ["", "G", "abc", "-8G", "8 G", "Infinity", "8Gi", "8KG"] {
|
||||||
assert!(validate_memory_max(v).is_err(), "{v} should be rejected");
|
assert!(validate_memory_max(v).is_err(), "{v} should be rejected");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -376,14 +376,16 @@ pub(super) async fn post_resource_limits(
|
||||||
{
|
{
|
||||||
return (StatusCode::UNPROCESSABLE_ENTITY, e).into_response();
|
return (StatusCode::UNPROCESSABLE_ENTITY, e).into_response();
|
||||||
}
|
}
|
||||||
if let Some(v) = memory_max
|
// Returns the value to store, not just an ok/err verdict — see
|
||||||
&& let Err(e) = crate::resource_limits::validate_memory_max(v)
|
// `validate_memory_max`'s own doc for why that isn't always `v`.
|
||||||
{
|
let memory_max = match memory_max.map(crate::resource_limits::validate_memory_max) {
|
||||||
return (StatusCode::UNPROCESSABLE_ENTITY, e).into_response();
|
Some(Err(e)) => return (StatusCode::UNPROCESSABLE_ENTITY, e).into_response(),
|
||||||
}
|
Some(Ok(v)) => Some(v),
|
||||||
|
None => None,
|
||||||
|
};
|
||||||
let limits = crate::resource_limits::AgentLimits {
|
let limits = crate::resource_limits::AgentLimits {
|
||||||
cpu_quota: cpu_quota.map(str::to_owned),
|
cpu_quota: cpu_quota.map(str::to_owned),
|
||||||
memory_max: memory_max.map(str::to_owned),
|
memory_max,
|
||||||
};
|
};
|
||||||
if let Err(e) = crate::meta::commit_resource_limits(ident.as_str(), &limits).await {
|
if let Err(e) = crate::meta::commit_resource_limits(ident.as_str(), &limits).await {
|
||||||
return error_response(&format!("set limits {logical}: {e:#}"));
|
return error_response(&format!("set limits {logical}: {e:#}"));
|
||||||
|
|
|
||||||
|
|
@ -502,13 +502,17 @@ async fn handle_set_resource_limits(
|
||||||
if let Some(value) = cpu_quota {
|
if let Some(value) = cpu_quota {
|
||||||
crate::resource_limits::validate_cpu_quota(value).map_err(anyhow::Error::msg)?;
|
crate::resource_limits::validate_cpu_quota(value).map_err(anyhow::Error::msg)?;
|
||||||
}
|
}
|
||||||
if let Some(value) = memory_max {
|
// `validate_memory_max` returns the value to store, not just an
|
||||||
crate::resource_limits::validate_memory_max(value).map_err(anyhow::Error::msg)?;
|
// ok/err verdict: a friendly spelling like "8GB" is accepted but
|
||||||
}
|
// normalized to the "8G" systemd's own parser actually takes.
|
||||||
|
let memory_max = memory_max
|
||||||
|
.map(crate::resource_limits::validate_memory_max)
|
||||||
|
.transpose()
|
||||||
|
.map_err(anyhow::Error::msg)?;
|
||||||
tracing::info!(%name, ?cpu_quota, ?memory_max, "set_resource_limits");
|
tracing::info!(%name, ?cpu_quota, ?memory_max, "set_resource_limits");
|
||||||
let limits = crate::resource_limits::AgentLimits {
|
let limits = crate::resource_limits::AgentLimits {
|
||||||
cpu_quota: cpu_quota.map(ToOwned::to_owned),
|
cpu_quota: cpu_quota.map(ToOwned::to_owned),
|
||||||
memory_max: memory_max.map(ToOwned::to_owned),
|
memory_max,
|
||||||
};
|
};
|
||||||
// Goes through `meta::commit_resource_limits`, not the bare
|
// Goes through `meta::commit_resource_limits`, not the bare
|
||||||
// `resource_limits::set_limits`: the write has to be staged +
|
// `resource_limits::set_limits`: the write has to be staged +
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue