Skip to content

Commit 23f6665

Browse files
committed
New error_chain() helper flattens the error source() chain. Idempotency sweep across the code base to make sure 409 tend to OK.
Signed-off-by: Erick Bourgeois <erick@jeb.ca>
1 parent a34abfa commit 23f6665

6 files changed

Lines changed: 334 additions & 15 deletions

File tree

.claude/CHANGELOG.md

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,103 @@ The format is based on the regulated environment requirements:
99

1010
---
1111

12+
## [2026-06-24 10:30] - Idempotency sweep: 409 AlreadyExists / duplicate finalizers no longer Error the ScheduledMachine
13+
14+
**Author:** Erick Bourgeois
15+
16+
### Changed
17+
- `src/reconcilers/helpers.rs` (`create_dynamic_resource`): treat a `409 AlreadyExists`
18+
from `api.create(...)` as success instead of propagating it. The bootstrap config,
19+
infrastructure object, and CAPI Machine are all created through this helper; a prior
20+
reconcile having created them IS the desired state.
21+
- `src/reconcilers/helpers.rs` (`add_finalizer`): only append our finalizer if it isn't
22+
already present — a double-call can no longer write a duplicate (which would wedge GC).
23+
- `src/reconcilers/helpers_tests.rs`: +3 unit tests (create 409 → Ok; create 500 still
24+
propagates; add_finalizer writes exactly one copy).
25+
- Audited the rest of the codebase for idempotency and confirmed it already holds: the
26+
only other `.create()` is a `SelfSubjectAccessReview` (a virtual auth check, never
27+
409s); all `.delete()` sites swallow 404; the kata ConfigMap uses `Patch::Apply` (SSA);
28+
Events go through the kube `recorder`; status/spec writes use `Patch::Merge`/`Apply`.
29+
30+
### Why
31+
On every reconcile after the first, `add_machine_to_cluster` re-issues the create and
32+
got `409 AlreadyExists` (e.g. `k0sworkerconfigs "…" already exists`). That bubbled up as
33+
`MachineCreationFailed`, flipping the ScheduledMachine into the **Error** phase — which
34+
skips ALL Active-phase work, including node-taint reconciliation. On the k0smotron
35+
workshop tier this presented as **Flag 1 (worker joins) passing but Flag 2 (the Node
36+
carries the spot taint) never happening**: the SM was wedged in Error, so it never ran
37+
`handle_active_phase`. Idempotent create is standard controller behavior and fixes it.
38+
39+
### Impact
40+
- [ ] Breaking change
41+
- [x] Requires cluster rollout
42+
- [ ] Config change only
43+
- [ ] Documentation only
44+
45+
## [2026-06-24 03:10] - Fix Mermaid flow diagrams broken by special characters
46+
47+
**Author:** Erick Bourgeois
48+
49+
### Changed
50+
- `docs/architecture/calm/architecture.json`: escaped Mermaid-hostile characters in
51+
three flow transition descriptions using Mermaid entity codes (which render as the
52+
literal characters):
53+
- `flow-emergency-reclaim` t1: `reclaim-requested="true"``reclaim-requested=#quot;true#quot;`.
54+
The embedded double quotes terminated the Mermaid label string early, causing a parse
55+
error that left the **Emergency Reclaim** diagram blank on the docs site.
56+
- `flow-kata-config-delivery` t2: `/etc/5spot/kata-config/<key>``…/#lt;key#gt;`.
57+
- `flow-kata-config-delivery` t5: `restart <restartService>``restart #lt;restartService#gt;`.
58+
Raw `<…>` placeholders were parsed as HTML tags and silently dropped from the rendered label.
59+
- `docs/src/architecture/flows.md`: regenerated via `make calm-diagrams` (auto-generated;
60+
not hand-edited).
61+
- `docs/src/concepts/kata-config-delivery.md`: hand-written `sequenceDiagram` — replaced a
62+
`;` with `,` inside a `Note over Agent,Host: …` label. Mermaid treats `;` as a statement
63+
separator even inside note text, so it split the note mid-string and failed the whole
64+
diagram to parse.
65+
66+
### Why
67+
The `flows.md.hbs` template emits transition descriptions verbatim into `flowchart` node
68+
labels (`t1["…"]`). Mermaid cannot contain an unescaped `"` inside a quoted label, and
69+
treats `<…>` as HTML — so these characters either broke parsing (Emergency Reclaim) or
70+
ate label text (Kata Config Delivery). Fixing at the source-of-truth `architecture.json`
71+
keeps the diagrams correct after every regeneration. The `phase -> …` arrows in other
72+
flows contain only `>` and render fine, so they were left unchanged.
73+
74+
### Impact
75+
- [ ] Breaking change
76+
- [ ] Requires cluster rollout
77+
- [ ] Config change only
78+
- [x] Documentation only
79+
80+
---
81+
82+
## [2026-06-24 02:30] - Surface the real cause of child-cluster connection failures (error_chain)
83+
84+
**Author:** Erick Bourgeois
85+
86+
### Changed
87+
- `src/reconcilers/helpers.rs`: new `error_chain(&dyn Error)` helper that flattens an
88+
error's `source()` chain into one string, and wired it into all six child-cluster
89+
failure logs (taint GET/PATCH/reconcile, kata clear/stamp). These previously logged
90+
only the terse top-level `Display` ("client error (Connect)"), hiding the real cause.
91+
- `src/reconcilers/helpers_tests.rs`: +2 unit tests for `error_chain` (multi-level
92+
source chain; single error with no source).
93+
94+
### Why
95+
A k0smotron-hosted control plane (workshop k0smotron tier) exposes a self-signed API
96+
server cert that **rustls** (5-Spot's TLS backend, kube 3.1 default) rejects where
97+
OpenSSL/curl `-k` accept it — so 5-Spot's child client fails to taint/drain the worker
98+
Node with `client error (Connect)`, with no visible cause. Logging the full chain makes
99+
the exact rustls reason (unknown issuer / SAN mismatch / unsupported signature) visible
100+
so the fix can target it (a proper cert, `tls_server_name`, or `insecure-skip-tls-verify`
101+
on the child kubeconfig — kube already honors the latter via `Config::accept_invalid_certs`).
102+
103+
### Impact
104+
- [ ] Breaking change
105+
- [ ] Requires cluster rollout
106+
- [ ] Config change only
107+
- [x] Diagnostics only (no behavior change)
108+
12109
## [2026-06-23 11:45] - Ship the spot-schedule provider binaries in the image (ADR 0009 providers were unrunnable)
13110

14111
**Author:** Erick Bourgeois

Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

docs/architecture/calm/architecture.json

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -832,7 +832,7 @@
832832
{
833833
"sequence-number": 1,
834834
"relationship-unique-id": "rel-reclaim-agent-workload-kube-api",
835-
"description": "Agent detects a process matching spec.killIfCommands via /proc scan and PATCHes the reclaim annotation triple (5spot.finos.org/reclaim-requested=\"true\", /reclaim-reason, /reclaim-requested-at) onto its own Node via the kubelet node-scoped token. Field manager: 5spot-reclaim-agent."
835+
"description": "Agent detects a process matching spec.killIfCommands via /proc scan and PATCHes the reclaim annotation triple (5spot.finos.org/reclaim-requested=#quot;true#quot;, /reclaim-reason, /reclaim-requested-at) onto its own Node via the kubelet node-scoped token. Field manager: 5spot-reclaim-agent."
836836
},
837837
{
838838
"sequence-number": 2,
@@ -895,7 +895,7 @@
895895
{
896896
"sequence-number": 2,
897897
"relationship-unique-id": "rel-kata-agent-workload-kube-api",
898-
"description": "The kata config agent DaemonSet (scheduled onto the node by the opt-in label) watches its per-node ConfigMap and reads the projected drop-in content at /etc/5spot/kata-config/<key>.",
898+
"description": "The kata config agent DaemonSet (scheduled onto the node by the opt-in label) watches its per-node ConfigMap and reads the projected drop-in content at /etc/5spot/kata-config/#lt;key#gt;.",
899899
"direction": "destination-to-source"
900900
},
901901
{
@@ -911,7 +911,7 @@
911911
{
912912
"sequence-number": 5,
913913
"relationship-unique-id": "rel-kata-agent-writes-host",
914-
"description": "Agent runs `nsenter -t 1 -m -u -i -n -p -- systemctl restart <restartService>` to restart the host k0s service; containerd reloads the new drop-in. The restart SIGKILLs the agent pod; systemd still completes the D-Bus job. Kubelet restarts the agent; on the next loop hashes match the applied annotation, so it is a no-op. Single-cycle convergence."
914+
"description": "Agent runs `nsenter -t 1 -m -u -i -n -p -- systemctl restart #lt;restartService#gt;` to restart the host k0s service; containerd reloads the new drop-in. The restart SIGKILLs the agent pod; systemd still completes the D-Bus job. Kubelet restarts the agent; on the next loop hashes match the applied annotation, so it is a no-op. Single-cycle convergence."
915915
}
916916
],
917917
"controls": {

docs/src/concepts/kata-config-delivery.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ sequenceDiagram
8686
Note over Agent: Applied record written BEFORE<br/>the restart — the restart-loop guard
8787
Agent->>Host: nsenter -t 1 -m -u -i -n -p --<br/>systemctl restart k0sworker.service
8888
Host-->>Agent: containerd bounces → pod SIGKILLed
89-
Note over Agent,Host: kubelet restarts the agent;<br/>next tick: hash matches → no-op.<br/>Single-cycle convergence.
89+
Note over Agent,Host: kubelet restarts the agent,<br/>next tick: hash matches → no-op.<br/>Single-cycle convergence.
9090
else hashes match
9191
Agent->>Agent: no-op (sleep 30s)
9292
end

src/reconcilers/helpers.rs

Lines changed: 50 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,12 @@ pub async fn add_finalizer(
180180
let api: Api<ScheduledMachine> = Api::namespaced(ctx.client.clone(), &namespace);
181181

182182
let mut finalizers = resource.meta().finalizers.clone().unwrap_or_default();
183-
finalizers.push(FINALIZER_SCHEDULED_MACHINE.to_string());
183+
// Idempotent: only add ours if it is not already present, so a double-call (or a
184+
// caller that forgets the has_finalizer() guard) can never write a duplicate
185+
// finalizer — which would otherwise wedge deletion (the GC waits for every copy).
186+
if !finalizers.iter().any(|f| f == FINALIZER_SCHEDULED_MACHINE) {
187+
finalizers.push(FINALIZER_SCHEDULED_MACHINE.to_string());
188+
}
184189

185190
let patch = json!({
186191
"metadata": {
@@ -1557,9 +1562,24 @@ async fn create_dynamic_resource(
15571562
let dyn_obj: kube::core::DynamicObject =
15581563
serde_json::from_value(obj).map_err(kube::Error::SerdeError)?;
15591564

1560-
api.create(&kube::api::PostParams::default(), &dyn_obj)
1561-
.await?;
1562-
Ok(())
1565+
match api
1566+
.create(&kube::api::PostParams::default(), &dyn_obj)
1567+
.await
1568+
{
1569+
Ok(_) => Ok(()),
1570+
// Idempotent reconciliation: a prior reconcile already created this object
1571+
// (bootstrap / infrastructure / Machine), which IS the desired state. Treat
1572+
// 409 AlreadyExists as success — otherwise every re-reconcile after the first
1573+
// create fails the ScheduledMachine into the Error phase, which then skips all
1574+
// Active-phase work (node-taint reconciliation, status projection, …). This
1575+
// surfaced on the k0smotron tier as Flag 1 (worker joins) passing but Flag 2
1576+
// (node carries the spot taint) never happening.
1577+
Err(kube::Error::Api(ae)) if ae.code == 409 => {
1578+
debug!(kind = %kind, "resource already exists; treating create as idempotent success");
1579+
Ok(())
1580+
}
1581+
Err(e) => Err(e),
1582+
}
15631583
}
15641584

15651585
/// Split a Kubernetes `apiVersion` string into `(group, version)`.
@@ -3139,7 +3159,7 @@ pub async fn reconcile_kata_config_delivery(
31393159
)
31403160
.await
31413161
.map_err(|e| {
3142-
error!(node = %node_name, error = %e, "Failed to clear kata-config reference annotation");
3162+
error!(node = %node_name, error = %e, cause = %error_chain(&e), "Failed to clear kata-config reference annotation");
31433163
ReconcilerError::KubeError(e)
31443164
})?;
31453165
return Ok(KataDeliveryOutcome::Disabled);
@@ -3169,7 +3189,7 @@ pub async fn reconcile_kata_config_delivery(
31693189
.patch(node_name, &PatchParams::default(), &Patch::Merge(&patch))
31703190
.await
31713191
.map_err(|e| {
3172-
error!(node = %node_name, error = %e, "Failed to stamp kata-config Node opt-in");
3192+
error!(node = %node_name, error = %e, cause = %error_chain(&e), "Failed to stamp kata-config Node opt-in");
31733193
ReconcilerError::KubeError(e)
31743194
})?;
31753195
debug!(
@@ -3199,7 +3219,7 @@ pub async fn reconcile_kata_config_delivery(
31993219
)
32003220
.await
32013221
.map_err(|e| {
3202-
error!(node = %node_name, error = %e, "Failed to clear kata-config reference annotation");
3222+
error!(node = %node_name, error = %e, cause = %error_chain(&e), "Failed to clear kata-config reference annotation");
32033223
ReconcilerError::KubeError(e)
32043224
})?;
32053225

@@ -3354,6 +3374,26 @@ pub fn diff_node_taints(
33543374
plan
33553375
}
33563376

3377+
/// Flatten an error's [`source`](std::error::Error::source) chain into one string.
3378+
///
3379+
/// kube/hyper surface child-cluster connection failures with a terse top-level
3380+
/// `Display` ("client error (Connect)") that hides the real cause — most often a
3381+
/// rustls certificate rejection (unknown issuer, name/SAN mismatch, unsupported
3382+
/// signature scheme) when talking to a hosted control plane whose self-signed cert
3383+
/// OpenSSL/curl would accept but rustls (stricter) will not. Logging this chain at
3384+
/// the failure site makes that cause visible instead of guessing.
3385+
#[must_use]
3386+
pub fn error_chain(err: &dyn std::error::Error) -> String {
3387+
let mut out = err.to_string();
3388+
let mut source = err.source();
3389+
while let Some(cause) = source {
3390+
out.push_str(" -> ");
3391+
out.push_str(&cause.to_string());
3392+
source = cause.source();
3393+
}
3394+
out
3395+
}
3396+
33573397
/// Apply a [`NodeTaintPlan`] to the named Node via a single server-side apply
33583398
/// PATCH, and write the `5spot.finos.org/applied-taints` ownership annotation.
33593399
///
@@ -3383,7 +3423,7 @@ pub async fn apply_node_taints(
33833423

33843424
let nodes: Api<Node> = Api::all(client.clone());
33853425
let current_node = nodes.get(node_name).await.map_err(|e| {
3386-
error!(node = %node_name, error = %e, "Failed to GET Node for taint apply");
3426+
error!(node = %node_name, error = %e, cause = %error_chain(&e), "Failed to GET Node for taint apply");
33873427
ReconcilerError::KubeError(e)
33883428
})?;
33893429

@@ -3460,7 +3500,7 @@ pub async fn apply_node_taints(
34603500
.patch(node_name, &params, &Patch::Apply(&patch))
34613501
.await
34623502
.map_err(|e| {
3463-
error!(node = %node_name, error = %e, "Failed to PATCH Node taints");
3503+
error!(node = %node_name, error = %e, cause = %error_chain(&e), "Failed to PATCH Node taints");
34643504
ReconcilerError::KubeError(e)
34653505
})?;
34663506

@@ -3539,7 +3579,7 @@ pub async fn reconcile_node_taints(
35393579
return Ok(NodeTaintReconcileOutcome::NoNodeYet);
35403580
}
35413581
Err(e) => {
3542-
error!(node = %input.node_name, error = %e, "Failed to GET Node for taint reconcile");
3582+
error!(node = %input.node_name, error = %e, cause = %error_chain(&e), "Failed to GET Node for taint reconcile");
35433583
return Err(ReconcilerError::KubeError(e));
35443584
}
35453585
};

0 commit comments

Comments
 (0)