feat(helm): cleanups for chart - #11268
Closed
markhoerth wants to merge 1 commit into
Closed
Conversation
Bundles seven small-scope items from the chart enterprise readiness audit. None depend on each other. Scope deliberately excludes item 1 (Secret references), item 4 (existingConfigMap), and item 10 (OAuth schema), which are tracked separately. Changes: - (item 2) Remove gravitino.datastrato.* property references from ci-values.yaml and the commented examples in values.yaml. Replaced with vendor-neutral example property names so the visibleConfigs mechanism remains documented and exercised in CI. - (item 3) Add structured values.yaml paths for entity.maxConnections, lock.maxNodes, and lock.minNodes. Properties used by the production sample in docs/gravitino-server-config.md#production that previously required additionalConfigItems workarounds. Emitted conditionally from the gravitino.conf template so server defaults apply when unset. - (item 5) Ship dev-values.yaml and prod-values.yaml scenario files derived from the development and production samples in gravitino-server-config.md. The prod scenario uses structured paths for the properties added by item 3 (no additionalConfigItems). - (item 6) Replace NOTES.txt with a post-install deployment summary that surfaces the actual backend, persistence, IRC, and auth configuration, plus warnings for ephemeral H2 metadata, in-memory IRC backend, and simple-mode authentication. - (item 7) Remove testAdditionalConfigItems test-fixture properties from ci-values.yaml. Replaced with realistic eventListener example. - (item 8) Add default-credential warnings above the mysql and postgresql sections in values.yaml. NOTES.txt fires runtime warnings when the in-chart databases are enabled with defaults. - (item 9) Emit dynamic-config-provider sub-properties conditionally, so unset values do not render as empty-string property assignments in the rendered gravitino.conf. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
markhoerth
pushed a commit
to markhoerth/gravitino
that referenced
this pull request
May 27, 2026
Rework the Kubernetes installation page to take advantage of the Helm chart enhancements landing in apache/gravitino apache#11268: the new dev-values.yaml and prod-values.yaml scenario files plus the expanded structured values schema. Chart Values section rewrite. The previous one-paragraph stub plus a 'helm show values' command becomes a substantive foundation with four subsections: Inspecting the Defaults, Customizing With a Values File (using a small inline example), One-Off Overrides With --set (with precedence note), and Example Scenario Files (forward- referencing the scenarios shipped with the chart). Two new Deployment subsections: 'Deploy With the Dev Scenario' and 'Deploy With the Production Scenario'. Each describes the corresponding shipped scenario file, shows the helm pull --untar extract-and-install workflow, and links to gravitino-server-config.md for the equivalent gravitino.conf reference used by the binary install path. Two deletions for redundancy: - 'Deploy with Custom Configuration' is subsumed by the new Chart Values section's --set and -f content. - 'Deploy Gravitino Using an Existing MySQL Database' is subsumed by the production scenario, which covers external MySQL plus OIDC OAuth, cache tuning, audit, and tree lock configuration; the MySQL schema initialization prerequisite is referenced from the production scenario subsection and from prod-values.yaml's header comment. Also removes the orphan generic 'helm upgrade --install [RELEASE_NAME] ... [flags]' template at the top of the Deployment section (no concrete value, every following subsection shows a real invocation) and the :::note admonition that lived inside the deleted MySQL subsection. The dev and production scenario files referenced from this page are added by apache/gravitino apache#11268. That PR may merge after this docs commit; once it does, the descriptions on this page match the shipped files. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Code Coverage Report
|
danhuawang
reviewed
May 28, 2026
| icebergRest: | ||
| catalogConfigProvider: dynamic-config-provider | ||
| dynamicConfigProvider: | ||
| metalake: test |
Contributor
There was a problem hiding this comment.
Why not put this in the value.yaml if it's a recommended configuration?
markhoerth
changed the title
Contributor
|
@danhuawang can you please help to continue to this PR? We want to get it merged in 1.3.0. |
Contributor
ok. |
Contributor
|
fixed by #11514 |
mchades
pushed a commit
that referenced
this pull request
Jun 10, 2026
… for chart (#11514) ### What changes were proposed in this pull request? This PR includes the enterprise readiness cleanups for the Helm chart (authored by @markhoerth) plus follow-up fixes for template correctness and CI stability issues discovered during review. **Commit 1 (by @markhoerth):** `feat(helm): enterprise readiness cleanups for chart` - Remove vendor-specific property references from ci-values.yaml and values.yaml - Add structured values.yaml paths for entity.maxConnections, lock.maxNodes, and lock.minNodes - Ship dev-values.yaml and prod-values.yaml scenario files - Replace NOTES.txt with a post-install deployment summary with runtime warnings - Add default-credential warnings for mysql and postgresql sections - Emit dynamic-config-provider sub-properties conditionally **Commit 2 (follow-up fixes):** 1. **Conditional OAuth rendering** (`gravitino.conf`): Wrap all `gravitino.authenticator.oauth.*` properties in `{{- if contains "oauth" .Values.authenticators }}` with per-property guards. Previously these properties were rendered unconditionally with empty values even in simple-auth mode, overriding server defaults. 2. **Fix H2 backend detection** (`NOTES.txt`): Replace exact-match `eq "jdbc:h2"` with `hasPrefix "jdbc:h2"` so H2 file-mode URLs are correctly identified. 3. **Fix CI crash** (`ci-values.yaml`): Replace `gravitino.eventListener.names: "audit,sync"` with `gravitino.server.shutdown.timeout: "5000"`. The eventListener config declared listeners without the required `.class` properties, causing server startup failure. 4. **Document additionalConfigItems usage** (`values.yaml`): Add complete commented example for event listener configuration. 5. **Migrate to non-deprecated entrypoint** (`values.yaml`, `pg-values.yaml`): Replace `bin/start-gravitino.sh` with `docker/docker-entrypoint.sh` to align with the Dockerfile ENTRYPOINT. ### Why are the changes needed? - Empty OAuth properties override server defaults silently. - The H2 detection bug suppresses the data-loss warning for H2 file-mode URLs. - The `eventListener.names` config without `class` properties crashes the server during CI chart tests. - The deprecated `start-gravitino.sh` generates unnecessary warning noise and will break when removed. Fix: #11268 ### Does this PR introduce _any_ user-facing change? - When `authenticators` is `simple` (default), `gravitino.conf` no longer contains empty `gravitino.authenticator.oauth.*` lines. - NOTES.txt correctly classifies H2 file-mode JDBC URLs as "Embedded H2". - The deprecation warning from `bin/start-gravitino.sh` no longer appears in pod logs. ### How was this patch tested? - Ran `helm template` with default values (simple auth) — no `authenticator.oauth` lines emitted. - Ran `helm template` with `prod-values.yaml` (oauth) — all configured OAuth properties render correctly. - Ran `helm template` with `ci-values.yaml` — `gravitino.server.shutdown.timeout = 5000` renders in additionalConfigItems section. - All template renderings compile without errors. --------- Co-authored-by: Mark Hoerth <mark@datastrato.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Qi Yu <yuqi@datastrato.com>
jerryshao
pushed a commit
that referenced
this pull request
Jun 11, 2026
…cleanups and follow-up fixes for chart (#11514) (#11564) **Cherry-pick Information:** - Original commit: 94d6045 - Target branch: `branch-1.3` - Status: ✅ Clean cherry-pick (no conflicts) Co-authored-by: Danhua Wang <danhua@datastrato.com> Co-authored-by: Mark Hoerth <mark@datastrato.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Qi Yu <yuqi@datastrato.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
All items are small-scope, none depend on each other, and the bundle deliberately excludes the items tracked separately.
Items in scope
gravitino.datastrato.*property references fromci-values.yamland the commented examples invalues.yaml. Vendor-neutral example names used so thevisibleConfigsmechanism remains documented and exercised in CI.values.yamlpaths forentity.maxConnections,lock.maxNodes, andlock.minNodes. Properties used by the production sample indocs/gravitino-server-config.md#productionthat previously requiredadditionalConfigItemsworkarounds. Emitted conditionally from thegravitino.conftemplate so server defaults apply when unset.dev-values.yamlandprod-values.yamlscenario files derived from the development and production samples ingravitino-server-config.md. The prod scenario uses structured paths for the properties added by item 3 (noadditionalConfigItems).NOTES.txtwith a post-install deployment summary that surfaces the actual backend, persistence, IRC, and auth configuration. Adds warnings for ephemeral H2 metadata, in-memory IRC backend, and simple-mode authentication.testAdditionalConfigItemstest-fixture properties fromci-values.yaml. Replaced with a realisticgravitino.eventListener.namesexample.mysqlandpostgresqlsections invalues.yaml.NOTES.txtfires runtime warnings when in-chart databases are enabled with default passwords.gravitino-uri,gravitino-metalake,default-catalog-name) conditionally, so unset values do not render as empty-string property assignments.Items deliberately out of scope
templates/deployment.yamland flipsSKIP_CONFIG_REWRITE. Tracked as a separate PR.existingConfigMapsupport): Deferred. Niche GitOps unlock; no current customer pressure.Validation
helm lintclean against defaults and both new scenario files (dev-values.yaml,prod-values.yaml).helm templaterenders expectedgravitino.confcontent for default, dev, and prod scenarios.gravitino.entity.store.relational.maxConnections = 200,gravitino.lock.maxNodes = 500000,gravitino.lock.minNodes = 5000), omit cleanly when unset.gravitino.iceberg-rest.gravitino-metalake = testline with no emptygravitino-uri =ordefault-catalog-name =lines.NOTES.txtwarnings verified viahelm install --dry-runfor H2 ephemeral, IRC in-memory, simple auth, and default MySQL/PostgreSQL credentials.grep -r datastrato dev/charts/gravitino/andgrep -r testAdditionalConfigItems dev/charts/gravitino/both return no results.grep additionalConfigItems dev/charts/gravitino/resources/scenarios/prod-values.yamlreturns no results.Runtime validation
Runtime validation is blocked by #11267 (chart-vs-image path mismatch in all currently published Gravitino images). The chart hardcodes
/opt/gravitinobut published images (1.2.0and1.3.0-SNAPSHOTboth verified) install Gravitino at/root/gravitino. A defaulthelm installagainstmainfails withCrashLoopBackOffidentically to this branch, so the blocker is not introduced by this PR.The path bug has a second layer where
/root/has 700 permissions incompatible with the chart's non-root container security context (runAsNonRoot: true, runAsUser: 1000). Any chart-side workaround requires disablingrunAsNonRoot, which is a security regression unacceptable for enterprise deployments. This is why Option A in #11267 (move the image install location to/opt/gravitino) is the only viable resolution.Once #11267 is resolved, this PR's edits will runtime-validate without modification since none of them touch the deployment template or path assumptions.
Documentation followup
Once this PR and the separate item 1 PR (Secret references) land,
docs/chart.mdcan describe a complete install-to-production flow including the dev and prod scenario files. The scenario files inresources/scenarios/are the canonical source the docs should link via raw GitHub URLs.