Add PVC for persistent PostgreSQL storage#107
Merged
openshift-merge-bot[bot] merged 1 commit intoMay 27, 2026
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
umago
force-pushed
the
persistent-database
branch
from
a9ac9e3 to
c22c241
Compare
umago
force-pushed
the
persistent-database
branch
from
c22c241 to
408cd23
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
api/v1beta1/openstacklightspeed_types.go (1)
52-55: 💤 Low valueConsider naming the field
storageClass(orstorageClassName) for native-API alignment.The Kubernetes convention for selecting a StorageClass on a PVC is
storageClassName(seePersistentVolumeClaimSpec.StorageClassName). The bareclassJSON key is unusual in this ecosystem and may surprise users writing CRs. Since this is a brand-new optional field and the API has not shipped, renaming now avoids a breaking change later.♻️ Suggested rename
- // +kubebuilder:validation:Optional - // StorageClass name for the PersistentVolumeClaim. If omitted, the cluster's - // default StorageClass is used. - Class string `json:"class,omitempty"` + // +kubebuilder:validation:Optional + // StorageClassName for the PersistentVolumeClaim. If omitted, the cluster's + // default StorageClass is used. + StorageClassName string `json:"storageClassName,omitempty"`Remember to update the sample manifest, the CRD bundle/base YAMLs, and any controller code that reads
spec.database.class.🤖 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 `@api/v1beta1/openstacklightspeed_types.go` around lines 52 - 55, Rename the PVC storage selector field from Class to StorageClassName (or storageClass if you prefer camelCase) to match Kubernetes conventions: update the struct field name (Class) to StorageClassName, change the json tag from `json:"class,omitempty"` to `json:"storageClassName,omitempty"` and adjust the kubebuilder validation marker if needed; then update all references in manifests, CRD YAMLs, sample manifests, and controller code that currently access spec.database.class to use spec.database.storageClassName (or spec.database.storageClass) so consumers and the controller read the new field name consistently.test/kuttl/tests/persistent-database/03-assert-openstack-lightspeed-instance.yaml (2)
40-49: ⚡ Quick winAssert the Postgres Deployment uses a PVC-backed data volume.
Current checks only validate Deployment readiness. Add an assertion on
spec.template.spec.volumesto confirm it referencesopenstack-lightspeed-data; otherwise anemptyDirregression could pass unnoticed.🤖 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 `@test/kuttl/tests/persistent-database/03-assert-openstack-lightspeed-instance.yaml` around lines 40 - 49, Add an assertion to the test that verifies the Deployment lightspeed-postgres-server in namespace openstack-lightspeed has a PVC-backed volume: check spec.template.spec.volumes contains an entry with persistentVolumeClaim.claimName equal to "openstack-lightspeed-data" (rather than an emptyDir), so update the test assertions to inspect spec.template.spec.volumes and fail if the PVC reference is missing or if only emptyDir is present.
10-18: ⚡ Quick winAdd an assertion that the PVC has no owner references.
This PR relies on the PVC surviving CR deletion. Please assert the PVC is not controller-owned to lock that behavior against regressions.
🤖 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 `@test/kuttl/tests/persistent-database/03-assert-openstack-lightspeed-instance.yaml` around lines 10 - 18, Add an assertion to the kuttl test for the PVC named "openstack-lightspeed-data" in namespace "openstack-lightspeed" to verify it has no ownerReferences (i.e., ownerReferences is absent or an empty array) so the PVC is not controller-owned; update the test manifest 03-assert-openstack-lightspeed-instance.yaml to include this check after the PVC exists assertion to lock the behavior that the PVC survives CR deletion.
🤖 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 `@api/v1beta1/openstacklightspeed_types.go`:
- Around line 45-56: The Size field on DatabaseSpec lacks a kubebuilder
validation pattern allowing invalid Kubernetes quantity strings to pass
admission; add a +kubebuilder:validation:Pattern tag on the Size field in type
DatabaseSpec using the standard k8s resource quantity regex (e.g. the usual
kubernetes quantity pattern used for memory/storage like
^([+-])?([0-9]+)([EPTGMkmunpf]i?)?$) so invalid values fail at admission, then
regenerate the CRD manifests (run controller-gen / your repo make target) so the
bundle/base YAML schemas pick up the new pattern.
In `@internal/controller/openstacklightspeed_controller.go`:
- Line 73: Update the kubebuilder RBAC marker in
openstacklightspeed_controller.go that currently grants PVC deletion rights:
locate the comment line containing
"+kubebuilder:rbac:groups="",resources=persistentvolumeclaims,namespace=openstack-lightspeed,verbs=..."
and remove the "delete" verb so the verbs list becomes
get;list;watch;create;patch;update to preserve the persistence contract and
prevent PVC deletion.
- Line 280: reconcilePostgresPVC currently creates/patches the PVC without
setting an ownerReference, so the controller's
Owns(&corev1.PersistentVolumeClaim{}) in openstacklightspeed_controller.go will
never be triggered; fix by adding
controllerutil.SetControllerReference(h.GetBeforeObject(), pvc, h.GetScheme())
inside the PVC mutate callback used by controllerutil.CreateOrPatch in
reconcilePostgresPVC so the PVC gets a controller ownerReference on
create/update; if you intentionally must avoid ownerReferences instead, replace
the Owns(&corev1.PersistentVolumeClaim{}) watch in
openstacklightspeed_controller.go with an explicit Watches(...) +
handler.EnqueueRequestsFromMapFunc that maps PVC events back to the owning
Postgres/OpenStack resource.
---
Nitpick comments:
In `@api/v1beta1/openstacklightspeed_types.go`:
- Around line 52-55: Rename the PVC storage selector field from Class to
StorageClassName (or storageClass if you prefer camelCase) to match Kubernetes
conventions: update the struct field name (Class) to StorageClassName, change
the json tag from `json:"class,omitempty"` to
`json:"storageClassName,omitempty"` and adjust the kubebuilder validation marker
if needed; then update all references in manifests, CRD YAMLs, sample manifests,
and controller code that currently access spec.database.class to use
spec.database.storageClassName (or spec.database.storageClass) so consumers and
the controller read the new field name consistently.
In
`@test/kuttl/tests/persistent-database/03-assert-openstack-lightspeed-instance.yaml`:
- Around line 40-49: Add an assertion to the test that verifies the Deployment
lightspeed-postgres-server in namespace openstack-lightspeed has a PVC-backed
volume: check spec.template.spec.volumes contains an entry with
persistentVolumeClaim.claimName equal to "openstack-lightspeed-data" (rather
than an emptyDir), so update the test assertions to inspect
spec.template.spec.volumes and fail if the PVC reference is missing or if only
emptyDir is present.
- Around line 10-18: Add an assertion to the kuttl test for the PVC named
"openstack-lightspeed-data" in namespace "openstack-lightspeed" to verify it has
no ownerReferences (i.e., ownerReferences is absent or an empty array) so the
PVC is not controller-owned; update the test manifest
03-assert-openstack-lightspeed-instance.yaml to include this check after the PVC
exists assertion to lock the behavior that the PVC survives CR deletion.
🪄 Autofix (Beta)
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0e3087b-f592-4e7d-bd2e-d6608c229946
📒 Files selected for processing (21)
api/v1beta1/openstacklightspeed_types.goapi/v1beta1/zz_generated.deepcopy.gobundle/manifests/lightspeed.openstack.org_openstacklightspeeds.yamlbundle/manifests/openstack-lightspeed-operator.clusterserviceversion.yamlconfig/crd/bases/lightspeed.openstack.org_openstacklightspeeds.yamlconfig/manifests/bases/openstack-lightspeed-operator.clusterserviceversion.yamlconfig/rbac/role.yamlconfig/samples/api_v1beta1_openstacklightspeed.yamlinternal/controller/constants.gointernal/controller/errors.gointernal/controller/openstacklightspeed_controller.gointernal/controller/postgres_deployment.gointernal/controller/postgres_reconciler.gotest/kuttl/tests/persistent-database/00-mock-resources.yamltest/kuttl/tests/persistent-database/01-assert-mock-objects-created.yamltest/kuttl/tests/persistent-database/02-create-openstack-lightspeed-instance.yamltest/kuttl/tests/persistent-database/03-assert-openstack-lightspeed-instance.yamltest/kuttl/tests/persistent-database/04-cleanup-openstack-lightspeed-instance.yamltest/kuttl/tests/persistent-database/05-errors-openstack-lightspeed-instance.yamltest/kuttl/tests/persistent-database/06-cleanup-mock-objects.yamltest/kuttl/tests/persistent-database/07-errors-mock-objects.yaml
umago
force-pushed
the
persistent-database
branch
3 times, most recently
from
d11b739 to
ba50b7e
Compare
lpiwowar
reviewed
May 26, 2026
Contributor
lpiwowar
left a comment
lpiwowar
left a comment
There was a problem hiding this comment.
Overall LGTM! 👍 Thank you! Just three things crossed my mind.
umago
force-pushed
the
persistent-database
branch
from
67f5245 to
63b0c1d
Compare
Contributor
Author
|
The resource. Quantity works nicely: |
umago
force-pushed
the
persistent-database
branch
2 times, most recently
from
b7e0886 to
49d47a6
Compare
Introduce a "database" field on the OpenStackLightspeed CR spec that allows configuring a PersistentVolumeClaim for the PostgreSQL data directory. A few details about the implementation: * Reuse existing PVC if one already exists with matching size * Block reconciliation with a clear error if existing PVC size does not match the requested size * If database is omitted from the CR, create a persistent storage with the default values * If no database.class is specified uses the cluster default * Default PVC size is "1Gi" The PVC is created without an owner reference so that data survives CR deletion and is reused on re-creation. Signed-off-by: Lucas Alvares Gomes <lucasagomes@gmail.com>
umago
force-pushed
the
persistent-database
branch
from
49d47a6 to
5b8e848
Compare
Akrog
reviewed
May 27, 2026
| PostgresConfigVolumeMountPath = "/usr/share/pgsql/postgresql.conf.sample" | ||
| PostgresDataVolume = "postgres-data" | ||
| PostgresDataVolumeMountPath = "/var/lib/pgsql" | ||
| PostgresDataPVCName = "openstack-lightspeed-data" |
Contributor
There was a problem hiding this comment.
nit: I would prefer it ending in -database or -db.
Contributor
Author
There was a problem hiding this comment.
Thanks for the review. Done in a follow-up PR: #108
| return fmt.Errorf("%w: requested size %s but existing PVC has %s", | ||
| ErrPostgresPVCSizeMismatch, requestedQty.String(), existingQty.String()) | ||
| } | ||
| h.GetLogger().Info("Postgres PVC already exists with matching size", "name", pvc.Name) |
Contributor
There was a problem hiding this comment.
nit: Say what happens: Reusing existing Postgress PVC with matching size
Contributor
Author
There was a problem hiding this comment.
Thanks for the review. Done in a follow-up PR: #108
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Akrog, umago The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 0f8b424
into
openstack-lightspeed:lcore-migration
7 checks passed
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
3 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.
Introduce a "database" field on the OpenStackLightspeed CR spec that allows configuring a PersistentVolumeClaim for the PostgreSQL data directory.
A few details about the implementation:
The PVC is created without an owner reference so that data survives CR deletion and is reused on re-creation.
Summary by CodeRabbit
Release Notes
New Features
Documentation