Skip to content

feat: add persistentVolumeClaimRetentionPolicy to Dragonfly CRD#448

Open
rafaelperoco wants to merge 1 commit intodragonflydb:mainfrom
rafaelperoco:feat/pvc-retention-policy
Open

feat: add persistentVolumeClaimRetentionPolicy to Dragonfly CRD#448
rafaelperoco wants to merge 1 commit intodragonflydb:mainfrom
rafaelperoco:feat/pvc-retention-policy

Conversation

@rafaelperoco
Copy link
Copy Markdown

Summary

Add support for persistentVolumeClaimRetentionPolicy field in the Dragonfly CRD, allowing users to configure whether PVCs are automatically deleted when the StatefulSet is deleted or scaled down.

Fixes #340

Changes

  • Add PersistentVolumeClaimRetentionPolicy field to DragonflySpec in api/v1alpha1/dragonfly_types.go
  • Apply the policy to the StatefulSet in internal/resources/resources.go
  • Regenerate CRD manifests and deepcopy functions

Example Usage

apiVersion: dragonflydb.io/v1alpha1
kind: Dragonfly
metadata:
  name: dragonfly-sample
spec:
  replicas: 3
  persistentVolumeClaimRetentionPolicy:
    whenDeleted: Delete
    whenScaled: Retain
  snapshot:
    persistentVolumeClaimSpec:
      accessModes:
        - ReadWriteOnce
      resources:
        requests:
          storage: 1Gi

Testing

Tested on kind cluster (Kubernetes v1.35.0):

Test Result
CRD installed with new field OK
Operator starts without errors OK
CR with persistentVolumeClaimRetentionPolicy accepted OK
StatefulSet created with policy whenDeleted: Delete, whenScaled: Retain OK
PVC deleted automatically when CR deleted (whenDeleted: Delete) OK
PVC retained when CR deleted (default behavior without policy) OK
Unit tests pass (go test ./internal/...) OK

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds support in the Dragonfly operator for configuring persistentVolumeClaimRetentionPolicy on the underlying StatefulSet, enabling users to control PVC deletion/retention behavior on delete and scale-down.

Changes:

  • Extend DragonflySpec with a PersistentVolumeClaimRetentionPolicy field and wire it into the generated deepcopy logic.
  • Propagate the new spec field into the generated StatefulSet in GenerateDragonflyResources.
  • Regenerate the CRD OpenAPI schema to expose the new spec.persistentVolumeClaimRetentionPolicy field, and apply minor gofmt alignment in the controller.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/resources/resources.go Wires spec.persistentVolumeClaimRetentionPolicy from the Dragonfly CR into the StatefulSet.Spec.PersistentVolumeClaimRetentionPolicy.
internal/controller/dragonfly_instance.go gofmt alignment of redis.Options.Addr fields; no functional changes.
config/crd/bases/dragonflydb.io_dragonflies.yaml Updates CRD schema and descriptions to expose spec.persistentVolumeClaimRetentionPolicy.whenDeleted and .whenScaled to users.
api/v1alpha1/zz_generated.deepcopy.go Updates autogenerated deepcopy to correctly copy the new PersistentVolumeClaimRetentionPolicy pointer field.
api/v1alpha1/dragonfly_types.go Extends DragonflySpec with the PersistentVolumeClaimRetentionPolicy field using appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +404 to +405
if df.Spec.PersistentVolumeClaimRetentionPolicy != nil {
statefulset.Spec.PersistentVolumeClaimRetentionPolicy = df.Spec.PersistentVolumeClaimRetentionPolicy
Copy link

Copilot AI Jan 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new PersistentVolumeClaimRetentionPolicy behavior in GenerateDragonflyResources isn’t covered by tests, even though this function already has unit tests in internal/resources/image_test.go. To prevent regressions (e.g., the field silently not being propagated to the StatefulSet in future refactors), consider adding a test that sets DragonflySpec.PersistentVolumeClaimRetentionPolicy and asserts that the resulting StatefulSet.Spec.PersistentVolumeClaimRetentionPolicy reflects the same value.

Copilot uses AI. Check for mistakes.
@miledxz
Copy link
Copy Markdown
Contributor

miledxz commented Jan 24, 2026

Hi @rafaelperoco thanks for contributing ! I checked PR, and it's important that e2e tests passes, it would be also a good idea to add e2e test to cover this patch update, like here e.g https://github.com/dragonflydb/dragonfly-operator/pull/353/changes

I see that changes in your PR are similar to #353 but there we have e2e test coverage, maybe to try to improve it a little bit ?

Let's stay in touch
WDYT ?

		It("Check for PVC retention policy", func() {
			err := k8sClient.Get(ctx, types.NamespacedName{
				Name:      name,
				Namespace: namespace,
			}, &df)
			Expect(err).To(BeNil())

			var ss appsv1.StatefulSet
			err = k8sClient.Get(ctx, types.NamespacedName{
				Name:      name,
				Namespace: namespace,
			}, &ss)
			Expect(err).To(BeNil())

			Expect(ss.Spec.PersistentVolumeClaimRetentionPolicy).To(Equal(&appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy{
				WhenDeleted: appsv1.RetainPersistentVolumeClaimRetentionPolicyType,
			}))

			df.Spec.PersistentVolumeClaimRetentionPolicy = &appsv1.StatefulSetPersistentVolumeClaimRetentionPolicy{
				WhenDeleted: appsv1.DeletePersistentVolumeClaimRetentionPolicyType,
			}

			err = k8sClient.Update(ctx, &df)
			Expect(err).To(BeNil())

			err = waitForStatefulSetReady(ctx, k8sClient, name, namespace, 3*time.Minute)
			Expect(err).To(BeNil())

			err = k8sClient.Get(ctx, types.NamespacedName{
				Name:      name,
				Namespace: namespace,
			}, &ss)
			Expect(err).To(BeNil())

			Expect(ss.Spec.PersistentVolumeClaimRetentionPolicy).To(Equal(df.Spec.PersistentVolumeClaimRetentionPolicy))
		})

Copy link
Copy Markdown
Contributor

@Abhra303 Abhra303 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @miledxz said, please add a test. Should be simple and easy to add. Otherwise LGTM.

@Abhra303 Abhra303 self-requested a review February 27, 2026 07:39
@Abhra303
Copy link
Copy Markdown
Contributor

@rafaelperoco, can you please fix the conflicts?

@Abhra303
Copy link
Copy Markdown
Contributor

gentle reminder, pls fix the conflicts :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Expose the ability to configure persistentVolumeClaimRetentionPolicy

4 participants