Skip to content

fix(installer): pre-create per-release hostPath dirs on Windows (#653) - #659

Merged
LukasWodka merged 2 commits into
developfrom
fix/653-windows-release-dirs
Aug 11, 2026
Merged

fix(installer): pre-create per-release hostPath dirs on Windows (#653)#659
LukasWodka merged 2 commits into
developfrom
fix/653-windows-release-dirs

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Closes#653 (backlog cleanup epic backend#1588).

Bug

bash `_ensure_release_dirs` (scripts/lib/cluster.sh) pre-creates `<HOST_DATA_DIR>//{logs,mysql}` + the `data` dir before install; `install-k8s.ps1` had no equivalent. On Windows the mount target then does not exist when the client first ingests, and the write fails with Permission denied.

Fix

Adds `Ensure-ReleaseDirs` (mirrors the bash: logs/mysql under HOST_DATA_DIR, data under HOST_DATASET_DIR when set else HOST_DATA_DIR; idempotent via `New-Item -Force`) and calls it before both helm paths (fresh install `$TB_NAMESPACE` and adopted-reuse `$existingName`).

Verified: PowerShell AST parses clean (pwsh 7.5.2). Functional validation is the Windows e2e journey (no local Windows).


Note

Low Risk
Installer-only change that creates local directories before Helm; no cluster auth or runtime logic changes, with fail-fast behavior on create errors.

Overview
Fixes Windows dataset ingest failing with "Permission denied" (#653) by bringing the PowerShell installer in line with bash _ensure_release_dirs.

Adds Ensure-ReleaseDirs, which idempotently creates <HOST_DATA_DIR>/<release>/logs, mysql, and data (with data under HOST_DATASET_DIR when set, per backend#743). Directory creation uses New-Item -Force with -ErrorAction Stop so real failures (ACL, AV locks) are not swallowed and later show up as permission errors after a successful-looking install.

The helper runs immediately before both Helm paths: fresh helm upgrade --install for $TB_NAMESPACE and adopted-release reconcile for $existingName. scripts/manifest.sha256 is updated for install-k8s.ps1.

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

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

Comment threadscripts/install-k8s.ps1
Ensure-ReleaseDirs used -ErrorAction SilentlyContinue, so a real create failure
(ACL, AV lock, path conflict) was swallowed and the installer printed connected
before the first ingest hit the same Windows 'Permission denied' this pre-create
exists to prevent. -Force already gives idempotency; switch to -ErrorAction Stop
to match bash's 'mkdir -p' under set -e (Bugbot, #653). Regenerate
scripts/manifest.sha256 for the edited .ps1 (supply-chain gate).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Fixed — Ensure-ReleaseDirs now uses -ErrorAction Stop, so a failed create aborts the install (fail closed, matching bash mkdir -p under set -e) instead of printing connected and deferring the error to first ingest. -Force keeps it idempotent. Manifest regenerated.

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

@LukasWodkaRecommend closing this in favor of #654 (cc @shujaatTracebloc). The R8 manifest here verifies clean (shasum -c → all 18 OK) and the PowerShell is well-written, but two things:

  1. It likely doesn't fix #653. This pre-creates the dirs on the Windows host via New-Item, but the failure is a permissions one: kubelet's DirectoryOrCreate creates the in-node mount path root:root 0755 and ignores fsGroup on hostPath (kubernetes#138411), so host-side existence alone doesn't make uid 1000 able to write — and New-Item can't set uid 1000 / mode 3777. Also, when HOST_DATASET_DIR is set the data binds at a different path, which this doesn't cover.
  2. It conflicts with #654. Both edit scripts/install-k8s.ps1 + regenerate scripts/manifest.sha256 with mutually-exclusive hashes, so they can't both merge — whichever lands second fails the manifest check.

#654 (already code-owner-approved, with Pester + hostpath-prep.bats coverage) does the in-node chown 1000:1000 + chmod 3777 that mirrors the chart's init-writable-data container and handles the dataset-mount path split — it's the correct/complete fix. Suggest closing #659 unless there's a Windows case #654 misses (if so, let's reconcile with @saadqbal since they collide on the manifest).

🤖 Generated with Claude Code

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

Approving — PowerShell Ensure-ReleaseDirs faithfully mirrors bash _ensure_release_dirs (logs/mysql under HOST_DATA_DIR, data under HOST_DATASET_DIR when set) and runs before both helm paths. Verified: manifest.sha256 matches the modified install-k8s.ps1 exactly; the node-local guard is correctly omitted (no Windows path, install-k8s.ps1:2718); chmod 777 correctly not mirrored (Docker Desktop handles Windows mount ownership). CI green incl. Pester (windows-latest). ✅

@LukasWodka
LukasWodka merged commit 573d8cc into developAug 11, 2026
37 checks passed
@LukasWodka
LukasWodka deleted the fix/653-windows-release-dirs branch August 11, 2026 07:53
shujaatTracebloc added a commit that referenced this pull request Aug 11, 2026
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>
shujaatTracebloc added a commit that referenced this pull request Aug 11, 2026
…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>
LukasWodka pushed a commit that referenced this pull request Aug 11, 2026
…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>
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)

3 participants

@LukasWodka@divyasinghds@aptracebloc