Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't create PVC if spec.persistence.storage==0 #786

Merged
merged 2 commits into from
Sep 3, 2021
Merged

Conversation

mkuratczyk
Copy link
Collaborator

For test environments, especially in CI/CD, it may be beneficial to skip
creating the PVC altogether since the whole cluster is disposable
anyway. With this change, if spec.persistence.storage is set to 0, the
PVC is not created and an emptyDir volume is used instead. Changing from
0 to anything else is not supported (and doesn't make much sense
anyway).

This closes #776

@mkuratczyk mkuratczyk requested a review from ChunyiLyu July 30, 2021 15:30
Copy link
Contributor

@ChunyiLyu ChunyiLyu left a comment

Choose a reason for hiding this comment

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

Everything works as expected. For an edge case, if someone set persistence.storage to 0 but defines PVCs through statefulSet override, they do not get any PVC created. Example manifest:

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name: multiple-disks
spec:
  replicas: 1
  persistence:
    storage: 0
  override:
    statefulSet:
      spec:
        volumeClaimTemplates:
          - apiVersion: v1
            kind: PersistentVolumeClaim
            metadata:
              name: persistence
              namespace: default
            spec:
              accessModes:
                - ReadWriteOnce
              resources:
                requests:
                  storage: 10Gi
              volumeMode: Filesystem

Is this the desired behavior? If yes, may I suggest to add a unit level test for this behavior to capture in the code? We are getting this behavior because the code block of

if instance.Spec.Persistence.Storage.Cmp(zero) == 0 {
		return []corev1.PersistentVolumeClaim{}, nil
	}

is before override was applied and would be great to add a test to ensure so we don't change the behavior by accident.

For test environments, especially in CI/CD, it may be beneficial to skip
creating the PVC altogether since the whole cluster is disposable
anyway. With this change, if spec.persistence.storage is set to 0, the
PVC is not created and an emptyDir volume is used instead. Changing from
0 to anything else is not supported (and doesn't make much sense
anyway).
This fixes the corner-case where storage is zero but overrides are used
to define it differently. To avoid unnecessarily confusing
configurations, if storage==0, all PVC overrides are ignored and a
warning is logged.
@mkuratczyk mkuratczyk merged commit 367e858 into main Sep 3, 2021
@ansd ansd deleted the allow-no-pvc branch September 3, 2021 16:59
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.

Allow creating clusters without persistence
3 participants