Skip to content

fix(chart): re-image control-plane pods by digest instead of relying on Always (#569) - #705

Merged
shujaatTracebloc merged 7 commits into
developfrom
fix/569-control-plane-offline-restart
Aug 13, 2026
Merged

fix(chart): re-image control-plane pods by digest instead of relying on Always (#569)#705
shujaatTracebloc merged 7 commits into
developfrom
fix/569-control-plane-offline-restart

Conversation

@shujaatTracebloc

@shujaatTracebloc shujaatTracebloc commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Closes #569. Splits out of #552 (the requests-proxy hardcoded-Always half landed in #570).

The problem

The always-running control-plane pods rendered imagePullPolicy: Always, so a Docker Desktop / WSL2 restart forced a registry round-trip and landed in ImagePullBackOff even with the image already cached in containerd. The edge showed Offline until docker.io was reachable again — up to ~6h behind Docker Hub's anonymous pull-rate limit.

Always could not simply be flipped, because it is the update mechanism: image-refresh's kubectl rollout restart only picks up a new build because the pull policy re-resolves the floating tag. Offline-safety and restart-driven updates are mutually exclusive unless the image reference changes on update.

The decision: option B (digest-on-update), not A (prodDigest pin)

The ticket framed both. B won on two grounds:

  • A is prod-only by construction (gated on CLIENT_ENV == "prod"). The incident client runs dev, so A would not have fixed the reported incident.
  • A couples releases across repos. These images ship from client-runtime, whose release train ran twice on 2026-08-12 and again on 08-13. Under A, every prod cut needs a client chart PR + lockstep version bump + chart publish before prod edges see it. ingestor.prodDigest shows how that ages: it is currently pinned to v0.8.2 while its own float resolves v0.8.4. For the ingestor that lag is deliberate (an ordering ceiling is being enforced); for jobs-manager there is no ceiling, so lag is just an unshipped fix.

The objections to B were weaker than the ticket implied. Auto-upgrade only runs helm upgrade when the published chart version is newer (sort -V compare, auto-upgrade-cronjob.yaml), so the re-render is not an hourly revert — it happens only on a chart release.

What changed

Pull policy. All four call sites render IfNotPresent unconditionally — jobs-manager (api + the pods-monitor-container sidecar), requests-proxy, resource-monitor.

Update path. image-refresh swaps kubectl rollout restart for kubectl set image <workload> <container>=repo@digest, across three workloads instead of one:

workload image before this PR
deployment/<release>-jobs-manager jobs-manager + pods-monitor reconciled (both containers now re-imaged in one patch → one rollout)
deployment/<release>-requests-proxy the same jobs-manager image never reconciled
daemonset/<release>-resource-monitor resource-monitor never reconciled

Two bugs the ticket did not name, both fixed here:

  • requests-proxy was never refreshed. The CronJob's DEPLOYMENT_NAME was only <release>-jobs-manager. So requests-proxy stayed on its old build until some unrelated restart, then jumped to whatever the tag pointed at — two pods routinely running different builds of one image. It now follows the jobs-manager digest in the same tick, with no second registry HEAD. images.requestsProxy.digest opts it out.
  • resource-monitor had no deliberate update path at all. A chart release does not change its image ref either, so it only ever moved when a pod happened to restart. That is also why a naive flip to IfNotPresent would have frozen it permanently — it needed the reconcile to come with it.

RBAC. requests-proxy needed no new rule (same-namespace Deployment, already covered). resource-monitor needed a second Role in tracebloc-node-agents — the one genuinely new piece of trust surface here:

  • get + patch are resourceNames-scoped to this chart's own DaemonSet. That namespace runs under the privileged PSA profile, so the CronJob cannot touch any other DaemonSet there.
  • list + watch cannot be resourceNames-scoped (the authorizer ignores resourceNames for collection verbs) and rollout status needs both, so they stay namespace-wide and read-only. That residual widening is deliberate and asserted with equal so it cannot grow silently.
  • Not rendered at all when resourceMonitor: false.

Helper. imageRefreshEnabled now requires all three refreshed images pinned before retiring the CronJob. Pinning only the two class-1 images used to render it away, which would have left the DaemonSet with no update path.

Private mirrors. The script resolves digests from docker.io, so pinning one onto a global.imageRegistry reference could pin an image the mirror does not hold. It logs and goes inert. Under rollout restart that mismatch was merely useless; with set image it has to fail closed.

Bug caught during verification

IMAGE_REGISTRY first rendered as "" — values.yaml ships global.imageRegistry: "", so the key exists and dig's default never applies. The mirror guard would then have seen "" != "docker.io" and gone inert on every default install, silently disabling auto-refresh fleet-wide. Fixed with a trailing | default "docker.io"; there is a test whose comment explains why that is not redundant.

Deliberate trade-offs — please review these specifically

The first-tick contract is kept. Re-imaging on the first tick would rewrite repo:tag to repo@digest for byte-identical content on every fresh install — rolling the Deployment, and the DaemonSet on every node, for nothing. So a fresh edge runs repo:tag until the first real digest change: restart-safe offline (which is the point of this PR), just not yet reproducible.

Two bounded limitations, documented in the script header rather than hidden. Both self-heal at the next upstream release, and neither can break a running edge — IfNotPresent holds regardless.

  1. Helm re-render. A chart version bump re-renders repo:tag and reverts the pin. This tick will not re-pin, because the annotation still records that digest and recorded == latest no-ops. Contrary to what the ticket predicted, refresh does not self-heal this; the edge floats until the next upstream release.
  2. Pre-existing skew is prevented, not repaired. An edge upgrading into this version may already have requests-proxy and jobs-manager on different builds. They converge on the next digest change; this script never compares pod-vs-pod.

The proper fix for both is the same: reconcile against each workload's live container image instead of a shared annotation. set image makes that possible for the first time — rollout restart was a blind action, which is why the annotation exists at all. I kept it out of this PR to hold the diff to one change; happy to fold it in if you would rather have it here.

Verification

helm unittest ./client
Tests: 5 failed, 5 errored, 405 passed, 410 total

405 passed, up from 395. The failures are exactly develop's pre-existing baseline (5 failed / 5 errored) — confirmed by diffing failing test names against a stashed baseline run on develop. Zero new failures.

Also green: helm lint, all four client/ci/*-values.yaml render, scripts/check-style.sh, scripts/check-facts.sh, scripts/gen-manifest.sh --check.

Chart bumped 1.9.38 → 1.9.39, version + appVersion in lockstep.

Note

I could not read #565, referenced by the ticket as the installer-side context — it does not resolve in client or any other tracebloc repo I have access to. If it constrains this design, it did not feed into it.

🤖 Generated with Claude Code


Note

High Risk
Changes core control-plane image update mechanics, CronJob RBAC across namespaces, and pull policies—misconfiguration could freeze images or break refresh fleet-wide.

Overview
Fixes offline Docker/WSL restarts that hit ImagePullBackOff because control-plane pods used imagePullPolicy: Always. Updates now come from kubectl set image repo@digest (replacing rollout restart) so pods can use IfNotPresent when a real update path exists.

Image-refresh reconciles three workloads: jobs-manager (+ pods-monitor containers), requests-proxy (same jobs-manager image, same digest in one tick), and resource-monitor (cross-namespace DaemonSet). Adds a separate RBAC Role in the node-agents namespace (tracebloc.imageRefreshNodeAgentsName) so Role names do not collide when node-agents shares the release namespace. requests-proxy falls back to images.jobsManager.digest when its own digest is unset; deployment name is centralized via tracebloc.requestsProxyName.

tracebloc.controlPlanePullPolicy unifies pull policy: IfNotPresent when digest-pinned or when refresh is enabled on docker.io; Always on private mirrors or imageRefresh.enabled: false so floating tags still update via restart. tracebloc.resourceMonitorRefreshPinned aligns CronJob retirement with runtime skip when resourceMonitor: false or digest pinned.

Operational fixes: mirror installs fail closed (script exits 0, reconcile inert); activeDeadlineSeconds default 3600 for three sequential rollouts; resource-monitor maxUnavailable: 10%. Chart 1.9.39; schema/values/docs and helm unittest coverage expanded.

Reviewed by Cursor Bugbot for commit c4d6015. Bugbot is set up for automated code reviews on this repo. Configure here.

…on Always (client#569)

The always-running control-plane pods rendered `imagePullPolicy: Always`, so a
Docker Desktop / WSL2 restart forced a registry round-trip and landed in
ImagePullBackOff even with the image already cached in containerd — the edge
came back only once docker.io was reachable, up to ~6h behind Docker Hub's
anonymous pull-rate limit.

`Always` could not simply be flipped: it IS the update mechanism. The
image-refresh CronJob's `kubectl rollout restart` only picks up a new build
because the pull policy re-resolves the floating tag. Offline-safety and
restart-driven updates are mutually exclusive unless the image REFERENCE
changes on update. So the reference is now what changes.

- All four control-plane call sites render IfNotPresent unconditionally:
  jobs-manager (api + pods-monitor sidecar), requests-proxy, resource-monitor.
- image-refresh swaps `rollout restart` for `kubectl set image repo@digest`.
- Two workloads come under refresh for the first time, both quietly broken
  before: requests-proxy runs the SAME jobs-manager image but was never
  reconciled (it skewed until an unrelated restart, then jumped to whatever the
  tag pointed at), and resource-monitor had no deliberate update path at all.
- requests-proxy follows the jobs-manager digest in the same tick, no second
  registry HEAD. images.requestsProxy.digest opts it out.
- resource-monitor needs a second Role in the node-agents namespace: get/patch
  resourceNames-scoped to the one DaemonSet, list/watch namespace-wide and
  read-only (RBAC ignores resourceNames for collection verbs, and
  `rollout status` requires them). Not rendered when resourceMonitor: false.
- imageRefreshEnabled now requires all three refreshed images pinned before
  retiring the CronJob; pinning only the two class-1 images used to render it
  away, leaving the DaemonSet with no update path.
- Private mirrors: the script resolves digests from docker.io, so pinning one
  onto a mirrored reference could pin an image the mirror does not hold. It
  logs and goes inert. Under `rollout restart` that mismatch was merely
  useless; with `set image` it has to fail closed.

The first-tick "record without acting" contract is kept deliberately: re-imaging
on the first tick would rewrite repo:tag to repo@digest for byte-identical
content on every fresh install, rolling the Deployment and the DaemonSet on
every node for nothing. A fresh edge therefore runs repo:tag until the first
real digest change — restart-safe offline, just not yet reproducible.

Two bounded limitations are documented in the script header rather than hidden:
a chart version bump re-renders repo:tag and this tick will not re-pin (the
annotation still matches), and skew predating this change is prevented but not
repaired. Both self-heal at the next upstream release and neither can break a
running edge. The proper fix for both is reconciling against each workload's
live container image instead of a shared annotation — which `set image` makes
possible for the first time, and which is a deliberate follow-up.

Verified: helm unittest 405 passed, failures exactly develop's pre-existing
baseline (5 failed / 5 errored, diffed against a stashed baseline run — zero
new). helm lint clean, all four client/ci value sets render, check-style and
check-facts pass, gen-manifest --check up to date. Chart 1.9.38 -> 1.9.39,
version + appVersion in lockstep.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shujaatTracebloc shujaatTracebloc self-assigned this Aug 13, 2026
@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review August 13, 2026 12:27
Comment thread client/templates/image-refresh-cronjob.yaml Outdated
Comment thread client/templates/_helpers.tpl
…le (client#569)

1. Jobs-manager pin skipped requests-proxy (_helpers.tpl:219).

   requests-proxy runs the jobs-manager IMAGE, and both the helper and the
   values docs claimed it follows the jobs-manager pin — but the Deployment read
   only `images.requestsProxy.digest`. Pinning jobs-manager ALONE was therefore a
   silent trap: the proxy kept rendering the floating `repo:tag` while
   jobs-manager ran the pinned digest, AND image-refresh skips pinned images, so
   nothing ever wrote a `set image` for the proxy either. It froze on the tag
   indefinitely, running a different build of the same image — precisely the skew
   #569 exists to close, re-introduced through the pinning path.

   `images.requestsProxy.digest` now falls back to `images.jobsManager.digest`
   when empty, so the claim is true by construction. The proxy key remains an
   explicit per-workload override. The helper comment now records that the test
   depends on that fallback, so removing it forces requests-proxy back into the
   "nothing left to do" test.

2. Refresh budget too small for three rollouts (image-refresh-cronjob.yaml).

   A tick now waits on up to three sequential `rollout status` calls, each up to
   rolloutTimeout (10m), while activeDeadlineSeconds was a hardcoded 1800 —
   exactly 3 x 10m, zero headroom. Blowing it is not a benign timeout: the Job is
   killed mid-wait, so under `set -e` the post-success annotate never runs, the
   recorded digest never advances, and the shared `refresh-attempt` counter stays
   incremented. Three such ticks trip the #563 flap lockout for EVERY
   control-plane image at once, while the CronJob still looks healthy.

   - activeDeadlineSeconds is now `imageRefresh.activeDeadlineSeconds`, default
     3600 (3 x the default rolloutTimeout plus 100% slack), with a schema entry
     (minimum 60) and the raise-both-together constraint documented on
     rolloutTimeout.
   - The resource-monitor DaemonSet gets an explicit
     `updateStrategy.rollingUpdate.maxUnavailable: 10%`. Kubernetes defaults to
     maxUnavailable: 1, so a digest change converged in (nodes x pull+start) and
     blew the 10m wait on any multi-node cluster long before the image was bad.
     Safe to widen here specifically: resource-monitor is a read-only node
     metrics reader, so a briefly absent pod degrades scheduling telemetry and
     nothing else. Kubernetes rounds 10% down and floors it at 1, so small
     clusters keep today's one-at-a-time behaviour.

Verified: helm unittest 411 passed, failures still exactly develop's pre-existing
baseline (5 failed / 5 errored, diffed against a stashed baseline run — zero
new). helm lint clean, check-style clean. Rendering confirms requests-proxy
resolves to the jobs-manager digest when only jobsManager is pinned,
activeDeadlineSeconds renders 3600 and honours an override, the DaemonSet
carries maxUnavailable 10%, and the schema rejects a sub-60 deadline.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread client/templates/image-refresh-cronjob.yaml
…#569)

Bugbot, High severity — a regression introduced by this PR's first commit.

Making `imagePullPolicy: IfNotPresent` UNCONDITIONAL removed the only update
path from every edge where the replacement mechanism cannot run. `Always` on a
floating tag is not merely offline-fragility; it IS an update path — restart the
pod and the kubelet re-resolves the tag. Two configurations relied on exactly
that and were left frozen on their cached image forever, with a green CronJob
and no signal:

  * `global.imageRegistry` (private mirror). The reconcile resolves digests from
    docker.io, so this PR deliberately makes it inert there rather than pin a
    digest the mirror may not hold. With IfNotPresent on top, syncing the mirror
    and restarting kept serving the cached tag.
  * `imageRefresh.enabled: false`. values.schema.json has always promised these
    operators the image stays put "until manual restart" — true only with Always.

The inert-path log message this PR added made it worse by telling operators to
"sync your mirror and restart the workloads", which under IfNotPresent does
nothing. That guidance is now correct because the policy is correct.

The policy is now resolved per image by one helper,
`tracebloc.controlPlanePullPolicy`, so the four call sites cannot disagree:

  1. explicit `digest` pin       -> IfNotPresent (immutable reference; updates
                                    come from changing the pin)
  2. reconcile can run here      -> IfNotPresent (`set image` changes the
                                    REFERENCE, which is what the kubelet pulls)
     i.e. the CronJob renders AND images come from docker.io
  3. neither                     -> Always       (floating tag + restart is the
                                    only update path that edge has)

The trade is deliberate: offline-restart safety is delivered precisely where the
digest reconcile can deliver updates. An edge that opts out of the mechanism
keeps pre-#569 semantics rather than silently freezing — a frozen control plane
with no signal is worse than a restart that needs the network.

Verified by rendering the full matrix: default and all-pinned resolve
IfNotPresent across jobs-manager (both containers), requests-proxy and
resource-monitor; mirror and refresh-disabled resolve Always across all of them;
a pin plus refresh-disabled correctly splits (pinned container IfNotPresent,
unpinned sibling Always). helm unittest 417 passed, failures still exactly
develop's pre-existing baseline (5 failed / 5 errored — zero new). helm lint,
check-style clean; all four client/ci value sets render.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment thread client/templates/_helpers.tpl
shujaatTracebloc and others added 2 commits August 13, 2026 14:53
…e schema (client#569)

Self-audit follow-through, same class as the Bugbot findings on this PR: behaviour
changed and the prose describing it did not. Per CLAUDE.md, a change that makes a
statement false fixes that statement in the same PR.

Docker Hub rate-limit arithmetic (the operationally significant one). #569 raised
the per-tick cost from 2 manifest HEADs to 3 — resource-monitor joined
jobs-manager and pods-monitor; requests-proxy adds none because it reuses the
jobs-manager digest. values.yaml still claimed "2 images x 4/hr = 8/hr, well under
the cap". It is now 12/hr, so 72 per 6h against the anonymous cap of 100, and the
headroom left for OTHER workloads sharing the egress IP fell from ~52 to ~28.

That is worth more than an arithmetic fix: exhausting this cap behind a shared
corporate NAT is one of the failure modes in the incident #569 exists to fix — a
rate-limited edge cannot pull for up to ~6h. The comment now states the real
numbers, names the shared-IP risk, and gives the concrete remedy (a 3/hr schedule
restores ~9/hr, and a digest pin drops that image's HEAD entirely). The default
schedule is deliberately UNCHANGED: it is still within the cap, and quietly
slowing drift pickup fleet-wide is a call for a human, not a side effect of a
doc fix.

values.schema.json descriptions, which were describing the pre-#569 mechanism:
- imageRefresh: said it "rolls the deployment"; now describes `kubectl set image`
  across the three workloads, and why that is what permits IfNotPresent.
- imageRefresh.enabled: says explicitly that disabling falls back to Always, so
  the long-standing "until manual restart" promise stays true.
- imageRefresh.schedule: three manifests per tick, not two, with the numbers.
- imageRefresh.maxRefreshAttempts: re-imaging, not re-restarting; notes the
  counter is shared across workloads.
- images.jobsManager.digest: records that requests-proxy inherits it.
- images.requestsProxy.digest: documents the follow-jobs-manager default (it had
  no description at all), so the pinning trap Bugbot found is discoverable from
  the schema rather than only from the template comment.
- images.podsMonitor / resourceMonitor.digest: descriptions added.

Checked and deliberately NOT changed: docs/SECURITY.md 6.1's `rollout restart`
after a secret rotation restarts the pod to re-read the Secret, not to pull an
image, so it is unaffected by the pull-policy change.

Verified: helm unittest 417 passed, failures still exactly develop's baseline
(zero new). helm lint and check-style clean; schema is valid JSON.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lient#569)

Bugbot, Medium — and, as it notes, the SAME helper-vs-runtime disagreement class
this PR already had to fix for requests-proxy. The rule was written twice and the
two copies drifted:

  * `tracebloc.imageRefreshEnabled` treated resource-monitor as done only when
    `images.resourceMonitor.digest` was set.
  * The CronJob's RESOURCE_MONITOR_PINNED env ALSO treated `resourceMonitor:
    false` as done — correctly, since with no DaemonSet a cross-namespace
    `set image` would just fail the tick.

So `resourceMonitor: false` plus both class-1 images pinned kept rendering a
CronJob that skipped every image and exited green every 15 minutes, forever.
Before #569 that combination retired the CronJob cleanly. It is also exactly the
green-forever-while-doing-nothing failure mode the script's own #571 comment
warns about.

Both consumers now read one helper, `tracebloc.resourceMonitorRefreshPinned`,
which is the single place the "nothing to do for resource-monitor" rule lives:
an explicit digest pin, or the DaemonSet disabled outright. Nil-safe, and an
absent `resourceMonitor` key reads as enabled to match the
`ne .Values.resourceMonitor false` gate on the DaemonSet itself.

Three tests: the combination now retires both the CronJob and its RBAC, and the
opposite direction is guarded too — disabling resource-monitor must NOT retire
the CronJob while jobs-manager or pods-monitor can still drift.

Verified: helm unittest 420 passed, failures still exactly develop's baseline
(zero new). Rendering confirms image-refresh is gone for the retiring
combination (only auto-upgrade's CronJob remains) and present in both keep
cases. helm lint, check-style clean; all four client/ci value sets render.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit c267ed4. Configure here.

Comment thread client/templates/image-refresh-rbac.yaml
shujaatTracebloc and others added 2 commits August 13, 2026 15:05
…client#569)

Bugbot, High. The node-agents Role and RoleBinding added by #569 reused
`tracebloc.imageRefreshName` — the same name as the release-namespace pair.

`nodeAgents.namespace.name` pointing back at the release namespace is a
SUPPORTED layout, not a misconfiguration: node-agents-namespace.yaml documents
it and deliberately skips creating the Namespace in that case. In that layout
both pairs land in one namespace, so the chart rendered two Roles and two
RoleBindings with identical names in identical namespaces. Confirmed by
rendering before the fix:

  Role        tracebloc  t-image-refresh  x2
  RoleBinding tracebloc  t-image-refresh  x2

Helm then either refuses the release or lets the later DaemonSet-only Role
overwrite the deployments Role. The second outcome is the dangerous one: it is
silent, and it strips image-refresh's patch on jobs-manager and requests-proxy.
Because #569 also moves those pods to IfNotPresent, they would be left with no
update path at all — the exact failure this PR exists to prevent, reached
through a different door.

The Role and RoleBinding now use `tracebloc.imageRefreshNodeAgentsName`
(`<release>-image-refresh-node-agents`). The RoleBinding SUBJECT deliberately
keeps the un-suffixed name: there is only one ServiceAccount and it lives in the
release namespace. A distinct name is correct in BOTH layouts — split namespaces
get one Role each, and the collapsed layout gets two complementary Roles
(deployments, daemonsets) bound to the same SA, which is the intended grant.

Verified by rendering both layouts: no duplicate (kind, namespace, name) in
either, subjects and roleRefs resolve to the right objects in both. Tests pin
the new names, the subject/roleRef split, and add a collapsed-layout regression
case. helm unittest 421 passed, failures still exactly develop's baseline (zero
new); helm lint and check-style clean; all four client/ci value sets render.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…pin the rest (client#569)

Proactive follow-up, not a review finding. Three of the five Bugbot findings on
this PR were the same mistake: one side of a two-sided contract moved and the
other did not (the requests-proxy digest pin, the resource-monitor pin signal,
and the node-agents RBAC name). That is one habit, not three coincidences, so I
audited the rest of the diff for the same shape and found two more live
instances — both in contracts #569 itself created or started depending on.

1. Workload names. #569 made image-refresh reconcile workloads BY NAME with
   `kubectl set image`, which gives those names a second consumer. A rename that
   reached only the workload template would leave the CronJob patching something
   that does not exist: the tick fails, the digest record freezes, and the shared
   flap counter eventually locks out refresh for every control-plane image.

   `<release>-requests-proxy` had exactly two call sites — the Deployment, and
   the CronJob env I added — so it is now one definition,
   `tracebloc.requestsProxyName`. resource-monitor already went through
   `tracebloc.resourceMonitorName`.

   `<release>-jobs-manager` is deliberately NOT unified here. It has six call
   sites across five files (NOTES.txt, the PDB, tracebloc.serviceAccountName,
   ...), most of which this change does not otherwise touch, and the repo
   convention is that refactors ship separately from behaviour changes.
   Half-migrating it would BE the bug this commit is about. A contract test pins
   the two sides in the meantime.

2. Container names. `kubectl set image <workload> <container>=<ref>` fails
   outright on a wrong container name, so `api`, `pods-monitor-container`,
   `proxy` and `tracebloc-resource-monitor` are a hard contract between the
   script and the workload templates, previously asserted from neither side.

Tests now pin both sides of everything still spelled out twice: the three
reconcile target names and all four container names, asserted from the CronJob
AND from jobs_manager / requests_proxy / resource_monitor. Renaming either side
alone now fails CI instead of silently breaking refresh on the fleet.

Verified: rendered names are byte-identical before and after the unification
(t-requests-proxy in both the Deployment and REQUESTS_PROXY_DEPLOYMENT), and the
requests-proxy Service selector is untouched — it matches the pod label
`app: requests-proxy`, not the Deployment name. helm unittest 426 passed,
failures still exactly develop's baseline (zero new); helm lint and check-style
clean; all four client/ci value sets render.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@saqlainsyed007 saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — reviewed for correctness; no defects found.

What this does

Fixes the offline-restart ImagePullBackOff (#569) by dropping imagePullPolicy: Always on the four always-running control-plane images in favour of IfNotPresent only where a non-Always update path exists, and swaps image-refresh's action from kubectl rollout restart to kubectl set image repo@digest across three workloads (jobs-manager+pods-monitor, requests-proxy, resource-monitor).

Correctness verification

Traced the load-bearing logic end-to-end; all internally consistent:

  • tracebloc.controlPlanePullPolicyIfNotPresent on a digest pin, or when refresh can actually reconcile the image (imageRefreshEnabled ∧ docker.io); Always otherwise. No path renders IfNotPresent for an image with no update mechanism (verified across mirror / imageRefresh.enabled:false / per-image-pin permutations, incl. requests-proxy's jobs-manager-digest fallback).
  • Mirror guard fails closed (exit 0, reconcile inert) and the pull policy stays Always there — the two agree, so no frozen-control-plane-with-green-CronJob.
  • IMAGE_REGISTRY / $mirror both carry the | default "docker.io" needed because global.imageRegistry: "" exists (dig's default never applies) — consistent in template and env.
  • Cross-namespace RBAC: second Role in node-agents is resourceNames-scoped for get/patch; list/watch namespace-wide+read-only (required by rollout status, cannot be name-scoped); RoleBinding correctly references the release-namespace SA; distinct name avoids the collapsed-namespace collision.
  • DaemonSet maxUnavailable: "10%" — confirmed against k8s DaemonSet controller behaviour that it floors at 1 when the percentage rounds to 0, so small clusters keep one-at-a-time (the comment's claim is accurate).
  • activeDeadlineSeconds 3600 ≥ 3×rolloutTimeout with slack; set image idempotency makes partial-tick retries safe; shared flap-counter blast radius is documented as a deliberate tradeoff.

All prior cursor[bot] findings (refresh budget, jobs-manager-pin-skips-proxy, mirror update path, disabled-monitor CronJob, RBAC name collision) are confirmed fixed and match the shipped code.

Evidence

  • helm unittest ./client: 431/431 passed on this branch (develop baseline 402/402; +29 tests, zero failures). Note: the PR body's "5 failed / 5 errored baseline" is stale — both develop and this branch are fully green now.
  • scripts/check-style.sh: clean · scripts/check-facts.sh: clean
  • helm lint shows only the pre-existing clientId/clientPassword minLength error (bare lint without secrets), identical on develop — unrelated to this PR.

@shujaatTracebloc
shujaatTracebloc merged commit 74b9e70 into develop Aug 13, 2026
22 checks passed
@shujaatTracebloc
shujaatTracebloc deleted the fix/569-control-plane-offline-restart branch August 13, 2026 13:41

@saadqbal saadqbal left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very careful PR 👍 Every long-running control-plane container (jobs-manager api + pods-monitor-container, requests-proxy, resource-monitor) now goes through tracebloc.controlPlanePullPolicy — IfNotPresent where an update path exists (digest pin, or a reconcile that can actually run), Always kept exactly on the edges that still need it (private mirror, imageRefresh disabled). Nothing left on unconditional Always, so the offline-restart goal is genuinely met. The node-agents Role is the only new trust surface and it's as tight as RBAC allows (get/patch resourceNames-scoped to the one DaemonSet; list/watch namespace-wide read-only only because rollout-status needs them), the distinct name avoids the same-namespace collision, and the CronJob can't thrash — set image is idempotent and the shared flap lockout still backstops a bad digest. New keys are nil-guarded/defaulted and the schema only adds descriptions + one optional key, so reuse-values upgrades are safe. helm unittest green: 431/431 across the whole chart (image_refresh 36, jobs_manager 46, requests_proxy 20, resource_monitor 11). Version bumped 1.9.38→1.9.39.

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.

Control-plane pods: offline-restart-safe update model (design) — jobs-manager/pods-monitor/resource-monitor (from #552)

4 participants