Skip to content

fix(installer): pre-create the hostPath PV dirs on Windows so the first ingest works - #654

Merged
LukasWodka merged 10 commits into
developfrom
fix/653-windows-hostpath-prep
Aug 11, 2026
Merged

fix(installer): pre-create the hostPath PV dirs on Windows so the first ingest works#654
LukasWodka merged 10 commits into
developfrom
fix/653-windows-hostpath-prep

Conversation

@shujaatTracebloc

@shujaatTraceblocshujaatTracebloc commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes#653.

The bug

A fresh Windows install completes clean, then the firstdata ingest fails:

remote tar stderr: mkdir: can't create directory '/data/shared/.tracebloc-staging/': Permission denied

Nothing in the install output hints at it. The cluster is healthy, the release is deployed, and the
failure only surfaces at first use.

Root cause — a bash/PowerShell asymmetry, not a chart problem

The chart's hostPath PVs bind /tracebloc/<release>/data and /tracebloc/<release>/logs (in the
pod: /data/shared, /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 can't create anything inside, so the staging tar dies on
its first mkdir.

The bash installer has always pre-created these — _ensure_release_dirs in
scripts/lib/cluster.sh, whose own comment names this exact failure:

Without pre-creating these as the host user, kubelet's DirectoryOrCreate makes them root:root
0755 and the host user can't drop training data into /data/shared.

install-k8s.ps1 had no equivalent — it creates HOST_DATA_DIR but never the per-release
data/logs subdirs. That asymmetry is the entire reason this 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 — but it isn't in the currently published chart, so every fresh
Windows install lands on the broken combination.

The fix

  • Get-ReleaseDirsPrepCommand (pure) builds the sh -c body; Initialize-ReleaseDataDirs runs it
    in the server node and reports honestly.
  • Runs before Helm, keyed on the release name Helm is about to touch (adopted vs fresh) — the
    PV path embeds it, so preparing the wrong release would look successful and fix nothing.
  • Matches 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.
  • Also repairs on the nothing-to-do fast path. That path exit 0s 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 non-k3d cluster, a docker exec timeout (code 124), or a mount that can't
    represent POSIX ownership all degrade to a warning plus a copy-pasteable repair command; the
    install completes either way, and on a current chart the init container fixes the same dirs anyway.
  • mysql's PV is deliberately out of scope: it has its own init container and its datadir
    permissions are the database's business.

A bug this code had during development, and what it changes about the tests

The first version read the mode with stat -c. That's a GNU/coreutils flag BSD stat rejects, and
it fails silently
— no output, empty mode string, so a correctly-chmodded directory reported
FAIL and the installer would warn on a perfectly healthy install. Same family as the
sha256sum --check trap (#429).

Asserting on the command string could never have caught that. The new bats tests execute the
generated command against temp dirs, which is what found it — so the fix now uses POSIX ls -ldn,
which behaves identically on busybox (rancher/k3s), coreutils (the CUDA node image) and BSD.

Verification

  • Pester: 608 passed, 0 failed (+8 new).
  • bats: 5 new tests in hostpath-prep.bats, all passing — POSIX/dash syntax check, OK path,
    FAIL path (mode 0755 not owned by 1000, i.e. what kubelet leaves), idempotence on re-run, and a
    stat -c regression guard.
  • Proved the tests bite: reintroducing stat -c fails bats 2, 3 and 5. (1 still passes — the
    syntax is still valid; 4 fails either way — that's the correct discrimination.)
  • bats-hygiene.bats clean (18 ok), so the new assertions are || return 1-hardened.
  • check-style.sh and check-facts.sh clean; scripts/manifest.sha256 regenerated (the .ps1 is
    manifested — 1 line changed).
  • No client/templates|values|schema changes, so no Chart.yaml bump is required.
  • Full scripts/tests/*.bats: 428 ok, 1 failure — install-bootstrap.bats "early bailout: healthy
    tracebloc doctor". Pre-existing: I reproduced it on a pristine origin/develop worktree, so
    it is untouched by this change and left alone.

What I could not verify locally

I develop on macOS, so the docker exec into a k3d node isn't exercised here — the shell it sends
is executed and tested, but not through Docker on Windows. Worth one confirmation on a Windows box:
a fresh install should ingest with no manual chmod, and a re-run on an existing cluster should
repair it.

Scope note

The durable fix for users on a stale published chart is publishing a chart that contains
init-writable-data. This PR makes the installer correct on its own terms regardless of which
chart version it lands on, which is the part that belongs in code.

🤖 Generated with Claude Code


Note

Medium Risk
Touches install-time host permissions and release PV paths on Windows/k3d; failures are warned-only but wrong modes could still break ingest or delete until repaired.

Overview
Parity with the bash installer:install-k8s.ps1 now pre-creates and chmods the chart’s data and logs hostPath dirs inside the k3d node before Helm (and again on the healthy “nothing to do” fast path), so kubelet doesn’t leave them root-owned 0755 and the first data ingest doesn’t hit Permission denied on /data/shared.

The prep uses per-dir modes aligned with chart init-writable-data (#667)2777 on shared data (no sticky, so cross-uid data delete works) and 3777 on logs — with data rooted at /tracebloc-data when the node has a dataset bind mount (Get-NodeDataBase via docker inspect, not ephemeral HOST_DATASET_DIR). The shell script is fed on stdin to docker exec … sh (avoids Windows argv quoting bugs), verifies writability with POSIX ls -ldn (not stat -c), requires an OK line per expected path, and on failure emits a copy-paste repair hint without aborting install.

Tests: large new Pester coverage plus hostpath-prep.bats that actually runs the extracted prep script; scripts/manifest.sha256 updated for install-k8s.ps1.

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

…st 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>
@shujaatTraceblocshujaatTracebloc self-assigned this Aug 10, 2026
@shujaatTracebloc
shujaatTracebloc marked this pull request as ready for review August 10, 2026 14:54
Comment threadscripts/install-k8s.ps1 Outdated
Comment threadscripts/install-k8s.ps1 Outdated
…aset 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>
saadqbal
saadqbal previously approved these changes Aug 11, 2026
aptracebloc
aptracebloc previously approved these changes Aug 11, 2026

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

@shujaatTraceblocApproved. Verified against a fresh clone of the head.

  • R8 manifestshasum -a 256 -c scripts/manifest.sha256 → all 18 OK incl. scripts/install-k8s.ps1 (head 3fbbb32).
  • batshostpath-prep.bats 6/6 (validates the POSIX ls -ldn cross-platform path and that the tests execute the generated shell incl. the FAIL path). Pester ran green in CI on windows + ubuntu (no pwsh locally to re-run).
  • Logic: prep runs before the adopt/fresh split with the release key the PV paths embed, guarded/idempotent/non-fatal, node paths POSIX (in-node hostPath, not Windows), no check-facts drift. The root cause is permissions (kubelet's DirectoryOrCreate makes the dir root:root 0755 and ignores fsGroup on hostPath — kubernetes#138411), and the in-node chown 1000:1000 + chmod 3777 matching the chart's init-writable-data is the correct fix. Both Bugbot findings (docker-exec quoting → stdin delivery; HOST_DATASET_DIR path) resolved.

Code-owner @saadqbal already approved the current head. The PR is still BLOCKED but not on anything in the diff — code-owner met, mergeable, 36 checks green; it looks like an org-level develop ruleset requiring a context that's stuck queued with 0 runs (claude/github-pages ghost suites). Worth an admin checking the develop ruleset's required-checks list — nothing to fix in this PR.

This is the correct/complete fix for #653; I've recommended closing the competing #659 in its favor (they conflict on the R8 manifest).

🤖 Generated with Claude Code

@LukasWodka

Copy link
Copy Markdown
Contributor

@aptracebloc@divyasinghds@shujaatTracebloc@saadqbal#659 has since merged, so the collision Arturo predicted is now live and needs reconciling. I dug into the mechanism to work out which fix is actually load-bearing. Posting the evidence rather than an opinion.

The premise that decides it: HOST_DATA_DIR is bind-mounted into the node

scripts/install-k8s.ps1:3192 passes:

-v ${HOST_DATA_DIR}:/tracebloc@all

and the chart's hostPath resolves to /tracebloc/<release>/data (client/templates/_helpers.tpl:113, datasetPath defaulting to /tracebloc).

So on this layout the Windows directory is the in-node path — creating HOST_DATA_DIR\<release>\data on the host makes /tracebloc/<release>/data exist inside the k3d node, through the same bind mount. kubelet never reaches DirectoryOrCreate, because the path is already there.

That means the DirectoryOrCreateroot:root 0755 + fsGroup-ignored reasoning (correct for a native hostPath) doesn't apply to a Docker Desktop bind mount, which supplies its own ownership/mode. Divya's note — "Docker Desktop handles Windows mount ownership" — matches that. It's also, tellingly, what this PR's own script already assumes: chown 1000:1000 … 2>/dev/null is non-fatal and the verifier accepts eithero = 1000or an other-writable mode:

case "$m" in ????????w*) w=1;; *) w=0;; esac; [ "$o" = 1000 ] && w=1

A Docker Desktop mount lands on the second branch. So the in-node step is written to tolerate exactly the situation where the host-side pre-create was already sufficient.

On the HOST_DATASET_DIR gap

That one is covered by #659: Ensure-ReleaseDirs branches on it ($dataBase = HOST_DATASET_DIR\<release>), and the installer both mounts it (install-k8s.ps1:3202, -v ${HOST_DATASET_DIR}:/tracebloc-data@all) and points the chart at it (:4389, datasetPath: /tracebloc-data).

What this PR still adds that #659 does not

Genuinely useful, and I would not want it dropped:

And one honest caveat: whether a Docker Desktop Windows bind mount really does present writable to uid 1000 is an empirical question, and I have no Windows host to settle it. If it does not, this PR's in-node step isn't a belt — it's the actual fix. That argues for keeping it either way.

Suggested reconciliation

Rather than closing either one:

  1. Rebase this PR onto develop (where fix(installer): pre-create per-release hostPath dirs on Windows (#653) #659's Ensure-ReleaseDirs + regenerated manifest.sha256 now live).
  2. Keep only what's additiveGet-ReleaseDirsPrepCommand / Initialize-ReleaseDataDirs, the verification + hint, and both test files — layered after the existing host-side pre-create.
  3. Regenerate scripts/manifest.sha256 once on top of develop. That dissolves the mutually-exclusive-hash problem by construction.

End state: host-side pre-create (fixes the common Docker-Desktop path) plus in-node ownership enforcement with verification (covers any layout where the mount doesn't present writable) plus the test coverage. Neither review gets overruled.

Happy to do the rebase/reduction myself if @aptracebloc / @shujaatTracebloc prefer — it's your PR, so I haven't touched it.

🤖 Generated with Claude Code

Resolves the scripts/manifest.sha256 conflict by regenerating it (the only
correct resolution -- the file is generated, so neither side's hash is right
after a merge that changes install-k8s.ps1).
develop gained Ensure-ReleaseDirs (#659), which pre-creates the per-release dirs
from the WINDOWS side. It complements this branch rather than duplicating it, and
both are needed:
- Ensure-ReleaseDirs is the only thing that can create the dirs before the
bind mount exists, but New-Item cannot set POSIX ownership or mode -- Windows
has neither concept.
- Initialize-ReleaseDataDirs fixes the half a container sees. kubelet ignores
fsGroup on hostPath (kubernetes#138411), so unless data/logs are world-writable
IN-NODE, the ingestion Job (uid 65534) and the CLI staging pod (uid 65532)
cannot write to a tree the chart chowns to 1000.
Creation without mode is not enough; mode without creation would race kubelet's
DirectoryOrCreate. Added a comment at both functions saying so, since the obvious
review reaction is to delete one as redundant -- which re-opens #653.
Tests after the merge: 613 Pester, 6 bats hostpath, check-style clean; manifest
regenerated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Comment threadscripts/install-k8s.ps1
Comment threadscripts/install-k8s.ps1
… 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>
Comment threadscripts/install-k8s.ps1
…pty 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>

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

Requesting changes — the implementation is strong (pure Get-ReleaseDirsPrepCommand, ls -ldn for BSD/busybox portability, ground-truth mount detection, honest per-dir OK/FAIL, thorough Pester+bats coverage, CI green) and it correctly complements the merged #659 rather than duplicating it. Two things block merge:

  1. It reintroduces the bug client#667 is removing. Get-ReleaseDirsPrepCommand runs chmod 3777 on every release dir, including the data dir that maps to /data/shared. That's setgid + sticky — the mode #667 deletes from /data/shared so the data delete teardown (a different uid) can clean up. In the scenarios this PR targets — the currently-published chart has no init-writable-data (per this PR's own body), and the 'nothing-to-do fast path' that exit 0s before Helm — nothing runs afterward to correct it, so /data/shared is left 3777 sticky and data delete breaks on Windows, re-opening #476. Please adopt #667's per-directory split: 2777 (setgid, no sticky) for the shared/data dir, 3777 for logs.

  2. Branch is diverged / behind_by=3 from develop (which now includes the merged #659). Per our 'merge develop back in before review / never build on a stale checkout' rule, please rebase — and regenerate the manifest.sha256 line for install-k8s.ps1 against the post-#659 file, or it'll clobber #659's hash.

Neither is a code-quality issue — the mode constant just predates #667's sticky split and the branch went stale. Adopt 2777 on the shared dir + rebase and I'll approve.

shujaatTraceblocand others added 2 commits August 11, 2026 14:01
…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>
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

@divyasinghds — both points were right, and the first one is a contradiction inside my own work. Fixed in a672e68.

1. Sticky bit reintroduced. 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. Your scenario analysis is exactly right about why that isn't cosmetic: the published chart has no init-writable-data, and the nothing-to-do fast path returns before Helm 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 — uid 65532 removing a tree the ingest wrote as 65534 — cannot unlink it. Table dropped, files stranded, re-opening the delete bug.

Now on #667's per-directory split:

DirMode
/data/shared2777setgid + world-write, no sticky
/data/logs3777setgid + sticky — nothing deletes another writer's logs

Dirs are emitted as path:mode pairs and split with ${e%:*} / ${e#*:} — the same idiom the chart's init container uses, so the installer and the chart can be diffed by eye instead of drifting. The modes are named constants for the same reason.

Reviewing that also turned up 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 a reorder can't quietly break it.

2. Behind develop. Merged origin/develop (now includes #659 and the merged #667). The manifest was regenerated against the post-merge file specifically so it can't clobber #659's hash — verified with shasum -a 256 -c scripts/manifest.sha256: 18/18 OK.

Verification: 625 Pester, 24 bats, check-style + check-facts clean. I executed the generated shell rather than only asserting on its text — data comes out drwxrwsrwx, logs drwxrwsrwt — and confirmed the guard bites: putting the shared dir back to 3777 takes the suite to 624/1. One more real bug surfaced while writing that: a bats substitution silently expanded $want, leaving the chmod in place so the "non-writable" case was quietly writable and the test proved nothing. Escaped and re-verified.

On your note about the earlier approval mentioning 3777 as correct — that was accurate against the chart at the time; #667 changed the target state for /data/shared, and this PR now follows it.

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

Comment threadscripts/install-k8s.ps1 Outdated
…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>
@shujaatTracebloc

Copy link
Copy Markdown
ContributorAuthor

@divyasinghds — both of your conditions are met; requesting re-review.

1. Per-dir sticky split adopted (a672e68). /data/shared2777 (setgid + world-write, no sticky), /data/logs3777. Dirs are emitted as path:mode pairs and split with ${e%:*} / ${e#*:} — the same idiom init-writable-data uses, so the installer and the chart can be diffed by eye rather than drifting. Verified by executing the generated shell: data comes out drwxrwsrwx, logs drwxrwsrwt.

2. Branch current + manifest regenerated (2acd00e). Merged origin/develop — now includes #659 and the merged #667 — and regenerated scripts/manifest.sha256 against the post-merge file so it can't clobber develop's hash. shasum -a 256 -c: 18/18 OK.

Your review also led to two further fixes worth flagging:

  • Bugbot then caught that the repair hint still printed chmod -R 3777 for both dirs — a user following the installer's own copy-paste would have put the sticky bit back on /data/shared and broken data delete. Rather than fix it in place, Get-ReleaseDirsSpec is now the single source of path+mode and the prep command, the verification list and the hint all derive from it, so they can't disagree by construction. The hint test asserted paths but not modes, which is how it slipped; it now asserts modes per dir. Dropped -R too — the directory's mode governs unlink, and recursing would stamp setgid onto every data file.
  • The shell loop used $m for both the desired and the observed mode, working only because chmod ran first. Now $want, with a test pinning it.

Current state: 626 Pester, 24 bats, check-style + check-facts clean, 34 CI checks green, Bugbot success on the current head, 0 unresolved threads. @saadqbal approved at 14:02 after these changes.

One thing I could not verify and don't want to imply I did: the prep path has never executed against a real k3d node — no CI runner drives Docker on Windows. CI proves the generated shell is correct and that argv/stdin delivery holds; the end-to-end confirmation is a fresh Windows install that ingests with no manual chmod, and then a data delete.

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

Approved, with follow-ups — none blocking.

The diagnosis is correct, the mechanism claims hold up under independent checking, and the tests are the good kind: they execute the shell instead of asserting on its string. Posting what I verified rather than just a verdict.

Verified independently at head 2acd00e

CheckResult
shasum -a 256 -c scripts/manifest.sha25618/18 OK, incl. the .ps1
bats scripts/tests/hostpath-prep.bats6/6 pass
bats scripts/tests/bats-hygiene.bats18 ok
Generated shell on busybox (the k3s node's shell), ubuntu 24.04 (CUDA node image) and macOS/BSD — piped on stdin exactly as the installer delivers itIdentical output on all three: data → drwxrwsrwx (2777), logs → drwxrwsrwt (3777)
Mode alignment vs merged #667Byte-for-byte the same idiom and modes as the chart's init-writable-data (for e in /data/shared:2777 /data/logs:3777 … chown 1000:1000)
Path split vs charttracebloc.clientDataHostPath = <datasetPath|/tracebloc>/<release>/data; logs-pvc.yaml hardcodes /tracebloc/<release>/logs — so parameterising only data is right
Call orderingNew-K3dCluster (step 3) precedes Install-ClientHelm (step 6), so the node exists when prep runs
Invoke-BoundedProcessOutput is a single concatenated string, so the (?m)^… anchors work; checked the mount regex and the OK regex including CRLF. Stdin plumbing (RedirectStandardInput/Write/Close) is correct

The ls -ldn portability fix is the real substance here and it holds up. I also chased a suspicion that the Should -Invoke Warn -Times 0 assertions were vacuous — they are not; Pester special-cases -Times 0 as exactly-zero even without -Exactly.

Findings

1. The writability check can't see the sticky bit it was tightened to prevent. The pass condition is other-write only (????????w*). Setting a data dir to 1777 and disabling the chmod yields OK … drwxrwxrwt — silently — and sticky on /data/shared is exactly what #667 removed because it makes cross-uid data delete impossible. Narrow trigger (chmod must fail while sticky is set; root_squash NFS is the realistic one), but by this PR's own standard — "absence of failure cannot stand in for success" — the verdict should also assert the sticky expectation derived from $want.

2. if (-not $Release) { return } in Initialize-ReleaseDataDirs is unreachable, and contradicts "Never fatal".[Parameter(Mandatory)][string] throws ParameterBindingValidationException on "" before the body ever runs. Not currently reachable — ConvertTo-WorkspaceName cannot return empty and $pvRelease falls back to $TB_NAMESPACE — but the sibling Ensure-ReleaseDirs($release) no-ops on empty while this one would abort the install. Pick one: drop Mandatory, or drop the guard.

3. The repair hint is chmod-only. If prep failed at mkdir (unwritable parent), the hint's chmod fails the same way — the copy-pasteable remedy doesn't remedy the case that produced it. Worth adding mkdir -p and the chown.

4. Out of scope, but worth its own issue: the merged chart's init-writable-data is chown … && chmod …. chmod is skipped whenever chown fails — precisely the root_squash / pre-provisioned mount its own error message names. This PR's PowerShell gets it right with two independent best-effort statements. That makes the installer prep more load-bearing than the description claims, not redundant.

5. bash is now the odd one out._ensure_release_dirs still does chmod -R 777 — no setgid, and recursive, which the new PowerShell comment argues against. No user-visible bug (777 has no sticky, so delete works), but a change framed as "fix the bash/PowerShell asymmetry" exits with a new one pointing the other way. Follow-up to align bash on the path:mode split.

6. Closes #653 is stale.#653 was closed by #659 at 07:52:59 on 2026-08-11, so that card is already terminal and this PR closes nothing. Retarget the description at a fresh issue, or state plainly "follow-up to #659, no separate ticket", so the board reflects reality.

On the "could not verify locally" note

Most of that gap is now closed — the shell is proven on busybox, coreutils and BSD, delivered on stdin. What's left is genuinely Windows-specific and worth the one confirmation you asked for: on a Docker Desktop bind mount from a Windows path, the in-node chown/chmod will most likely be a silent no-op (9p/virtiofs), so whether the dirs read back as world-writable decides whether this prints nothing or emits "Couldn't confirm the data directories are writable" on a perfectly healthy install — with a hint whose chmod also cannot work there. UX risk rather than a correctness one (the path is non-fatal, and #659 already creates the dirs Windows-side), but it is the same class as the stat -c bug: a scary warning on a good install.

Merge state

Zero unresolved threads and @saadqbal has approved at head, so the only thing still blocking is @divyasinghds's CHANGES_REQUESTED from b96d3327 — GitHub's stale-dismissal clears approvals but not change requests, so it stands until she re-reviews. Both her conditions look met to me (per-dir mode split in a672e68, branch current + manifest in 2acd00e).

@LukasWodka
LukasWodka merged commit f5f6492 into developAug 11, 2026
37 checks passed
shujaatTracebloc added a commit that referenced this pull request Aug 13, 2026
… 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>
shujaatTracebloc added a commit that referenced this pull request Aug 13, 2026
…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>
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.

Windows: first data ingest fails with Permission denied on /data/shared — installer never pre-creates the hostPath PV dirs (bash does)

5 participants

@shujaatTracebloc@LukasWodka@saadqbal@divyasinghds@aptracebloc