Skip to content

fix(chart): derive pvcAccessMode from hostPath mode so the single-node PVC binds (#559) - #658

Merged
LukasWodka merged 3 commits into
developfrom
fix/559-pvc-accessmode-hostpath
Aug 11, 2026
Merged

fix(chart): derive pvcAccessMode from hostPath mode so the single-node PVC binds (#559)#658
LukasWodka merged 3 commits into
developfrom
fix/559-pvc-accessmode-hostpath

Conversation

@LukasWodka

@LukasWodkaLukasWodka commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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

Bug

The 3 storage PVCs defaulted `pvcAccessMode` to ReadWriteMany, but in hostPath (single-node / bare-metal) mode the paired PersistentVolume is hardcoded ReadWriteOnce — so the PVC could not bind to its own PV. values.yaml already documented the intent (RWM for dynamic, RWO for hostPath); the flat default just didn't honor it, and single-node installs don't set the override.

Fix

Mode-derived default: `ternary "ReadWriteOnce" "ReadWriteMany" .Values.hostPath.enabled`. hostPath → RWO (matches the PV), dynamic/CSI → RWM (unchanged), explicit `pvcAccessMode` still wins.

Verification

  • `helm template`: RWO on hostPath, RWM on dynamic, override honored, PV unchanged.
  • `helm unittest`: 363 tests pass, including two new hostPath-default-RWO regression tests added here.

Note

Low Risk
Targeted Helm template defaulting for storage binding on bare-metal; dynamic-cluster behavior and explicit overrides are unchanged.

Overview
Fixes hostPath / bare-metal installs where logs, MySQL, and shared-images PVCs defaulted to ReadWriteMany while their paired PersistentVolumes stay ReadWriteOnce, so claims could not bind.

The chart now defaults pvcAccessMode with ternary "ReadWriteOnce" "ReadWriteMany" .Values.hostPath.enabled on those three PVC templates—RWO when hostPath.enabled is true (aligned with the PV), RWM for dynamic/CSI installs unchanged. An explicit pvcAccessMode in values still overrides.

Chart version is bumped to 1.9.29, with new helm-unittest cases asserting RWO defaults on hostPath for logs and MySQL PVCs.

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

…e PVC binds (#559)
The three storage PVCs (shared-images, logs, mysql) rendered
`accessModes: [{{ .Values.pvcAccessMode | default "ReadWriteMany" }}]`. In
hostPath (single-node / bare-metal) mode the paired PersistentVolume above them
is hardcoded ReadWriteOnce, so the PVC defaulting to ReadWriteMany could not
bind to its own PV. values.yaml already documented the intent
("ReadWriteMany for dynamic, ReadWriteOnce for hostPath") but the flat default
didn't honor it, and single-node installs don't set the override.
Make the default mode-derived:
{{ .Values.pvcAccessMode | default (ternary "ReadWriteOnce" "ReadWriteMany" .Values.hostPath.enabled) }}
- hostPath.enabled=true -> ReadWriteOnce (matches the PV)
- otherwise (dynamic/CSI) -> ReadWriteMany (unchanged)
- an explicit pvcAccessMode still wins.
Verified with `helm template` (RWO on hostPath, RWM on dynamic, override honored)
and `helm unittest` (363 tests pass, incl. two new hostPath-RWO regression tests).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The pvcAccessMode template edit ships as chart content, but v1.9.28 is already
the published tag — both the chart-version guard and the version-bump-gate fail
a client/* edit under an already-released version. Bump so the change actually
publishes under a new version instead of silently overwriting 1.9.28.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@cursorcursorBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Fix All in Cursor

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

Reviewed by Cursor Bugbot for commit 86b59e0. Configure here.

Comment threadclient/Chart.yaml
Bugbot: I bumped version but left appVersion at 1.9.28, so every resource's
app.kubernetes.io/version label (sourced from appVersion via tracebloc.labels)
would ship stale. This repo has always moved the two together (1.9.24..1.9.28),
so match it.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@LukasWodka

Copy link
Copy Markdown
ContributorAuthor

Fixed — bumped appVersion to 1.9.29 in lockstep with version (this chart has always moved the two together), so app.kubernetes.io/version won't ship stale.

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

@LukasWodkaApproved (non-code-owner; @saadqbal still required — see below).

  • Real single-node bug: the three storage PVCs defaulted to ReadWriteMany, but in hostPath mode their paired PV is hardcoded ReadWriteOnce, so the PVC couldn't bind. The mode-derived default pvcAccessMode | default (ternary "ReadWriteOnce" "ReadWriteMany" .Values.hostPath.enabled) is correct.
  • Verified via helm template in both modes: hostPath=true → PVC ReadWriteOnce on all three (now matches the PV → binds); dynamic → ReadWriteMany (unchanged); explicit override still wins. ternary + nil-safety confirmed. helm unittest 33/33 on the two PVC suites incl. the new regression tests.
  • Bugbot's "version bump omits appVersion" resolved (9d0b1bb, appVersion → 1.9.29 in lockstep); no unresolved findings.

Code-owner:/client/templates/ is owned by @saadqbal, so his approval is required to merge — my approval doesn't satisfy that gate. @saadqbal, this is the sole blocker.

⚪ Non-blocking: the new hostPath-RWO regression test was added to the logs + mysql-storage suites but not shared_images_pvc_test.yaml, though the template change applies there too (verified correct by render). Worth adding for symmetry.

🤖 Generated with Claude Code

@LukasWodka
LukasWodka merged commit 37d3e99 into developAug 11, 2026
22 checks passed
@LukasWodka
LukasWodka deleted the fix/559-pvc-accessmode-hostpath branch August 11, 2026 07:51
shujaatTracebloc added a commit that referenced this pull request Aug 11, 2026
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>
LukasWodka pushed a commit that referenced this pull request Aug 11, 2026
…eleted (#667)
* fix(chart): drop the sticky bit on /data/shared so a dataset can be deleted
`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>
* chore(chart): bump to 1.9.30 for the /data/shared sticky-bit change
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>
---------
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.

Chart default pvcAccessMode: ReadWriteMany is wrong for every shipped single-node storage mode

2 participants

@LukasWodka@aptracebloc