Skip to content

fix: harden the temporal config init container; test the two halves are wired - #73

Merged
alimobrem merged 7 commits into
mainfrom
claude/jovial-mendel-d4de2e
Sep 1, 2026
Merged

fix: harden the temporal config init container; test the two halves are wired#73
alimobrem merged 7 commits into
mainfrom
claude/jovial-mendel-d4de2e

Conversation

@alimobrem

@alimobrem alimobrem commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

The reported bug was not a code gap

The v0.6.0 CHANGELOG carried a "Known issue (open)" saying the server container mounts no volumes, so spec.temporal still crash-loops on unable to create open /etc/temporal/config/docker.yaml: permission denied.

That note described the state of a live Deployment, not the code. 10ca82a already mounts the temporal-config emptyDir at /etc/temporal/config in the server container, and has since it landed.

Correction. An earlier version of this description attributed the mount's disappearance on dev05 to a second manual edit. That was wrong. de1c283 on main has the right root cause, and I verified it: the v0.6.0 mutate function assigns Spec.Template.Spec.Containers but never Volumes or InitContainers. So a v0.6.0 reconcile over the hand-patched Deployment rewrote the container list — dropping the main-container mount — while leaving the init container and volume untouched. That is precisely the shape observed, and it explains why the two ReplicaSets were created one second apart: the second was the operator reacting to the patch, not a human. The merge in this PR keeps main's account and drops mine.

dev05 runs pulse-operator.v0.6.0 — the release before the fix:

ReplicaSet rev pod spec desired
pulse-temporal-c484f7bbd 1 no init container, no volumes — v0.6.0 operator output 0
pulse-temporal-86db7c8bc 2 init + volume + main-container mount — the live prototype patch 1
pulse-temporal-85db575d6 3 init + volume, mount stripped by the v0.6.0 reconcile 1

Verified live on 2026-09-01: the revision that carries the mount reaches 1/1 Running. auto-setup creates temporal and temporal_visibility, docker.yaml is written into the emptyDir by the namespace-allocated UID, and the frontend, matching and worker services all start. The two-ReplicaSet condition was a wedged rollout, not a reconcile loop creating ReplicaSets: rev 3 scaled itself to 0 the moment a pod went ready.

One real bug, from the init container's own log

cp: can't preserve ownership of '/config-work/./config_template.yaml': Operation not permitted

cp -a implies -p, and an arbitrary UID cannot chown its copies. busybox warns and still exits 0 — which is why this has worked — but GNU coreutils exits 1, so a spec.temporal.image override built on a coreutils base would hard-fail the init container. Copies with cp -r now; nothing here needs source ownership, the server only reads these files.

What else changed

  • Envtest asserts the linkage. The old assertion checked c.VolumeMounts[0].MountPath positionally and never tied it to the volume the init container writes, so a disconnected pair would still pass. It now resolves that volume by name on the init container and requires the server to mount the same one over the path it reads, via a mountPathFor helper that returns "" for exactly the disconnected case.
  • Constants temporalConfigDir / temporalConfigVolume so the two halves cannot drift by string.
  • CHANGELOG: main's resolved known-issue note is kept as the canonical explanation, plus the live verification and the CREATEDB caveat below.

Merge with main

main moved twice while this PR was open; both merges are in (da900ac, fc8b3ad) and origin/main is now fully an ancestor of this branch:

  • de1c283 — resolved the known-issue note. Kept over this branch's version, per the correction above.
  • ac93a15spec.temporal.ui. Its test Describe appended at the same point as mountPathFor; both are purely additive and both kept. Its UI Deployment sets only Containers and mounts nothing, so it does not carry the partial-ownership hazard.
  • 006b0e5 — backfilled proper v0.6.1, v0.7.0 and v0.8.0 changelog sections. Purely additive against the Unreleased block here; Unreleased sits above v0.8.0, since the cp -r and test changes are not yet released.
  • 3ba22ef (v0.8.3) — the same failure family as this PR: the Temporal UI image renders config into a workdir owned by its own UID, OpenShift assigns an arbitrary one, pod crash-loops. Its config dir ships empty, so an emptyDir over it is the whole fix. Reconciler and test auto-merged. I also applied this branch's by-name linkage assertion to its new test — see below.
  • 5cc42fb, e7e21c4, 2bb3c6b — version-guard and catalog fixes; OperatorVersion now reads 0.8.3 against tag v0.8.3.
  • 7b28df6, a1dac89 — no overlap.

One addition beyond the original scope

3ba22ef's new UI test pins the mount by index and length:

Expect(c.VolumeMounts).To(HaveLen(1))
Expect(c.VolumeMounts[0].MountPath).To(Equal("/home/ui-server/config"))
Expect(deploy.Spec.Template.Spec.Volumes).To(HaveLen(1))

Nothing there ties the mount to the volume. Rename one side and the mount references a volume that does not exist, the pod never schedules, and all three assertions still pass — the same shape that let the server ship a disconnected init container in the first place. Switched to mountPathFor by name. This strengthens the assertion only; the v0.8.3 fix itself is untouched, and it is a one-line revert if you would rather keep that test as authored.

For the reviewer

The CREATEDB grant is the caveat that actually bit here. It rides the PostgreSQL pod template, which is create-only, so it never lands on an install that already has a PostgreSQL pod. On dev05 the mount fix cleared the config-permission error and the pod then crash-looped on pq: permission denied to create database instead — which reads like the same bug and is not. Enabling Temporal on an existing install still needs the one-time grant documented in the README. That behaviour is unchanged by this PR; it's now written down in the CHANGELOG.

Written without a local Go toolchain, so CI was the first compiler to see this. It has now done so: lint passes and all 188 envtest specs pass, including the new init/server volume linkage assertions.

The one test failure was TestOperatorVersionNotBehindLatestReleasedTagOperatorVersion said 0.7.0 while v0.8.0 was tagged. Pre-existing on main and unrelated to this branch (006b0e5 failed the same way; this PR touches no version constant). main fixed it across 5cc42fb and e7e21c4; both are merged in here, and the constant now reads 0.8.1 against tag v0.8.1.

Separately, dev05 is still on v0.6.0. It is healthy only because of the prototype patch that the deployed operator will strip again on its next reconcile of that field. Rolling it to v0.6.1 is what makes this stick. No cluster changes were made in this work — all oc calls were reads.

🤖 Generated with Claude Code

amobrem and others added 7 commits September 1, 2026 12:48
…re wired

The v0.6.0 "known issue" was an artefact of a hand-edited Deployment, not a
code gap: 10ca82a already mounts the temporal-config emptyDir at
/etc/temporal/config in the server container. dev05 runs pulse-operator
v0.6.0 — the release before that fix — and its Deployment carried two manual
revisions a second apart, the later of which dropped the mount again. Verified
live: the revision that does carry the mount reaches 1/1 Running, auto-setup
creates temporal and temporal_visibility, and frontend/matching/worker start.
The changelog note is removed and replaced with what the run actually showed.

One real bug did surface, in the init container's own log:

    cp: can't preserve ownership of '/config-work/./config_template.yaml'

cp -a implies -p, and an arbitrary UID cannot chown its copies. busybox warns
and still exits 0, which is why this worked, but GNU coreutils exits 1 — so a
spec.temporal.image override built on a coreutils base would hard-fail the
init container. Copy with -r instead; nothing here needs source ownership.

The envtest asserted the mount only positionally and never tied it to the
volume the init container writes, so a disconnected pair would still pass. It
now resolves that volume by name and requires the server to mount the same one
over the path it reads. Config path and volume name are constants so the two
halves cannot drift by string.

Also recorded: the CREATEDB grant rides a create-only pod template, so
enabling Temporal on an existing install still needs the README's one-time
grant — without it the server clears the config-permission error and then
crash-loops on "permission denied to create database", which reads like the
same bug and is not. That was the actual blocker on dev05.

Not built or tested: no Go toolchain on the machine this was written on.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three commits landed on main that touch the same ground.

CHANGELOG: main's de1c283 resolved the known-issue note with the correct root
cause, and it supersedes the account in this branch. The v0.6.0 mutate
function assigns Spec.Template.Spec.Containers but never Volumes or
InitContainers, so its reconcile over the live prototype rewrote the container
list — dropping the main-container mount — while leaving the init container
and volume untouched. That is exactly the shape observed on dev05, and it
explains the two ReplicaSets created one second apart. This branch had guessed
a second hand-edit instead; main's explanation is kept and that claim dropped.
The two additive bullets are retained: the end-to-end verification, and the
CREATEDB grant caveat for existing installs, which was the actual blocker.

Test file: main's ac93a15 appended a "TemporalReconciler UI" Describe at the
same point this branch appended mountPathFor. Both are purely additive; both
kept. The import block auto-merged with unstructured and schema added.

Reconciler: auto-merged. main's UI constants sit alongside this branch's
temporalConfigDir/temporalConfigVolume, and the cp -r change is intact. The UI
Deployment sets only Containers and mounts nothing, so it does not have the
partial-ownership hazard described above.

Still not built or tested: no Go toolchain on this machine.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
006b0e5 backfilled proper v0.6.1, v0.7.0 and v0.8.0 sections while this branch
was open. Purely additive against the Unreleased block here; both kept, with
Unreleased above v0.8.0 since the cp -r and test changes are not yet released.

Nothing to reconcile in content: main's new v0.6.1 entry already notes the
CREATEDB grant covers fresh installs, and this branch's v0.6.0 bullet spells
out what that means for an existing one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI's "test" job failed on TestOperatorVersionNotBehindLatestReleasedTag:
OperatorVersion said 0.7.0 while v0.8.0 was tagged. Pre-existing on main —
006b0e5 failed the same way — and unrelated to this branch: all 188 envtest
specs passed, including the new init/server volume linkage assertions.

main fixed it in 5cc42fb (constant to 0.8.0, plus a release-workflow gate that
refuses to build when constant and tag disagree). CHANGELOG conflict was the
usual additive pair: Unreleased stays above the new v0.8.1 section.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2bb3c6b renders the full upgrade graph into the catalog and bumps
OperatorVersion to 0.8.2, matching the v0.8.2 tag, so the version guard stays
green. Only the CHANGELOG conflicted, in the usual additive way: Unreleased
stays above the new v0.8.2 section.

Nothing here touches the temporal reconciler or its tests.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
3ba22ef fixes the same failure family this branch is about: the UI image
renders config into a workdir owned by the image's UID, OpenShift assigns an
arbitrary one, pod crash-loops. Its config dir ships empty, so an emptyDir
over it is the whole fix — no init container, no template copy. Reconciler and
test files auto-merged; only the CHANGELOG conflicted, additively as before.

Also applied this branch's by-name linkage assertion to that new UI test,
which pinned the mount by index and length:

    Expect(c.VolumeMounts).To(HaveLen(1))
    Expect(c.VolumeMounts[0].MountPath).To(Equal("/home/ui-server/config"))
    Expect(deploy.Spec.Template.Spec.Volumes).To(HaveLen(1))

Nothing there ties the mount to the volume. Rename one side and the mount
references a volume that does not exist, the pod never schedules, and all
three assertions still pass — the same shape that let the server ship a
disconnected init container. mountPathFor resolves it by name instead. This
strengthens the assertion only; the fix itself is untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@alimobrem
alimobrem merged commit 965efcf into main Sep 1, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants