Repository navigation
fix(openbao): garbage-collect the data PVC on delete (#3058) - #3073
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a storage leak where OpenBAO data PVCs remained orphaned in the tenant namespace after an application was deleted. By explicitly setting the persistentVolumeClaimRetentionPolicy in the HelmRelease, the StatefulSet now correctly cleans up associated volumes upon deletion while maintaining data safety during scale-down operations. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
github-actions
Bot
added
area/uncategorized
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughOpenBao now passes ChangesOpenBao PVC retention passthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
dosubot
Bot
added
the
area/database
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request configures the persistent volume claim (PVC) retention policy for the OpenBAO StatefulSet, ensuring that the data PVC is deleted when the release is deleted, but retained when scaled down. It also adds a corresponding unit test suite and a test target in the Makefile. The review feedback suggests declaring the test target as .PHONY in the Makefile to prevent potential conflicts with a file or directory named test.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| test: | ||
| helm unittest . |
There was a problem hiding this comment.
Declaring the test target as .PHONY is a best practice to prevent make from skipping the recipe if a file or directory named test is ever created in this directory.
.PHONY: test
test:
helm unittest .
scooby87
removed
the
area/uncategorized
After deleting an OpenBAO app CR, the StatefulSet data PVC (data-<release>-0) was left orphaned in the tenant namespace, consuming storage permanently. StatefulSet-templated PVCs are never garbage-collected on StatefulSet deletion by default, and the chart set no persistentVolumeClaimRetentionPolicy. Pass a retention policy through the HelmRelease values. The vendored upstream chart renders it onto the StatefulSet from server.persistentVolumeClaimRetentionPolicy (not server.dataStorage.*): whenDeleted: Delete reclaims the data PVC on release deletion, while whenScaled: Retain keeps a member's data PVC on HA scale-down. Also add a 'test' Makefile target so the package's helm-unittest suite runs in CI (hack/helm-unit-tests.sh only runs packages that define a 'test' target). Signed-off-by: Алексей Артамонов <[email protected]>
bbdb766 to
9dfb078
Compare
github-actions
Bot
added
the
area/uncategorized
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — passes server.persistentVolumeClaimRetentionPolicy={whenDeleted: Delete, whenScaled: Retain} through to the upstream chart so the data PVC is garbage-collected on release deletion instead of orphaned.
Business context: the OpenBAO StatefulSet's volumeClaimTemplate PVC is never GC'd by default, leaving it orphaned after the instance is deleted (#3058).
Verified: the vendored upstream chart consumes this value — packages/system/openbao/charts/openbao/templates/server-statefulset.yaml renders persistentVolumeClaimRetentionPolicy from .Values.server.persistentVolumeClaimRetentionPolicy (k8s >= 1.23), and the wrapper's openbao.server.* path maps onto it. whenScaled: Retain correctly preserves raft member data on scale-down.
Non-blocking follow-ups
whenDeleted: Deleteis a destructive behavior change for a secrets store: deleting an instance now also destroys its data PVC, whereas before the data survived (orphaned). The sibling ClickHouse change used a breaking-change marker (fix(clickhouse)!); consider the same here, and/or a one-line release note so operators know instance deletion is now irreversible for the data volume.- The helm-unittest only asserts the value is passed through the HelmRelease (it can't render the subchart) — the upstream-consumption check above is what actually proves the fix end-to-end. Fine as-is; noting the test's scope.
What this PR does
After deleting an
OpenBAOapp CR, the StatefulSet data PVC (data-<release>-0)was left orphaned in the tenant namespace, consuming storage permanently. The
OpenBAO app renders a Flux
HelmReleaseover the vendored upstream OpenBAO chart,and StatefulSet-templated PVCs are never garbage-collected on StatefulSet deletion
by default — the chart set no
persistentVolumeClaimRetentionPolicy.This passes a retention policy through the HelmRelease values. The upstream chart
renders it onto the StatefulSet from
server.persistentVolumeClaimRetentionPolicy(verified in the vendored
server-statefulset.yaml) — not fromserver.dataStorage.*:A
testtarget is added to the package Makefile so the new helm-unittest suite(asserting the passthrough) actually runs in CI —
hack/helm-unit-tests.shonlyruns packages that define a
testtarget.Warning
Deleting an OpenBAO app now permanently removes its data PVC and the data it holds.
This is intentional and fixes the storage leak.
whenScaled: Retainis kept onpurpose so scaling an HA/raft cluster down does not destroy a member's data —
only full deletion of the release reclaims storage. Back up before deleting.
Fixes #3058.
Release note
Summary by CodeRabbit