feat(helm): add persistence.existingClaim support#166
Open
white1033 wants to merge 2 commits intoopenabdev:mainfrom
Open
feat(helm): add persistence.existingClaim support#166white1033 wants to merge 2 commits intoopenabdev:mainfrom
white1033 wants to merge 2 commits intoopenabdev:mainfrom
Conversation
Contributor
|
Hey @white1033 — nice PR, this is exactly what's needed for the migration path. One suggestion: could we also add metadata:
name: {{ include "openab.agentFullname" $d }}
labels:
{{- include "openab.labels" $d | nindent 4 }}
annotations:
"helm.sh/resource-policy": keepThe two fixes are complementary:
This is the same pattern already used on the Secret template in this chart. Context: #117, and this writeup on the exact data loss scenario. Happy to open a follow-up PR if you'd prefer to keep this one scoped, but it's a one-line addition so might be easiest to include here. |
Prevent accidental PVC deletion on helm uninstall or upgrade. This complements the existingClaim migration path by acting as a passive safety net for users who may not be aware of the rename. Same pattern already used in secret.yaml.
Author
|
Hey @shaun-agent — great suggestion! I've added the |
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
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 problem does this solve?
When upgrading from an older Helm release name (e.g.
agent-broker) toopenab, the automatically generated PVC name changes — Helm creates a new PVC and the old one (containing CLI auth tokens and session data) is left behind. There is no built-in way to tell the chart to adopt a pre-existing PVC. Users either lose persistent state or have to manually edit the deployment after install.The same problem occurs in environments with pre-provisioned storage (storage classes that require administrator allocation, or organizations with specific storage policies).
Closes #120
At a Glance
Prior Art & Industry Research
OpenClaw:
OpenClaw ships no Helm chart. It handles persistence via Docker Compose bind-mounts configured through environment variables —
docker-compose.ymlmounts${OPENCLAW_CONFIG_DIR}:/home/node/.openclawand${OPENCLAW_WORKSPACE_DIR}:/home/node/.openclaw/workspace. Users control the storage path by setting env vars before running Compose. This approach gives full flexibility but is entirely outside the Kubernetes/PVC model.Hermes Agent:
Hermes Agent also ships no Helm chart. Its
DockerfiledeclaresVOLUME ["/opt/data"]and thedocker/entrypoint.shhandles UID/GID remapping andchownwhen a volume is mounted at runtime. Storage identity is carried through the volume mount — if you want to migrate data you mount the same volume. Again, no PVC or existingClaim concept.Other references:
Since neither reference project ships a Helm chart, the canonical prior art for this specific pattern is the Kubernetes Helm ecosystem itself:
persistence.existingClaimis a first-class value in virtually every stateful chart (PostgreSQL, Redis, WordPress, etc.)persistence.existingClaimfield and skip-PVC-if-set logicThe
existingClaimname and semantics are a de facto Helm community standard.Proposed Solution
Add
persistence.existingClaim(default:"") to the Helm values. When set:pvc.yamlskips PVC creation entirely (the resource is not rendered)deployment.yamlusesexistingClaimas theclaimNameinstead of the generated nameAlso adds
helm.sh/resource-policy: keepannotation topvc.yamlas a passive safety net — prevents accidental PVC deletion onhelm uninstallor release name changes, using the same pattern already present insecret.yaml.Files changed
charts/openab/values.yamlpersistence.existingClaim: ""to both active kiro block and commented multi-agent examplecharts/openab/templates/pvc.yamlexistingClaimis set; addhelm.sh/resource-policy: keepannotationcharts/openab/templates/deployment.yamlexistingClaimasclaimNamewhen set, fall back to generated nameWhy this approach?
existingClaimas the field name: Follows the Bitnami/Grafana de facto standard. Helm users already know this field — choosing a different name (adoptClaim,externalPvc, etc.) would surprise them and make it harder to find in docs.Skip PVC entirely vs create then ignore: When
existingClaimis set,pvc.yamlrenders no resource at all. An alternative would be to always render the PVC and just swap theclaimName. That alternative is wrong: creating an unused PVC wastes storage quota and could confuse operators who see two PVCs in the namespace.helm.sh/resource-policy: keep: Without this annotation,helm uninstalldeletes the PVC and its data. The annotation makes data loss opt-in (user must delete the PVC manually) rather than opt-out. The pattern was already used insecret.yamlfor the same reason.No API or behavior change:
existingClaimdefaults to""(falsy), so existing values files work without modification — the chart behaves identically to before.Alternatives Considered
persistence.claimName(rename instead ofexistingClaim) — Confusingly implies the chart-generated PVC also uses this field.existingClaimsignals "this PVC already exists outside this chart." Rejected.Separate
persistence.adopt: trueboolean + keeppersistence.claimName— Two fields for one feature. More config surface, same result. Rejected.Data migration Job in the chart — A Kubernetes Job that copies data from the old PVC to the new one during upgrade. Much more complex, error-prone, and outside the scope of what a Helm chart should own. Rejected.
Validation
All scenarios verified with
helm templateandhelm lint.Scenario A — no
existingClaim(default, backward compatible)PVC is created and the deployment references it by its generated name.
helm template myrelease charts/openab \ --set agents.kiro.discord.botToken=fake \ --set-string 'agents.kiro.discord.allowedChannels[0]=111111'Output (pvc + deployment volume section)
Scenario B —
existingClaim=my-old-pvc(core feature)No PVC is created. The deployment references the pre-existing PVC by name.
helm template myrelease charts/openab \ --set agents.kiro.discord.botToken=fake \ --set-string 'agents.kiro.discord.allowedChannels[0]=111111' \ --set agents.kiro.persistence.existingClaim=my-old-pvcOutput (no PVC resource, deployment uses existing claim)
Scenario C —
persistence.enabled=falseNo PVC, no volume mount — unchanged behavior. ✅
Helm lint