swarm-controller: make agent-creation's wanted-state write idempotent
Intent::Create now no-ops when the agent already has a non-Destroyed declaration, instead of unconditionally overwriting it to the create path's state. Without this, re-running create against a name that already has a wanted-state entry (an operator migrating a pre-existing agent into this bookkeeping, or a retried request) would silently pause an agent already running under some other state. write() skips the network round-trip entirely when apply returns the declaration unchanged. mara: this does not match the expectation that agent creation is idempotent so pre existing agents can be migrated
This commit is contained in:
parent
48aa7a1e79
commit
75af27abe0
1 changed files with 58 additions and 0 deletions
|
|
@ -127,6 +127,25 @@ fn apply(
|
||||||
}
|
}
|
||||||
.into());
|
.into());
|
||||||
}
|
}
|
||||||
|
// `Create` against a name that already has a *non-terminal* declaration
|
||||||
|
// is not a creation — it's the create-agent endpoint being asked to
|
||||||
|
// declare a name that turns out to already exist (a retried request, or
|
||||||
|
// an operator migrating a pre-existing agent into this bookkeeping for
|
||||||
|
// the first time). Creation is expected to be idempotent, so this is a
|
||||||
|
// no-op that leaves the existing declaration exactly as it stood rather
|
||||||
|
// than stomping it back to whatever state the create path passes —
|
||||||
|
// otherwise calling create against a name that is already `Up` would
|
||||||
|
// silently pause a running agent. `Destroyed` is excluded on purpose:
|
||||||
|
// that's the one case a fresh create *does* need to write over (see
|
||||||
|
// `Intent::Create`'s own doc comment).
|
||||||
|
if intent == Intent::Create
|
||||||
|
&& let Some(existing) = declaration.agents.get(agent)
|
||||||
|
&& existing.state != AgentState::Destroyed
|
||||||
|
{
|
||||||
|
let encoded =
|
||||||
|
serde_json::to_vec(&declaration).context("encoding the current declaration")?;
|
||||||
|
return Ok((declaration, encoded));
|
||||||
|
}
|
||||||
declaration
|
declaration
|
||||||
.agents
|
.agents
|
||||||
.insert(agent.to_owned(), AgentWanted { state });
|
.insert(agent.to_owned(), AgentWanted { state });
|
||||||
|
|
@ -201,6 +220,12 @@ impl WantedWriter {
|
||||||
/// the time this runs the swarm has already built that agent's identity,
|
/// the time this runs the swarm has already built that agent's identity,
|
||||||
/// repo and config from nothing.
|
/// repo and config from nothing.
|
||||||
///
|
///
|
||||||
|
/// Idempotent against any other existing declaration: an agent already
|
||||||
|
/// declared in a non-terminal state (an operator migrating a pre-existing
|
||||||
|
/// agent into this bookkeeping, or a retried request) is left exactly as
|
||||||
|
/// it stood — creation must not silently pause an agent already running
|
||||||
|
/// under some other wanted state.
|
||||||
|
///
|
||||||
/// A separate method rather than an `Intent` parameter on `set`: the
|
/// A separate method rather than an `Intent` parameter on `set`: the
|
||||||
/// caller that may overwrite a terminal state is the agent-creation job
|
/// caller that may overwrite a terminal state is the agent-creation job
|
||||||
/// node and nothing else, and an argument every other caller has to pass
|
/// node and nothing else, and an argument every other caller has to pass
|
||||||
|
|
@ -239,6 +264,15 @@ impl WantedWriter {
|
||||||
intent,
|
intent,
|
||||||
)?;
|
)?;
|
||||||
|
|
||||||
|
// `apply`'s `Intent::Create` no-op (an already-declared,
|
||||||
|
// non-terminal agent) re-encodes the declaration unchanged —
|
||||||
|
// skip the network round-trip for it entirely rather than
|
||||||
|
// writing identical bytes back.
|
||||||
|
if entry.as_ref().map(|e| e.value.as_ref()) == Some(encoded.as_slice()) {
|
||||||
|
tracing::debug!(hive, agent, "declaration already current, no write needed");
|
||||||
|
return Ok(declaration);
|
||||||
|
}
|
||||||
|
|
||||||
// `update` and `create` have separate error types, and only one
|
// `update` and `create` have separate error types, and only one
|
||||||
// variant of each means "someone else got there first". Every
|
// variant of each means "someone else got there first". Every
|
||||||
// other failure returns immediately: retrying a disconnect or a
|
// other failure returns immediately: retrying a disconnect or a
|
||||||
|
|
@ -402,6 +436,30 @@ mod tests {
|
||||||
assert_eq!(declaration.agents["iris"].state, AgentState::Destroyed);
|
assert_eq!(declaration.agents["iris"].state, AgentState::Destroyed);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// mara: "this does not match the expectation that agent creation is
|
||||||
|
/// idempotent so pre existing agents can be migrated" — an agent already
|
||||||
|
/// declared in some other, non-terminal state (say an operator migrating
|
||||||
|
/// a pre-existing agent into this bookkeeping) must not be stomped back
|
||||||
|
/// to whatever state the create path passes.
|
||||||
|
#[test]
|
||||||
|
fn creating_an_agent_does_not_disturb_an_existing_non_terminal_declaration() {
|
||||||
|
let current = br#"{"agents":{"atlas":{"state":"up"}}}"#;
|
||||||
|
let (declaration, _) =
|
||||||
|
apply(Some(current), "atlas", AgentState::Paused, Intent::Create).unwrap();
|
||||||
|
assert_eq!(declaration.agents["atlas"].state, AgentState::Up);
|
||||||
|
}
|
||||||
|
|
||||||
|
/// The idempotent no-op only inspects the agent being created — it must
|
||||||
|
/// not touch any other agent's declaration on the same hive.
|
||||||
|
#[test]
|
||||||
|
fn creating_an_agent_that_already_exists_leaves_every_other_agent_alone() {
|
||||||
|
let current = br#"{"agents":{"atlas":{"state":"up"},"iris":{"state":"offline"}}}"#;
|
||||||
|
let (declaration, _) =
|
||||||
|
apply(Some(current), "atlas", AgentState::Paused, Intent::Create).unwrap();
|
||||||
|
assert_eq!(declaration.agents["atlas"].state, AgentState::Up);
|
||||||
|
assert_eq!(declaration.agents["iris"].state, AgentState::Offline);
|
||||||
|
}
|
||||||
|
|
||||||
/// `Intent::Create` relaxes exactly one rule. An undecodable current
|
/// `Intent::Create` relaxes exactly one rule. An undecodable current
|
||||||
/// value is still an error, for the same reason it is on a redeclare:
|
/// value is still an error, for the same reason it is on a redeclare:
|
||||||
/// writing over it discards every other agent on the hive.
|
/// writing over it discards every other agent on the hive.
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue