Skip to content

feat(helm): make gateway PVC size and StorageClass configurable - #3854

Merged
purp merged 1 commit into
NVIDIA:mainfrom
lunarwhite:feat/3216-gateway-pvc-storage/lunarwhite
Oct 16, 2026
Merged

feat(helm): make gateway PVC size and StorageClass configurable#3854
purp merged 1 commit into
NVIDIA:mainfrom
lunarwhite:feat/3216-gateway-pvc-storage/lunarwhite

Conversation

@lunarwhite

@lunarwhite lunarwhite commented Sep 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

The chart hard-codes the gateway StatefulSet's claim to 1Gi with no storageClassName, so operators need a post-render patch to size the gateway database or choose its storage tier. This adds server.dataStorageSize and server.dataStorageClass; both default to empty, which keeps the current claim.

Related Issue

Closes #3216

Changes

  • templates/statefulset.yaml: render the size (default 1Gi) and, when set, storageClassName into the claim.
  • templates/_helpers.tpl: openshell.validateValues rejects a size without a unit, such as 20, which Kubernetes reads as 20 bytes.
  • values.yaml: both keys beside dbUrl; the class comment requires a block-backed class, not NFS, since SQLite runs in WAL mode (perf(server): enable WAL for the SQLite store; relax sync only for SSH session issuance #3543).
  • tests/gateway_persistence_test.yaml: default and overridden rendering, and the unit-less rejection.
  • Docs: a Data PVC storage section in the chart README, two rows and an immutability warning in docs/kubernetes/setup.mdx, and two rows in skills/debug-openshell-cluster/SKILL.md. gateway-config.mdx is unchanged because nothing renders into gateway.toml.

Design notes

  • Empty omits storageClassName, so the cluster default applies, matching the driver's workspace_storage_class. Static PV binding ("") stays unsupported, as before.
  • Changing storage later. volumeClaimTemplates is immutable, so growing the claim is in-place PVC expansion and changing its class means copying data. The chart README documents both.
  • Naming. feat(helm): migrate gateway configuration to gatewayConfig #3384 keeps Kubernetes resource inputs out of gatewayConfig, so the keys sit under server beside dbUrl and externalDbSecret, named after the openshell-data claim they configure.

Testing

  • mise run pre-commit passes
  • Unit tests added/updated
  • E2E tests added/updated (if applicable) — N/A

Three cases cover default rendering, overridden rendering, and the unit-less rejection. With unchanged values, the rendered StatefulSet is identical to main's in the default render and every CI overlay, parsed as objects with annotations included, so upgrades leave the immutable claim template and the pods untouched.

Checklist

  • Follows Conventional Commits
  • Commits are signed off (DCO)
  • Architecture docs updated (if applicable) — N/A

@copy-pr-bot

copy-pr-bot Bot commented Sep 29, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@EmilienM EmilienM mentioned this pull request Oct 5, 2026
Open
8 tasks
@lunarwhite
lunarwhite force-pushed the feat/3216-gateway-pvc-storage/lunarwhite branch from 6f0e9db to 59d0590 Compare October 15, 2026 13:02
politerealism
politerealism approved these changes Oct 15, 2026
View reviewed changes

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

Nice, this is a clean change and the README/.gotmpl stay in sync. Two small nits, neither blocking:

  1. server.databaseStorageSize is rendered as-is, so --set server.databaseStorageSize=20 (no unit) parses as a number and renders storage: "20", which Kubernetes reads as 20 bytes. The docs already warn about this. Would a regexMatch + fail in the template, or a values.schema.json pattern, be worth it so it fails at render time?
  2. The unit tests cover the default and the explicit-set cases. It might be worth adding a case for workload.kind=deployment, since the README says both keys are ignored there, and one for a unitless size if you add validation. templates/gateway-config.yaml is listed in the suite's templates: but nothing uses it.

@purp

purp commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

/ok to test 59d0590

@purp purp added the test:e2e Requires end-to-end coverage label Oct 15, 2026
@github-actions

github-actions Bot commented Oct 15, 2026

Copy link
Copy Markdown

Label test:e2e applied for 59d0590. Open the existing run and click Re-run all jobs to execute with the label set. The run will execute the standard E2E suite after building the required gateway, sandbox, and supervisor images once. The matching required CI gate status on this PR will flip green automatically once the run finishes.

@purp

purp commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

The independent code review found no blocking findings. This focused Helm change implements accepted issue #3216, preserves the default gateway claim, and documents the new settings and StatefulSet/PVC migration constraints.

Action required: A maintainer must use Re-run all jobs on Branch E2E Checks now that test:e2e is set, and approve Trivy Changes so the required scan can run.

Blocking findings: None.

Carried findings: None.

Non-blocking suggestions: None.

Gator metadata
  • Validation: Implements accepted gateway PVC configuration issue feat(helm): make gateway PVC size and StorageClass configurable #3216.
  • Docs: Helm README and template, Fern Kubernetes setup page, and debug-openshell-cluster skill updated; no navigation change needed.
  • Checks: DCO and issue acceptance passed; Branch Checks and Helm Lint running; Trivy Changes awaits workflow approval.
  • E2E: Added test:e2e; previous current-head E2E run skipped runtime suites. Maintainer rerun required before pipeline handoff. Windows and GPU test labels not needed for this patch.
  • Head SHA: 59d0590911eb38f7f00b6359d8e71d1e0ba62e47
  • Base SHA: ef718a4f62afefc82cb8741d303410b8475167ad
  • Merge base SHA: ef718a4f62afefc82cb8741d303410b8475167ad
  • Patch ID: 64da07e64d062c20c5a29899e2565ee0da154901
  • Gator payload: 12
  • Review mode: initial
  • Previous reviewed SHA: none
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:blocked
  • Blocked reason: test_dispatch_required

@purp purp added the gator:blocked Gator is blocked by process or repository gates label Oct 15, 2026
purp
purp previously approved these changes Oct 15, 2026
View reviewed changes

@purp purp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

One nit that may be worth addressing: the PVC template name param openshell-data is more broadly named than the params databaseStorageSize and databaseStorageClass; might be good to make them dataStorage* instead.

@purp purp added gator:merge-ready and removed gator:blocked Gator is blocked by process or repository gates labels Oct 15, 2026
@lunarwhite
feat(helm): make gateway PVC size and StorageClass configurable
1bbcdb7 
Signed-off-by: Yuedong Wu <dwcn22@outlook.com>
@lunarwhite
lunarwhite force-pushed the feat/3216-gateway-pvc-storage/lunarwhite branch from 59d0590 to 1bbcdb7 Compare October 15, 2026 22:23
@purp

purp commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

/ok to test 1bbcdb7

@purp

purp commented Oct 15, 2026

Copy link
Copy Markdown
Collaborator

gator-agent

PR Review Status

The independent follow-up review found no blocking findings. @purp, I checked your suggestion to match the openshell-data claim name: the templates, values, tests, chart README, Fern setup page, and troubleshooting skill consistently use dataStorageSize and dataStorageClass. The added size validation and its regression case introduce no blocking issue.

I refreshed the test mirror with /ok to test for this head; Branch Checks, Helm Lint, and Branch E2E Checks are now queued with test:e2e already applied.

Action required: A maintainer must approve Trivy Changes so the required security scan can run. Maintainer approval of the PR is also required again after this update.

Blocking findings: None.

Carried findings: None.

Gator metadata
  • Validation: Focused implementation of accepted issue feat(helm): make gateway PVC size and StorageClass configurable #3216; no scope growth.
  • Docs: Helm README/template, Fern Kubernetes setup page, and debug-openshell-cluster skill updated consistently; no navigation change needed.
  • Checks: DCO and issue acceptance passed. Current-head Branch Checks and Helm Lint queued. Trivy Changes awaits workflow approval.
  • E2E: test:e2e retained; current-head Branch E2E Checks queued. No GPU or Windows label required by this patch.
  • Head SHA: 1bbcdb7ab235cb8fd130d54df8d82aed342a8bf1
  • Base SHA: 6546691d5e493343c7877573252e835d28bf9e23
  • Merge base SHA: ef718a4f62afefc82cb8741d303410b8475167ad
  • Patch ID: d5e4090f7e2f6fa8ac7bba497fc38de376b7072d
  • Gator payload: 12
  • Review mode: follow_up
  • Previous reviewed SHA: 59d0590911eb38f7f00b6359d8e71d1e0ba62e47
  • Review budget exhausted: no
  • Maintainer decision required: no
  • Next state: gator:blocked
  • Blocked reason: workflow_approval_required

@purp purp added gator:blocked Gator is blocked by process or repository gates gator:watch-pipeline Gator is monitoring PR CI/CD status and removed gator:merge-ready gator:blocked Gator is blocked by process or repository gates labels Oct 15, 2026
purp
purp approved these changes Oct 15, 2026
View reviewed changes

@purp purp left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM 🚢

@purp
purp added this pull request to the merge queue Oct 15, 2026
@purp purp added gator:merge-ready and removed gator:watch-pipeline Gator is monitoring PR CI/CD status labels Oct 16, 2026
Merged via the queue into NVIDIA:main with commit 44c07da Oct 16, 2026
127 of 131 checks passed
@lunarwhite
lunarwhite deleted the feat/3216-gateway-pvc-storage/lunarwhite branch October 16, 2026 00:09
@purp

purp commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

gator-agent

Monitoring Complete

Monitoring is complete because this PR has merged. The final Gator state was merge-ready, with no blocking review findings and maintainer approval present.

I removed the active gator:* label because there is nothing left for Gator to monitor on this PR.

Gator metadata
  • Head SHA: 1bbcdb7ab235cb8fd130d54df8d82aed342a8bf1
  • Gator payload: 12

@BrewTestBot BrewTestBot mentioned this pull request Oct 20, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@purp purp purp approved these changes
@mrunalp mrunalp Awaiting requested review from mrunalp
@sjenning sjenning Awaiting requested review from sjenning
@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr
+1 more reviewer
@politerealism politerealism politerealism approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

test:e2e Requires end-to-end coverage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat(helm): make gateway PVC size and StorageClass configurable

3 participants

@lunarwhite @purp @politerealism