Skip to content

Commit 9e5f02a

Browse files
mattia-eleuterimattia-eleuteri
committed
fix(kubernetes): assemble the node group once in kubernetes-nodes
The TalosConfigTemplate name is a hash of the machine-config spec, and two templates have to agree on it: talos-reconcile-job.yaml creates the object, nodegroup.yaml points MachineDeployment.bootstrap.configRef at it. Sharing the rendering was not enough — each file also assembled its own $group dict, one with ten keys and one with four, so the two names agreed only by coincidence of the keys the spec helper happens to read. Dropping "gpus" from the Job's dict diverged them (ffdbe1 vs 0bbd2a on a GPU pool) with the whole suite still green, which is the "templates do not exist" deadlock this fix exists to make impossible. Move the assembly into a single kubernetes-nodes.group helper, read back with fromYaml at both call sites, so there is one input as well as one rendering. Cover it too, since the seam can be reintroduced. The chart's helm-unittest suites pin each name against its own literal in its own file, which catches a divergence but reports it as one literal having moved — indistinguishable from a stale fixture, so re-pasting the new value is the obvious and wrong repair. hack/check-talosconfigtemplate-name-agreement.bats renders the chart once and compares the two values against each other, so the failure states the invariant instead of a number. It asserts on a GPU-shaped pool as well as the default one: the default renders gpus [], kubelet {} and instanceType "" as falsy, so a consumer reading a subset of the group is indistinguishable there from one reading all of it. A third case pins that the two shapes hash differently, so the first two cannot hold vacuously. Document that talos.registryMirrors now sits inside the hashed spec, so changing it rolls the pool's existing workers instead of only affecting nodes created later. No rendered output changes on default values: 84 + 150 helm unittests pass and the golden hashes are unchanged. Signed-off-by: Mattia Eleuteri <[email protected]> Assisted-by: LLM
1 parent 03c32e9 commit 9e5f02a

13 files changed

Lines changed: 253 additions & 46 deletions

File tree

‎api/apps/v1alpha1/kubernetesnodes/types.go‎

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

‎hack/check-talosconfigtemplate-name-agreement.bats‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
#!/usr/bin/env bats
2+
# -----------------------------------------------------------------------------
3+
# The TalosConfigTemplate is named after a content hash of the worker machine
4+
# config, and two templates in kubernetes-nodes have to arrive at that name
5+
# independently:
6+
#
7+
# * talos-reconcile-job.yaml creates the object, passing the name to the Job
8+
# as TCT_NAME;
9+
# * nodegroup.yaml points MachineDeployment.spec.template.spec.bootstrap
10+
# .configRef.name at it.
11+
#
12+
# If they disagree, the MachineDeployment references a TalosConfigTemplate that
13+
# is never created. CAPI does not fail: it blocks on "templates do not exist"
14+
# and no worker is ever built, so the divergence surfaces as a cluster that
15+
# quietly never scales rather than as a render error.
16+
#
17+
# The chart's helm-unittest suites pin both names, but each pins its own
18+
# literal in its own file. That catches a divergence, yet the output only says
19+
# one literal moved -- which reads as a stale fixture, and re-pasting the new
20+
# value is then the obvious and wrong fix. This guard renders the chart ONCE and
21+
# compares the two values against each other, so the failure states the
22+
# invariant instead of a number.
23+
#
24+
# Rendered with explicit resources rather than an instanceType: an
25+
# instanceType-sized pool resolves its kubelet reservations through a live
26+
# `lookup` of the VirtualMachineClusterInstancetype, which is empty outside a
27+
# cluster. The invariant under test does not depend on which sizing source is
28+
# used -- both consumers read the same assembled group either way.
29+
#
30+
# Compatible with both `bats` directly and the in-repo cozytest.sh runner, which
31+
# runs each @test in a fresh subshell under `set -u` and honors no setup() or
32+
# teardown(), so TMP is provisioned inline per test and cleaned without an EXIT
33+
# trap (docs/agents/e2e-testing.md bans those here).
34+
35+
REPO_ROOT="$(cd "$(dirname "${BATS_TEST_FILENAME:-$0}")/.." && pwd)"
36+
CHART="$REPO_ROOT/packages/apps/kubernetes-nodes"
37+
38+
render_pool() {
39+
# $1: output file, remaining args: extra --set flags for the pool shape
40+
out="$1"; shift
41+
helm template kubernetes-nodes-myk8s-md0 "$CHART" \
42+
--namespace tenant-test \
43+
--set cluster=myk8s \
44+
--set _cluster.cluster-domain=cozy.local \
45+
--set version=v1.35 \
46+
--set minReplicas=0 --set maxReplicas=3 \
47+
--set instanceType="" \
48+
--set diskSize=20Gi --set storageClass=replicated \
49+
--set 'roles[0]=ingress-nginx' \
50+
--set resources.cpu=2 --set resources.memory=4Gi \
51+
"$@" > "$out" 2>"$out.err"
52+
[ -s "$out" ] || { cat "$out.err" >&2; return 1; }
53+
}
54+
55+
job_tct_name() {
56+
awk '/name: TCT_NAME/ { getline; sub(/^[[:space:]]*value:[[:space:]]*/, ""); gsub(/"/, ""); print; exit }' "$1"
57+
}
58+
59+
md_config_ref_name() {
60+
awk '/configRef:/ { f=1; next } f && /name:/ { sub(/^[[:space:]]*name:[[:space:]]*/, ""); print; exit }' "$1"
61+
}
62+
63+
assert_agreement() {
64+
tmp="$1"; shape="$2"
65+
job=$(job_tct_name "$tmp/r.yaml")
66+
md=$(md_config_ref_name "$tmp/r.yaml")
67+
68+
# An empty capture means the render stopped emitting one of the two fields,
69+
# which would make a naive string comparison pass on "" == "".
70+
if [ -z "$job" ] || [ -z "$md" ]; then
71+
echo "FAIL: could not read both names out of the $shape render."
72+
echo " Job TCT_NAME: '${job:-<missing>}'"
73+
echo " MachineDeployment ref: '${md:-<missing>}'"
74+
echo "Either the field moved or the render is incomplete; this guard cannot"
75+
echo "compare what it cannot find, so it fails rather than pass vacuously."
76+
return 1
77+
fi
78+
79+
if [ "$job" != "$md" ]; then
80+
echo "FAIL: the two TalosConfigTemplate names must be equal, and are not ($shape pool)."
81+
echo " talos-reconcile-job.yaml creates: $job"
82+
echo " nodegroup.yaml references: $md"
83+
echo "The MachineDeployment now points at a TalosConfigTemplate nothing creates."
84+
echo "CAPI blocks on \"templates do not exist\" and builds no worker, without"
85+
echo "failing the release. Both names come from a hash of the group assembled"
86+
echo "by kubernetes-nodes.group in templates/_helpers.tpl: they diverge when a"
87+
echo "call site stops feeding that helper, or when a key it carries is dropped."
88+
echo "Fix the input they share; do not re-pin either literal."
89+
return 1
90+
fi
91+
}
92+
93+
@test "Job and MachineDeployment agree on the TalosConfigTemplate name" {
94+
TMP=$(mktemp -d)
95+
render_pool "$TMP/r.yaml"
96+
assert_agreement "$TMP" "default"
97+
rm -rf "$TMP"
98+
}
99+
100+
@test "they still agree once the hashed spec changes (GPU pool)" {
101+
# A GPU pool renders extra nodeLabels into the machine config, so the hash --
102+
# and therefore both names -- must move together. Pinning the agreement on a
103+
# second shape is what catches a call site that reads a subset of the group:
104+
# the default shape renders gpus [], kubelet {} and instanceType "" as falsy,
105+
# so a consumer missing those keys is indistinguishable from one carrying them.
106+
TMP=$(mktemp -d)
107+
render_pool "$TMP/r.yaml" --set 'gpus[0].name=nvidia.com/GH200'
108+
assert_agreement "$TMP" "GPU"
109+
rm -rf "$TMP"
110+
}
111+
112+
@test "the two shapes hash differently" {
113+
# Guards the guard: if both renders collapsed to the same name, the agreement
114+
# assertions above would hold trivially and stop proving anything.
115+
TMP=$(mktemp -d)
116+
render_pool "$TMP/plain.yaml"
117+
render_pool "$TMP/gpu.yaml" --set 'gpus[0].name=nvidia.com/GH200'
118+
plain=$(job_tct_name "$TMP/plain.yaml")
119+
gpu=$(job_tct_name "$TMP/gpu.yaml")
120+
if [ "$plain" = "$gpu" ]; then
121+
echo "FAIL: the plain and GPU pools hashed to the same name ($plain)."
122+
echo "The content hash is supposed to track the rendered spec, and a GPU pool"
123+
echo "renders different nodeLabels. Equal names mean the hash input no longer"
124+
echo "covers the field that changed."
125+
false
126+
fi
127+
rm -rf "$TMP"
128+
}

‎packages/apps/kubernetes-nodes/Makefile‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,18 +10,17 @@ generate:
1010
../../../hack/update-crd.sh
1111

1212
# Keep the worker-image inputs (Talos<->Kubernetes support matrix and the
13-
# kubectl.tag used by the talos-reconcile Job) and the worker machine-config
14-
# template in lockstep with the parent kubernetes chart. They are copied, not
15-
# symlinked, so `helm package` bundles them; run this after the parent chart's
16-
# `make update`.
13+
# kubectl.tag used by the talos-reconcile Job) in lockstep with the parent
14+
# kubernetes chart. They are copied, not symlinked, so `helm package` bundles
15+
# them; run this after the parent chart's `make update`.
1716
#
18-
# _talosconfigtemplate.tpl must stay byte-identical to the parent's copy: the
19-
# TalosConfigTemplate it renders is content-hash named, so any divergence would
20-
# point this pool's MachineDeployment at a different bootstrap template than the
21-
# parent rendered for the same group and roll every live worker. tests/render-
22-
# parity.sh compares the MachineDeployment across both charts and fails on it.
17+
# _talosconfigtemplate.tpl is not copied from anywhere: the parent chart no
18+
# longer carries worker node pools, so this chart owns the worker machine
19+
# config outright. It stays here because the TalosConfigTemplate it renders is
20+
# content-hash named and both consumers in this chart - nodegroup.yaml, which
21+
# writes bootstrap.configRef, and talos-reconcile-job.yaml, which creates the
22+
# object - have to hash the same bytes.
2323
update:
2424
cp ../kubernetes/files/versions.yaml files/versions.yaml
2525
cp ../kubernetes/images/kubectl.tag images/kubectl.tag
26-
cp ../kubernetes/templates/talos/_talosconfigtemplate.tpl templates/_talosconfigtemplate.tpl
2726
$(MAKE) generate

0 commit comments

Comments
 (0)