Skip to content

docs(telemetry): #1906 cannot drain these spools, and four comments said it would - #775

Merged
LukasWodka merged 2 commits into
developfrom
docs/2217-retire-1906-forwarder-claim
Aug 21, 2026
Merged

docs(telemetry): #1906 cannot drain these spools, and four comments said it would#775
LukasWodka merged 2 commits into
developfrom
docs/2217-retire-1906-forwarder-claim

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Part of backend#2217. Split out deliberately so the correction isn't buried inside the delivery work, which is blocked on a credential decision (see below).

Correction (was: "comment-only, no behaviour change"). That framing was wrong and @saqlainsyed007 / Bugbot caught it. scripts/lib/telemetry.sh is bootstrap-fetched and hash-pinned, so editing a comment changes its digest — and install.sh verifies the fetched file against scripts/manifest.sha256, aborting before the privileged install on a mismatch. For a hash-pinned bootstrap file the digest is part of the contract. scripts/manifest.sha256 is regenerated in 789e4e1 (exactly one line moves: 38b2a2e9…d773c8a1…). The prose below is unchanged; this note stays rather than being edited away, because the wrong claim is the instructive part.

What was wrong

scripts/lib/telemetry.sh said in four places that #1906's forwarder would read or drain the spool files this module writes. It cannot, and the reason is structural rather than a matter of sequencing:

#1906 is an OpenTelemetry Collector running as a pod inside the customer's Kubernetes cluster, reading container stdout through a filelog receiver. Neither spool is container stdout and neither is reachable from a pod — $HOST_DATA_DIR/telemetry/pending.jsonl and the $TMPDIR fallback are files on the operator's own machine. Worse, the pre-log failures the fallback exists for (validate_config, early_data_dir_guard) happen when there is often no cluster to run a Collector in at all.

This is why the host transport went unowned. backend#1907 was closed on the understanding that the remaining transport belonged to #1905 plus #1906. It didn't, and these comments are what made that reading look right. This file is the first place the next person looks, so it now points at backend#2217 and says why #1906 is the wrong answer — so the same wrong conclusion isn't re-derived a third time.

Also recorded: the trim's disagreement with D7 is deliberate

The spool's tail -n trim is drop-oldest, which is deliberately not what RFC-BACKEND-1872 D7's overflow row says since rfcs#36. D7 was amended to drop-newest because exporterhelper sheds at the entrance and offers nothing else — a constraint on the Collector's queue, not a preference. This spool is our own code and can do what D7 originally wanted, so it does: the newest records describe the failure in progress.

Without that note the next reader has a standing invitation to "fix" the disagreement in the wrong direction. The same note is in the Go spool on cli#542.

Test plan

  • bash -n scripts/lib/telemetry.sh — OK
  • shellcheck -S warning -x scripts/lib/telemetry.sh — clean (CI severity, not -S error)
  • scripts/tests/telemetry.bats44/44 pass
  • scripts/tests/telemetry-vocabulary-agreement.sh — green, all six agreement checks

No mutation table: this diff contains no logic, so there is nothing to mutate. Asserting otherwise would be the vacuous-guard pattern this epic keeps finding.

The PowerShell twin needed no matching changeinstall-k8s.ps1 contains no telemetry at all (grep -ci telemetry → 0), so there is no twin drift. That absence is arguably its own gap: a Windows install currently reports nothing. Not filed, since it's outside this ticket.

What is NOT in this PR, and why

The actual POST. backend#2217 assumes the installer can authenticate, and it cannot today. The ingest endpoint requires Authorization: Token … or Bearer … (IsAuthenticatedEdge), and the installer holds TRACEBLOC_CLIENT_ID/TRACEBLOC_CLIENT_PASSWORD — a provisioning pair, not a bearer token. It also never reads the CLI's ~/.tracebloc/config.json (zero hits across scripts/), so there is no existing path to a token.

Options and a recommendation are on backend#2217. That is a design decision, not an implementation detail, so it isn't being made in a PR.

🤖 Generated with Claude Code


Note

Low Risk
Documentation and checksum only; no executable telemetry or delivery logic changes.

Overview
Comment-only correction in telemetry.sh: host JSONL spools are not drained by #1906 (in-cluster Collector on container stdout). Ownership is now backend#2217, because those files live on the operator machine and often exist before any cluster.

Also documents that _telemetry_trim_spool's drop-oldest tail -n is intentional vs RFC D7's drop-newest Collector queue. Updates manifest.sha256 for the file. No runtime change.

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

…aid it would
Comment-only. No behaviour change, and deliberately separate from the delivery
work so the correction is not buried in it.
`scripts/lib/telemetry.sh` said in four places that #1906's forwarder would read
or drain the spool files this module writes. It cannot, and the reason is
structural rather than a matter of sequencing: #1906 is an OpenTelemetry
Collector running as a POD inside the customer's Kubernetes cluster, reading
container stdout through a `filelog` receiver. Neither spool is container stdout
and neither is reachable from a pod --
`$HOST_DATA_DIR/telemetry/pending.jsonl` and the `$TMPDIR` fallback are files on
the operator's own machine. The pre-log failures the fallback exists for
(`validate_config`, `early_data_dir_guard`) happen when there is often no cluster
to run a Collector in at all.
That is not a pedantic fix. The host transport went unowned after backend#1907
closed precisely because these comments pointed at a ticket that could never
deliver it, and this file is the first place the next person looks. It now points
at backend#2217 and says why #1906 is the wrong answer, so the same conclusion is
not re-derived.
Also records, at the trim, that `tail -n` is DROP-OLDEST on purpose and is
deliberately not what RFC-BACKEND-1872 D7's overflow row says since rfcs#36. D7
was amended to drop-newest because `exporterhelper` sheds at the entrance and
offers nothing else -- a constraint on the Collector's queue, not a preference.
This spool is our own code and can do what D7 originally wanted. Without that
note the next reader has a standing invitation to "fix" the disagreement in the
wrong direction.
The PowerShell twin needed no matching change: `install-k8s.ps1` contains no
telemetry at all, so there is no drift between the two.
Verified: `bash -n`, `shellcheck -S warning -x` clean, all 44
`scripts/tests/telemetry.bats` tests pass, and
`scripts/tests/telemetry-vocabulary-agreement.sh` green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@cursorcursorBot 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 ec4c0c1. Configure here.

Comment threadscripts/lib/telemetry.sh

@saqlainsyed007saqlainsyed007 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.

The correction itself is right and worth landing — #1906 is an in-cluster OTel Collector reading container stdout, so it genuinely cannot reach $HOST_DATA_DIR/telemetry/pending.jsonl or the $TMPDIR fallback on the operator's machine, and pointing the next reader at #2217 (plus the deliberate drop-oldest-vs-D7 note) is the right fix for a comment that mislabelled ownership twice already.

But this isn't behaviour-free, and the open Bugbot finding at telemetry.sh:141 is exactly why — it's a real High, and it's what's reddening CI:

scripts/manifest.sha256 is stale.telemetry.sh is a bootstrap-fetched script pinned in the manifest, so even a comment-only edit changes its digest — the drift check shows it moving 38b2a2e9…d773c8a1…. All three failures are this one cause:

  • Source-of-truth drift (make drift): [ERROR] scripts/manifest.sha256 is out of date. Run scripts/gen-manifest.sh and commit.
  • bats / Unit tests: not ok 418 a clean tree passes --check in gen-manifest.bats.

And it's not just CI cosmetics: at runtime install.sh verifies the fetched telemetry.sh against the pinned hash and would abort before the privileged install on the mismatch. So the "comment-only, no behaviour change" framing doesn't hold for a hash-pinned bootstrap file — the digest is part of the contract.

Fix: run scripts/gen-manifest.sh and commit the updated scripts/manifest.sha256 (the drift log spells out the same command). That greens all three checks and closes the Bugbot thread. Nothing else needed — the prose is good; re-request me once the manifest is regenerated and I'll approve.

…viour change here
@saqlainsyed007 and Bugbot, and the finding falsifies this PR's own framing rather
than just reddening CI.
`scripts/lib/telemetry.sh` is bootstrap-FETCHED and hash-pinned, so editing a
comment changes its digest: 38b2a2e9... -> d773c8a1..., exactly as reported. All
three failures were the one cause -- `make drift` refusing the stale manifest, and
`gen-manifest.bats` "a clean tree passes --check".
The part that matters more than CI: at runtime `install.sh` verifies the fetched
`telemetry.sh` against the pinned hash and ABORTS BEFORE THE PRIVILEGED INSTALL on
a mismatch. So "comment-only, no behaviour change" was wrong -- for a hash-pinned
bootstrap file the digest is part of the contract, and shipping this without the
manifest would have broken every fresh install while every local check stayed
green. The PR description is corrected too; leaving that claim standing would
teach the next person the same wrong thing.
Ran `scripts/gen-manifest.sh`. Exactly one line moves -- telemetry.sh's digest --
which is the check that this is the manifest catching a real edit rather than an
unrelated drift riding along.
Verified: `scripts/gen-manifest.sh --check` reports up to date; `make drift` green
on all 6 guards; `scripts/tests/gen-manifest.bats` 20/20; `scripts/tests/telemetry.bats`
44/44.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saqlainsyed007 — fixed in 789e4e1. Bugbot's thread auto-resolved on the push.

You caught something better than a red check: my "comment-only, no behaviour change" claim was false, and falsely in the direction that matters. telemetry.sh is bootstrap-fetched and hash-pinned, so install.sh verifies it against scripts/manifest.sha256 and aborts before the privileged install on a mismatch — meaning this would have broken every fresh install while every local check I ran stayed green. For a hash-pinned bootstrap file the digest is part of the contract, and I treated it as metadata.

Ran scripts/gen-manifest.sh. Exactly one line moves38b2a2e9…d773c8a1…, the digests you quoted — and that single-line diff is itself the check that this is the manifest catching a real edit rather than unrelated drift riding along on my commit.

I have corrected the PR description rather than deleting the wrong sentence, and left the correction visible as a note. A PR body that quietly stops being wrong teaches nobody; this file's whole subject is comments that mislabelled ownership twice, so silently editing away my own mislabel would have been the same defect one level up.

Verified: gen-manifest.sh --check up to date · make drift green on all 6 guards · gen-manifest.bats 20/20 · telemetry.bats 44/44. Re-requested you.

@saqlainsyed007saqlainsyed007 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.

The manifest fix is exactly right — 789e4e1c regenerates scripts/manifest.sha256 for the new telemetry.sh digest, and the commit title ("a comment edit IS a behaviour change" for a hash-pinned bootstrap file) captures precisely why. That closes the Bugbot :141 thread and the root cause of all three red checks. CI is re-running (Prereqs / Unit / bats pending) — I'll approve as soon as it's green; nothing else outstanding.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saqlainsyed007CI is green: 42 passed, 0 failed, 0 pending (4 skipped). Source-of-truth drift, Unit tests and bats — the three that were red — all pass now, and the Bugbot :141 thread auto-resolved on the push.

Worth restating the bit your review made me correct, since it's the durable lesson: I shipped this as "comment-only, no behaviour change" and that was false for a hash-pinned bootstrap file. install.sh verifies the fetched telemetry.sh against manifest.sha256 and aborts before the privileged install, so it would have broken every fresh install while every local check I ran stayed green. The PR description carries that correction visibly rather than having quietly stopped being wrong.

@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

@saqlainsyed007 — this one is stuck on a formality rather than anything outstanding, so flagging it precisely.

Your CHANGES_REQUESTED is still the governing review decision, which is what keeps the PR BLOCKED. You then commented that the manifest fix is "exactly right" with "nothing else outstanding" and that you'd approve once green — and it has been green for a while now: 42 passed, 0 failed, 0 pending, 0 unresolved threads, Bugbot's :141 thread auto-resolved.

The thing that needs you specifically: only you can clear your own change-request. An approval from another reviewer does not dismiss it, and you had dropped off the requested-reviewer list, so nothing was prompting you to look again. I have re-requested you.

Nothing to re-review unless you want to — the only change since your comment is 789e4e1c, the manifest regeneration you asked for.

@saqlainsyed007saqlainsyed007 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.

Approving — and thanks for splitting this out so the correction isn't buried in the delivery work.

I verified the thing my earlier note was actually about: scripts/lib/telemetry.sh is bootstrap-fetched and hash-pinned, so editing even a comment changes its digest, and install.sh aborts before the privileged install on a manifest.sha256 mismatch. I recomputed the digest of telemetry.sh at this head and it matches the regenerated manifest line exactly:

d773c8a11fa80d6880b5a104e2159de1dc9aaa35234ad6a764e4e1c49f601aec scripts/lib/telemetry.sh

So the hash gate is satisfied — this won't brick the installer. The prose correction itself is right and important: #1906 is an in-cluster OTel Collector reading container stdout via filelog, and neither $HOST_DATA_DIR/telemetry/pending.jsonl nor the $TMPDIR fallback is container stdout or reachable from a pod — and the pre-log failures the fallback exists for happen when there may be no cluster at all. Pointing the next reader at backend#2217 (and saying why #1906 is the wrong answer) is what stops the unowned-transport conclusion being re-derived a third time. Recording that the drop-oldest trim's disagreement with D7 is deliberate — D7 went drop-newest only because exporterhelper sheds at the entrance — is a good guard against someone "fixing" it the wrong way. LGTM.

@LukasWodka
LukasWodka merged commit 2357820 into developAug 21, 2026
46 checks passed
@LukasWodka
LukasWodka deleted the docs/2217-retire-1906-forwarder-claim branch August 21, 2026 08:14
LukasWodka added a commit that referenced this pull request Aug 21, 2026
…ft SCC (backend#1906)
Both of Bugbot's findings on #779. Both verified against upstream before fixing —
they are real, and the second one is the more dangerous of the two.
1. THE DISK-BACKED QUEUE WAS UNWRITABLE. `runAsNonRoot: false` PERMITS root; it
does not SELECT it. The pinned otelcol-contrib image declares `USER 10001`
(`ARG USER_UID=10001` in the upstream Dockerfile), and the kubelet creates a
DirectoryOrCreate hostPath root-owned 0755 — so the Collector ran as 10001 and
could not write /var/lib/tracebloc/<release>/telemetry. D7's whole point is a
queue that survives a restart; it would have failed the moment the Collector
was enabled, which is exactly the kind of defect that hides behind a chart that
renders clean.
Fixed by pinning `runAsUser: 0`. Root rather than an init-container chown for a
reason beyond one fewer container: the READ side needs it too — container log
files under /var/log/pods are root-owned and not world-readable on every
runtime, so a non-root collector may not be able to read the logs it exists to
read. fluent-bit and the cloudwatch-agent already on these clusters run as root
for the same reason. It is a NARROW root, and that is the trade being made
rather than glossed: all capabilities dropped, no privilege escalation,
read-only root filesystem, and the only writable paths are the queue and /tmp.
2. ON OPENSHIFT THE COLLECTOR HAD NO SCC AT ALL. Chart-managed SCCs gated on
`resourceMonitor` alone and bound only that ServiceAccount, so a second
hostPath DaemonSet with its own SA was refused at admission. A SEPARATE SCC,
not a widening: resource-monitor's declares MustRunAsNonRoot, which is correct
for it — read-only mounts, no write anywhere — and loosening it to RunAsAny to
serve the Collector would hand a broader run-as rule to a workload that does
not need one.
WHY A NEW DRIFT GUARD. This is the second cross-document gap on this PR to get
through a green 519-test suite (the first was the Class A globs), because the
defect is the ABSENCE of a relationship between two documents and helm-unittest
asserts within one template at a time. scripts/tests/openshift-scc-coverage.sh
renders the chart with OpenShift on and compares the hostPath workloads against
the SCCs and their `users:` lists — holding no list of its own, deriving both
sides, failing closed when either side is empty.
It checks run-as compatibility too, and that is the point rather than a flourish:
adding the Collector's SA to resource-monitor's existing SCC would satisfy "is it
covered" while still failing at admission, since that SCC refuses root. A guard
that checked only membership would have gone green on the wrong fix.
MUTATION-PROVEN, anchors asserted applied in every case:
guard M1 run-as -> MustRunAsNonRoot KILLED (root refused by ...)
M2 allowHostDirVolumePlugin off KILLED (no SCC)
M3 users emptied KILLED (no SCC)
M4 template deleted KILLED — reproduces the original defect
unittests M1 drop `runAsUser: 0` 1 failed <- the shipped regression
M2 SCC -> MustRunAsNonRoot 1 failed
M3 gate on openshift only 2 failed
M4 gate on Collector only 1 failed
M5 users emptied 1 failed
No survivors; green again on restore.
525/525 chart tests, 9/9 drift guards, shellcheck -S warning clean.
`scripts/manifest.sha256` deliberately NOT regenerated: it covers only the
sub-scripts install.sh fetches and hash-verifies, and a CI guard under
scripts/tests/ is not one — checked rather than assumed, after #775.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
LukasWodka added a commit that referenced this pull request Aug 21, 2026
…ckend#1906) (#779)
* feat(telemetry): the edge Collector, Class A only, shipping inert (backend#1906)
RFC-BACKEND-1872 D6/D7. An OpenTelemetry Collector DaemonSet that reads Class A
container stdout with `filelog` and forwards to the backend's ingest endpoint.
SHIPS DISABLED, and that is the load-bearing decision rather than caution. The
exporter authenticates with a token this chart does not create: jobs-manager
writes it into a Secret and does not do so yet. Enabling it first would put a
DaemonSet on every customer node spooling to disk with nothing it can deliver --
filling toward the 1 GiB cap per node for no benefit, which is exactly the
"telemetry must never be the reason a node fills" risk D7 exists to bound. Same
posture as `egressProxy.routeWorkloads`.
A DAEMONSET, because the kubelet writes container stdout per NODE. A Deployment
would see only the node it landed on and report healthy while collecting a
fraction of the fleet -- the shape of silence this epic exists to remove.
CLASS A ONLY (D12). The include globs name the four control-plane containers this
chart owns; a bare wildcard would sweep in training and ingestion pods, which are
Class B and gated on backend#1908. Today's bounding is SECRET redaction, not
CONTENT redaction, and raw customer cell values have already reached central
telemetry once (backend#1879), so the narrow scope is the protection.
THE REDACTION FLOOR IS RELOCATED, not re-invented -- D6 precondition 2. Reading
stdout through `filelog` BYPASSES controller.py's app-side handler, so without it
this change would silently REMOVE a protection. All six `_LOG_REDACTIONS`
patterns are carried (client-runtime@45006ec), translated Python -> RE2
deliberately: `\1` becomes `$1`, `(?i)`/`(?m)` kept, and no pattern uses
lookaround or in-pattern backreferences, which RE2 rejects -- checked, because a
silently-invalid regex in OTTL is a processor that starts and scrubs nothing. All
six were compiled as RE2 and run against sample secrets; the
`SharedAccessKeyName` carve-out controller.py documents survives.
THIS IS A CROSS-REPO SECOND COPY THAT CANNOT BE MACHINE-CHECKED FROM HERE, and
the comment says so rather than implying coverage: the producer is Python in
another repo, Helm cannot import it, and no test in this repo can detect drift.
D7's bounds, all three: byte cap 1 GiB via `sizer: bytes`; `max_elapsed_time: 0`
so the cap is the ONLY bound; disk-backed `file_storage` on a hostPath so the
queue survives the pod restart that is the common case during an outage.
`block_on_overflow: false` is drop-NEWEST per D7's 2026-08-20 amendment
(rfcs#36) -- `exporterhelper` sheds at the entrance and has no evict-oldest
option -- which is why the Collector's own metrics are exposed: the drop count is
the only signal separating a quiet edge from a shedding one.
`scheme: Token`, NOT the extension's default `Bearer`. The edge holds a DRF user
token and the endpoint accepts it via `TokenAuthentication` under
`IsAuthenticatedEdge`; `Bearer` is the other credential type and 401s silently.
The token is read through `bearertokenauth`'s `filename`, which watches the
projected file (credentialsfile.ValueResolver + WithOnChange) -- verified in the
extension's source, because the README does not mention reloading and #1906's
"a projected update needs no restart" rests on it.
`logs_endpoint`, not `endpoint`: otlphttp appends /v1/logs to the latter and the
ingest boundary is a versioned path of ours.
One defect caught by rendering rather than reading: Helm parses 1073741824 as a
float64 and emitted `queue_size: 1.073741824e+09`, a YAML float where the
Collector wants an integer -- the cap would not have been the number in
values.yaml. `| int64` fixes it and a test asserts no scientific notation.
Verified: helm lint clean; the WHOLE chart suite 510/510 across 34 suites; the
new suite 19/19; `chart-version-guard.bats` 23/23 after bumping version and
appVersion together to 1.9.51 (the guard requires a bump when chart content
changes). 15 mutations, all killed -- enabling by default, widening filelog to a
wildcard, dropping redaction from the pipeline, losing a redaction pattern,
losing `int64`, reverting `sizer`, adding a second time bound, falling back to
memory, reverting to `Bearer`, using `endpoint`, mounting host logs writable,
making the Secret required, moving the queue to an emptyDir, removing the config
checksum, and granting a ClusterRole.
NOT in scope: retiring the two App Insights entries from the squid ACL. That cuts
client-runtime's live AzureLogHandler path and belongs to #1910's cutover, not
here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(telemetry): the Collector gate broke `helm upgrade --reuse-values`
Caught by the Fleet auto-upgrade E2E, and it was far worse than the feature not
working: it broke the UPGRADE, on every existing customer.
Error: UPGRADE FAILED: template: telemetry-collector-rbac.yaml:1:14:
<.Values.telemetryCollector.enabled>: nil pointer evaluating interface {}.enabled
`helm upgrade --reuse-values` replays the values STORED WITH THE PREVIOUS RELEASE
and does not merge values.yaml defaults. Every release made before this block
existed therefore has no `telemetryCollector` key at all, so the bare gate is a
nil-pointer dereference and the whole release fails.
THE CHART ALREADY DOCUMENTS THIS TRAP AND I DID NOT FOLLOW IT.
resource-monitor-daemonset.yaml says of `images.resourceMonitor`: "older releases
that upgrade via `helm upgrade --reuse-values` won't have the block in their
stored values, so read it defensively (nested `default dict` tolerates a missing
`images` map AND a missing `resourceMonitor` entry). `dig` is not usable here --
it rejects chartutil.Values." Same idiom now applies here.
Every read goes through `{{- $tc := default (dict) .Values.telemetryCollector -}}`
with nested maps defaulted the same way, and each defaulted value repeats the
values.yaml default so a partial stored map cannot render a half-configured
Collector. An old release upgrading this way gets NO Collector, which is right
twice over: it matches `enabled: false`, and a feature must never arrive on a
cluster via a values map the operator never saw.
The regression test sets `telemetryCollector: null` -- ABSENT, not `false`, because
absent is the case that broke and false is a key that exists (already covered).
Mutation-proved: reverting the gate reproduces the E2E's exact error and the test
errors rather than passing.
Chart bumped to 1.9.52 (the version guard requires it on chart content change).
Whole chart suite 511/511 across 34 suites; the new suite 20/20.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(telemetry): the Collector collected nothing, and ran an image that could not start
Two Bugbot Highs on #779, both correct, and both the same class: a Collector that
starts, reports healthy, and ships nothing.
1. EVERY FILELOG GLOB MATCHED NOTHING. `classAContainers` listed
`tracebloc-jobs-manager`, `egress-proxy` and `requests-proxy` -- WORKLOAD names.
`filelog` matches the kubelet's on-disk path, which carries the CONTAINER name,
and the real ones are `api` (jobs-manager), `pods-monitor-container`
(Dockerfile.controller, i.e. D6's `controller.py`) and `squid`. Worse, every
path was scoped to `.Release.Namespace` while resource-monitor is a DaemonSet
in `nodeAgents.namespace` -- so a whole Class A component was unreachable even
had its name been right. All four globs targeted nothing.
`proxy` (requests-proxy) is deliberately NOT added: it runs the jobs-manager
image but is not in D6's Class A list, and admitting a container because it
shares an image is how a class boundary stops meaning anything.
2. THE IMAGE PREDATED ITS OWN CONFIG. Pinned 0.109.0, while the pipeline sets
`block_on_overflow` (upstream 2025-03, ~0.122+) and `sizer: bytes` on a
persistent queue (~0.130). The Collector REJECTS unknown configuration keys, so
this is not a cap silently unapplied -- it is a container that does not start,
on every node. Now 0.159.0.
The mistake worth naming: I verified `sizer` and `block_on_overflow` against
CURRENT upstream docs and then pinned an image from before they existed.
Verifying a feature is not verifying the version that has it.
WHAT KEEPS BOTH FIXED. A new derived guard,
`scripts/tests/collector-class-a-agreement.sh`, wired into DRIFT_GUARDS (7 now).
It holds NO list of names: it renders the chart, reads container names out of
every workload, reads the globs out of the Collector's own ConfigMap, and compares.
A hand-written expectation would have agreed with whichever side it was copied
from -- which is exactly how the original passed review, and why the helm-unittest
assertions could not catch it either (they asserted the same wrong names).
It is a cross-DOCUMENT agreement, so helm-unittest cannot express it: that plugin
asserts within one template at a time. Hence a shell test. It fails closed --
zero globs or zero containers is a finding, since two empty sets compare equal.
Mutation-proved against BOTH findings: restoring the old names, and re-scoping
node-agents to the release namespace, each reddens it with the offending pair named.
The image floor is asserted as a FLOOR in the helm suite, not an exact tag -- an
exact pin there would be a second place to update on every bump and would pass by
agreeing with itself.
shellcheck earned its place too: it caught `render | python3 - <<'PY'`, where the
heredoc overrides the pipe so the comparison read an empty document set. It failed
CLOSED ("found 0 Collector ConfigMaps") rather than passing, which is the design
working, but SC2259 named the cause directly.
Chart 1.9.53. Whole chart suite 512/512 across 34 suites; the Collector suite
21/21; `make drift` green on all 7 guards; manifest up to date.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(telemetry): three more on the Collector — token pre-flight, mirror, namespace gate
Bugbot's second round on #779. All three verified against the rendered chart
before fixing.
1. HIGH — `optional: true` DOES NOT MEAN "buffer, don't fail". It stops the
kubelet failing the MOUNT; it does not make the file appear, and
`bearertokenauth` needs `filename` to resolve. Whether a missing file aborts
the extension's Start is version-dependent and lives in core's
`credentialsfile.ValueResolver`, not the extension — I could not establish it
for 0.159.0 from that tag or from `main`.
So this does not BET on the answer. A `lookup`-guarded pre-flight (the same
idiom as resource-monitor's metrics-server probe: empty during `helm
template`, so offline rendering is unblocked) refuses the RELEASE with a
message naming the missing Secret and namespace. Either upstream behaviour is
then fine, because the case is unreachable — which beats reading the source
correctly and depending on it. It is also the right direction for a fail-soft
component: "the install said no, here is the Secret it wants" is actionable in
a way that CrashLoopBackOff on 200 nodes is not.
2. MEDIUM — `global.imageRegistry` was ignored. Squid and the other third-party
images use #585 precedence: global mirror, then per-image registry, then
docker.io. Passing `image.registry` alone meant a mirrored or air-gapped fleet
would ImagePullBackOff the Collector while everything else pulled fine. Both
ends of the chain are now asserted — a precedence chain tested at one end only
is half-tested.
3. MEDIUM — the Collector is a SECOND TENANT of the node-agents namespace, and
the gates had not caught up. `node-agents-namespace.yaml` and the mirrored
pull Secret in `docker-registry-secret.yaml` were still gated on
`resourceMonitor != false`, so `resourceMonitor: false` +
`telemetryCollector.enabled: true` produced a DaemonSet targeting a namespace
this chart never created, with no pull Secret. Every dependent resource now
shares the feature's gate. Proven both ways: with the fix, 4 resources land in
that namespace; reverting the pull-Secret gate drops it to 3.
FOUND WHILE FIXING, AND MINE: `telemetryCollector` was the ONLY chart-owned
top-level key of 36 absent from `values.schema.json` (`global` is a Helm
built-in). So the whole block was unvalidated — a typo in `classAContainers`
would have been silently accepted and the Collector would have collected nothing,
which is finding #1 of the first round arriving by a different route. Added, with
`authScheme` as a CLOSED enum so a typo cannot pick a third value, and
`type: [object, null]` so `--reuse-values` from a release predating the block
still validates.
Tests: 25 in the Collector suite. The three fixes are mutation-proved, including
a vacuity check — widening the namespace gate to `true` also reddens, so the
positive test is not passing for free.
Chart 1.9.54. Whole suite 516/516 across 34 suites; `make drift` green on 7
guards; lint clean; manifest current.
A process note for my own future reference: three of my debugging renders came
back empty and I briefly read that as the fix not working. All three were invalid
test inputs the schema was correctly rejecting — with `2>/dev/null` hiding the
reason. Don't suppress stderr while diagnosing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(telemetry): pin the pull-secret gate, and that the pre-flight stays offline-safe
@aptracebloc's two non-blocking follow-ups on #779. Taken now rather than deferred
because the conflict-resolution push dismissed the approval anyway, so they cost no
extra review round.
1. THE PULL-SECRET GATE WAS NOT MUTATION-PINNED while the namespace gate was. I
had verified it by hand — render diff, 4 node-agents resources with the fix and
3 without — and a by-hand check leaves nothing behind. That was the gap, not the
fix. Now pinned in BOTH directions: narrowing the gate back reddens the positive
test, and widening it to `true` reddens the negative one, so neither passes for
free.
It needs real registry values, which is worth recording: `dockerRegistry.server`
is `format: uri` and `email` is required by its `allOf`, so a partial set is
rejected by values.schema.json before any template renders — and helm-unittest
surfaces that as a plugin ERROR rather than a template failure, which reads
exactly like a code defect. It cost me three confused renders earlier, all of
them my own invalid inputs with stderr suppressed.
2. THE PRE-FLIGHT `fail` IS UN-PINNABLE and he is right that it is: `lookup`
returns empty under `helm template`, so the guard never fires in a unit test.
But the INVERSE is testable and is the failure that would actually hurt — a
pre-flight that fired offline would break every `helm template`, every CI render
and every `--dry-run` in the fleet. That property is now asserted directly
instead of resting on the guard's own comment.
Chart 1.9.53. Whole chart suite 519/519 across 34 suites; the Collector suite
28/28; `make drift` green on all 8 guards.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(telemetry): select root for the Collector, and give it an OpenShift SCC (backend#1906)
Both of Bugbot's findings on #779. Both verified against upstream before fixing —
they are real, and the second one is the more dangerous of the two.
1. THE DISK-BACKED QUEUE WAS UNWRITABLE. `runAsNonRoot: false` PERMITS root; it
does not SELECT it. The pinned otelcol-contrib image declares `USER 10001`
(`ARG USER_UID=10001` in the upstream Dockerfile), and the kubelet creates a
DirectoryOrCreate hostPath root-owned 0755 — so the Collector ran as 10001 and
could not write /var/lib/tracebloc/<release>/telemetry. D7's whole point is a
queue that survives a restart; it would have failed the moment the Collector
was enabled, which is exactly the kind of defect that hides behind a chart that
renders clean.
Fixed by pinning `runAsUser: 0`. Root rather than an init-container chown for a
reason beyond one fewer container: the READ side needs it too — container log
files under /var/log/pods are root-owned and not world-readable on every
runtime, so a non-root collector may not be able to read the logs it exists to
read. fluent-bit and the cloudwatch-agent already on these clusters run as root
for the same reason. It is a NARROW root, and that is the trade being made
rather than glossed: all capabilities dropped, no privilege escalation,
read-only root filesystem, and the only writable paths are the queue and /tmp.
2. ON OPENSHIFT THE COLLECTOR HAD NO SCC AT ALL. Chart-managed SCCs gated on
`resourceMonitor` alone and bound only that ServiceAccount, so a second
hostPath DaemonSet with its own SA was refused at admission. A SEPARATE SCC,
not a widening: resource-monitor's declares MustRunAsNonRoot, which is correct
for it — read-only mounts, no write anywhere — and loosening it to RunAsAny to
serve the Collector would hand a broader run-as rule to a workload that does
not need one.
WHY A NEW DRIFT GUARD. This is the second cross-document gap on this PR to get
through a green 519-test suite (the first was the Class A globs), because the
defect is the ABSENCE of a relationship between two documents and helm-unittest
asserts within one template at a time. scripts/tests/openshift-scc-coverage.sh
renders the chart with OpenShift on and compares the hostPath workloads against
the SCCs and their `users:` lists — holding no list of its own, deriving both
sides, failing closed when either side is empty.
It checks run-as compatibility too, and that is the point rather than a flourish:
adding the Collector's SA to resource-monitor's existing SCC would satisfy "is it
covered" while still failing at admission, since that SCC refuses root. A guard
that checked only membership would have gone green on the wrong fix.
MUTATION-PROVEN, anchors asserted applied in every case:
guard M1 run-as -> MustRunAsNonRoot KILLED (root refused by ...)
M2 allowHostDirVolumePlugin off KILLED (no SCC)
M3 users emptied KILLED (no SCC)
M4 template deleted KILLED — reproduces the original defect
unittests M1 drop `runAsUser: 0` 1 failed <- the shipped regression
M2 SCC -> MustRunAsNonRoot 1 failed
M3 gate on openshift only 2 failed
M4 gate on Collector only 1 failed
M5 users emptied 1 failed
No survivors; green again on restore.
525/525 chart tests, 9/9 drift guards, shellcheck -S warning clean.
`scripts/manifest.sha256` deliberately NOT regenerated: it covers only the
sub-scripts install.sh fetches and hash-verifies, and a CI guard under
scripts/tests/ is not one — checked rather than assumed, after #775.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(telemetry): the filelog receiver kept its read offsets in memory (backend#1906)
Bugbot's find, and correct. `file_storage` backed only the exporter queue. The
`filelog` receiver set no `storage`, so its read offsets lived in memory — and
`start_at` governs where a NEWLY DISCOVERED file is read from, not where a known
one resumes. With nothing persisted, every file looks new after a restart, `end`
wins, and everything written while the Collector was down is skipped. Silently:
no error, no gap in any metric.
D7's disk queue covers records already ingested. Nothing covered the read side,
and restart-during-a-backend-outage is precisely the path that needs both.
THE COMMENT THAT SAT THERE ASSERTED THE OPPOSITE — that `start_at: end` was what
stopped re-reads across restarts. It is why this was not caught in review: a wrong
comment is worse than none, because it answers the question before anyone asks it
(backend#1729 rule 7). Corrected rather than deleted, since the reason `end` is
still right needs saying: with offsets persisted it applies only to files with no
stored position, so a fresh install does not ingest the whole existing backlog
while a known file resumes exactly where it stopped.
The receiver shares the exporter's extension. `file_storage` keys entries by
component so they cannot collide, and offsets cost kilobytes against the queue's
cap. It writes to the same hostPath — which is only writable because the previous
commit pinned `runAsUser: 0`, so the two findings are more connected than they
looked.
WHY A GUARD AND NOT A helm-unittest ASSERTION. The Collector's config is a YAML
document embedded in a string inside the ConfigMap. helm-unittest can only regex
that string, and a regex for `storage: file_storage` matches the EXPORTER's queue
setting just as happily as the receiver's — passing while the receiver has none,
which is the exact defect. Parsing is the only way to say where the key is, so
scripts/tests/collector-offsets-persisted.sh parses it and checks the named
extension is both declared and enabled in `service.extensions`: a storage
extension that is configured but not switched on is silently ignored.
Mutation-proven, anchors asserted applied:
filelog `storage` removed KILLED — reproduces the original defect
points at an undeclared extension KILLED
declared but absent from service.exts KILLED
No survivors; green on restore.
525/525 chart tests, 10/10 drift guards, shellcheck -S warning clean.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(telemetry): one predicate for the node-agents namespace, not five copies (backend#1906)
@saadqbal's review of #779, and he is right that Bugbot's High was one instance of
a class. Five templates put something in `nodeAgents.namespace` and each held its
own copy of "is resource-monitor on"; this PR widened two of them for the
Collector and left the rest, so `resourceMonitor: false` +
`telemetryCollector.enabled: true` — the configuration the Collector exists to
enable — created the namespace, landed the DaemonSet, and left the RBAC that
manages it behind.
`tracebloc.nodeAgentsInUse` is now the single predicate, and all five call sites
read it. The nil-guard lives there too rather than five times over.
I VERIFIED EACH INSTANCE RATHER THAN TAKING THE COUNT, and the count was wrong in
both directions:
* THERE IS A FOURTH the review did not list — `rbac.yaml:183`, jobs-manager's
node-agents Role — found by diffing the two renders rather than by reading
gates.
* TWO OF THE THREE MUST NOT BE WIDENED, and widening them on request would have
made this worse:
- `secrets.yaml:112` mirrors CLIENT_ID/CLIENT_PASSWORD there so the
resource-monitor DaemonSet can read them via secretKeyRef. The Collector
authenticates with its own telemetry token and never reads them, so
widening this would copy CUSTOMER CREDENTIALS into a namespace for a
workload that has no use for them. Left gated on resource-monitor.
- `rbac.yaml:183` grants daemonsets get/list/watch so jobs-manager can read
resource-monitor's version for the heartbeat inventory. Read-only, about a
specific workload, and nothing asks it for the Collector's version — so it
is correctly absent. backend#2274 WILL need it, when jobs-manager starts
writing the Collector's token Secret there; that is a Secret write, and it
belongs to that ticket's chart half.
So two gates widened (auto-upgrade, image-refresh — both mutate DaemonSets in that
namespace, and the Collector is one), two deliberately not.
GUARDED ONCE TOO, because "fix it once" is only half of it. Three careful
readings produced three different counts, which is the argument for a machine
check. scripts/tests/node-agents-tenancy.sh renders the chart twice and asserts
that the Roles which MUTATE DaemonSets in that namespace do not depend on WHICH
DaemonSet is there. Deliberately narrower than "the two renders must match": the
mirrored Secret and the read-only jobs-manager Role SHOULD differ, and a stricter
check would have to be wrong about them.
Writing it caught a defect in itself worth recording: matching the literals
`apps`/`daemonsets` missed auto-upgrade's Role, which is
`apiGroups: ["*"], resources: ["*"], verbs: ["*"]` — the very Role Bugbot flagged.
A guard that cannot see the finding it was written for is worse than none.
Wildcards count now.
Mutation-proven, anchors asserted applied:
auto-upgrade gate reverted to resourceMonitor guard KILLED + 1 unittest failed
image-refresh gate reverted guard KILLED
the predicate hardcoded to `true` (over-gating) 7 unittests failed
the nil-guard removed from the predicate 1 failed, 1 errored
That last row SURVIVED at first and the reason matters: Go's `or` short-circuits,
so my null test set `resourceMonitor: true`, the second operand was never
evaluated, and the test passed happily with the guard DELETED. It sets
`resourceMonitor: false` now — which is the whole test. Without the mutation it
would have shipped asserting nothing, which is this epic's own dominant defect
class appearing in the fix for it.
The over-gating direction is covered by helm-unittest's negatives rather than by
the guard, and that division is stated in both places instead of left implicit.
529/529 chart tests, 11/11 drift guards.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(telemetry): one edge's Collector could ingest another edge's logs (backend#1906)
Bugbot's find, and it is a cross-tenant leak rather than a noisy metric.
The kubelet's log directory is `<ns>_<podName>_<uid>`. A bare pod wildcard is fine
in the release namespace — that namespace belongs to one release — but
`nodeAgents.namespace` is SHAREABLE ON PURPOSE, which is exactly why the workloads
in it are release-scoped. Container names are not: resource-monitor's container is
`tracebloc-resource-monitor` in every release. So the node-agents glob matched
EVERY release's resource-monitor in a shared namespace, and one edge's Collector
would have read another edge's Class A logs and shipped them under its own ingest
token. Both Collectors look healthy throughout.
The pod portion is now scoped to the release. DaemonSet pods are
`<daemonsetName>-<hash>` and the DaemonSet name is release-scoped, so the release
prefix is what separates them.
THE RESIDUAL IS STATED, NOT PAPERED OVER. The wildcard crosses `-`, so a release
named `edge` and one named `edge-2` sharing this namespace would still cross-match.
Closing that needs the workload name rather than the release name, and this list
cannot give it: it is a list of CONTAINER names, and container-to-workload is not
1:1 in general. Two releases whose names are prefixes of one another, in one shared
namespace, is what remains; the ordinary case is closed.
GUARDED, and the guard now checks the property rather than the shape. Updating
collector-class-a-agreement.sh's parse for the new glob would have been enough to
make it pass, which is the trap — so it asserts the invariant instead: a glob into
a namespace that is NOT the release namespace may not use a bare pod wildcard.
Both sides derived from the render, including which namespace counts as "the
release namespace", read off the Deployments rather than written down.
Mutation-proven, anchor asserted applied: reverting the glob to a bare pod
wildcard → guard KILLED, naming the namespace and the leak. The unit test pins the
release prefix too, so dropping it reddens both tiers.
A Go template comment ends at the first `star slash`, so spelling the glob out
inline inside the explanatory comment truncated it and broke the render — one
debug cycle, and worth the note in the file since the next person to document a
glob there will hit it.
529/529 chart tests, 11/11 drift guards.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(telemetry): two more ways the Collector could run healthy and blind (backend#1906)
Both Bugbot's, both real, and both the same shape as everything else on this PR:
the DaemonSet is Ready, the metrics look quiet, and no records exist.
1. `DirectoryOrCreate` ON A PATH WE DO NOT OWN. `/var/log/pods` is the KUBELET'S
directory — we read it. With `DirectoryOrCreate`, a wrong or missing
`hostLogsPath` made the kubelet helpfully create an empty directory, and the
Collector then started, reported healthy, and matched no files forever. That is
the Class A glob guard's failure mode arriving by a different route, and the
guard cannot see it: the globs are correct, the directory is just empty.
`Directory` fails the pod instead, which is the right direction for a path we
expect to already exist — resource-monitor's /proc and /sys have always used it.
THE QUEUE KEEPS `DirectoryOrCreate`, and the asymmetry is the design rather than
an oversight: that path is ours and does not exist on a node's first install, so
`Directory` there would refuse to start the Collector on every fresh node. Both
are asserted in one test, because a well-meaning "make these consistent" edit
breaks exactly one of the two and the tests should say which.
2. `seLinuxContext: MustRunAs` CANNOT READ CONTAINER LOGS. I copied that from
resource-monitor's SCC, where it is correct — that workload reads /proc and
/sys. Container logs under /var/log/pods are labelled `container_log_t` with
per-container MCS categories, and a namespace MCS context cannot read them. So
on OpenShift the pods would be admitted, run as root, and take a permission
error on every include glob. `RunAsAny`, the same widening already applied to
`runAsUser` on this SCC and for the same reason — resource-monitor's own SCC is
untouched and keeps both tighter policies.
That copy-from-the-sibling error is worth naming, because it is the second time on
this PR: the sibling SCC is the right template to start from and the wrong one to
finish with, since every field on it was chosen for a workload that only reads
kernel pseudo-filesystems read-only.
Mutation-proven, anchors asserted applied:
host-logs back to DirectoryOrCreate 1 failed
queue changed to Directory 1 failed
SELinux back to MustRunAs 1 failed
531/531 chart tests, 11/11 drift guards.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for freeto 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.

3 participants

@LukasWodka@saqlainsyed007@saadqbal