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

Apply pvc override in Update() rather than Build() #819

Merged
merged 1 commit into from
Aug 27, 2021

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Aug 24, 2021

This closes #

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed

Summary Of Changes

This PR addresses a bug with storage capacity. When a PVC override is provided with the default PVC name "persistence", the created PVC will have the storage from spec.persistence.storage rather than the storage capacity set in the PVC override. This can be reproduced with the below manifest, where the created storage would be 10Gi rather than 15Gi from the override:

apiVersion: rabbitmq.com/v1beta1
kind: RabbitmqCluster
metadata:
  name: test
spec:
  override:
    statefulSet:
      spec:
        template:
          spec:
            containers: []
        volumeClaimTemplates:
          - apiVersion: v1
            kind: PersistentVolumeClaim
            metadata:
              name: persistence
              namespace: default
            spec:
              accessModes:
                - ReadWriteOnce
              resources:
                requests:
                  storage: 15Gi
              volumeMode: Filesystem

This is due to: PVC override is currently handled in Build(). Update() is called after Build() and we currently allow default PVC expansion which updates the storage capacity in Update(). So the storage from override for the default persistence is being overwritten by spec.persistence.storage because of updatePersistenceStorageCapacity().

To fix this issue (when PVC override is provided, storage capacity should be what's set in the override rather than the top level spec), I've moved the applying PVC override logic to Update() which will happen after updatePersistenceStorageCapacity(). This means that when users set default persistence through PVC override, storage capacity would be whatever set in the override. An additional change of behavior is: updates to the list of PVC override will be attempted by the operator instead of silently ignored. Previously, when updates to PVC override is ignored by the cluster operator because the PVC override is only handled in Build(), not in Update(). This this change, updates to the PVC override will be applied and it will always fail because updates to volume templates is forbidden by the sts controller.

Tested in unit tests.

Additional Context

Local Testing

@mkuratczyk mkuratczyk assigned mkuratczyk and unassigned mkuratczyk Aug 27, 2021
@mkuratczyk mkuratczyk self-requested a review August 27, 2021 09:21
internal/resource/statefulset.go Outdated Show resolved Hide resolved
@ChunyiLyu ChunyiLyu force-pushed the pvc-storage-capacity branch from 5b0c483 to d3e8cc2 Compare August 27, 2021 12:13
@ChunyiLyu ChunyiLyu merged commit 8a6c77e into main Aug 27, 2021
@ChunyiLyu ChunyiLyu deleted the pvc-storage-capacity branch August 27, 2021 12:34
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.

3 participants