fix(chart): render control-plane images from the last-refreshed digest so helm upgrade preserves the pin - #1013
Conversation
…t so a helm upgrade preserves the pin (client-runtime#199) Control-plane images render as repo:tag (IfNotPresent) when unpinned, and image-refresh keeps them current out-of-band via kubectl set image repo@digest. A helm upgrade --reset-then-reuse-values re-renders the bare tag and drops that pin; on a node whose :tag cache is stale the pod then silently runs an OLD image. New tracebloc.controlPlaneDigest helper seeds the effective digest from the jobs-manager Deployment last-refreshed-<image>-digest annotation via lookup, so a re-render renders repo@digest instead. Operator pins still win; lookup is empty during helm template/diff/first install (-> :tag, correct there). No new RBAC (auto-upgrade SA already reads the Deployment in the release ns). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
✅ Validated on a real cluster
Before (current develop chart) vs after (this PR), same command:
Each rendered digest matches the Deployment's Update: this path is now also covered clusterless. helm-unittest 0.5.2 fakes (Edited to remove cluster/namespace identifiers and correct the "can't be unit-tested" framing — this repo is public.) |
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Changes requested. The mechanism is right and the fallback is genuinely safe — I confirmed helm template with no cluster renders :tag + IfNotPresent at all four sites and exits 0 (aks and bm), so the Helm chart renders gate is fine, and your no-new-RBAC claim checks out: the auto-upgrade Role is * on * in the release namespace. CI is green; the org scan's CI=FAILURE is a superseded-run artifact (a CANCELLED Helm unit tests alongside two SUCCESS runs on the same sha).
Two things I think have to change before this lands, and one merge-order point.
1. BLOCKING — the annotation reaches image: with no validation (_helpers.tpl:532). Every one of the 11 digest keys that can reach an image: field is schema-guarded by ^(sha256:[a-f0-9]{64})?$ — --set images.jobsManager.digest=sha256:deadbeefcafe is rejected outright, I checked. The annotation path has no equivalent, and tracebloc.image drops the tag when a digest is present, so a malformed value renders an unstartable ref that helm cannot detect (the kubelet reports InvalidImageName; the apiserver accepts the spec happily).
This is already live in the repo: scripts/tests/e2e-auto-upgrade.sh:195 writes tracebloc.io/last-refreshed-jobs-manager-digest=sha256:e2e-sentinel, then runs helm upgrade --reuse-values and --reset-then-reuse-values against the working-tree chart — and line 278 asserts the annotation is still there afterwards. With defaults (imageRefresh.enabled: true, docker.io, no operator pin) branch 2 fires and renders docker.io/tracebloc/jobs-manager@sha256:e2e-sentinel onto the init container, api and requests-proxy. That script's only readiness wait is kubectl wait ... nodes, and it never asserts a control-plane image ref — so Fleet auto-upgrade E2E (k3d) passed on this head while doing exactly that. A sha256:[a-f0-9]{64} guard in the helper closes it and is unit-testable with no cluster, which also partly answers your "can't be tested" caveat.
2. BLOCKING — imageRefresh.suspend is never consulted. tracebloc.imageRefreshEnabled gates only on not $ir.enabled, but suspend is a real key (values.yaml:1833, consumed at image-refresh-cronjob.yaml:702). With enabled: true, suspend: true — supported, and the state the local-k3d recipe leaves behind — the CronJob never runs, yet the helper still treats refresh as the live update path and pins the last annotation permanently. It hurts most on the DaemonSet: before this change a newly joined node pulled the current :tag; after it, the node pulls a frozen digest and silently runs a stale build with nothing reporting it. That is the same "silently runs an OLD control-plane image" class as client-runtime#199, arriving from the other direction.
3. MERGE ORDER — B depends on A, it does not merely pair with it. I'd said A and B compose, and in the steady state they do: with recorded == latest, B renders that digest and A's live-spec check reads on-digest and no-ops. But the annotation LAGS the live spec in two reachable states, and in both, B pulls the workload backwards:
- a rollout that times out —
set imagehas moved the spec,set -eexits before the annotate, so spec = NEW and annotation = OLD; helm rollback, which re-applies a stored manifest rather than re-rendering, so it restores the digest that was live at that revision's render time. Answering the rollback question directly: yes, a rendered digest means a rollback can undo a security refresh.
Both are self-healing only because A's live-spec reconcile re-pins within a tick. Without A, recorded == latest no-ops and the stale digest sticks until the next upstream publish. So A first is a correctness precondition, not a preference — worth stating in the header next to the lookup caveats.
Non-blocking, but I'd like them addressed or written down:
4. This narrows #569's offline-restart guarantee, and the body doesn't mention it. Keeping the pull policy is necessary but not sufficient: IfNotPresent only helps when the REFERENCE is cache-satisfiable. A node that pulled :tag -> sha256:OLD satisfies repo@sha256:OLD from cache but cannot satisfy repo@sha256:NEW. Since the first-observation path records latest WITHOUT re-imaging, we can now render a digest the node has never pulled: online that is the fix working, offline it turns "restarts on a stale image" into "cannot start". Same root cause makes a side-loaded k3d image unpullable — k3d image import stores a tag alias with no resolvable digest, and the mirror gate doesn't help because a local k3d install IS docker.io.
5. First-observation roll. The script records without re-imaging precisely because "rewriting repo:tag to repo@digest for identical content would roll every pod on install". This reintroduces that roll through helm: first tick writes the annotations, the next upgrade changes all four pod specs for a zero-content change — recreating jobs-manager (strategy: Recreate on RWO PVCs, the #545 wedge) and every resource-monitor pod on every node. Not hourly, to be fair: auto-upgrade only fires on a chart-version bump. Relatedly, the helper's doc says "the digest image-refresh last APPLIED" — on this path it is what refresh last OBSERVED and deliberately did not apply. Worth correcting; it changes how a reader reasons about the value.
6. A --set env.CLIENT_ENV=stg switch becomes inert on the image: the annotation key isn't env-scoped and the tag is dropped when a digest is present, so the edge renders the other env's digest while every label reads stg. It self-corrects next tick unless the switch leaves the Deployment unsettled — then the settled guard skips forever and each upgrade re-pins the old env's digest.
7. Seven uncached lookups per render (5 in jobs-manager, 1 each in requests-proxy and resource-monitor), and a non-NotFound error RAISES rather than returning empty — failing the whole release, which is what resource-monitor-daemonset.yaml:22-27 documents for backend#2469. The RBAC half is fine as you argued; the transient-apiserver-error half is a new failure mode for a render that used to work offline, and unlike the metrics-server lookup there's no escape hatch equivalent to nodeAgents.metricsServerPreflight: false.
8. Routing the effective digest into controlPlanePullPolicy (:277, :912) is a no-op: wherever the helper can return non-empty, imageRefreshEnabled && docker.io already held, so the policy was already IfNotPresent. Harmless, just two more lookups.
On the Helm 4 server-side-apply question from the last round: I said this would get strictly worse with the chart rendering a digest, and I want to withdraw that. The premise was wrong — repo:tag is itself a rendered value for .image, so helm has always declared and owned that field; that ownership is exactly why the #199 revert happens. This changes the VALUE helm writes, not the ownership set, and adds no manager. On the next upgrade after a kubectl set image, helm reclaims the field and writes what the chart rendered: it does not error and does not preserve the digest. Before, it reclaimed and wrote the wrong value; after, the right one in the steady state. So: neutral on ownership, better on outcome, with the lag window in (3) as the exception. Also relevant for @LukasWodka: the fleet path is Helm 3 today — autoUpgrade pins alpine/helm 3.16.4 and CI's render job pins v3.15.4 — so SSA ownership doesn't reach the fleet until someone bumps that tag to 4.x.
On testing: I mutated the helper to always return "" and 5 tests across the jobs_manager / requests_proxy / resource_monitor suites go red, so the operator-pin branch is genuinely covered (695 pass clean). Branch 2 has nothing that can redden, as you say — but I don't think it needs a new suite in full-seal-e2e. The rig already exists: e2e-auto-upgrade.sh runs on real k3d, already seeds that exact annotation, and already runs both upgrade paths. It needs one assertion on the rendered image ref (and, given finding 1, a real 64-hex digest instead of the sentinel).
What I could NOT verify here, stated plainly: I have no cluster, so branch 2 — the lookup path, the whole point of the PR — was never actually executed by me; every claim about it is read from the template and from the CronJob's annotation writer. No k3d locally, so the side-loaded-digest hazard in (4) is reasoned from the tag-alias behaviour, not reproduced. And no Helm 4 server-side-apply rig, so the ownership analysis above is from managedFields semantics plus the fact that the revert demonstrably happens today (which proves helm reclaims the field rather than erroring) — not from an observed conflict.
…spend (client-runtime#199) blocking 1 (@shujaatTracebloc on #1013): the last-refreshed annotation reaches an image: field unchecked, unlike the 11 values digest keys the schema guards with ^(sha256:[a-f0-9]{64})?$. A malformed value renders an unstartable ref helm cannot detect. controlPlaneDigest now regexMatches the annotation and degrades to :tag otherwise. blocking 2: a suspended CronJob never re-pins, so its frozen annotation must not pin the render -- gate controlPlaneDigest and controlPlanePullPolicy on not imageRefresh.suspend so a suspended edge stays on :tag with Always. Updates the e2e sentinel to a valid 64-hex digest; adds two jobs-manager pullPolicy unit tests. helm-unittest 697, lint clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed all three in 8e1fc65. 1 — BLOCKING, annotation reaches 2 — BLOCKING, 3 — merge order, B depends on A. Corrected the "Relationship to #1008" section: it now states B depends on A (the rollout-timeout and helm-unittest 697/697, |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… jobs-manager image (client-runtime#199) The e2e-auto-upgrade path-2 check asserted only that the last-refreshed annotation survived the upgrade, not that tracebloc.controlPlaneDigest actually rendered that digest onto the container image. The annotation is a k8s object helm never touches, so it survives even if the helper is removed and the image reverts to a floating :tag -- the exact revert this PR prevents. Add jm_controlplane_image_digest() and assert the rendered image digest equals the annotation, killing that mutation. Bind the sentinel digest once (E2E_REFRESH_DIGEST) and reuse it across both checks. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
# Conflicts: # scripts/tests/e2e-auto-upgrade.sh
|
bugbot run |
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Approving. Both blockers are closed, and I verified each by rendering rather than by reading.
1 — annotation validation: CLOSED. tracebloc.controlPlaneDigest now regexMatches ^sha256:[a-f0-9]{64}$ before the value can reach image:. I stubbed the lookup with a literal Deployment dict in a throwaway worktree at 62ca905 and rendered client/ci/bm-values.yaml both ways:
- annotation
sha256:deadbeefcafe-> all four control-plane sites renderdocker.io/tracebloc/jobs-manager:prod,pods-monitor:prod,resource-monitor:prod. It degrades to:tag, which is what I asked for. - annotation a real 64-hex digest -> jobs-manager (init + api), requests-proxy, pods-monitor and resource-monitor all render
repo@sha256:..., with all threeannotationImagekeys resolving. That is the first time branch 2 has actually been executed rather than reasoned about, mine included.
Making the e2e sentinel a valid 64-hex digest is the right call, and the assertion you built on top of it (jm_controlplane_image_digest, path 2) is the one I asked for last round and the one Bugbot asked for: the annotation check cannot redden if the helper is dropped, the rendered-image check can. I also checked the blast radius of seeding an unpullable digest into a live k3d cluster: the only readiness wait in that script is kubectl wait ... nodes at line 158, before the seed, and everything after it is spec-only reads — so paths 2-5 cannot wedge on it, and Fleet auto-upgrade E2E (k3d) is green on this head.
2 — imageRefresh.suspend: CLOSED. Both helpers now gate on not $ir.suspend. Rendered with --set imageRefresh.suspend=true and a valid annotation: all four workloads go back to :prod with imagePullPolicy: Always, so a newly joined node re-pulls the current tag instead of silently running a frozen digest. Keeping it out of imageRefreshEnabled is correct — that also gates the CronJob and RBAC, which must still render when suspended. And Always here is the same trade imageRefresh.enabled: false already documents in values.schema.json, so this applies the chart's existing contract to the equivalent state rather than inventing one.
Mutation-registered rather than trusted: removing (not $ir.suspend) from controlPlanePullPolicy reddens exactly your new case ("falls back to Always when image-refresh is suspended") — 1 failed, 696 passed. Otherwise 697/697 clean, matching your count. Two mutations do NOT redden anything, and I would rather say so than imply coverage you do not have: neutering the regexMatch to if true, and removing (not $ir.suspend) from controlPlaneDigest itself, both leave 697 passing, because lookup is empty under helm-unittest. So the guard and the digest-side suspend gate rest on the stub render above and on your pullPolicy tests as a proxy. That is the ceiling without a cluster and I am not asking for more here.
3 — merge order: CLOSED. The header now states that B depends on A and that A lands first, with both lag states and the rollback answer. Worth noting the practical gate: #1008 still carries my change-request (the latched flap is silent — FLAP_KEY is never written, so the WARN never prints), so that is what holds this, not anything on this PR.
Non-blocking, and none of it holds the approval — but two items got sharper while I was verifying, and since A has to land first anyway I would rather they were written down than dropped:
autoUpgraderunshelm upgrade --reset-then-reuse-values --atomic --cleanup-on-fail --timeout(auto-upgrade-cronjob.yaml:260;--atomicimplies--wait). That upgrades my points (4) and (5) from "an extra roll, and offline it cannot start" into a retry loop: if a rendered digest does not go ready inside the timeout,--atomicrolls the release back, the annotation is unchanged, and the next tick renders the same digest and fails identically. It bites hardest on the first-observation path, where image-refresh recordslatestdeliberately WITHOUT re-imaging, so that digest never proved a rollout. The thing that wedges is the security-fix delivery path. A follow-up issue, not a change here.- the init container is seeded too (
jobs-manager-deployment.yaml:103), butjm_set_argsonly ever namesapiandpods-monitor-container(image-refresh-cronjob.yaml:547,558), sokubectl set imagenever moveswait-for-mysql. Each digest change therefore rolls jobs-manager once out-of-band and again on the next chart bump —Recreate, inside that--atomic --wait. It is a MySQL TCP wait, so the pin buys nothing there; dropping the digest from that one site is the cheap version. - coverage: the e2e asserts the jobs-manager seed only. I confirmed the
pods-monitorandresource-monitorkeys render correctly via the stub, but nothing in CI would catch a rename or a typo in those two hand-written strings. - the helper doc still says "the digest image-refresh last APPLIED" (
_helpers.tpl:505) where on this path it is what refresh last OBSERVED and deliberately did not apply; and thecontrolPlanePullPolicycontract block (:441-482) still enumerates three cases naming onlyenabledand docker.io, with no mention of the newsuspendterm. Both are reader-facing. - the values-level
suspendgate cannot see akubectl patch cronjob ... spec.suspend=true, or the flap lockout, both of which freeze refresh withimageRefresh.suspend: falsestill in values. Same residual class, no cheap fix — just noting the bound. jobs_manager_test.yaml: "control-plane pullPolicy is IfNotPresent when image-refresh is active" duplicates "both containers are IfNotPresent by default (refresh on, docker.io)" at:1130and is weaker (containers[0]vs both containers); both new cases sit at the end of the file rather than in the #569 pull-policy block a maintainer would read; and the comment citesbackend#199, which is not the issue.- the body is stale in two places: it still opens "DRAFT for discussion", and still says "101/101 unit tests pass" (it is 697 across 40 suites).
CI: green on 62ca905. Every required context has a SUCCESS on this head; the rollup reads FAILURE only because of a CANCELLED Helm unit tests sibling with two SUCCESS runs on the same sha — the superseded-run artifact we both called — and the deduped gh pr checks shows zero failing and zero pending. Chart 1.9.110 > develop 1.9.108. Merge after #1008.
LukasWodka
left a comment
LukasWodka
left a comment
There was a problem hiding this comment.
Reviewed 62ca905 against develop (1.9.108 → 1.9.110). The mechanism is sound and I reproduced the key evidence: helm template with the aks and bm CI values renders :prod + IfNotPresent at all five image sites and exits 0, 697/697 unit tests pass, and the k3d Fleet auto-upgrade E2E job on this head passed the new path-2 assertion (rendered image: digest == the seeded annotation) and ran through to E2E PASS. Bugbot is clean and its thread is resolved. Mutation check: disabling the operator-pin branch reddens 5 tests; removing the suspend gate reddens the new pullPolicy test. Both of those guards are real.
Requesting changes on two points, plus a merge-order hold.
1. imageRefresh.suspend: true now rolls the whole control plane onto the floating tag. Adding (not $ir.suspend) to both branch-2 conditions means a paused refresh renders repo:tag + Always instead of repo@digest + IfNotPresent. Steady state: annotation = D1, everything live on repo@D1. Operator sets suspend: true to freeze the edge during an incident; the next hourly bump re-renders jobs-manager (Recreate), requests-proxy and the resource-monitor DaemonSet to :tag/Always, so every node pulls whatever the tag points at now. Un-pausing renders @D1 again and rolls everything back to D1, then the next tick rolls forward. Three unplanned rollouts and one downgrade from a knob whose schema text (values.schema.json L1371) promises "pause without removing the resources". Before this PR the render was :tag + IfNotPresent either way, so flipping suspend changed no spec. Suggested fix: keep rendering the annotation digest while suspended (suspend should stop polling, not un-pin), and leave pullPolicy IfNotPresent on that branch. If you do want suspend to un-pin, that is a behaviour change that needs its own schema/values text and a line in the migration notes.
2. Branch 2 is unit-testable clusterless; please add the suite. The body and the 13:10 comment both say the lookup path can't be unit-tested. helm-unittest 0.5.2, the version CI pins, fakes lookup via kubernetesProvider. I ran this against HEAD and it passes 3/3; discarding the lookup result in the helper makes the first case fail, so it is a real guard:
kubernetesProvider:
scheme:
"apps/v1/Deployment": {gvr: {group: apps, version: v1, resource: deployments}, namespaced: true}
objects:
- apiVersion: apps/v1
kind: Deployment
metadata:
name: RELEASE-NAME-jobs-manager
namespace: NAMESPACE
annotations:
tracebloc.io/last-refreshed-jobs-manager-digest: "sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
tracebloc.io/last-refreshed-pods-monitor-digest: "sha256:not-a-digest"
tests:
- it: renders api image from the last-refreshed annotation
documentIndex: 0
asserts:
- equal: {path: 'spec.template.spec.containers[?(@.name == "api")].image', value: docker.io/tracebloc/jobs-manager@sha256:aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa}
- it: malformed annotation degrades to :tag
documentIndex: 0
asserts:
- equal: {path: 'spec.template.spec.containers[?(@.name == "pods-monitor-container")].image', value: docker.io/tracebloc/pods-monitor:prod}
- it: suspend beats a valid annotation
documentIndex: 0
set: {imageRefresh: {suspend: true}}
asserts:
- equal: {path: 'spec.template.spec.containers[?(@.name == "api")].image', value: docker.io/tracebloc/jobs-manager:prod}(Adjust the third case to whatever you decide for point 1.) This matters because today the only thing that reddens if the lookup path breaks is the k3d e2e, which is path-filtered and not a required status check on develop, so a regression of the seed merges green. Helm unit tests is required. Please cover the matrix clusterless: the five sites (init, api, pods-monitor, requests-proxy following the jobs-manager annotation, resource-monitor), operator pin beating the annotation, malformed → :tag, and the suspend decision. Then drop the "can't be unit-tested" sentences from the body; that claim is what would keep the next person from writing the test. Also: the new "IfNotPresent when image-refresh is active" case at tests/jobs_manager_test.yaml:1236 is a verbatim duplicate of the existing case at L1130, and the block comment above it claims the helper "must NOT render that digest" while no assertion touches image:.
3. Merge order (hold). The body now states B depends on A (#1008) for the rollout-timeout and helm rollback lag states. #1008 is still open, so this cannot merge first. Only client/Chart.yaml conflicts (1.9.109 vs 1.9.110); rebase after A lands and keep 1.9.110. One extra note for A: when the refresh tick's rollout times out after set image but before annotate, the annotation lags the live spec, and once the flap lockout from #563 latches, nothing re-pins until a human clears it, so A's reconcile needs to run before, not behind, the lockout check.
Non-blocking, please pick up in the same push:
- Docs this PR makes false, in the same PR: the "HELM RE-RENDER" limitation in the image-refresh CronJob header (L50-57: re-render "writes repo:tag and so reverts an earlier set image pin") and
docs/SEAL-CHECK.mdL289-294 (chart rendersrepository:tag, so pin in values to survive--reset-then-reuse-values) both describe the pre-PR behaviour. An operator following SEAL-CHECK today would pin values to fix a revert the chart no longer performs. - First-observation churn: the first tick records the digest without
set imageso a fresh install never rolls pods for byte-identical content (header L92-105). After this PR the first chart bump after install renders@D0where the live spec says:tag, so jobs-manager (Recreate), requests-proxy and the DaemonSet on every node roll once for identical content, and--atomicon a slow single-node edge can time out on that. One-time cost, but the header should say so, or the first-observation path shouldset imagetoo. - Helper docstring: "the digest image-refresh last APPLIED" is what refresh last observed on the first-observation path. That distinction is exactly the lag window the body describes; say "observed".
- The
#569offline guarantee is narrower now: after a first observation the next upgrade can render a digest the node has never pulled, so an offline restart goes from "runs a stale image" to "cannot start". Worth one paragraph at the#569comment block or indocs/MIGRATIONS.md. docs/MIGRATIONS.md§server-side-apply conflict: one line that from 1.9.110 the chart renders the last-refreshed digest, so a Helm 4.imageconflict now signals annotation/live lag (wait a refresh tick, or--force-conflicts) rather than steady-state drift. In the steady state Helm applies the same value thekubectl-setmanager holds, so the conflict disappears. That is a real improvement and deserves to be written down.tests/jobs_manager_test.yaml:1231: the ticket reference in the new comment points at issue 199 of the wrong repo (the runtime ticket is the one meant), and this repo is public, so keep private-repo refs out of new comments.scripts/tests/e2e-auto-upgrade.sh:248: with the unpullable sentinel digest, path 1 now Recreates jobs-manager onto an image that cannot pull, so it sits inInit:ImagePullBackOfffor paths 2-5. Harmless today only because nothing after L248 reads a live pod; either use a pullable digest or assert that state explicitly so the next author is not surprised.- Please scrub the cluster and namespace names from the 07:56 validation comment, and the RFC / private-ticket refs from the body. This repo is public.
Merge order A → B, then I'll re-review.
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
There was a problem hiding this comment.
@LukasWodka's change-request lands on two things I said, and he is right on both. Flagging that explicitly so you are not stuck between an approval and a block on the same head — follow his direction, not mine, on both points below. My approval stands on everything else and I am not asking for anything further.
1. The suspend gate was my ask, and his objection defeats it. I required (not $ir.suspend) on the branch-2 conditions because a suspended CronJob would otherwise pin the last annotation forever, so a newly joined node would silently run a frozen build. That concern is real but his is worse, and I had not thought it through:
suspend: trueexists to freeze an edge during an incident, andvalues.schema.jsonpromises "pause without removing the resources".- With my gate, the next hourly bump re-renders jobs-manager (Recreate), requests-proxy and the DaemonSet to
:tag/Always, so every node pulls whatever the tag points at right now — during the incident you paused for. Un-pausing rolls everything back to D1 and the next tick rolls forward again. - So the knob costs three unplanned rollouts and a possible downgrade, where before this PR flipping
suspendchanged no spec at all.
A frozen digest is at least known and reproducible; floating on a mutable tag with Always mid-incident is strictly less so. His framing is the correct one — suspend should stop polling, not un-pin — and keeping IfNotPresent on that branch is right. My original worry is about visibility of a stale pin, not about un-pinning: if it needs addressing, it wants a surfaced signal that refresh is paused, not a spec change. If you do end up wanting suspend to un-pin, his condition applies: that is a behaviour change needing its own schema text and a migration note.
2. My "cannot be unit-tested without a cluster" was wrong, and I let you off the hook on it. I wrote that the regexMatch guard and the digest-side suspend gate "rest on the stub render above and on your pullPolicy tests as a proxy. That is the ceiling without a cluster and I am not asking for more here." He points out helm-unittest 0.5.2 — the version CI pins — fakes lookup via kubernetesProvider, and he ran it 3/3 against HEAD with a mutation that reddens the first case. So branch 2 is unit-testable clusterless, and the two mutations I reported as un-reddenable can be pinned properly. Take his suite over my proxy; it closes the exact coverage gap I flagged and then waved.
Nothing else changes from my side: the two blockers I raised are closed and verified by render, and the merge order (A #1008 before B) still holds — #1008 is still carrying my change-request on the silent-flap line.
…up tests (client-runtime#199) Addresses @LukasWodka's review on #1013. RECONCILES A REVIEWER CONFLICT (@shujaatTracebloc). shujaat's earlier BLOCKING-2 asked that imageRefresh.suspend UN-PIN — render :tag + Always so a newly joined node re-pulls the current tag. LukasWodka's point 1 shows that is wrong: suspend is used to FREEZE an edge during an incident, and un-pinning re-renders jobs-manager (Recreate), requests-proxy and the resource-monitor DaemonSet onto :tag/Always — three unplanned rollouts plus a downgrade to whatever :tag points at now — contradicting the values.schema.json promise that suspend "pause[s] without removing the resources". Decision: suspend stops POLLING, not un-pinning. Removed the (not $ir.suspend) gate from both tracebloc.controlPlaneDigest and tracebloc.controlPlanePullPolicy, so a suspended edge keeps rendering the last-observed @digest + IfNotPresent. This still answers shujaat's newly-joined-node concern: that node pulls the frozen @digest — a real, previously-resolved image — not a missing one. Also in this push (LukasWodka's other points): - Add client/tests/control_plane_digest_test.yaml — a clusterless suite that fakes `lookup` via helm-unittest 0.5.2's kubernetesProvider. Covers the five image sites (init, api, pods-monitor, requests-proxy-follows-jobs-manager, resource-monitor), operator-pin beats the annotation, malformed -> :tag, the suspend-keeps-pin decision, and the disabled/mirror/first-install :tag edges. Mutation-verified: discarding the lookup reddens the digest cases, re-adding the suspend gate reddens the suspend case, neutering the regexMatch reddens the malformed case — the exact clusterless ceiling shujaat noted, now lifted. - Remove the duplicate/misleading pull-policy cases appended to jobs_manager_test.yaml (the L1236 verbatim dup of L1130 and the now-wrong suspend->Always case, whose block comment wrongly claimed the helper "must NOT render that digest"); the private backend#199 ref in that comment goes with it. - Docs the PR made false: image-refresh CronJob "HELM RE-RENDER" limitation now marked RESOLVED as of 1.9.110; SEAL-CHECK.md no longer tells operators to pin values to survive a revert the chart no longer performs. - First-observation churn documented in the CronJob first-tick contract; helper docstring "last APPLIED" -> "last OBSERVED"; #569 offline-guarantee narrowing noted at controlPlanePullPolicy; MIGRATIONS.md server-side-apply gets a line that from 1.9.110 a Helm 4 .image conflict signals annotation/live lag. - e2e-auto-upgrade.sh: the unpullable sentinel now documented and asserted explicitly (path 1 spec-only check + Init:ImagePullBackOff-by-design comment) so the next author is not surprised. Merge order unchanged: B stays behind A (#1008). Chart stays 1.9.110. helm lint clean; helm unittest 706/706 (41 suites); helm template renders :tag clusterless and @digest on the operator pin; shellcheck + bash -n clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @LukasWodka — all addressed in 616b843. Point by point: 1.
The pull-policy branch reverts exactly to the pre-PR render ( 2. Branch 2 IS unit-testable clusterless → added the suite. New
Dropped the "can't be unit-tested" sentences from the body and the 07:56 comment. And fixed the 3. Merge order (hold) — kept A→B. B stays behind #1008; the only rebase conflict is Non-blocking, all in the same push:
Verify: Re-requesting your review. |
|
bugbot run |
LukasWodka
left a comment
LukasWodka
left a comment
There was a problem hiding this comment.
Re-reviewed 616b843 against my 62ca905 review. Approving.
Blocking 1 — suspend un-pinning: fixed. (not $ir.suspend) is gone from both tracebloc.controlPlanePullPolicy (L503) and tracebloc.controlPlaneDigest (L559), with the reasoning in the helper header. A suspended edge now keeps rendering the last-observed repo@digest + IfNotPresent, and the clusterless render with imageRefresh.suspend=true is byte-identical to the pre-PR render (:tag + IfNotPresent), so flipping the knob no longer changes any spec. Re-adding the gate locally reddens exactly the new suspend case and nothing else. Thanks to @shujaatTracebloc for reconciling this explicitly on-thread.
Blocking 2 — clusterless lookup suite: fixed. client/tests/control_plane_digest_test.yaml covers the matrix I asked for: the five image sites (api, wait-for-mysql init, pods-monitor on its own key, requests-proxy following the jobs-manager key, resource-monitor DaemonSet), operator pin beating the annotation, malformed → :tag, suspend keeps the pin, plus the disabled / mirror / first-install :tag edges. I mutation-checked it locally against 616b843 (helm-unittest 0.5.2, anchor verified applied each time, tree restored):
- discard the lookup result → 6 fail (all five sites + suspend);
- regex accept-all → only the malformed case fails; regex reject-all → the six valid-digest cases fail, malformed stays green;
- fix the annotation key to
jobs-manager→ exactly pods-monitor and resource-monitor fail.
The verbatim duplicate atjobs_manager_test.yaml:1236and its misleading block comment are gone, and the "can't be unit-tested" claim is out of the body. 706/706 across 41 suites locally;Helm unit testsgreen in CI.
Non-blocking, all landed: CronJob header limitation 1 marked resolved as of 1.9.110; docs/SEAL-CHECK.md no longer tells operators to pin values to survive an upgrade; first-observation roll written into the first-tick contract (incl. the --atomic --wait timeout on a slow single-node edge); "applied" → "observed" at the helper and both call sites; the #569 offline-narrowing paragraph at controlPlanePullPolicy with the mirror and k3d corollaries; the MIGRATIONS.md server-side-apply line; the e2e sentinel is now documented and asserted at path 1 (spec-only, Init:ImagePullBackOff by design); the 07:56 comment and the body are scrubbed for this public repo.
One nit, no action needed now: the body says the #569 narrowing is documented "in docs/MIGRATIONS.md" — it lives at controlPlanePullPolicy in _helpers.tpl; MIGRATIONS carries only the SSA-conflict line. Fine either way.
Gate: 54 checks pass / 4 skipping, Bugbot clean on 616b843, one review thread and it is resolved, mergeable.
Do not merge before #1008. #1008 is still open; the body's dependency analysis (rollout-timeout and helm rollback lag states are self-healing only with A's live-spec reconcile) still holds. Land A, rebase for the client/Chart.yaml conflict keeping 1.9.110, then merge B.
| {{- if .operatorDigest -}} | ||
| {{- .operatorDigest -}} | ||
| {{- else if and (include "tracebloc.imageRefreshEnabled" .root) (eq $mirror "docker.io") -}} | ||
| {{- $dep := lookup "apps/v1" "Deployment" .root.Release.Namespace (printf "%s-jobs-manager" (include "tracebloc.fullname" .root)) -}} |
There was a problem hiding this comment.
BLOCKING (High) — a third annotation-vs-live lag state that #1008 cannot heal, and here it becomes a chart-enforced downgrade.
The merge-order section names two lag states (rollout timeout mid-tick, helm rollback). There is a third, and it is the one that does not self-heal: the image-refresh flap lockout.
Mechanism, all in image-refresh-cronjob.yaml on this branch:
annotate_argsis written in ONE batchedkubectl annotatethat runs after all threeset image+rollout statuspairs (~L700). Underset -e, arollout statustimeout onrequests-proxyor the resource-monitor DaemonSet exits the tick before that annotate — soapiis already at@D1while all threelast-refreshed-*annotations still say@D0.- Each attempt increments
ATTEMPT_KEY, which resets only on a fully settled rollout. AfterMAX_REFRESH_ATTEMPTSthe flap guardexit 0s before the annotate block (~L616-631) and requires a human to clear the annotation to re-arm.
So the annotations are pinned at D0 indefinitely while the live spec is at D1. With this PR every subsequent helm upgrade now renders @D0 onto all five sites and, because auto-upgrade applies client-side (--reset-then-reuse-values --atomic --cleanup-on-fail, no --server-side), Helm's 3-way merge overwrites the live @D1 with @D0 — an active, deterministic downgrade of jobs-manager, pods-monitor and requests-proxy, potentially off a security refresh. Before this PR the re-render wrote a bare :tag, which was #199 (stale) but never an enforced move backwards.
And #1008 does not cover it: its live-spec reconcile sits inside the recorded == latest branch. Here recorded (D0) != latest (D1), so the tick takes the "digest changed" path, hits the flap guard, and the reconcile branch is never reached. Landing #1008 first fixes the rollback state but not this one.
Suggested fix in scope for B: make the helper ignore an annotation the live spec has already moved past — or in A, write each image's last-refreshed-* annotation immediately after that workload's own rollout status instead of batching all three at the end, and write the annotation before the flap guard's exit 0.
There was a problem hiding this comment.
Agreed this is real and needs settling before merge, not leaving to merge order. Going with (a) — fix the annotation timing in #1008, not (b) here. Reasoning:
- The annotation is the source of truth this helper trusts, so the correct fix is to stop it ever lying, not to teach the render path to detect that it's lying. (a) makes it truthful by construction: annotate each image right after that image's own
rollout statussucceeds, and move a settled workload's annotate before the flap guard'sexit 0, so a lockout tick still records what actually rolled. Thenrecorded == latestholds and fix(image-refresh): re-pin the digest when a helm re-render reverts the workload to :tag #1008's existing live-spec reconcile covers the state — it's no longer stranded in therecorded != latestpath. - (b) would add a second
lookupof the live container image with exactly the fail-open semantics you flag one thread down at_helpers.tpl:561(empty map on RBAC/apiserver failure), and it only masks the symptom in the render path — the annotation stays wrong for anything else that reads it, and "live has moved past" is ambiguous about which digest is then authoritative. - fix(image-refresh): re-pin the digest when a helm re-render reverts the workload to :tag #1008 is this PR's declared hard-prerequisite and must land first regardless, so putting the fix there costs no extra gating.
So this lands in #1008 (per-workload annotate, before the flap-guard exit 0), #1013 rebases on it, and this thread resolves once that's in. Leaving it open here until then.
On the related MIGRATIONS.md point: agreed the SSA-conflict line is Helm-4 hand-run only; I'll scope it to say the automated fleet path (Helm 3.16 client-side) overwrites rather than conflicts.
There was a problem hiding this comment.
Real, and you're right that #1008 as written doesn't heal it — its live-spec reconcile sits in the recorded == latest branch, which the flap-lockout state never reaches. This is a BLOCKING precondition on A (#1008), not something B can fix alone, so per that I'm leaving this open and flagging it on #1008: the robust fix is A-side — write each image's last-refreshed-* annotation immediately after that workload's own rollout status (not one batched annotate after all three), and write it before the flap-guard's exit 0, so the annotation can never lag the live spec into a chart-enforced backward move. Your own 9666cf0 note confirms the mechanism: the fleet path is Helm-3 client-side, whose 3-way merge overwrites live @D1 with the rendered @D0 rather than conflicting. B stays held behind A until A covers this. (The tag-lookup-failed signal I added for the lookup thread makes a :tag fall-back observable, but not this backward-move — that needs the A-side ordering fix.)
There was a problem hiding this comment.
Answering the D0-overwrite directly, and settling it rather than leaving it to merge order: option (a), in #1008. The last-refreshed-<image> annotation should be written per-workload, right after that workload's own rollout status, and before the flap guard's exit 0 — so a rollout that times out on requests-proxy or the resource-monitor DaemonSet can never leave api at @D1 with the annotation frozen at @D0. With that, this helper only ever renders a digest the live spec has actually reached, and the "healthy jobs-manager moved backwards D1→D0 under Recreate, every bump while the lockout holds" scenario cannot arise.
I'm keeping this in A, not B, because the annotation write-ordering is A's code (image-refresh-cronjob.yaml), and B renders whatever A records — a B-side "ignore an annotation the live spec moved past" (option b) would have to re-read the live container image and duplicate A's reconcile logic in a template, which is the wrong place for it. B stays held behind A (already its declared prerequisite), and this specific ordering fix is now an explicit precondition on A — I've flagged it on #1008 with your analysis, tied to @LukasWodka's "A's reconcile must run before, not behind, the lockout check." So: not a merge-order hope, a named A-side change that must land before B.
Leaving this thread open until #1008 carries it. On the related MIGRATIONS line — you're right; it's scoped to the hand-run Helm-4 SSA path now (the author's 9666cf0 already narrowed it: the Helm-3.16 client-side fleet path overwrites rather than conflicting).
There was a problem hiding this comment.
Refining the settlement now that A is essentially done. Option (a), split to an internal follow-up ticket rather than held inside #1008 — because your own re-review of A concluded its current scope is complete ("nothing else outstanding from me" after the rebump), and this is ordering hardening, not a gap in what A set out to do. The follow-up carries the exact spec: write each last-refreshed-<image>-digest annotation immediately after that workload's own rollout status, and before the flap guard's exit 0, so the annotation can never trail the live spec.
It must land before the exposure can matter on a fleet running B, and it's bounded until then: it needs the flap lockout (repeated rollout timeouts — e.g. a cordoned/NotReady node keeping the DaemonSet's desiredNumberScheduled from settling), A's recorded == latest reconcile heals every non-locked state, and @D0 is a valid, previously-pulled image, not an unstartable ref.
So: question answered (option a), fix specced and tracked, A landing first, B held behind it. If that settles it for you, this thread is yours to resolve / approve on; if you'd rather the ordering change land inside A before it merges, say so and I'll move it there instead of the follow-up — no strong preference on my side either, matching yours.
There was a problem hiding this comment.
Settled — taking option (a) and the split, and I'm not re-opening the choice. Your reasoning for keeping it A-side is the right one: (b) would put a second live-image lookup into the render path with exactly the fail-open semantics of the _helpers.tpl:561 thread, and duplicate A's reconcile inside a template. The annotation should stop lying rather than the renderer learn to detect that it is.
backend#3575 captures it faithfully — the per-workload annotate, the exit 0 ordering, the rejected alternative, and the bound (needs the durable lockout; @D0 is a valid previously-pulled image). Good ticket.
Your direct question — "or move it inside A instead" — is moot now: A merged at 07:22:04Z, about five minutes after you wrote that. I checked merged develop rather than assuming: image-refresh-cronjob.yaml still writes every last-refreshed-*-digest through the single batched kubectl annotate $annotate_args at line 879, after the rollout status calls (833/842), with the flap guard's exit 0 at line 804 ahead of it. So #3575 is the only remaining home for the fix — the in-A option is gone.
That is the one thing I'd still like nailed down, and it's the only reason I'm leaving this open. Your own condition is "it must land before the exposure can matter on a fleet running B." With A merged, nothing sequences #3575 against this PR: #3575 is open, unassigned and unlabelled, and B is otherwise green. The condition is stated in two comment threads and an issue body, none of which can stop B merging first.
So this thread is no longer the original finding — that's answered — it's now just the sequencing marker. Resolve it when either #3575 lands, or the dependency is recorded somewhere that actually gates this PR. Happy either way; I only want the ordering to survive the fact that A is already in.
For the record, I'm not the blocker here — @LukasWodka's change-request is the standing one, and it's on the current head.
|
bugbot run |
…en provenance (client-runtime#199) Addresses @shujaatTracebloc's review threads on #1013 (the B-scope ones). #2 (High) — the wait-for-mysql INIT container is no longer seeded from the last-refreshed annotation. A digest a node never pulled fails as Init:ImagePullBackOff (the pod never starts) on exactly the offline/side-loaded k3d / Docker-Desktop edge #569 protects, and `kubectl set image` never touches this container so the chart render would be its only, un-reconciled driver. The pin buys nothing on a MySQL TCP wait. The site keeps honouring an explicit operator pin (`images.jobsManager.digest`); it just no longer seeds the floating annotation. #3 (Medium) — `lookup` fails OPEN: Helm returns an empty map on every failure (RBAC denial, apiserver 5xx, client timeout, kubectl-less renderer), so a read failure was indistinguishable from a first install and would silently drop the pin to `:tag` (client-runtime#199, now non-deterministic and unsignalled). New `tracebloc.controlPlaneDigestSource` renders `tracebloc.io/controlplane-digest-source` on the jobs-manager Deployment — `values` | `annotation` | `tag` | `tag-lookup-failed` — so a monitored edge can alert on the fallback. On an upgrade the Deployment MUST exist, so `.Release.IsUpgrade` + an empty lookup ⇒ `tag-lookup-failed` (the anomaly). No hard `fail` (would break `helm diff upgrade`, where lookup is empty too); metadata-only, so it never rolls the pods. #5 (Medium, doc) — documented the env-scoping caveat next to the suspend note in controlPlaneDigest: a CLIENT_ENV/tag change at upgrade time is inert once a digest is seeded (tag dropped when a digest is present), permanently so while suspended. The containment (record the tag alongside the digest, honour the annotation only when it matches) is an A-side change, called out here per the review. Tests: control_plane_digest_test.yaml flips the init case (init stays :tag with a valid annotation present; still honours an operator pin) and adds the four digest-source cases incl. tag-lookup-failed on `.Release.IsUpgrade`. helm-unittest 712/712 across 41 suites; helm lint clean; helm template renders :prod + source `tag` clusterless and `tag-lookup-failed` under --is-upgrade. Mutation-checked: re-seeding the init reddens its guard. Not in this push (replied on-thread): #1 (BLOCKING, flap-lockout downgrade) and #5's actual fix are A-side changes (#1008); #4's misleading e2e comment is already fixed on-branch (a54deb0) and the explicit --atomic negative path is proposed as a CI-validated follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
LukasWodka
left a comment
LukasWodka
left a comment
There was a problem hiding this comment.
Re-review of 1c78acf (delta from 9666cf0: _helpers.tpl +72, jobs-manager-deployment.yaml +24/-1, control_plane_digest_test.yaml +106/-2).
Verified in the delta.
- The wait-for-mysql init container is no longer seeded from the last-refreshed annotation and floats on
:tag, while an explicitimages.jobsManager.digestpin still applies. That answers the High on the init site: an init container pinned to a digest a node never pulled isInit:ImagePullBackOffwith no opt-out, andset imagenever reconciles that site. Both cases are pinned by tests. tracebloc.io/controlplane-digest-sourcerecords which branch produced the image ref (values/annotation/tag/tag-lookup-failed), withtag-lookup-failedgated on.Release.IsUpgradeso a first install is not misread as a read failure. The five source tests cover every branch, including the empty-lookup-on-upgrade anomaly. This makes the fail-openlookupobservable, which is the right containment given a hardfailwould breakhelm diff upgrade.
Nit, not blocking. controlPlaneDigestSource re-implements controlPlaneDigest's branch order and relies on a "must stay in lockstep" comment. That is a restated rule, not a derived one: a future change to the digest helper's priority silently desynchronises the provenance. Consider having one helper emit both (for example a source|digest pair the callers split) so there is a single decision.
Verdict: hold, not a change-request. Three threads stay open and none are mine: @shujaatTracebloc's BLOCKING flap-lockout finding on _helpers.tpl, his env-scoping one on the deployment, and Bugbot's High that duplicates the blocking thread. Saqlain has agreed the containment lives on the runtime side in #1008, and the bugbot / review job is red on this head because of that open thread. So this PR is still gated on #1008 landing first, exactly as my earlier approval said, and I am not approving over a blocking thread and a red check. Once #1008 is in and the threads above are resolved, ping me and I will approve on the next pass.
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Re-reviewed at 1c78acf. The three asks I raised that were in scope for B are genuinely fixed in code, not just in replies — I checked each against the tree:
- init container (High) —
wait-for-mysqlnow renderstracebloc.imagewith.Values.images.jobsManager.digestdirectly and never touches the annotation seed. The two unit cases pin both halves (stays:tagwith a valid annotation present; still honours an operator pin). Right fix, right reasoning. lookupfails open (Medium) —tracebloc.controlPlaneDigestSource+tracebloc.io/controlplane-digest-sourceis the recorded-signal shape I was after, and the load-bearing distinction is implemented correctly: on an upgrade the jobs-manager Deployment must exist, so.Release.IsUpgradeplus an empty lookup is a read failure, rendered astag-lookup-failed. Metadata-only, so no pod roll. Five test cases including the first-install non-anomaly.- doc/e2e asks — the operator-facing surfaces (
values.yaml#569 NOTE, first-observation block,values.schema.jsonimageRefresh) now carry the narrowing, and the e2e asserts the renderedimage:digest on both paths rather than annotation survival.
I also re-derived the fallback safety rather than taking it on trust: controlPlaneDigest regexMatches ^sha256:[a-f0-9]{64}$ before emitting and returns "" everywhere else, and tracebloc.image guards on if $digest, so every degraded path renders :tag — there is no repo@ malformed-ref reachable here. Whitespace trimming is clean in both defines, and the exact-string assertions in the new suite would redden if it were not. No new env keys, so the --set-string rule is not touched.
Still blocked, on three things:
-
CI is red and correctly so.
bugbot / reviewfails with one OPEN High — "Flap lockout pins a stale digest" — which is the duplicate of my BLOCKING thread on_helpers.tpl. That gate is derived from the threads, so a push will not clear it; only the fix (or a resolve) plus a re-run will. -
#1008 has not landed, and as it stands it does not yet carry either routed fix. I read A's current diff: the live-spec reconcile is inside the
recorded == latestbranch,annotate_argsis still one batched annotate after all three rollout waits, and there is nolast-refreshed-<image>-tagkey anywhere. So the flap-lockout downgrade and the env-scoping gap are uncovered by A as well as by B today. A is still open and blocked on its own gates. Agreed with your routing — the annotation should not be taught to lie and then be second-guessed at render time — but B stays held until A actually carries it. -
Chart version collides with #1008. Observed directly just now:
develop= 1.9.109, #1008 headeafc68c= 1.9.110, this PR = 1.9.110 — the two Chart.yaml blobs are byte-identical. This PR needs 1.9.111 (versionandappVersionin lockstep). Worth flagging thatchart content ⇒ Chart.yaml version bumpis green and cannot help here: it diffs${BASE_SHA}...HEADagainst the frozen merge base at 1.9.109, so it structurally cannot see a number a sibling PR claimed after the fork. The PR body's "Chart conflict on rebase is onlyclient/Chart.yaml— keep 1.9.110" is now the wrong instruction and should say 1.9.111.
Two non-blocking notes on the new helper, either fine to take here or defer:
controlPlaneDigestSourcere-statescontrolPlaneDigest's three-branch resolution verbatim, kept in lockstep by a comment plus parallel test cases. A shared sub-helper that returns the branch, with the digest helper deriving from it, would make drift impossible rather than merely detectable.- The provenance annotation is rendered once with
annotationImage: "jobs-manager"hard-coded, but pods-monitor and resource-monitor read their own annotation keys — soannotationcan be reported while one of those sites actually fell back to:tag. Thetag-lookup-failedvalue is unaffected (same single lookup), so the alert this helper exists for is still accurate; this is a completeness gap, not a wrong signal.
Happy to approve as soon as A lands with the per-workload annotate (and the tag-scoping key), this rebases to 1.9.111, and the Bugbot gate goes green on a re-run.
LukasWodka
left a comment
LukasWodka
left a comment
There was a problem hiding this comment.
Re-requested at 1c78acf, the same head I commented on, so nothing in the diff has changed and my earlier notes stand as written.
On the flap-lockout thread: I agree with option (a), and I checked that #1008 does not carry it yet. At #1008's current head the last-refreshed-* annotations are still one batched kubectl annotate after all three set image + rollout status pairs, and the flap guard's exit 0 runs before any of them. So the lag state Shujaat described is live in A today, and B's helper would render it. Your 07:00 note on #1008 is the right place for the fix; once A annotates each workload right after its own rollout status and before the flap guard exits, the recorded == latest reconcile covers the case and this helper only ever renders a digest the live spec has reached.
Held, and what unblocks it, in order:
- #1008 gets the per-workload annotate, goes green, and lands.
- This branch rebases onto it and takes chart 1.9.111.
- The Bugbot thread here is the same finding; it can be resolved as a duplicate once (1) is in, and
bugbot / reviewneeds to be green on the rebased head. - Shujaat's BLOCKING thread is his to close.
I will re-review on the rebased head without needing another re-request. Nothing further from me on this head.
…ent-runtime#199) develop took 1.9.109 (client#1017), and A (#1008) rebumped to 1.9.110, so B at 1.9.110 would collide with A on the release train — one version number covering two different charts, exactly what the version-bump guard exists to prevent (@shujaatTracebloc on #1008). Bump B one above A: version + appVersion to 1.9.111, and re-point the feature-landing version in the doc comments (_helpers.tpl, image-refresh CronJob header, MIGRATIONS.md, SEAL-CHECK.md, e2e-auto-upgrade.sh) from 1.9.110 to 1.9.111. helm lint clean; helm unittest 712/712 across 41 suites; shellcheck + bash -n clean on the edited e2e. A→B merge order unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
bugbot run |
LukasWodka
left a comment
LukasWodka
left a comment
There was a problem hiding this comment.
Re-review at 3b4dce6. The rebump to 1.9.111 is right and every version reference moved with it (helper header, cronjob comment, MIGRATIONS, SEAL-CHECK, the e2e script). Nothing else in the chart changed, so my earlier notes stand.
Blocker, and the only one: this push commits seven review-reply drafts into the repository.
replies/bugbot-flap.md
replies/bugbot-lookup.md
replies/bugbot-tag.md
replies/clientenv.md
replies/flap.md
replies/init.md
replies/lookup.md
They are your thread replies verbatim, sitting at the repo root of a public chart repo. Nothing consumes them, the chart guard does not look there, and they carry reviewer names and cross-repo ticket references that #1020 and #1022 just spent two PRs keeping out of this repo. Please drop them from the branch (git rm -r replies/ and amend, or a follow-up commit) before anything else here moves.
Everything else is as before and unchanged by this head: Shujaat's BLOCKING flap-lockout thread waits on the A-side annotate ordering in #1008, which is not pushed there yet, and this branch needs to rebase once it lands. CI is running. Once the stray files are gone and #1008 carries the fix, I re-review on the rebased head without another re-request.
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
There was a problem hiding this comment.
A (#1008) merged at 07:22 — and it did not carry either fix we routed to it. Flagging that now rather than at your next push, because the plan we agreed on has quietly lost its home and I would rather you hear it from me than rediscover it.
I approved #1008 on its own merits — the rebump landed, CI was green, and the scope it actually shipped was correct. What it never grew was the A-side work this PR is waiting on. Checked against develop at 3fbcb11:
- Per-workload annotate: not there. The
for entry in "$@"loop still runs 502→757 withannotate_argsaccumulating inside it and applied in a single batchedkubectl annotateafter the loop. The shape we agreed on — write each image'slast-refreshed-*immediately after that workload's ownrollout status, before the flap guard'sexit 0— is not implemented. - Tag/env-scoped key: not there.
grep -c 'last-refreshed-[^"]*-tag'returns 0 across bothimage-refresh-cronjob.yamland_helpers.tplon develop.last-refreshed-appears 4 times, all unscoped.
So both concerns need a new PR against develop, not #1008. Concretely:
PRRT_kwDOMzdex86gtjSW(still open here) — the flap-lockout downgrade. Your reasoning for routing it to A was right and I am not reopening it: the annotation should be made truthful by construction rather than second-guessed at render time. That argument is unchanged; only its destination is.- The env/tag-scoping containment you documented in
1c78acfand resolved on my other thread — the doc landed, the containment did not, and it was also A-side.
Neither is a criticism of the merge. A shipped what it was scoped to ship. But "tracked on #1008" stopped being true fifteen minutes ago, and a routed fix whose destination has merged is exactly how a known defect becomes a forgotten one.
Also, this PR is now CONFLICTING — the Chart.yaml collision I flagged, arriving on schedule. develop is 1.9.110 (A's), this branch is 1.9.111. Merge develop and keep 1.9.111; the numbers are already sequenced correctly, so it is a one-line resolution and no further rebump is needed.
Where that leaves the gate: CONFLICTING/DIRTY, one mutation-check still running, and PRRT_kwDOMzdex86gtjSW open. Nothing here is new review debt on your side beyond the merge — the three asks I raised last round are all still verified fixed in code, and the 1.9.111 rebump was exactly right.
Happy to approve once the conflict is resolved, the flap-lockout fix has a landing branch (here or a new A2 — your call, and if you would rather do it here I will not hold the routing against you), and CI is green.
# Conflicts: # client/Chart.yaml # client/templates/image-refresh-cronjob.yaml # client/values.yaml
|
bugbot run |
LukasWodka
left a comment
LukasWodka
left a comment
There was a problem hiding this comment.
Re-requested at a7553fc. This head merges develop, so the conflict is gone, the chart stays at 1.9.111 above develop's 1.9.110, and the branch now carries the merged CronJob side. Good.
The one blocker is unchanged. replies/ is still in the tree at this head, all seven files:
replies/bugbot-flap.md replies/bugbot-lookup.md replies/bugbot-tag.md
replies/clientenv.md replies/flap.md replies/init.md replies/lookup.md
git rm -r replies/ and push; that is the whole ask. My change-request stands until then, and I will re-review without another re-request as soon as the tree is clean.
Still open beyond that, unchanged: Shujaat's BLOCKING thread waits on the per-workload annotate ordering, which the merged CronJob does not yet have and which needs its own follow-up PR; CI is running on this head.
shujaatTracebloc
left a comment
shujaatTracebloc
left a comment
There was a problem hiding this comment.
Conflict resolved cleanly and the version is right — 1.9.111 over develop's 1.9.110, exactly as sequenced. Thanks for turning that around fast.
One thing to fix before this merges: seven scratch files came along with the merge.
replies/bugbot-flap.md
replies/bugbot-lookup.md
replies/bugbot-tag.md
replies/clientenv.md
replies/flap.md
replies/init.md
replies/lookup.md
All seven are status=added, one line each, and replies/ does not exist on develop — so this PR would create it. The content is your draft review-reply text: replies/flap.md opens "Real, and you're right that #1008 as written doesn't heal it…", which is verbatim what you posted on the flap-lockout thread. Classic git add -A sweep of a scratch directory.
Nothing in CI will catch this — there is no lint rule for stray markdown, and it does not affect the chart — which is exactly why it is worth a second pair of eyes. git rm -r --cached replies/ and a line in .gitignore if you want to keep using that directory locally.
On the rest — no new asks.
The image-refresh-cronjob.yaml change (+30/−11) is comments only, and the re-framing is right: documenting the revert as HANDLED from both sides, chart-side here from 1.9.111 and script-side in the merged #1008, is a more accurate description of the world now that A has landed. Reclassifying the flap-lockout residue from "blocked on A" to a tracked follow-up is the correct response to A having merged without it — I would rather see it named in the comment than silently dropped. The first-observation roll block is a genuinely useful addition: spelling out that the next chart bump rolls all three workloads once for byte-identical content, and that --atomic --wait can time out on a slow single-node edge and re-attempt, is the kind of thing an operator hits at 2am and finds nowhere.
Still open, unchanged: PRRT_kwDOMzdex86gtjSW — the flap-lockout downgrade. I am leaving it open deliberately, not as a merge condition argument but as the marker that the fix now needs a home; per my last comment it is a new PR against develop, and I confirmed the containment is still absent (grep -c 'last-refreshed-[^"]*-tag' in image-refresh-cronjob.yaml returns 0 both here and on develop — the only hits in this diff are a doc comment and one of the stray files above). Your call whether that lands here or as an A2; say which and I will treat the thread accordingly.
CI is mid-run on the new head. Drop the replies/ directory and, once it is green, I will approve — the chart work itself has been ready for a while.
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a7553fc. Configure here.
|
Heads-up from a neighbouring PR, not a review: #1028 routes the same five image lines (the This PR's |
|
Not a re-review — the head is still a7553fc, the same commit my change-request is on, so nothing has changed on my side: Two new facts since then that you will hit on the next push:
Same deal as before: |
|
Still the same head ( The blocker is unchanged and is the only thing stopping this: Drift has gotten worse while this sat:
Everything else is in good shape: CI is fully green, Bugbot is clean, and Shujaat's flap-lockout thread is settled to a pure sequencing marker on Same offer as before: |
|
One addition to my 09:51 rebase notes, still the same head (
Everything else stands as written in the earlier comments: |
3 participants
Fix B of A+B for the image-drift class. Companion to #1008 (which fixes the
image-refreshside). This one makeshelm upgradenever revert the digest in the first place, closing the window #1008 only bounds.Refs tracebloc/client-runtime#199
The problem (recap)
Control-plane images render as
repo:tag+IfNotPresentwhen unpinned (pertracebloc.controlPlanePullPolicy, #569/#705 — deliberately, to survive an offline restart).image-refreshkeeps them current out-of-band viakubectl set image repo@digest. Buthelm upgrade --reset-then-reuse-values(the hourlyauto-upgrade) re-renders the bare:tagand drops that pin; on a node whose:taglayer is stale the pod then silently runs an old control-plane image (client-runtime#199). #1008 catches this on the nextimage-refreshtick (≤ ~15 min); this PR removes the revert entirely.The fix (this PR — "B")
New
tracebloc.controlPlaneDigesthelper resolves the effective digest:images.<name>.digest) — wins, unchanged;tracebloc.io/last-refreshed-<image>-digestannotation vialookup;""(bare:tag).Wired into all five control-plane image sites (jobs-manager init + api + pods-monitor, requests-proxy following the jobs-manager annotation, resource-monitor). A
helm upgradenow rendersrepo@<current-digest>instead of reverting to:tag.imageRefresh.suspendKEEPS the pin (reviewer reconciliation)An earlier round gated the digest render on
not suspend, so a suspended edge un-pinned to:tag+Always. @LukasWodka showed that is wrong:suspendis used to freeze an edge during an incident, and un-pinning re-renders jobs-manager (Recreate), requests-proxy and the resource-monitor DaemonSet onto the floating tag — three unplanned rollouts plus a downgrade to whatever:tagpoints at now — contradicting thevalues.schema.jsonpromise that suspend "pause[s] without removing the resources". So suspend now stops polling, not un-pinning: a suspended edge keeps rendering the last-observed@digest+IfNotPresent. A newly joined node still pulls a real, previously-resolved digest — not a missing one — so the original newly-joined-node concern is still answered.Safety / caveats to review
image:field; the helperregexMatches it against^sha256:[a-f0-9]{64}$before rendering, so a malformed value degrades to""→:tag(the safe fallback) instead of an unstartable ref.lookupis empty duringhelm template/helm diff/ the FIRST install → helper returns""→:tag. Correct there (nothing pinned yet).helm diffwill show a:tag→@digestchurn that won't actually happen on apply — worth a note for anyone who diffs upgrades.@D0where the live spec still says:tag, rolling jobs-manager/requests-proxy/DaemonSet once for byte-identical content (--atomiccan time out on a slow single-node edge). One-time cost, documented in the image-refresh CronJob header.controlPlanePullPolicyhelper note, the auto-upgrade#569NOTE and first-observation block invalues.yaml, and theimageRefreshdescription invalues.schema.json. (docs/MIGRATIONS.mdcovers a different narrowing — the server-side-apply.image-conflict remedy — not this offline-restart one.)Testing
client/tests/control_plane_digest_test.yaml: helm-unittest 0.5.2 fakeslookupviakubernetesProvider, so the lookup path is now covered without a cluster — the five image sites, operator-pin precedence, malformed →:tag, the suspend-keeps-pin decision, and the disabled/mirror/first-install:tagedges. Mutation-verified: discarding the lookup reddens the digest cases, re-adding the suspend gate reddens the suspend case, neutering theregexMatchreddens the malformed case.scripts/tests/e2e-auto-upgrade.shseeds a valid 64-hex annotation and asserts the rendered jobs-managerimage:digest after both--reuse-values(path 1) and--reset-then-reuse-values(path 2), not merely that the annotation survived.helm lintclean,helm templaterenders:tagclusterless and@digeston the operator pin, 712/712 unit tests across 41 suites, Chart.yaml 1.9.111.Merge order — B depends on A (#1008), land A first
In the steady state (
recorded == latest) A and B compose. But the annotation lags the live spec in two reachable states, and in both B renders a digest that pulls the workload backwards:set imagemoved the spec,set -eexits before the annotate, so spec = NEW while the annotation = OLD;helm rollback, which re-applies a stored manifest (it does not re-render), restoring the digest that was live at that revision's render time — so a rendered digest means a rollback can undo a security refresh.Both are self-healing only because A's live-spec reconcile re-pins within a tick. So #1008 (A) must land first; B alone would strand those states. Chart conflict on rebase is only
client/Chart.yaml— keep 1.9.111.Not changing the pull policy
Always-when-unpinned would re-break #569/#705 (offline restart →ImagePullBackOff).🤖 Generated with Claude Code
Note
Medium Risk
Changes control-plane image resolution on every upgrade for docker.io + image-refresh edges (one-time digest rollouts, offline pull failures, annotation lag until #1008); mitigated by validation, suspend semantics, and observability annotations.
Overview
Chart 1.9.111 stops
helm upgradefrom undoing image-refresh’s out-of-band digest pins (client-runtime#199): control-plane workloads no longer re-render as barerepo:tagwhen image-refresh has already observed a digest.New helpers
tracebloc.controlPlaneDigestandtracebloc.controlPlaneDigestSourceresolve the effective image digest (operator values pin → last-refreshed annotation on jobs-manager vialookup→ floating tag), validate annotation shape, keep the pin whenimageRefresh.suspendis true, and expose provenance on jobs-manager astracebloc.io/controlplane-digest-source(includingtag-lookup-failedon upgrades when lookup is empty).Wiring:
api,pods-monitor, requests-proxy, and resource-monitor use the helper forimage:and pull policy;wait-for-mysqlinit stays on:tag(or explicit operator pin) to avoid offline/k3dInit:ImagePullBackOff.Docs narrow the #569 offline-restart guarantee and update MIGRATIONS/SEAL-CHECK for steady-state digest rendering. Tests: clusterless
control_plane_digest_test.yamland e2e auto-upgrade checks on the rendered jobs-manager digest, not just the annotation.Reviewed by Cursor Bugbot for commit a7553fc. Bugbot is set up for automated code reviews on this repo. Configure here.