124 · T109 — orphan adoption, tombstones, and ownership revocation - #134
Merged
Merged
Conversation
…ip revocation
Two suites, written before the mechanism, one per half of the split ADR 004
imposes:
* crates/k8s/tests/pod_adoption.rs — the pure decisions: the discovery
selector and the completion key it excludes, the three build surfaces a
refusal names, the three label patches (each writing exactly one key), the
deterministic resolution when several pods claim one attempt, and the
reading that tells a revoked pod from an externally deleted one.
* crates/cli/tests/orphan_adoption_and_ownership.rs — the acts: a labels-only
adoption compared field by field against the pod before it, a consumed
outcome tombstoned with the key a later discovery excludes, revocation
asserted as an ORDERING (patch then delete) rather than an end state,
composition with resume, and a kill-and-restart that finishes the run with
the node executed exactly once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… as values
`dagr_k8s::adoption` is the pure half: the discovery selector (this run's pods
minus the ones carrying the completion tombstone), the three build surfaces a
refusal names with BOTH values, the three labels-only patches, the deterministic
resolution when several pods claim one attempt, and the reading that tells a
revoked pod from an externally deleted one.
Two changes beyond the new module, each for a reason a test names:
* `PodLifecycle` gains `patch_labels` — the one write adoption needs, and the
reason it is safe: a running pod is reclaimed by moving a label, never by
recreating the object. `KubePodApi` implements it as a merge patch scoped to
`metadata.labels`, because only a merge patch can send the JSON `null` that
REMOVES a label, which is what revocation does. That is why `serde_json`
becomes an optional dependency behind the existing `client` quarantine.
* `RunSelector::label_selector` now IS `adoption_selector`, and
`ObserverCore::observe` skips a tombstoned pod client-side as well. Not
tidiness: the observer retires an attempt key once a final observation lands,
so on a restart an already-consumed pod's terminal phase would retire the
waiter registered for the NEW pod of the same attempt.
The shipped RBAC manifest is deliberately untouched — it grants get/list/watch,
its own test fails if a write verb appears, and provisioning create/delete/patch
is T112's.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…at uses it
`dagr_cli::adoption::discover` is one pass before any node is submitted: list
through the read port, patch the owner through the write port, revoke the
duplicates, report everything it did not touch. `K8sNodeRunner::with_adoptions`
is how a runner consults it, and the point is that there is ONE code path after
adoption rather than two — a claimed attempt takes the same await-read-replay
route a freshly created pod takes.
Three acts in the runner are about ownership rather than execution:
* a claim that is a REFUSAL fails the node with `AdoptionRefused`, naming the
pod and both disagreeing values; the pod is left running, because deleting
another program's work is not dagr's call;
* a consumed outcome is tombstoned, so the next discovery excludes it — and a
tombstoned pod squatting on the attempt's own object name is revoked rather
than adopted, which is the case the tombstone exists for;
* every orchestrator-initiated delete (cancel, timeout, pre-start failure) is
now the two-step revocation, so a watcher can recognise dagr's teardown.
Also: the await loop now filters observations to the object it is waiting on.
Several pods can carry one attempt key, the waiter is keyed by the attempt, and
without the filter a revoked duplicate's disappearance would decide the adopted
pod's fate.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ticket's own two (same-run-id adoption, and what to do with an un-adoptable foreign pod) are confirmed as proposed, with the consequence of the first stated rather than hidden: a resumed run revokes its parent's still-live pods instead of adopting them, because resume mints a new run id. Seven more the implementation had to decide and the ticket did not name: where the patch verb lives and why its RBAC is T112's, the tie-break when several pods claim one attempt, why a foreign pod alongside one of ours reports rather than fails, why discovery ships as a library seam, what happens to a tombstoned pod on the attempt's own name, why the run's watch now excludes consumed pods, and what a failed ownership patch degrades to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`plan` builds its candidate and refusal lists by inserting into a map, so neither
can be empty when it is read back — but stating that with `expect` gives the
function a documented panic it can never take. A `let … else { continue }` says
the same thing with no panic to document and none to hit.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four exhaustive match arms in place of wildcards over a two-variant enum, three bindings renamed out of `similar_names`'s way, the world's temp root no longer underscore-prefixed now that a test reads it, and one redundant explicit doc link dropped — `LABEL_OWNER` is in scope, so the label resolves on its own. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The workspace runs two deliberate philosophies and asserts that every production lock site names which one it takes. This registry recovers: the map behind it is decisions a startup pass already finished making, so a panic elsewhere cannot leave it half-updated, and escalating would turn one node's panic into "no other node can find the pod it was told to adopt" — the duplicate-execution outcome the module exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Ticket: T109 — docs/implementation/124-T109-orphan-adoption-and-ownership.md
Summary
Lets one orchestrator process reclaim work it started. Startup discovers the run's pods
by label (excluding tombstoned ones), adopts the ones that still match by patching
only the owner label — never recreating the pod — and revokes the rest by clearing
the owner label then deleting, in that asserted order. Consumed outcomes are
tombstoned so they are never adopted or re-run. An adopted pod's terminal transition
and shard flow through T108's path unchanged, so
seqstays gapless and the streamfolds. Decisions live in a pure
dagr_k8s::adoptionmodule; the startup pass isdagr_cli::adoption::discover.Tests-first
Confirmed — failing tests committed first in a9c8d0c.
Definition of done
seqstays gapless and the stream folds.satisfied-from-priornodes seek no pod; resume refusals are unchanged.ubuntu-latestandmacos-latest— pending, confirmed by this PR's CI (fake API surface; the real cluster kill-restart is T112).GATE=PASS(36 checks), verified on the committed tree, but CI on the PR is the authoritative verdict.Open questions resolved
The ticket's two are confirmed as proposed, plus seven the implementation had to
decide — all recorded in the ticket's
## Open questions — resolvedsection(
docs/tasks.mdenumerates M0–M4 only and carries no T109 entry; the dagx prior-artrouting table routes no section here):
prior_run_id.PodLifecycle::patch_labels; the grant is T112's.Deviations
None.
Notes
RBAC is deliberately untouched. The manifest still grants exactly
get/list/watchon pods in one namespace, while this ticket's adoption path patches labels and its
revocation path deletes. That is not a gap introduced here: ticket 127 (T112) explicitly
owns shipping the complete least-privilege grant — create, get, list, watch, delete, and
patch — and requires a clear failure when a verb is missing. T109's own DoD makes no RBAC
claim, and resolution 3 records the deferral.
Two changes reach slightly past the literal ticket text, both for correctness:
RunSelector::label_selectornow isadoption_selector— without it, a consumed pod'sterminal phase would retire the waiter belonging to the new pod of the same attempt
(resolution 8) — and the runner's await loop now filters observations to the object it
launched or adopted.
PodLifecyclegainedpatch_labels(a merge patch, wherenullremoves a key), withserde_jsonadded as an optional dependency behind the existingclientfeaturequarantine. The runtime-placement boundary holds:
dagr-k8sstill carries no tokio orother async-runtime edge,
dagr-coreremains zero-runtime-dependency, and noscripts/check-*.shwas modified — no allowlist widened, no assertion weakened.One verification caveat: the gate's own
cargo test --workspaceleg is a personallyobserved pass, but a separate
cargo test --workspace --all-featuresrun was killedafter 90 minutes inside
coverage_matrixand is not claimed as verified. Thefeature legs were instead checked individually — clippy under
--features k8s,dagr-k8s --features client, and--features blob; a--no-default-featuresbuild;check-feature-matrix.sh; andRUSTDOCFLAGS='-D warnings' cargo doc --workspace --no-deps— all green. CI covers the remainder.