feat(ess) add helm charts - #723
Conversation
github-release auto already synthesizes a local floor tag when a service has no release tag, but the floor was hardcoded to 0.0.0, so a newly registered subproject always started its release line at 0.1.0. Add an optional initial_version field to the release metadata and use it as the synthesized floor, with SemVer validation. This lets an imported subproject continue an existing upstream version line without a manual anchor push. Default behavior is unchanged when the field is absent. Signed-off-by: Nilesh Vaghela <nvaghela@nvidia.com>
Import the ESS API Helm chart from the upstream ess-colocated-deploy repository into deploy/helm/ess, following the api-keys-colocated peer layout: the chart lives in the nested ess-api/ directory alongside a wrapper Makefile, README, and values.local.yaml. Governance and CI files owned centrally by the monorepo are dropped (.gitlab-ci.yml, .oss-allowlist, CODEOWNERS, SECURITY.md, license-header scripts). The Makefile license-header targets are removed with them. Register ess-helm as a release subproject with initial_version 1.7.0 to continue the upstream chart version line, and add a CI-only values file so the chart, which requires image.registry and image.repository, can be linted and templated in CI. Signed-off-by: Nilesh Vaghela <nvaghela@nvidia.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR imports the ESS API Helm chart into the monorepo. It adds Kubernetes templates, Vault integration, deployment tooling, documentation, CI validation values, and release automation metadata with initial-version handling. ChangesESS Helm chart release
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Helm
participant Kubernetes
participant VaultAgent
participant ESSAPI
Helm->>Kubernetes: Apply ESS API Deployment and Service
Kubernetes->>VaultAgent: Request injected secrets
VaultAgent->>ESSAPI: Mount rendered secrets
Kubernetes->>ESSAPI: Start configured container
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…l_version tag for ess as it is already initialized
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
tools/ci/helm-validate-values/ess.yaml (1)
1-5: 🎯 Functional Correctness | 🔵 TrivialValidate the chart with the CI values file.
The values provide the required
ess.image.registryandess.image.repositorykeys. Run both checks before merge:(cd deploy/helm/ess && helm lint ess-api -f ../../../tools/ci/helm-validate-values/ess.yaml) (cd deploy/helm/ess && helm template ess-api ess-api -f ../../../tools/ci/helm-validate-values/ess.yaml)Based on learnings, run both Helm validations with the ESS CI values file.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/ci/helm-validate-values/ess.yaml` around lines 1 - 5, Run both Helm validations for the ESS chart using tools/ci/helm-validate-values/ess.yaml: helm lint and helm template from the deploy/helm/ess directory, ensuring both checks pass before merge.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/ess/ess-api/templates/_helpers.tpl`:
- Line 5: Update the chart naming helpers around the `nameOverride` and
`fullnameOverride` expressions to read `.Values.ess.nameOverride` and
`.Values.ess.fullnameOverride` instead of root-level values, while preserving
the existing truncation and suffix handling.
In `@deploy/helm/ess/ess-api/vault-agent-templates/secrets.json.tmpl`:
- Around line 4-10: Update the Vault template entries for cassandraUsername,
cassandraPassword, audit-hmac-keys, and audit-hmac-kid to apply the Helm toJSON
function directly to each Vault value, removing the surrounding template quotes
so special characters and control characters produce valid JSON.
In `@deploy/helm/ess/Makefile`:
- Around line 84-85: Remove the unconditional packaged-charts deletion from the
clean target and the post-helm-push flow in deploy/helm/ess/Makefile at lines
84-85 and 102-103. Preserve artifacts by default, or gate deletion behind an
explicit user confirmation variable.
- Around line 62-80: Update the Makefile by defining validation_values with the
CI validation file default and using it in the lint and template targets, while
retaining values for deployment targets; ensure validate receives
validation_values through template. Add explicit confirmation before clean and
push-oci remove ./packaged-charts, and keep lint/template invocations aligned
with the documented Helm commands.
In `@deploy/helm/ess/README.md`:
- Around line 66-70: Update the README configuration section to match the
chart’s actual supported values: replace the incorrect ess.config.* reference
with ess.configuration.* and remove ess.vault.* unless corresponding values and
template consumers are added; do not document keys the chart does not read.
- Around line 76-123: Update the autoscaling documentation to remove all
Markdown bold markers, including emphasis around disabled by default, request,
and the memory caveat text. Replace the em dash in the JVM explanation with
plain ASCII punctuation or wording, while preserving the existing content and
formatting otherwise.
In `@tools/ci/github-release`:
- Around line 314-318: The initial_floor_version function must default only when
initial_version is absent or None, not for false-y values such as an empty
string; preserve explicit values for SemVer validation so invalid ones raise
SystemExit. In tools/ci/github-release lines 314-318, update the fallback
condition accordingly. In tools/ci/test-github-release.py lines 508-517, add
coverage asserting that an empty-string initial_version raises SystemExit.
---
Nitpick comments:
In `@tools/ci/helm-validate-values/ess.yaml`:
- Around line 1-5: Run both Helm validations for the ESS chart using
tools/ci/helm-validate-values/ess.yaml: helm lint and helm template from the
deploy/helm/ess directory, ensuring both checks pass before merge.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7acedae8-54f2-4664-9fcf-dea218ce4958
📒 Files selected for processing (21)
deploy/helm/ess/.gitignoredeploy/helm/ess/AGENTS.mddeploy/helm/ess/CLAUDE.mddeploy/helm/ess/Makefiledeploy/helm/ess/README.mddeploy/helm/ess/ess-api/.helmignoredeploy/helm/ess/ess-api/Chart.yamldeploy/helm/ess/ess-api/templates/_helpers.tpldeploy/helm/ess/ess-api/templates/configmap-env.yamldeploy/helm/ess/ess-api/templates/configmap-vault-agent-template.yamldeploy/helm/ess/ess-api/templates/deployment.yamldeploy/helm/ess/ess-api/templates/hpa.yamldeploy/helm/ess/ess-api/templates/service.yamldeploy/helm/ess/ess-api/templates/serviceaccount.yamldeploy/helm/ess/ess-api/values.yamldeploy/helm/ess/ess-api/vault-agent-templates/secrets.json.tmpldeploy/helm/ess/values.local.yamltools/ci/github-releasetools/ci/github-release-subprojects.jsontools/ci/helm-validate-values/ess.yamltools/ci/test-github-release.py
The vault-agent template interpolated cassandraUsername, cassandraPassword, audit-hmac-keys, and audit-hmac-kid inside literal JSON quotes, so a secret containing a quote, backslash, newline, or other control character produced invalid JSON. Pipe each value through toJSON, which emits a properly quoted and escaped JSON string, and drop the surrounding quotes. Rendered by Vault Agent at runtime; the chart embeds the template verbatim via Files.Get, so Helm does not evaluate these expressions. Signed-off-by: Nilesh Vaghela <nvaghela@nvidia.com>
The name and fullname helpers read root-level .Values.nameOverride and .Values.fullnameOverride, but values.yaml nests both under the ess key, so setting either override had no effect. Point the helpers at .Values.ess.nameOverride and .Values.ess.fullnameOverride, keeping the existing truncation and suffix handling. Signed-off-by: Nilesh Vaghela <nvaghela@nvidia.com>
The chart requires image.registry/repository, which the default values.yaml leaves empty, so `make template` and `make validate` failed to render. Add a validation_values variable defaulting to the CI validation values and use it in the lint and template targets, so local rendering matches the documented helm commands in AGENTS.md. Deployment targets keep using values.
Stop silently deleting ./packaged-charts after a successful push. Preserve the pushed artifact by default and print its path plus how to remove it (make clean), matching the llm-api-gateway peer chart. The clean target still removes ./packaged-charts on explicit invocation.
Replace the incorrect ess.config.* reference with ess.configuration.* (springProfile, additionalJavaOpts), which is what deployment.yaml reads, and drop the ess.vault.* bullet since the chart has no such values (Vault Agent settings are hardcoded in _helpers.tpl).
Align the autoscaling section with the repo documentation style: remove Markdown bold emphasis and replace the em dash in the JVM note with plain ASCII punctuation. Content and formatting are otherwise preserved.
initial_floor_version now falls back to the release floor only when initial_version is absent or None. An explicit empty string (or other non-None value) is preserved and SemVer-validated, so invalid values raise SystemExit instead of silently defaulting. Add a test covering the empty-string case.
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
deploy/helm/ess/README.md (1)
128-128: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the rendered HPA name in the verification command.
<release>-helm-nvcf-ess-apiis valid only with default naming values and a release name that does not already contain the chart name. Usekubectl get hpa -n <namespace>first, or document these naming constraints.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/ess/README.md` at line 128, Update the HPA verification instructions near the kubectl describe command to avoid assuming the default rendered name: instruct users to discover the rendered HPA name with kubectl get hpa -n <namespace> first, then use that actual name in kubectl describe hpa.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/ess/README.md`:
- Around line 109-110: Update the two scaling bullet phrases in the README to
begin with “Scale-up” and “Scale-down” instead of “Scale up” and “Scale down,”
preserving the remaining wording unchanged.
- Around line 107-110: Update the scaling-timings description in the HPA
behavior section to qualify ~15s and 5 minutes as typical Kubernetes defaults
rather than guaranteed timings. Mention that actual response may vary with
controller sync settings, metrics collection, and readiness delays.
---
Outside diff comments:
In `@deploy/helm/ess/README.md`:
- Line 128: Update the HPA verification instructions near the kubectl describe
command to avoid assuming the default rendered name: instruct users to discover
the rendered HPA name with kubectl get hpa -n <namespace> first, then use that
actual name in kubectl describe hpa.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c25948b1-7f5c-4d09-aa0d-07ff674a479d
📒 Files selected for processing (6)
deploy/helm/ess/Makefiledeploy/helm/ess/README.mddeploy/helm/ess/ess-api/templates/_helpers.tpldeploy/helm/ess/ess-api/vault-agent-templates/secrets.json.tmpltools/ci/github-releasetools/ci/test-github-release.py
🚧 Files skipped from review as they are similar to previous changes (4)
- tools/ci/github-release
- deploy/helm/ess/ess-api/vault-agent-templates/secrets.json.tmpl
- deploy/helm/ess/ess-api/templates/_helpers.tpl
- tools/ci/test-github-release.py
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
deploy/helm/ess/Makefile (1)
90-91: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire confirmation before deleting packaged charts.
Line [91] still runs
rm -rfwithout confirmation. Becausepackagedepends onclean, a normal package operation deletes existing chart artifacts. Gate deletion with an explicit confirmation variable, or remove the destructive prerequisite and use a fresh output directory.As per coding guidelines, never delete artifact directories without explicit user confirmation.
Proposed fix
clean: - rm -rf ./packaged-charts + `@test` "$(CONFIRM_CLEAN)" = "1" || { \ + echo "Set CONFIRM_CLEAN=1 to delete ./packaged-charts."; \ + exit 1; \ + } + rm -rf ./packaged-charts🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/ess/Makefile` around lines 90 - 91, Update the clean target in the Makefile so removing packaged-charts requires explicit user confirmation, and ensure the package dependency path cannot delete existing artifacts without that confirmation. Preserve packaging behavior by either gating rm -rf with a confirmation variable or removing the destructive clean prerequisite and using a fresh output directory.Source: Coding guidelines
deploy/helm/ess/README.md (1)
107-110: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDescribe HPA timings as typical defaults.
The text presents
~15sand5-minutetiming as operational behavior. The HPA sync period can change, and metrics collection or pod readiness can delay scaling. State that these are typical defaults and that actual timing varies with cluster settings.Verify the wording against the Kubernetes version and controller configuration used by deployments.
Proposed wording
-The chart does not set a custom `behavior` block, so Kubernetes defaults apply: +The chart does not set a custom `behavior` block, so typical Kubernetes defaults apply. Actual timing depends on controller sync settings, metrics collection, and pod readiness: -- Scale up is fast: the controller reacts within one sync interval (~15s) with no stabilization delay. -- Scale down is conservative: it requires sustained low utilization across the default 5-minute stabilization window before reducing replicas. +- Scale-up is fast by default: the controller can react after roughly one sync interval when metrics are available. +- Scale-down is conservative by default: the controller uses a 5-minute stabilization window before reducing replicas.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@deploy/helm/ess/README.md` around lines 107 - 110, Update the HPA timing description in the chart README to identify the ~15-second sync interval and 5-minute scale-down stabilization as typical defaults, not guaranteed operational timings. Add that actual scaling may vary with Kubernetes version, controller or cluster configuration, metrics collection, and pod readiness, and verify the wording against the deployment configuration.
🧹 Nitpick comments (1)
tools/ci/test-github-release.py (1)
492-506: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExercise the registered ESS metadata in this test.
This test hard-codes an ad hoc service and
0.58.5. It does not loadtools/ci/github-release-subprojects.json, so an incorrect ESS path, service name, orinitial_versioncould pass the test. Load theess-helmentry and assert the registereddeploy/helm/ess/v1.7.0anchor.Proposed test update
- service = { - "id": "ess-helm", - "path": "deploy/helm/ess", - "service_name": "helm-nvcf-ess-api", - "initial_version": "0.58.5", - } + metadata = json.loads( + SCRIPT_PATH.with_name("github-release-subprojects.json").read_text() + ) + service = next(item for item in metadata["services"] if item["id"] == "ess-helm") ... - self.assertIn("deploy/helm/ess/v0.58.5", tags) + self.assertIn("deploy/helm/ess/v1.7.0", tags)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/ci/test-github-release.py` around lines 492 - 506, Update test_initial_version_anchor_honors_metadata to load the registered ess-helm entry from tools/ci/github-release-subprojects.json instead of constructing an ad hoc service, then pass that metadata to synthesize_initial_version_anchor and assert deploy/helm/ess/v1.7.0 is created while the v0.0.0 anchor is absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@deploy/helm/ess/ess-api/templates/deployment.yaml`:
- Around line 93-96: Update the JAVA_TOOL_OPTIONS value in the deployment
template to read additionalJavaOpts from
.Values.ess.configuration.additionalJavaOpts, preserving the existing
default-empty behavior and app.version option.
In `@deploy/helm/ess/Makefile`:
- Line 20: Remove the non-empty default assignment for values so the existing
ifndef values validation rejects make install when no override is provided.
Preserve the current Helm invocation behavior when callers explicitly supply
values.
- Around line 42-43: Update the .PHONY declaration to list the Make targets
actually defined in this file: install, uninstall, clean, validate, package, and
push-oci, replacing the unrelated deploy, delete, status, lint, and template
entries.
- Line 18: Update the standalone ESS Makefile’s namespace default from ess to
the documented ess-api value. Leave the self-managed Helmfile namespace
explicitly set to ess.
---
Duplicate comments:
In `@deploy/helm/ess/Makefile`:
- Around line 90-91: Update the clean target in the Makefile so removing
packaged-charts requires explicit user confirmation, and ensure the package
dependency path cannot delete existing artifacts without that confirmation.
Preserve packaging behavior by either gating rm -rf with a confirmation variable
or removing the destructive clean prerequisite and using a fresh output
directory.
In `@deploy/helm/ess/README.md`:
- Around line 107-110: Update the HPA timing description in the chart README to
identify the ~15-second sync interval and 5-minute scale-down stabilization as
typical defaults, not guaranteed operational timings. Add that actual scaling
may vary with Kubernetes version, controller or cluster configuration, metrics
collection, and pod readiness, and verify the wording against the deployment
configuration.
---
Nitpick comments:
In `@tools/ci/test-github-release.py`:
- Around line 492-506: Update test_initial_version_anchor_honors_metadata to
load the registered ess-helm entry from tools/ci/github-release-subprojects.json
instead of constructing an ad hoc service, then pass that metadata to
synthesize_initial_version_anchor and assert deploy/helm/ess/v1.7.0 is created
while the v0.0.0 anchor is absent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6a00cf1b-2664-4d3e-9d88-804f517144f0
📒 Files selected for processing (21)
deploy/helm/ess/.gitignoredeploy/helm/ess/AGENTS.mddeploy/helm/ess/CLAUDE.mddeploy/helm/ess/Makefiledeploy/helm/ess/README.mddeploy/helm/ess/ess-api/.helmignoredeploy/helm/ess/ess-api/Chart.yamldeploy/helm/ess/ess-api/templates/_helpers.tpldeploy/helm/ess/ess-api/templates/configmap-env.yamldeploy/helm/ess/ess-api/templates/configmap-vault-agent-template.yamldeploy/helm/ess/ess-api/templates/deployment.yamldeploy/helm/ess/ess-api/templates/hpa.yamldeploy/helm/ess/ess-api/templates/service.yamldeploy/helm/ess/ess-api/templates/serviceaccount.yamldeploy/helm/ess/ess-api/values.yamldeploy/helm/ess/ess-api/vault-agent-templates/secrets.json.tmpldeploy/helm/ess/values.local.yamltools/ci/github-releasetools/ci/github-release-subprojects.jsontools/ci/helm-validate-values/ess.yamltools/ci/test-github-release.py
Hyphenate Scale-up/Scale-down, qualify the ~15s and 5-minute figures as typical Kubernetes defaults that vary with controller sync, metrics collection, and readiness delays, and tell users to discover the rendered HPA name with kubectl get hpa before describing it.
Document the initial_version metadata field and add a section on seeding the first release tag for a new service or chart: auto floor synthesis, explicit anchoring, the already-released plain-tag fallback, and worked ess and ess-helm examples.
|
🌿 Preview your docs: https://nvidia-preview-feat-ess-helm-charts.docs.buildwithfern.com/nvcf |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/dev/github-release-process.md`:
- Around line 413-417: Update the tag-workflow trigger statement in the
release-process documentation to qualify that tag pushes start the workflow only
when performed with NV_GITHUB_TOKEN or another workflow-capable token; preserve
the existing dry-run gate behavior and clarify that default GITHUB_TOKEN pushes
do not trigger the follow-up workflow.
- Around line 323-325: Update the initial_version documentation to state that
omitting it starts from a 0.0.0 floor, with the next version determined by
commit type: feat yields 0.1.0, fix yields 0.0.1, and release-neutral commits
produce no release; present 0.1.0 only as an example while preserving the
empty-string validation guidance.
- Around line 410-411: Update the tag-workflow documentation around the git
ls-remote command to retain the existing command and clarify that Git matches
slash-boundary suffixes; explicitly state that tags pushed with NV_GITHUB_TOKEN
trigger the follow-up workflow, while tags pushed with GITHUB_TOKEN do not.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8367489c-bc46-4533-b8f6-9b3b3f74e7fe
📒 Files selected for processing (2)
deploy/helm/ess/README.mddocs/dev/github-release-process.md
🚧 Files skipped from review as they are similar to previous changes (1)
- deploy/helm/ess/README.md
- deployment: read additionalJavaOpts from ess.configuration so the configured value is actually applied - Makefile: correct .PHONY to the targets defined in this file - README: document namespace ess consistently and note HPA timings are typical defaults that vary with cluster/controller configuration - test: load the registered ess-helm entry and derive expected tags instead of hardcoding versions
Clarify that omitting initial_version starts from a 0.0.0 floor with the next version set by commit type (feat 0.1.0, fix 0.0.1, release-neutral none), note that git ls-remote matches the path pattern on slash boundaries, and qualify that only tags pushed with a workflow-capable token (NV_GITHUB_TOKEN) trigger the follow-up workflow.
Reframe the section as seeding and pinning service or chart versions and split it into three cases: new line via initial_version (automatic), new line seeded manually with anchor, and pinning a new floor on a line that already has tags. Add the ess-helm registration example and drop the local auto preview, which creates a local tag as a side effect.
Reword the intro so it plainly states automation bumps the highest existing tag and a new line has nothing to bump from until seeded.
|
🎉 This PR is included in version helm-nvcf-ess-api-v1.7.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Additional Details (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
For the Reviewer
For QA (optional for docs, build, test, refactor, ci, chore, style, and revert PRs)
Issues
Closes: #722
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores