Skip to content

Commit 367e858

Browse files
mkuratczykmkuratczyk
authored
Don't create PVC if spec.persistence.storage==0 (#786)
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).
1 parent dbee453 commit 367e858

File tree

4 files changed

+192
-28
lines changed

4 files changed

+192
-28
lines changed

‎controllers/reconcile_no_persistence_test.go‎

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
package controllers_test
2+
3+
import (
4+
"context"
5+
"fmt"
6+
7+
. "github.com/onsi/ginkgo"
8+
. "github.com/onsi/gomega"
9+
rabbitmqv1beta1 "github.com/rabbitmq/cluster-operator/api/v1beta1"
10+
"github.com/rabbitmq/cluster-operator/internal/status"
11+
v1 "k8s.io/api/core/v1"
12+
apierrors "k8s.io/apimachinery/pkg/api/errors"
13+
k8sresource "k8s.io/apimachinery/pkg/api/resource"
14+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
15+
"k8s.io/apimachinery/pkg/types"
16+
"k8s.io/utils/pointer"
17+
runtimeClient "sigs.k8s.io/controller-runtime/pkg/client"
18+
)
19+
20+
var _ = Describe("Persistence", func() {
21+
var (
22+
cluster *rabbitmqv1beta1.RabbitmqCluster
23+
defaultNamespace = "default"
24+
ctx = context.Background()
25+
)
26+
27+
BeforeEach(func() {
28+
zeroGi := k8sresource.MustParse("0Gi")
29+
cluster = &rabbitmqv1beta1.RabbitmqCluster{
30+
ObjectMeta: metav1.ObjectMeta{
31+
Name: "rabbitmq-shrink",
32+
Namespace: defaultNamespace,
33+
},
34+
Spec: rabbitmqv1beta1.RabbitmqClusterSpec{
35+
Replicas: pointer.Int32Ptr(5),
36+
Persistence: rabbitmqv1beta1.RabbitmqClusterPersistenceSpec{
37+
Storage: &zeroGi,
38+
},
39+
},
40+
}
41+
Expect(client.Create(ctx, cluster)).To(Succeed())
42+
waitForClusterCreation(ctx, cluster, client)
43+
})
44+
45+
AfterEach(func() {
46+
Expect(client.Delete(ctx, cluster)).To(Succeed())
47+
Eventually(func() bool {
48+
rmq := &rabbitmqv1beta1.RabbitmqCluster{}
49+
err := client.Get(ctx, types.NamespacedName{Name: cluster.Name, Namespace: cluster.Namespace}, rmq)
50+
return apierrors.IsNotFound(err)
51+
}, 5).Should(BeTrue())
52+
})
53+
54+
It("does not allow changing the capcity from zero (no persistence)", func() {
55+
By("failing a statefulSet update", func() {
56+
Expect(updateWithRetry(cluster, func(r *rabbitmqv1beta1.RabbitmqCluster) {
57+
storage := k8sresource.MustParse("1Gi")
58+
cluster.Spec.Persistence.Storage = &storage
59+
})).To(Succeed())
60+
Consistently(func() []v1.PersistentVolumeClaim {
61+
sts, err := clientSet.AppsV1().StatefulSets(defaultNamespace).Get(ctx, cluster.ChildResourceName("server"), metav1.GetOptions{})
62+
Expect(err).NotTo(HaveOccurred())
63+
return sts.Spec.VolumeClaimTemplates
64+
}, 10, 1).Should(BeEmpty())
65+
})
66+
67+
By("setting 'Warning' events", func() {
68+
Expect(aggregateEventMsgs(ctx, cluster, "FailedReconcilePersistence")).To(
69+
ContainSubstring("changing from ephemeral to persistent storage is not supported"))
70+
})
71+
72+
By("setting ReconcileSuccess to 'false' with failed reason and message", func() {
73+
Eventually(func() string {
74+
rabbit := &rabbitmqv1beta1.RabbitmqCluster{}
75+
Expect(client.Get(ctx, runtimeClient.ObjectKey{
76+
Name: cluster.Name,
77+
Namespace: defaultNamespace,
78+
}, rabbit)).To(Succeed())
79+
80+
for i := range rabbit.Status.Conditions {
81+
if rabbit.Status.Conditions[i].Type == status.ReconcileSuccess {
82+
return fmt.Sprintf(
83+
"ReconcileSuccess status: %s, with reason: %s and message: %s",
84+
rabbit.Status.Conditions[i].Status,
85+
rabbit.Status.Conditions[i].Reason,
86+
rabbit.Status.Conditions[i].Message)
87+
}
88+
}
89+
return "ReconcileSuccess status: condition not present"
90+
}, 5).Should(Equal("ReconcileSuccess status: False, " +
91+
"with reason: FailedReconcilePVC " +
92+
"and message: changing from ephemeral to persistent storage is not supported"))
93+
})
94+
})
95+
})

‎controllers/reconcile_persistence.go‎

Lines changed: 14 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,14 +35,16 @@ func (r *RabbitmqClusterReconciler) reconcilePVC(ctx context.Context, rmq *rabbi
3535
func (r *RabbitmqClusterReconciler) expandPVC(ctx context.Context, rmq *rabbitmqv1beta1.RabbitmqCluster, current, desired *appsv1.StatefulSet) error {
3636
logger := ctrl.LoggerFrom(ctx)
3737

38-
currentCapacity, err := persistenceStorageCapacity(current.Spec.VolumeClaimTemplates)
39-
if err != nil {
40-
return err
41-
}
38+
currentCapacity := persistenceStorageCapacity(current.Spec.VolumeClaimTemplates)
4239

43-
desiredCapacity, err := persistenceStorageCapacity(desired.Spec.VolumeClaimTemplates)
44-
if err != nil {
45-
return err
40+
desiredCapacity := persistenceStorageCapacity(desired.Spec.VolumeClaimTemplates)
41+
42+
// don't allow going from 0 (no PVC) to anything else
43+
if (currentCapacity.Cmp(k8sresource.MustParse("0Gi")) == 0) && (desiredCapacity.Cmp(k8sresource.MustParse("0Gi")) != 0) {
44+
msg := "changing from ephemeral to persistent storage is not supported"
45+
logger.Error(errors.New("unsupported operation"), msg)
46+
r.Recorder.Event(rmq, corev1.EventTypeWarning, "FailedReconcilePersistence", msg)
47+
return errors.New(msg)
4648
}
4749

4850
logger.Info(fmt.Sprintf("updating storage capacity from %s to %s", currentCapacity.String(), desiredCapacity.String()))
@@ -89,15 +91,9 @@ func (r *RabbitmqClusterReconciler) updatePVC(ctx context.Context, rmq *rabbitmq
8991
func (r *RabbitmqClusterReconciler) needsPVCExpand(ctx context.Context, rmq *rabbitmqv1beta1.RabbitmqCluster, current, desired *appsv1.StatefulSet) (bool, error) {
9092
logger := ctrl.LoggerFrom(ctx)
9193

92-
currentCapacity, err := persistenceStorageCapacity(current.Spec.VolumeClaimTemplates)
93-
if err != nil {
94-
return false, err
95-
}
94+
currentCapacity := persistenceStorageCapacity(current.Spec.VolumeClaimTemplates)
9695

97-
desiredCapacity, err := persistenceStorageCapacity(desired.Spec.VolumeClaimTemplates)
98-
if err != nil {
99-
return false, err
100-
}
96+
desiredCapacity := persistenceStorageCapacity(desired.Spec.VolumeClaimTemplates)
10197

10298
cmp := currentCapacity.Cmp(desiredCapacity)
10399

@@ -116,13 +112,13 @@ func (r *RabbitmqClusterReconciler) needsPVCExpand(ctx context.Context, rmq *rab
116112
return false, nil
117113
}
118114

119-
func persistenceStorageCapacity(templates []corev1.PersistentVolumeClaim) (k8sresource.Quantity, error) {
115+
func persistenceStorageCapacity(templates []corev1.PersistentVolumeClaim) k8sresource.Quantity {
120116
for _, t := range templates {
121117
if t.Name == "persistence" {
122-
return t.Spec.Resources.Requests[corev1.ResourceStorage], nil
118+
return t.Spec.Resources.Requests[corev1.ResourceStorage]
123119
}
124120
}
125-
return k8sresource.Quantity{}, errors.New("cannot find PersistentVolumeClaim 'persistence'")
121+
return k8sresource.MustParse("0")
126122
}
127123

128124
// deleteSts deletes a sts without deleting pods and PVCs

‎internal/resource/statefulset.go‎

Lines changed: 34 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -160,18 +160,27 @@ func applyStsOverride(instance *rabbitmqv1beta1.RabbitmqCluster, scheme *runtime
160160
}
161161

162162
if len(stsOverride.Spec.VolumeClaimTemplates) != 0 {
163-
volumeClaimTemplatesOverride := stsOverride.Spec.VolumeClaimTemplates
164-
pvcOverride := make([]corev1.PersistentVolumeClaim, len(volumeClaimTemplatesOverride))
165-
for i := range volumeClaimTemplatesOverride {
166-
copyObjectMeta(&pvcOverride[i].ObjectMeta, volumeClaimTemplatesOverride[i].EmbeddedObjectMeta)
167-
pvcOverride[i].Namespace = sts.Namespace // PVC should always be in the same namespace as the Stateful Set
168-
pvcOverride[i].Spec = volumeClaimTemplatesOverride[i].Spec
169-
if err := controllerutil.SetControllerReference(instance, &pvcOverride[i], scheme); err != nil {
170-
return fmt.Errorf("failed setting controller reference: %v", err)
163+
// If spec.persistence.storage == 0, ignore PVC overrides.
164+
// Main reason for that is that there is no default PVC in such case (emptyDir is used instead)
165+
// other PVCs could technically be still overridden/added but we would be entering a very confusing territory
166+
// where storage is set to 0 and yet there are PVCs with data
167+
if instance.Spec.Persistence.Storage.Cmp(k8sresource.MustParse("0Gi")) == 0 {
168+
logger := ctrl.Log.WithName("statefulset").WithName("RabbitmqCluster")
169+
logger.Info(fmt.Sprintf("Warning: persistentVolumeClaim overrides are ignored for cluster \"%s\", becasue spec.persistence.storage is set to zero.", sts.GetName()))
170+
} else {
171+
volumeClaimTemplatesOverride := stsOverride.Spec.VolumeClaimTemplates
172+
pvcOverride := make([]corev1.PersistentVolumeClaim, len(volumeClaimTemplatesOverride))
173+
for i := range volumeClaimTemplatesOverride {
174+
copyObjectMeta(&pvcOverride[i].ObjectMeta, volumeClaimTemplatesOverride[i].EmbeddedObjectMeta)
175+
pvcOverride[i].Namespace = sts.Namespace // PVC should always be in the same namespace as the Stateful Set
176+
pvcOverride[i].Spec = volumeClaimTemplatesOverride[i].Spec
177+
if err := controllerutil.SetControllerReference(instance, &pvcOverride[i], scheme); err != nil {
178+
return fmt.Errorf("failed setting controller reference: %v", err)
179+
}
180+
disableBlockOwnerDeletion(pvcOverride[i])
171181
}
172-
disableBlockOwnerDeletion(pvcOverride[i])
182+
sts.Spec.VolumeClaimTemplates = pvcOverride
173183
}
174-
sts.Spec.VolumeClaimTemplates = pvcOverride
175184
}
176185

177186
if stsOverride.Spec.Template == nil {
@@ -192,6 +201,11 @@ func applyStsOverride(instance *rabbitmqv1beta1.RabbitmqCluster, scheme *runtime
192201
}
193202

194203
func persistentVolumeClaim(instance *rabbitmqv1beta1.RabbitmqCluster, scheme *runtime.Scheme) ([]corev1.PersistentVolumeClaim, error) {
204+
zero := k8sresource.MustParse("0Gi")
205+
if instance.Spec.Persistence.Storage.Cmp(zero) == 0 {
206+
return []corev1.PersistentVolumeClaim{}, nil
207+
}
208+
195209
pvc := corev1.PersistentVolumeClaim{
196210
ObjectMeta: metav1.ObjectMeta{
197211
Name: defaultPVCName,
@@ -430,6 +444,16 @@ func (builder *StatefulSetBuilder) podTemplateSpec(previousPodAnnotations map[st
430444
}}}})
431445
}
432446

447+
zero := k8sresource.MustParse("0Gi")
448+
if builder.Instance.Spec.Persistence.Storage.Cmp(zero) == 0 {
449+
volumes = append(volumes, corev1.Volume{
450+
Name: "persistence",
451+
VolumeSource: corev1.VolumeSource{
452+
EmptyDir: &corev1.EmptyDirVolumeSource{},
453+
},
454+
})
455+
}
456+
433457
rabbitmqContainerVolumeMounts := []corev1.VolumeMount{
434458
{
435459
Name: "rabbitmq-erlang-cookie",

‎internal/resource/statefulset_test.go‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,38 @@ var _ = Describe("StatefulSet", func() {
147147
actualPersistentVolumeClaim := statefulSet.Spec.VolumeClaimTemplates[0]
148148
Expect(actualPersistentVolumeClaim).To(Equal(expectedPersistentVolumeClaim))
149149
})
150+
151+
It("doesn't create the default PersistentVolumeClaim when storage == 0", func() {
152+
zero := k8sresource.MustParse("0Gi")
153+
154+
builder.Instance.Spec.Persistence.Storage = &zero
155+
// we shouldn't create the `persistence` PVC if storage==0, even if overrides are used
156+
builder.Instance.Spec.Override.StatefulSet = &rabbitmqv1beta1.StatefulSet{
157+
Spec: &rabbitmqv1beta1.StatefulSetSpec{
158+
VolumeClaimTemplates: []rabbitmqv1beta1.PersistentVolumeClaim{
159+
{
160+
EmbeddedObjectMeta: rabbitmqv1beta1.EmbeddedObjectMeta{
161+
Name: "persistence",
162+
Namespace: instance.Namespace,
163+
},
164+
Spec: corev1.PersistentVolumeClaimSpec{
165+
Resources: corev1.ResourceRequirements{
166+
Requests: corev1.ResourceList{
167+
corev1.ResourceStorage: k8sresource.MustParse("10Gi"),
168+
},
169+
},
170+
},
171+
},
172+
},
173+
},
174+
}
175+
176+
obj, err := stsBuilder.Build()
177+
Expect(err).NotTo(HaveOccurred())
178+
statefulSet := obj.(*appsv1.StatefulSet)
179+
180+
Expect(statefulSet.Spec.VolumeClaimTemplates).To(BeEmpty())
181+
})
150182
})
151183
Context("Override", func() {
152184
It("overrides statefulSet.spec.selector", func() {
@@ -985,6 +1017,23 @@ var _ = Describe("StatefulSet", func() {
9851017
Entry("Only advanced config is set", "", "advanced-config-is-set"),
9861018
Entry("No configs are set", "", ""),
9871019
)
1020+
1021+
It("defines an emptyDir volume when storage == 0", func() {
1022+
zero, _ := k8sresource.ParseQuantity("0")
1023+
1024+
stsBuilder := builder.StatefulSet()
1025+
stsBuilder.Instance.Spec.Persistence.Storage = &zero
1026+
Expect(stsBuilder.Update(statefulSet)).To(Succeed())
1027+
1028+
expectedVolume := corev1.Volume{
1029+
Name: "persistence",
1030+
VolumeSource: corev1.VolumeSource{
1031+
EmptyDir: &corev1.EmptyDirVolumeSource{},
1032+
},
1033+
}
1034+
1035+
Expect(statefulSet.Spec.Template.Spec.Volumes).To(ContainElement(expectedVolume))
1036+
})
9881037
})
9891038

9901039
It("uses the correct service account", func() {

0 commit comments

Comments
 (0)