KEEP-818-keeperhub-stack-umbrella-chart#69
Merged
Conversation
Wrap the common chart once per KeeperHub execution-pipeline component (app, executor, schedule + block dispatchers, metrics collector) as aliased dependencies so the whole stack deploys as a single Helm release. A single release lets 'helm upgrade --install --atomic --wait' roll all components together and roll all of them back if any one fails its readiness check, instead of the current per-release rollovers that span several minutes. That staggered window is what orphaned in-flight executions in incident Q1005I00XZ1ZPH (KEEP-818): triggers created mid-rollout were never picked up and got reaped 30 min later as system errors, paging P2. The common subchart is vendored (Chart.lock + charts/common-0.3.0.tgz) so the chart renders under the repo's existing CI, which has no helm-dependency-build step. Validated locally: helm lint and helm template pass with defaults and test-values; all five Deployments render with RFC1123-valid names; component enable/disable toggles work.
❌ Helm Chart Tests FailedSome charts failed validation:
Summary:
🔍 Debugging Information:
Please fix the issues before merging. 🔧 |
common's NOTES.txt evaluates 'contains "NodePort" .Values.service.type' unconditionally (Go template 'and' does not short-circuit), so every component needs service.type as a string even when service.enabled is false. The dispatchers and metrics collector were missing it, which failed the CI dry-run (helm renders subchart NOTES.txt during install; plain 'helm template' skips it, so it slipped past local checks). Verified with a full --dry-run install.
🎉 Helm Chart Tests Passed!All modified charts have been successfully validated:
Summary:
📋 Detailed Information:
The PR is ready for review! 🚀 Note: Charts will be automatically released when this PR is merged to main. |
sanbotto
reviewed
Jun 12, 2026
Comment on lines
+2
to
+16
| - name: common | ||
| repository: file://../common | ||
| version: 0.3.0 | ||
| - name: common | ||
| repository: file://../common | ||
| version: 0.3.0 | ||
| - name: common | ||
| repository: file://../common | ||
| version: 0.3.0 | ||
| - name: common | ||
| repository: file://../common | ||
| version: 0.3.0 | ||
| - name: common | ||
| repository: file://../common | ||
| version: 0.3.0 |
Contributor
There was a problem hiding this comment.
This feels like it was supposed to have different names instead of all being called "common".
Contributor
Author
There was a problem hiding this comment.
@sanbotto yes, this is a weird limitation of Helm -- a dependency's name must match the actual chart name at its repository 😕
sanbotto
approved these changes
Jun 12, 2026
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
2 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.
What
Adds a new umbrella chart
charts/keeperhub-stackthat deploys the KeeperHub execution pipeline as a single Helm release so the whole stack can be upgraded and rolled back atomically.It depends on the existing
commonchart once per component, aliased:app,executor,schedule,block,metricsCollector.commonis not modified.Why
Today each component is its own Helm release of
common, deployed by separate CI workflows on independent triggers, so a deploy rolls the components over a multi-minute window. While the dispatchers are already on the new version but the executor/app are still cycling, triggers keep being created that no runner picks up. Those executions sit until the 30-minute reaper marks themExecution timed out: no progress for 30 minutes, which classifies as a system error and paged P2 in incident Q1005I00XZ1ZPH (KEEP-818).A single release lets
helm upgrade --install --atomic --waitflip the whole stack in one coordinated operation and roll everything back if any component fails its readiness check, instead of leaving a half-upgraded stack.How to use
Real image tags / env / SSM secrets come from the keeperhub repo's per-env values via
-f, same as today. The chart's ownvalues.yamlis placeholders.Notes for reviewers
helm-release.yml) runshelm template/--dry-runwith nohelm dependency buildstep, and every existing chart is standalone. To make the first dependent chart pass CI without touching the shared workflow, thecommonsubchart is vendored:Chart.lock+charts/common-0.3.0.tgz(built fromfile://../common). If you'd rather not commit the archive, the alternative is adding ahelm dependency buildstep to the workflow - happy to switch to that.commontest-template quirk.common'stests/test-connection.yamldereferences.Values.service.port, andcertificate.yamldoesand .Values.service.enabled .Values.service.tls.enabled(Goandevaluates both operands). So every component carries a fullserviceblock (withtls.enabled) even when it runs no HTTP server. Worth fixing incommonseparately (guard those templates); noted but out of scope here.metricsCollectorname. Alias is camelCase for clean values access;nameOverride: metrics-collectorkeeps the rendered resources RFC1123-valid.Validation (local)
helm lintpasses with defaults and withtest-values.yaml.helm templaterenders 5 Deployments (keeperhub-app/executor/schedule/block/metrics-collector), Services only for app + executor, all names lowercase/RFC1123.--set metricsCollector.enabled=falsedrops its resources).Out of scope (follow-ups)