handle_batch: destroy correctness + provision seam + DNS write lock#64
Merged
Conversation
The per-host provision logic (provision + set jet_ssh_hostname/jet_ssh_user) was inlined in both handle_batch and async_handle_batch, with no injectable seam — so the provision outcome couldn't be exercised without real infrastructure. Extract it into a tested helper parameterized over the provision function, returning a tri-state ProvisionOutcome (Ready/Destroyed/Failed) so callers own control-flow policy. Pure addition: no caller is wired yet (landed separately so the behavior change is reviewable in isolation). Production passes ensure_host_provisioned; tests pass fakes. Co-Authored-By: Claude <noreply@anthropic.com>
…royed plays
In the sequential path, Ok(Destroyed) -> continue only skipped provisioning the
NEXT host; it never removed the destroyed host from the task pool. So the task
phase still targeted destroyed (now-gone) hosts -> "SSH timeout", then
"no hosts remaining" failed the play. An all-destroy run was therefore reported
as a failure even though it had done exactly what was asked.
Fix by routing handle_batch's provision through the provision_host_with seam and
applying the outcomes with a small policy:
- Ready -> stays in the task pool
- Destroyed -> removed from the pool via the new context.destroy_host (an
intentional lifecycle outcome: it does NOT count as a failed task)
- Failed -> abort the play (preserving the existing abort-on-any-failure
semantics)
If every host was intentionally destroyed, the play returns Ok without running
tasks. A genuinely empty play still falls through to the task phase, preserving
prior behavior.
Tests: context.destroy_host (remove-without-failing, no-op on unknown host),
apply_provision_outcomes (exclude-destroyed/keep-ready, all-destroyed empties
pool, abort-on-failure). Side fix: tests/playbooks/context.rs was orphaned (not
declared as a module) because of one stale Role-YAML test; fix the stale test and
wire the module in so the whole file runs in CI.
The handle_batch wiring composes these tested units; the per-host SSH readiness
behavior itself is verified manually against the test cluster.
Co-Authored-By: Claude <noreply@anthropic.com>
Parallel host provisioning (coming next) calls add_host_record / remove_host_record concurrently. The zone-file read-modify-write in zone.rs plus the octodns/gravity sync that follows it are not concurrency-safe: two hosts adding records to the same zone would lose updates and race the provider sync. Guard the five record-mutating entry points (add/remove_host_record, set_service_records, add_cname_alias, remove_record) with a LazyLock<Mutex> held for the whole body (zone edit + sync). The internally-called helpers add_ptr_record and sync deliberately take no lock: they are only reached through these entry points, and std Mutex is not reentrant (locking them would self-deadlock). Test: 16 threads each add a distinct host record into one tempdir zone; assert no thread panicked/deadlocked and every record survives. Catches both the lost-update race and a re-entrancy mistake. Co-Authored-By: Claude <noreply@anthropic.com>
Proxmox /cluster/nextid returns max+1 without reserving it, so two hosts provisioned in parallel on the same cluster can be handed the same VMID and collide at create time. Hold a process-wide VMID_LOCK across get_next_vmid → create_empty_vm when the VMID is auto-assigned. (Explicit vmid: host_vars skip the lock — no nextid call, no race.) VM creation is a single API call (~1s); PXE imaging (~90s) still runs in parallel afterward, so this serializes only the cheap part. A global lock is sufficient and simplest: the cross-cluster serialization it adds is negligible next to imaging, and test-k8s is a single cluster anyway. Pre-existing latent bug, exposed by the parallel provision in the next commit. Not unit-tested — the lock guards external Proxmox API calls; validated by the real test-k8s converge. Co-Authored-By: Claude <noreply@anthropic.com>
The sequential path provisioned hosts serially (one ensure_host_provisioned at
a time), so imaging N hosts took N × ~90s and — worse — operators disabled the
SSH-readiness wait (wait_for_host: false) to escape the slowness, which then
raced tasks against mid-PXE hosts ("No route to host").
Provision via the provision_host_with seam under rayon par_iter, so all hosts
image concurrently and each is individually gated on its own SSH readiness. The
provision outcomes then feed the existing apply_provision_outcomes policy
(destroy exclusion / abort-on-failure), unchanged.
Safe to parallelize now that the concurrent writes are locked: the DNS write
lock (ac247a6) guards zone-file mutations and the VMID lock (f6de7fe) guards
nextid+create. Mirrors the host-parallel provision already proven in
async_handle_batch.
Parallelism itself is not unit-tested (it can't be observed without sleeps,
which are forbidden); correctness rests on the tested seam + policy, and the
real test-k8s converge validates it end to end.
Co-Authored-By: Claude <noreply@anthropic.com>
The parallel provision phase emits only interleaved per-host "SSH available after Ns" lines; under a starvation repro there's no single view of which host is the straggler or how the parallel wall-clock breaks down. Time each host's provision_host_with call alongside the par_iter collect, then emit a one-line-per-host summary after apply_provision_outcomes: total parallel wall-clock, outcome counts (ready/destroyed/failed), per-host duration, and the slowest host flagged as the straggler (only when >1 host). This is the controller plane of the PXE-starvation harness — it shows, from Jetpack's view, exactly which host stalls and for how long. format_provision_summary is pure (no I/O) so it's unit-tested directly: counts + straggler-on-the-slowest, and no-straggler-for-a-single-host. Co-Authored-By: Claude <noreply@anthropic.com>
…provision)
A `provision:` block in group_vars/<group> now deep-merges onto each member
host's host_vars provision config, so a whole fleet's lifecycle can be toggled
from one file (e.g. the PXE-starvation harness flips test-k8s between
`state: destroyed` and `state: present`). Host-specific fields win on conflict,
matching every other variable.
Previously this silently didn't work: the provision config was parsed from
host_vars only and group vars were never re-derived into Host.provision (the
provisioner reads Host.provision, not the variable store). Two fixes:
1. Stop excluding the `provision` block from the host variable store at load
(loading.rs) — keeping the raw, default-free mapping lets the deep-blend
merge a group overlay onto it. The previous exclusion meant the blended
variables only ever held the group's provision (missing the host's required
type/cluster), so re-parsing failed and the overlay was dropped.
2. In propagate_group_vars_to_hosts, re-derive Host.provision from the resolved
provision mapping after the blend, so the provisioner sees the overlay.
(ProvisionConfig gains Serialize as a side effect — not used here, but it's
a reasonable capability for the config struct.)
Tests: group overlay merges onto host config (state: destroyed overlays a
host whose host_vars omit state); host field wins on conflict (host state:present
beats group state:destroyed). Full suite green (430). Documented in
docs/content/docs/inventory/_index.md.
Co-Authored-By: Claude <noreply@anthropic.com>
-e / --extra-vars now drive provisioning, not just templating. Previously
extra_vars flowed only into the context's templating blend
(get_complete_blended_variables), so `-e provision.state=destroyed` was visible
to `{{ provision.state }}` but invisible to the provisioner (which reads
Host.provision). Now propagate_group_vars_to_hosts blends extra_vars into each
host's variable store at the highest precedence (above group and host vars) and
re-derives Host.provision, so an ad-hoc CLI flag can drive a fleet's lifecycle
without touching inventory files — the natural toggle for the PXE-starvation
harness repro.
Also teach store_extra_vars the convenient key=value form (with dotted nesting):
`-e provision.state=destroyed` instead of `-e '{"provision":{"state":"destroyed"}}'`.
The value is taken as a string; JSON remains for typed values, and @file for
YAML. (extra_vars_from_key_value, alongside convert_json_vars.)
load_inventory gains an extra_vars parameter (threaded from the CLI parser /
library Config through main, main_new, cli/playbooks, api). Tests: extra_vars
overlay reaches Host.provision and beats host_vars; key=value dotted nesting,
equals-in-value, and malformed-input rejection. Full suite green (434).
Co-Authored-By: Claude <noreply@anthropic.com>
Document that -e provision.state=... reaches the provisioner (not just templating) and accepts key=value with dotted nesting — the ad-hoc complement to the persistent group_vars overlay. Co-Authored-By: Claude <noreply@anthropic.com>
…nventory-wide) The destroy-confirmation gate scanned the WHOLE inventory for provision.state absent/destroyed, ignoring which playbook (-p) and environment the operator specified. So `apply --environment test -p repro.yml -e provision.state=destroyed` prompted to DESTROY 16 hosts incl. prod k8s01-11, even though the actual run (handle_batch, play-scoped) only touches test-k8s (5). The comment hand-waved "can't account for play groups" — but at confirm time we have the playbook path and a loaded inventory. Fix: resolve the -p playbook's play-groups at confirm time (new resolve_playbook_targets, reusing resolve_target_groups/get_play_hosts) and intersect destroy_bearing_hosts with that target set. So the prompt names exactly what the run will destroy — no --limit needed, no training users to remember a flag to avoid nuking prod. Safe-direction preserved: resolve_playbook_targets returns None when there is no playbook, or any play's groups can't be resolved (read/parse error, templating failure, empty groups); destroy_bearing_hosts then falls back to the inventory-wide list. The prompt can never silently under-count a real destroy — only narrow when certain. Tests: playbook_targets_scope_the_destroy_prompt (Some narrows to the targeted destroy-bearing host; None = inventory-wide over-approximation). Full suite green. Co-Authored-By: Claude <noreply@anthropic.com>
A task templating `{{ inventory_hostname }}` failed strict-mode rendering
("Failed to access variable in strict mode Some(\"inventory_hostname\")"),
which surfaced running the pxe-starvation repro (`msg: provisioned
{{ inventory_hostname }}`). Jetpack only injected jet_hostname /
jet_hostname_short in Host::get_blended_variables and never aliased the
canonical Ansible magic-variable names, so every Ansible-familiar operator
hit the error by muscle memory.
Alias inventory_hostname -> full hostname and inventory_hostname_short ->
short hostname alongside the jet_* builtins in get_blended_variables (the
single injection site for host-name magic vars), and add both names to
RENDER_BUILTINS in secrets_diagnostic.rs so the missing-secret diagnostic
never flags them as unresolved.
Same "do not train the user wrong" principle as the play-scoped destroy
prompt: the canonical name must resolve, not be a footgun.
Test: magic_variables_alias_inventory_hostname asserts both aliases land in
the blended map AND that a strict-mode render of `{{ inventory_hostname }}`
against a host's blended variables (the exact task `msg:` path) resolves to
the hostname. Full suite green (436 lib + 275 integration).
Co-Authored-By: Claude <noreply@anthropic.com>
Reframe the in-code comment so the alias is explicitly a legacy Ansible-compat shim, not a first-class name — and record WHY it's honest so the decision isn't relitigated: - jet_hostname is the inventory identity, verbatim (Host::new stores the inventory key as self.name, unnormalized); the connection target is the separate jet_ssh_hostname. This is the exact Ansible split: inventory_hostname (identity) vs ansible_host (connection). - inventory_hostname_short and jet_hostname_short both mean "before the first dot". So Ansible's inventory_hostname means the same thing as jet_hostname; the alias gives muscle memory, not a semantic lie. jet_* stays canonical. Co-Authored-By: Claude <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.
What
Three commits fixing the sequential execution path (
fn handle_batch) and layinggroundwork for parallel imaging.
8fea22f— destroy correctness (the bug)A destroy run provisioned
Ok(Destroyed)→continue, which only skippedprovisioning the next host — it never removed the destroyed host from the task
pool. So the task phase still targeted gone hosts →
SSH … timeout, thenno hosts remainingfailed the play. An all-destroy run was reported as afailure despite doing exactly what was asked.
Fix: provision through a new testable seam and apply a small policy:
context.destroy_host(an intentionallifecycle outcome — does not count as a failed task)
A play that intentionally destroys all its hosts now returns
Ok.Validated: destroyed a 5-node test-k8s fleet cleanly — no tasks ran against
gone hosts, no spurious failure.
18d8e75—provision_host_withseamThe per-host provision logic was inlined in
handle_batch/async_handle_batchwith no injectable seam, so provision outcomes couldn't be tested without real
infrastructure. Extracted into a tested helper parameterized over the provision
function, returning
ProvisionOutcome { Ready, Destroyed, Failed }.ac247a6— DNS write lockThe zone-file read-modify-write in
zone.rs+ the octodns/gravity sync are notconcurrency-safe. Parallel host provisioning (coming next) would call
add_host_record/remove_host_recordconcurrently and lose updates. Guard thefive record-mutating entry points with a process-wide
LazyLock<Mutex>held forthe whole body. Internal helpers (
add_ptr_record,sync) take no lock — theyare only reached through the entry points, and
std::Mutexis not reentrant.Tests
provision_host_with: outcome mapping, SSH-var setting, check-mode /no-provision short-circuits (fake provision fn, no real infra).
context.destroy_host: removes from pool without failing; no-op on unknown host.apply_provision_outcomes: exclude-destroyed/keep-ready, all-destroyed emptiespool, abort-on-failure.
add_host_recordinto one zone — no lost records, no deadlock.tests/playbooks/context.rswas orphaned (not declared as a module)because of one stale
Role-YAML test; fixed the stale test and wired the modulein so the whole file runs in CI.
Full suite green (
cargo fmt --check,cargo clippy -- -W clippy::all,cargo test).What's next (this branch, follow-up commits)
proxmox_vm.rs— parallelensure_existswouldrace Proxmox's non-reserving
/cluster/nextid. test-k8s uses auto-vmids..iter()→.par_iter()), so imaging runsconcurrently and each host is gated on its own SSH readiness (the actual fix for
the
No route to hostrace, oncewait_for_host: falseis removed from thetest-k8s host_vars — that + the real converge need a greenlight).
🤖 Generated with Claude Code