Skip to content

Commit da28b5a

Browse files
kvapskvaps
committed
fix(platform): guard the FoundationDB version carry-over on all values
Migration 57 tested only cluster.version before adding version, so a version set on the release between the scan and the patch would have been overwritten. The patch now tests the whole values object it read. resourceVersion is not used for this: helm-controller keeps writing the release status, and a mismatch there would abort the platform upgrade for no change to the values. Assisted-by: LLM Signed-off-by: Andrei Kvapil <[email protected]>
1 parent 97c0c45 commit da28b5a

2 files changed

Lines changed: 27 additions & 23 deletions

File tree

‎hack/migration-57-foundationdb-version.bats‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
# upgrade. These drive the real migration script against a fake kubectl
99
# (hack/testdata/migration-57-foundationdb/) and pin that:
1010
# - each release that set cluster.version on 7.1, 7.3 or 7.4 gets version for
11-
# that line, through a JSON patch that tests the value it read and leaves
11+
# that line, through a JSON patch that tests the values it read and leaves
1212
# cluster.version in place for the old chart;
1313
# - releases that never set cluster.version, already have version, or are not
1414
# FoundationDB are not touched;
@@ -66,10 +66,10 @@ JSON
6666
cat "$FAKE_CMDLOG"
6767
[ "$rc" -eq 0 ]
6868

69-
grep -qxF 'PATCH tenant-a foundationdb-old71 json [{"op":"test","path":"/spec/values/cluster/version","value":"7.1.67"},{"op":"add","path":"/spec/values/version","value":"v7.1"}]' "$FAKE_CMDLOG"
70-
grep -qxF 'PATCH tenant-a foundationdb-new74 json [{"op":"test","path":"/spec/values/cluster/version","value":"7.4.3"},{"op":"add","path":"/spec/values/version","value":"v7.4"}]' "$FAKE_CMDLOG"
71-
grep -qxF 'PATCH tenant-b foundationdb-pinned73 json [{"op":"test","path":"/spec/values/cluster/version","value":"7.3.63"},{"op":"add","path":"/spec/values/version","value":"v7.3"}]' "$FAKE_CMDLOG"
72-
grep -qxF 'PATCH tenant-c foundationdb-unlabelled json [{"op":"test","path":"/spec/values/cluster/version","value":"7.1.0"},{"op":"add","path":"/spec/values/version","value":"v7.1"}]' "$FAKE_CMDLOG"
69+
grep -qxF 'PATCH tenant-a foundationdb-old71 json [{"op":"test","path":"/spec/values","value":{"cluster":{"version":"7.1.67","redundancyMode":"double"}}},{"op":"add","path":"/spec/values/version","value":"v7.1"}]' "$FAKE_CMDLOG"
70+
grep -qxF 'PATCH tenant-a foundationdb-new74 json [{"op":"test","path":"/spec/values","value":{"cluster":{"version":"7.4.3"}}},{"op":"add","path":"/spec/values/version","value":"v7.4"}]' "$FAKE_CMDLOG"
71+
grep -qxF 'PATCH tenant-b foundationdb-pinned73 json [{"op":"test","path":"/spec/values","value":{"cluster":{"version":"7.3.63"}}},{"op":"add","path":"/spec/values/version","value":"v7.3"}]' "$FAKE_CMDLOG"
72+
grep -qxF 'PATCH tenant-c foundationdb-unlabelled json [{"op":"test","path":"/spec/values","value":{"cluster":{"version":"7.1.0"}}},{"op":"add","path":"/spec/values/version","value":"v7.1"}]' "$FAKE_CMDLOG"
7373

7474
[ "$(grep -c '^PATCH ' "$FAKE_CMDLOG")" -eq 4 ]
7575
if grep -q 'PATCH [^ ]* foundationdb-defaults ' "$FAKE_CMDLOG"; then echo "unexpected line matching 'PATCH [^ ]* foundationdb-defaults '"; exit 1; fi

‎packages/core/platform/images/migrations/migrations/57‎

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,10 @@
2424
# a read through the API fills in the schema defaults, so a cluster that never
2525
# set cluster.version would look as if it had and get pinned for no reason.
2626
#
27-
# Idempotent: a release that already has version is left alone. A patch carries
28-
# a test on the cluster.version it read, so a concurrent edit fails the patch.
27+
# Idempotent: a release that already has version is left alone. A patch tests
28+
# the values it read, so any concurrent edit to them fails the patch instead of
29+
# being overwritten. It does not test resourceVersion: helm-controller writes
30+
# the release status all the time, and that alone would abort the upgrade.
2931
# Fail-closed on any read or patch error (exit 1 before the stamp) so the new
3032
# chart does not roll out over a cluster whose version was not carried over.
3133
# A cluster.version outside those three lines cannot be carried over; it is
@@ -46,12 +48,10 @@ DRY_RUN="${MIGRATION_DRY_RUN:-0}"
4648
CHART_REF_NAME="cozystack-foundationdb-application-default-foundationdb"
4749
INLINE_CHART_NAME="foundationdb"
4850

49-
# One line per FoundationDB release that needs attention:
50-
# <namespace> <name> carry <version> <cluster.version>
51-
# <namespace> <name> unmapped - <cluster.version>
52-
# Releases are matched by the application label or by the chart they install,
53-
# since the label postdates some releases (see migration 56).
54-
plan=$(kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output json | jq -r \
51+
# One JSON line per FoundationDB release that needs attention, carrying the
52+
# values as read. Releases are matched by the application label or by the chart
53+
# they install, since the label postdates some releases (see migration 56).
54+
plan=$(kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output json | jq -c \
5555
--arg chart "$CHART_REF_NAME" --arg inlinechart "$INLINE_CHART_NAME" '
5656
.items[]
5757
| select(
@@ -63,31 +63,35 @@ plan=$(kubectl get helmreleases.helm.toolkit.fluxcd.io --all-namespaces --output
6363
| select(($v.cluster | type) == "object" and ($v.cluster.version | type) == "string" and $v.cluster.version != "")
6464
| $v.cluster.version as $cv
6565
| (($cv | capture("^(?<line>7\\.(1|3|4))\\.[0-9]+$") | .line) // null) as $line
66-
| if $line then "\(.metadata.namespace) \(.metadata.name) carry v\($line) \($cv)"
67-
else "\(.metadata.namespace) \(.metadata.name) unmapped - \($cv)" end')
66+
| {ns: .metadata.namespace, name: .metadata.name, cv: $cv, values: $v,
67+
version: (if $line then "v\($line)" else null end)}')
6868

6969
CARRIED=0
7070
UNMAPPED=""
7171

72-
while read -r ns name action version cluster_version; do
73-
[ -n "$ns" ] || continue
74-
case "$action" in
75-
carry)
72+
while IFS= read -r row; do
73+
[ -n "$row" ] || continue
74+
ns=$(printf '%s' "$row" | jq -r '.ns')
75+
name=$(printf '%s' "$row" | jq -r '.name')
76+
cluster_version=$(printf '%s' "$row" | jq -r '.cv')
77+
version=$(printf '%s' "$row" | jq -r '.version // ""')
78+
case "$version" in
79+
v*)
7680
if [ "$DRY_RUN" = "1" ]; then
7781
echo "[dry-run] would set version=$version on FoundationDB $ns/$name (cluster.version $cluster_version)"
7882
CARRIED=$((CARRIED + 1))
7983
continue
8084
fi
81-
patch=$(jq -cn --arg cv "$cluster_version" --arg v "$version" '[
82-
{op: "test", path: "/spec/values/cluster/version", value: $cv},
83-
{op: "add", path: "/spec/values/version", value: $v}
85+
patch=$(printf '%s' "$row" | jq -c '[
86+
{op: "test", path: "/spec/values", value: .values},
87+
{op: "add", path: "/spec/values/version", value: .version}
8488
]')
8589
kubectl patch helmreleases.helm.toolkit.fluxcd.io --namespace "$ns" "$name" \
8690
--type json --patch "$patch" >/dev/null
8791
echo "Set version=$version on FoundationDB $ns/$name (cluster.version $cluster_version)"
8892
CARRIED=$((CARRIED + 1))
8993
;;
90-
unmapped)
94+
*)
9195
echo "WARN: FoundationDB $ns/$name has cluster.version $cluster_version, which is not on a supported line (7.1, 7.3, 7.4); it will render the v7.3 default after the upgrade" >&2
9296
UNMAPPED="${UNMAPPED}${UNMAPPED:+,}${ns}/${name}=${cluster_version}"
9397
;;

0 commit comments

Comments
 (0)