Uh oh!
There was an error while loading. Please reload this page.
fix(chart): drop the sticky bit on /data/shared so a dataset can be deleted - #667
Conversation
…eleted `data delete` dropped the table and then failed to remove the files, leaving a half-deleted dataset: gone from the database, still on disk. init-writable-data set BOTH shared dirs to 3777 -- setgid plus sticky, /tmp semantics, so one writer can't unlink another writer's files. That protection is right for /data/logs and wrong for /data/shared, because on /data/shared something legitimately HAS to: `data delete` removes a tree the ingest wrote, and the two run as different identities -- * ingestion Job: uid 65534 (`nobody`), or HOST_UID * CLI staging/teardown pod: uid 65532, fsGroup 65532 * the mount itself: chowned 1000:1000 Sticky permits an unlink only by the entry's owner, the directory's owner, or root. The teardown pod is none of the three, so sticky made a documented product operation impossible. fsGroup, which normally aligns identities like this by making kubelet recursively chgrp the volume, does nothing on hostPath -- kubelet ignores it there (kubernetes/kubernetes#138411), which is the layout every installer-provisioned cluster uses. So the mode is now per-directory rather than one value for both: /data/shared 2777 setgid, NO sticky -- teardown must be able to clean up /data/logs 3777 setgid + sticky -- unchanged; nothing deletes another writer's logs, so the protection is free Both keep setgid, so new entries still inherit GID 1000, and each dir is still fixed independently and best-effort (a root_squash export still degrades to a logged skip rather than blocking startup). Verified by executing the generated shell: /data/shared comes out drwxrwsrwx and /data/logs drwxrwsrwt. helm unittest jobs_manager 34/34, and the two new assertions were confirmed to FAIL when /data/shared is put back to 3777. Full chart suite and helm lint are unchanged from develop (both have the same pre-existing 5 failed / 5 errored and the clientId/clientPassword minLength lint errors, which need real values). This is the chart half of the delete failure; the ingestor half -- the dataset dir itself being created group-writable when no group is shared -- is data-ingestors#476. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
develop had already moved to 1.9.29 (#658) by the time this branch was cut, so the earlier edit was a silent no-op and this PR changed chart content with no version bump -- which the chart-version-guard rejects. Bump version + appVersion in lockstep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
divyasinghds
left a comment
There was a problem hiding this comment.
Approving — correct, minimal, well-scoped.
init-writable-data set both shared dirs to 3777 (setgid + sticky). Sticky is right for /data/logs but wrong for /data/shared, where data delete's teardown pod (uid 65532 — not the entry owner, dir owner, or root) legitimately must unlink the ingest's files; sticky made that impossible (table dropped, files stranded), and fsGroup can't rescue it on hostPath (k8s#138411). Splitting to /data/shared=2777 (no sticky) / /data/logs=3777 removes the protection only where it provably blocks a required op — exactly the right scoping. Loop refactor stays best-effort, both keep setgid for GID inheritance, tests assert both modes, chart bump gated and green.
This is the mount-root half; data-ingestors#477 is the
-subdir half — land them together.One cross-PR flag (not on this PR): client#654's Windows prep currently chmods the data dir to 3777, which would re-introduce the sticky bit you're removing here. I've asked for it to adopt this 2777/3777 split. ✅
Uh oh!
There was an error while loading. Please reload this page.
…a/shared Review found this reintroduced the bug the chart change fixes, and it was right -- this is a contradiction inside my own work: I identified the sticky bit as what makes `data delete` impossible, removed it from the chart in #667, and left this installer setting `chmod 3777` on the data dir. It matters exactly where this PR is supposed to help. The published chart has no init-writable-data, and the nothing-to-do fast path returns before Helm ever runs, so in both cases nothing comes along afterwards to correct a sticky bit the installer set. /data/shared would be left 3777 and the teardown -- a pod running as uid 65532 removing a tree the ingest wrote as 65534 -- cannot unlink it. Table dropped, files stranded. Adopt #667's per-directory split, so the installer and the chart's init container agree about the mode rather than fighting: /data/shared 2777 setgid + world-write, NO sticky /data/logs 3777 setgid + sticky (nothing deletes another writer's logs) The dirs are now emitted as path:mode pairs and split with ${e%:*} / ${e#*:} -- the same idiom the chart uses, so the two can be diffed by eye. Modes live in named constants for the same reason. Also fixed a flaw in my first pass: the loop used $m for BOTH the desired mode and the ls-derived one, which only worked because chmod happened to run first. The wanted mode is now $want, with a test pinning it, so the next edit can't quietly break it by reordering. Second review point -- branch was behind develop -- addressed by merging origin/develop (now includes #659 and the merged #667), and the manifest was regenerated against the post-merge file so it can't clobber #659's hash. Verified with `shasum -a 256 -c scripts/manifest.sha256`: 18/18 OK. Tests: 625 Pester (up 1 net: one stale single-mode assertion rewritten, two added), 24 bats, check-style + check-facts clean. Verified by EXECUTING the generated shell that data comes out drwxrwsrwx and logs drwxrwsrwt, and that the guard bites -- setting the shared dir back to 3777 takes the suite to 624/1. A bats substitution that silently expanded $want (leaving the chmod in place, so the "non-writable" case was quietly writable) is fixed too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ky on /data/shared Bugbot, and it is a follow-through miss from the previous commit: the prep moved to per-directory modes, but the failure-path hint still printed `chmod -R 3777` for BOTH dirs. A user following the installer's own copy-paste would put the sticky bit back on /data/shared and break `data delete` across uids -- ingest looking fixed while delete stayed broken, which is the precise failure #667 removes. My existing test asserted the hint's PATHS and not its MODES, which is exactly how this survived the switch. That gap is closed: the hint test now asserts the mode per dir, and that neither `3777` nor `-R` is applied to the data dir. Removed the possibility rather than testing for its absence twice. Get-ReleaseDirsSpec is now the single source for path+mode, and the prep command, the verification list and the repair hint all derive from it -- so they cannot disagree by construction. Drift here is invisible until someone actually runs the hint, which is the worst place to find it. Dropped -R while I was at it: the DIRECTORY's mode governs unlink, and recursing would stamp setgid/sticky onto every data file. Tests: 626 Pester (up 1 net), 24 bats, check-style clean, manifest 18/18 OK. Verified the guard bites -- restoring the `chmod -R 3777` hint takes the suite to 624/2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…st ingest works (#654) * fix(installer): pre-create the hostPath PV dirs on Windows so the first ingest works A fresh Windows install completed clean, then the user's FIRST `data ingest` failed: remote tar stderr: mkdir: can't create directory '/data/shared/.tracebloc-staging/': Permission denied The chart's hostPath PVs bind /tracebloc/<release>/data and /tracebloc/<release>/logs (mounted in the pod as /data/shared and /data/logs). When those host paths don't exist, kubelet's DirectoryOrCreate creates them root:root 0755 -- and kubelet IGNORES fsGroup on hostPath volumes (kubernetes#138411). uid 1000 then cannot create anything inside, and the staging tar dies on its first mkdir. The bash installer has always pre-created these (lib/cluster.sh _ensure_release_dirs, whose comment names this exact failure). install-k8s.ps1 never did -- it creates HOST_DATA_DIR but not the per-release data/logs subdirs. That asymmetry is the whole reason the failure was Windows-only. Current charts also ship an init-writable-data init container that fixes the same two paths at pod start, which masks the gap; it is not in the published chart, so every fresh Windows install lands on the broken combination. Prepare both dirs before Helm runs, matching init-writable-data's end state (chown 1000:1000, chmod 3777) so a cluster on an older published chart ends up in the same state as one on a current chart. mysql's PV is deliberately out of scope: it has its own init container and its datadir permissions are the database's business. Also repair on the nothing-to-do fast path. That path exits before Helm, so a cluster installed before this fix is healthy, shortcuts every re-run, and would keep failing at first ingest -- "re-run the installer" has to be a real remedy, not advice that quietly does nothing. Never fatal: a cluster that isn't k3d-shaped, a docker exec timeout, or a mount that can't represent POSIX ownership all degrade to a warning plus a copy-pasteable repair command. The install still completes. Reads the mode with POSIX `ls -ldn`, not `stat -c`. This code had that bug during development: -c is a GNU/coreutils flag BSD stat rejects, and it fails SILENTLY -- empty mode string, so a correctly-chmodded dir reports FAIL and the installer warns on a healthy install. Same family as the sha256sum --check trap (#429). The new bats tests EXECUTE the generated shell, which is what caught it; asserting on the command string alone could not have. Tests: 608 Pester (up 8) + 5 new bats that run the real command against temp dirs (OK path, FAIL path, idempotence, POSIX/dash syntax, stat -c guard). Verified the bats tests fail when `stat -c` is reintroduced. bats-hygiene, check-style, check-facts clean; manifest regenerated. No chart files touched, so no Chart.yaml bump. Closes#653 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): deliver the hostPath prep on stdin and follow the dataset mount Two Bugbot findings on #654, both real -- the prep would not have worked on Windows at all, and would have targeted the wrong path on a dataset-mount install. 1. Docker exec quoting (High). Invoke-BoundedProcess joins arguments into ONE command line and quotes any argument containing whitespace WITHOUT escaping inner quotes; its contract is "callers pass space-free tokens" (see its own comment). The prep script has both spaces and embedded "$d", so as an `sh -c <script>` argument Windows' command-line parser ends the quoted string at the script's first inner quote and hands sh a TRUNCATED program: the prep silently does nothing while the install reports success, and the Permission denied it exists to prevent survives. Same failure family as the kubectl patch that had to move to --patch-file. Send the script on STDIN instead -- `docker exec -i <node> sh` reads its program from stdin, and every argv token is then space-free. My tests could not have caught this: they execute the command string directly under sh, which skips the Windows argv layer entirely. Added a test that asserts the contract itself (no argv token contains whitespace, stdin carries the script), and switched the bats delivery test to pipe into `sh` the way the installer actually does. Verified the new test fails when the argv form is restored. 2. Dataset mount path (Medium). tracebloc.clientDataHostPath (_helpers.tpl) resolves data to <hostPath.datasetPath>/<release>/data, and the installer writes datasetPath: /tracebloc-data whenever HOST_DATASET_DIR is set. Prep hardcoded /tracebloc/<release>/data, so on a dataset-mount install it prepared a path nothing mounts while kubelet still created the real one root:root 0755 -- fixed-looking and still broken. Logs always stay on the local /tracebloc tree (logs-pvc.yaml hardcodes it), so only the data base is parameterised, matching how bash splits it in lib/cluster.sh _ensure_release_dirs. The repair hint now names the same paths that were prepared. Tests: 613 Pester (up 5) + 6 bats (up 1), all passing. bats-hygiene, check-style, check-facts clean; manifest regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): no false OK on a failed chmod, and read the data base from the node Two Bugbot findings on #654, both real. 1. False OK without world-write (Medium). The check passed a dir owned by uid 1000 regardless of its mode -- which contradicts the premise of the whole function. The processes that must write here are the ingestion Job (uid 65534, or HOST_UID) and the CLI staging pod (uid 65532); neither is 1000 and neither shares a group with it, so ONLY the other-write bit helps them. A chown that succeeded while the chmod failed therefore left a 0755 dir that no writer can use, and the owner shortcut called it OK, skipped the warning, and left the first ingest to die on Permission denied -- the exact silent-success shape this function exists to remove. Ownership is no longer a pass condition; the uid is still printed for diagnosis. 2. Dataset base ignored the live cluster (Medium). The base was chosen from $HOST_DATASET_DIR, which is not persisted in install state. A re-run or fast-path repair started without it prepared /tracebloc/<release>/data while the live release still mounted /tracebloc-data/<release>/data: successful output, nothing fixed. New Get-NodeDataBase asks the NODE's mount table instead, which is ground truth -- k3d bakes bind mounts in at cluster-create and cannot change them on a running cluster. It degrades in order: mount table -> env-var hint -> local tree, because a docker that cannot be reached tells us nothing about the mounts and must not be read as "no dataset mount". Tests: 619 Pester (up 6), 6 bats hostpath, bats-hygiene and check-style clean. Each guard was verified to fail when its own defect is reintroduced. That check also caught a gap in my first attempt: guarding Get-NodeDataBase alone still let the CALL SITE regress to the env var silently, so there is now a source assertion on the call site too. The stale assertion that encoded the refuted owner-is-writable premise was updated rather than left passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): require positive proof per dir, don't fail open on empty output Third Bugbot finding on #654, real: Initialize-ReleaseDataDirs treated any zero exit without a "FAIL " line as success. Empty or partial output passed the check. That matters because the interesting failure prints nothing at all: if the program never reaches `sh` -- stdin not attached, an empty here-doc, a docker exec that starts and immediately ends -- sh exits 0 having printed nothing, so there is no "FAIL " line to notice. The prep silently does nothing, the warning is skipped, the install reports fine, and the first ingest still dies on Permission denied. It is the SAME fail-open shape as the argv-quoting bug fixed earlier in this PR, which is the point: absence of failure cannot stand in for success here, because the mechanism most likely to break is the one that produces no output. Now every expected dir must report its own "OK <dir>" line, anchored, or the warning fires. Get-ReleaseDirsList is the single source of truth for that dir list, shared by the command builder and the verifier, so "what we prepared" and "what we demand proof for" cannot drift apart. Tests: 624 Pester (up 5), 24 bats, check-style clean. Verified the guard bites -- restoring "exit 0 + no FAIL = success" takes the suite to 621/3. The new cases cover empty output, a partial result, and output naming a DIFFERENT release (a stale or misrouted exec must not satisfy the check). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): don't re-create the sticky bit #667 removes from /data/shared Review found this reintroduced the bug the chart change fixes, and it was right -- this is a contradiction inside my own work: I identified the sticky bit as what makes `data delete` impossible, removed it from the chart in #667, and left this installer setting `chmod 3777` on the data dir. It matters exactly where this PR is supposed to help. The published chart has no init-writable-data, and the nothing-to-do fast path returns before Helm ever runs, so in both cases nothing comes along afterwards to correct a sticky bit the installer set. /data/shared would be left 3777 and the teardown -- a pod running as uid 65532 removing a tree the ingest wrote as 65534 -- cannot unlink it. Table dropped, files stranded. Adopt #667's per-directory split, so the installer and the chart's init container agree about the mode rather than fighting: /data/shared 2777 setgid + world-write, NO sticky /data/logs 3777 setgid + sticky (nothing deletes another writer's logs) The dirs are now emitted as path:mode pairs and split with ${e%:*} / ${e#*:} -- the same idiom the chart uses, so the two can be diffed by eye. Modes live in named constants for the same reason. Also fixed a flaw in my first pass: the loop used $m for BOTH the desired mode and the ls-derived one, which only worked because chmod happened to run first. The wanted mode is now $want, with a test pinning it, so the next edit can't quietly break it by reordering. Second review point -- branch was behind develop -- addressed by merging origin/develop (now includes #659 and the merged #667), and the manifest was regenerated against the post-merge file so it can't clobber #659's hash. Verified with `shasum -a 256 -c scripts/manifest.sha256`: 18/18 OK. Tests: 625 Pester (up 1 net: one stale single-mode assertion rewritten, two added), 24 bats, check-style + check-facts clean. Verified by EXECUTING the generated shell that data comes out drwxrwsrwx and logs drwxrwsrwt, and that the guard bites -- setting the shared dir back to 3777 takes the suite to 624/1. A bats substitution that silently expanded $want (leaving the chmod in place, so the "non-writable" case was quietly writable) is fixed too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(installer): the repair hint must not tell the user to re-set sticky on /data/shared Bugbot, and it is a follow-through miss from the previous commit: the prep moved to per-directory modes, but the failure-path hint still printed `chmod -R 3777` for BOTH dirs. A user following the installer's own copy-paste would put the sticky bit back on /data/shared and break `data delete` across uids -- ingest looking fixed while delete stayed broken, which is the precise failure #667 removes. My existing test asserted the hint's PATHS and not its MODES, which is exactly how this survived the switch. That gap is closed: the hint test now asserts the mode per dir, and that neither `3777` nor `-R` is applied to the data dir. Removed the possibility rather than testing for its absence twice. Get-ReleaseDirsSpec is now the single source for path+mode, and the prep command, the verification list and the repair hint all derive from it -- so they cannot disagree by construction. Drift here is invisible until someone actually runs the hint, which is the worst place to find it. Dropped -R while I was at it: the DIRECTORY's mode governs unlink, and recursing would stamp setgid/sticky onto every data file. Tests: 626 Pester (up 1 net), 24 bats, check-style clean, manifest 18/18 OK. Verified the guard bites -- restoring the `chmod -R 3777` hint takes the suite to 624/2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ed (#672) The rule said "for two releases". Verified against git instead: the `chown … && chmod …` shape entered in chart 1.9.20 (#611/#612, commit a07f76b) and survived every version through 1.9.33 — thirteen chart versions, not two. #667 (7852f02) rewrote the modes on that exact line and left the chain untouched, which is the more useful half of the lesson: the line was re-read for its modes and not for its control flow. Also corrects the issue's attribution of the chain to #667. Refs #672, #667, #611 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… fails (#672) (#689) * fix(chart): stop init-writable-data skipping the chmod when the chown fails (#672) init-writable-data ran `chown 1000:1000 "$d" && chmod "$m" "$d" || echo …`, so a refused chown short-circuited the chmod and the mode was never applied — while the message said "leaving as-is", implying nothing could be done. That inverts the priority. kubelet ignores fsGroup on hostPath (kubernetes/kubernetes#138411), so the MODE is what makes these trees usable: /data/shared must be other-writable for the ingestion Job (uid 65534, or HOST_UID) and the CLI staging/teardown pod (uid 65532), neither of which is 1000 nor shares a group with it. The chown is cosmetic next to that, and it is also the call most likely to be refused — on a Windows/Docker-Desktop bind mount or an NFS root_squash export it is precisely what fails. So the failure that mattered least was cancelling the one that mattered most, silently nullifying the 2777/3777 split from #667 on the platform that split was written for. Symptom: #653's `mkdir: can't create directory '/data/shared/.tracebloc-staging/': Permission denied`. The chown and the chmod are now separate best-effort statements, each recording whether it failed, and the per-dir verdict is graded on the mode OBSERVED afterwards via `ls -ldn` rather than on either exit status — a bind mount can accept a chmod and ignore it, so an exit code is not evidence. A partial result is reported as such ("chown failed; mode applied anyway") instead of implied. Unchanged: per-dir modes, per-dir independence, non-fatal behaviour, POSIX sh for busybox. Kept diffable by eye against the installer's Get-ReleaseDirsPrepCommand, which already does it this way. Verified by executing the helm-rendered command[2], not by reading it: - sh -n, dash -n, bash --posix -n all clean - busybox:1.35 as root: /data/shared drwxrwsrwx, /data/logs drwxrwsrwt, exit 0 - busybox:1.35 with --cap-drop CHOWN (chown refused, chmod permitted): modes STILL land drwxrwsrwx / drwxrwsrwt; the old command leaves both at drwxr-xr-x - /data/shared read-only (both calls fail): FAIL reported with the real errno, /data/logs still fixed, exit 0 - end-to-end on a shared volume after a refused chown: uid 65534 creates .tracebloc-staging and writes /data/logs; uid 65532 unlinks uid 65534's entries in /data/shared (no sticky) but not in /data/logs (sticky) — both splits intact Tests: the new #672 case fails against the old command and passes against the fix. The obvious comment-scoped guard (`^[^#\n]*chown.*&&.*chmod`) is silently VACUOUS — `${e#*:}` puts a '#' before the chown — so the guard is unscoped and the template describes the old shape in words instead. Existing assertions kept, updated for the multi-line command. jobs_manager_test.yaml 34 -> 35 passing; full suite 379 -> 380 passing with develop's 5 failed / 5 errored baseline unchanged. Also adds the recurring-finding rule to .cursor/BUGBOT.md per CLAUDE.md. Refs #672, #667, #653, #654 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(chart): report only what init-writable-data actually observed (#672) Two overclaims in the first commit's own reporting, both found by running the failure paths rather than reading them — the same family as the bug being fixed. 1. The verdict grades other-writability alone (correctly: that is what decides whether uid 65534/65532 can work, and failing a setgid-stripped-but-writable mount would cry wolf on a working install). But it labelled that bare "OK", which reads as "the whole mode landed". Now says "OK <dir> other-writable" and always prints want vs got, so a mount that granted other-write while dropping S_ISGID is visible instead of implied. 2. Worse: the partial-result note said "mode applied anyway" whenever any call failed. On a dir that was ALREADY other-writable and where BOTH calls were refused, that is simply false — nothing this container did applied anything. Reproduced in busybox:1.35 (pre-set 1777, run as a non-owner uid so chown and chmod are both refused): want 2777 got drwxrwxrwt uid 0 (chown+chmod failed; mode applied anyway) Now reads "(chown+chmod failed; other-writable regardless)" — it claims the observation, not a causal link it cannot support. Re-verified on the helm-rendered command[2]: sh -n / dash -n / bash --posix -n clean; root happy path lands drwxrwsrwx + drwxrwsrwt; chown-refused still lands both modes; already-1777 with both calls refused now reports truthfully; read-only /data/shared still FAILs with the real errno while /data/logs is still fixed; exit 0 throughout. Tests pin both strings, including a notMatchRegex on the old "mode applied anyway" wording. 35 passing, full suite 380 with develop's 5 failed / 5 errored baseline unchanged. Refs #672 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(chart): move the verdict rationale out of the container command (#672) The long "why other-writability alone is the pass condition" prose was inside the script passed to `sh -c`, so it shipped in the pod spec and showed up in every `kubectl get deploy -o yaml`. It belongs in the YAML comment above, which does not. Left a two-line pointer where a script editor will see it. Also records the one intentional divergence from the installer's Get-ReleaseDirsPrepCommand: the chart does not redirect chown/chmod stderr to /dev/null, so the real errno (Operation not permitted vs Read-only file system) lands in `kubectl logs` next to the verdict. The installer suppresses it because its output is a user-facing progress line; an init container's log is a debugging surface, and hiding the errno there would remove the evidence a reader needs. Comment-only inside command[2]: re-rendered and re-ran the chown-refused path in busybox:1.35 to confirm byte-identical output and modes (drwxrwsrwx / drwxrwsrwt, exit 0). 35 passing; full suite 380 passing, baseline unchanged. Refs #672 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(bugbot): correct how long the chained chown/chmod actually shipped (#672) The rule said "for two releases". Verified against git instead: the `chown … && chmod …` shape entered in chart 1.9.20 (#611/#612, commit a07f76b) and survived every version through 1.9.33 — thirteen chart versions, not two. #667 (7852f02) rewrote the modes on that exact line and left the chain untouched, which is the more useful half of the lesson: the line was re-read for its modes and not for its control flow. Also corrects the issue's attribution of the chain to #667. Refs #672, #667, #611 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…d the Windows prep (#673) (#700) _ensure_release_dirs applied a flat, recursive `chmod -R 777` to data and logs, while Get-ReleaseDirsPrepCommand (#654) and the chart's init-writable-data (#667) both apply a per-dir 2777/3777 split without recursing. Three implementations of one intent, two agreeing and one not — and the odd one out was the copy #667 said should be diffable by eye. Nothing was user-visibly broken: 777 is other-writable and carries no sticky bit, so cross-uid `data delete` worked on the bash path, and on Linux the chart's init container rewrote both dirs at pod start anyway. That absence of a symptom is why the divergence survived two PRs, and why this lands with a test rather than just a fix. - data -> 2777 (setgid, NO sticky: `data delete` unlinks as another uid, #667) - logs -> 3777 (setgid + sticky: nothing has to delete another writer's logs) - drop -R: the dir's own mode governs creation and unlink; recursing stamped setgid/sticky onto every data FILE and walked the whole dataset tree to do it - mysql keeps its recursive 777 — one writer, its own init container, datadir permissions are the database's business (out of scope in #654 for the same reason) - split the pairs on the LAST colon, so a HOST_DATA_DIR containing one can't silently chmod a path that does not exist Tests: hostpath-prep.bats now extracts path:mode pairs from all three sources (bash _release_dirs_spec, the ps1's Get-ReleaseDirsSpec rows + $TB_*_DIR_MODE constants, the chart's init-writable-data loop) and fails if any pair disagrees; cluster.bats asserts the applied modes, that a pre-existing file under data/logs keeps its mode, that mysql stays recursive, and the colon case. Each guard was mutation-checked in all three sources. Closes#673 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The chart half of a
data deletethat strands its files. The ingestor half istracebloc/data-ingestors#476.
The bug
data deletedrops the table, then fails to remove the files — leaving a half-deleted dataset:gone from the database, still occupying storage, and un-retryable because the table the retry
looks for no longer exists.
Root cause
init-writable-dataset both shared dirs to3777— setgid plus sticky, /tmp semantics,so one writer can't unlink another writer's files. That protection is correct for
/data/logsandwrong for
/data/shared, because on/data/sharedsomething legitimately has to:data deleteremoves a tree the ingest wrote, and the two run as different identities.
nobody), orHOST_UIDfsGroup655321000:1000The sticky bit permits an unlink only by the entry's owner, the directory's owner, or root — and
the teardown pod is none of the three.
fsGroup, which normally aligns identities like this bymaking kubelet recursively
chgrpthe volume, does nothing here: kubelet ignoresfsGrouponhostPath volumes (kubernetes#138411),
the layout every installer-provisioned cluster uses.
The change
The mode becomes per-directory instead of one value for both:
/data/shared2777— setgid, no sticky/data/logs3777— setgid + stickyBoth keep setgid, so new entries still inherit GID 1000, and each dir is still fixed independently
and best-effort — a
root_squashexport still degrades to a logged skip rather than blockingstartup.
I removed the protection only where it provably blocks a required operation, rather than
everywhere: keeping sticky on
/data/sharedwould mean protecting writers from each other at thecost of never being able to clean up after them.
Verification
/data/sharedcomesout
drwxrwsrwxand/data/logsdrwxrwsrwt. Valid under bothsh -nanddash -n.helm unittestjobs_manager: 34/34./data/sharedback to3777failsasserts[6]andasserts[7].helm lintare unchanged from develop — both have the same pre-existing5 failed / 5 errored and the
clientId/clientPasswordminLengthlint errors, which needreal values. I verified this by stashing and re-running on clean
develop.🤖 Generated with Claude Code
Note
Medium Risk
Touches hostPath volume permissions for a multi-writer data path; lowers unlink protection on /data/shared while fixing a broken delete path—behavior change on upgrade/re-init, not auth or payment logic.
Overview
Fixes
data deleteleaving dataset files on disk after the DB row is gone on hostPath installs, by changing howinit-writable-datachmods the shared volume.Previously both
/data/sharedand/data/logswere set to3777(setgid + sticky). Sticky blocked the CLI teardown pod (uid 65532) from unlinking trees written by the ingest job (uid 65534), because neither is owner of those entries./data/sharedis now2777(setgid only);/data/logsstays3777. The init loop applies per-path modes via/data/shared:2777 /data/logs:3777.Chart version/appVersion bump to 1.9.30, with helm unittest updates so regressions to
3777on shared fail.Reviewed by Cursor Bugbot for commit 392a82a. Bugbot is set up for automated code reviews on this repo. Configure here.