fix(openbao): make script hook configmaps idempotent - #680
Open
nvjmcnamee wants to merge 2 commits into
Open
Conversation
Signed-off-by: James McNamee <jmcnamee@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe Helm chart updates the deletion policy for the initialization and utilities ConfigMaps. Both policies now include ChangesHelm hook policy
Estimated code review effort: 1 (Trivial) | ~2 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
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.
TL;DR
Add
before-hook-creationto the OpenBao init script hook ConfigMaps so stale hook resources do not cause Helm install/upgrade retries to fail withconfigmaps "openbao-server-init-script" already exists.Additional Details
The OpenBao chart creates
openbao-server-init-scriptandopenbao-server-utils-scriptaspost-install,post-upgradehook ConfigMaps.Previously they only used:
helm.sh/hook-delete-policy: hook-succeededThat works for the normal success path, but if one of those fixed-name hook ConfigMaps is left behind, a later Helm upgrade attempts to create it again and fails with an
AlreadyExistscollision.This keeps the existing
hook-succeededbehavior and intentionally does not addhook-failed, preserving the existing retry behavior documented in the chart comments.For the Reviewer
Please focus on:
deploy/helm/openbao/helm/templates/hook-post-01-initcluster.yamlThis mirrors the existing idempotent hook cleanup pattern used by other Helm hooks while preserving the OpenBao-specific decision not to delete these script ConfigMaps on hook failure.
For QA
Verified locally:
helm dependency build deploy/helm/openbao/helmhelm lint deploy/helm/openbao/helmhelm template test-openbao deploy/helm/openbao/helm --namespace vault-system --set openbao.migrations.image.registry=example.invalid --set openbao.migrations.image.repository=nvcf-openbao-migrations --set openbao.migrations.image.tag=testRendered output confirms both
openbao-server-init-scriptandopenbao-server-utils-scriptnow have:helm.sh/hook-delete-policy: before-hook-creation,hook-succeededIssues
NO-REF
Checklist
Summary by CodeRabbit