Skip to content

Commit f9a9c08

Browse files
dsessler7dsessler7
committed
Change behavior so cleanup happens if feature gate is on but cluster has snapshots turned off. Do nothing if feature gate is off. Adjust tests.
1 parent 2033618 commit f9a9c08

File tree

2 files changed

+50
-72
lines changed

2 files changed

+50
-72
lines changed

‎internal/controller/postgrescluster/snapshots.go‎

Lines changed: 12 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,10 @@ import (
4848
func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
4949
postgrescluster *v1beta1.PostgresCluster, pvc *corev1.PersistentVolumeClaim) error {
5050

51-
// Get feature gate state
52-
volumeSnapshotsFeatureEnabled := feature.Enabled(ctx, feature.VolumeSnapshots)
51+
// If VolumeSnapshots feature gate is disabled. Do nothing and return early.
52+
if !feature.Enabled(ctx, feature.VolumeSnapshots) {
53+
return nil
54+
}
5355

5456
// Check if the Kube cluster has VolumeSnapshots installed. If VolumeSnapshots
5557
// are not installed, we need to return early. If user is attempting to use
@@ -59,7 +61,7 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
5961
return err
6062
}
6163
if !*volumeSnapshotKindExists {
62-
if postgrescluster.Spec.Backups.Snapshots != nil && volumeSnapshotsFeatureEnabled {
64+
if postgrescluster.Spec.Backups.Snapshots != nil {
6365
return errors.New("VolumeSnapshots are not installed/enabled in this Kubernetes cluster; cannot create snapshot.")
6466
} else {
6567
return nil
@@ -69,7 +71,7 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
6971
// If user is attempting to use snapshots and has tablespaces enabled, we
7072
// need to create a warning event indicating that the two features are not
7173
// currently compatible and return early.
72-
if postgrescluster.Spec.Backups.Snapshots != nil && volumeSnapshotsFeatureEnabled &&
74+
if postgrescluster.Spec.Backups.Snapshots != nil &&
7375
clusterUsingTablespaces(ctx, postgrescluster) {
7476
r.Recorder.Event(postgrescluster, corev1.EventTypeWarning, "IncompatibleFeatures",
7577
"VolumeSnapshots not currently compatible with TablespaceVolumes; cannot create snapshot.")
@@ -83,7 +85,7 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
8385
}
8486

8587
// If snapshots are disabled, delete any existing snapshots and return early.
86-
if postgrescluster.Spec.Backups.Snapshots == nil || !volumeSnapshotsFeatureEnabled {
88+
if postgrescluster.Spec.Backups.Snapshots == nil {
8789
return r.deleteSnapshots(ctx, postgrescluster, snapshots)
8890
}
8991

@@ -164,8 +166,10 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
164166
clusterVolumes []corev1.PersistentVolumeClaim,
165167
) (*corev1.PersistentVolumeClaim, error) {
166168

167-
// Get feature gate state.
168-
volumeSnapshotsFeatureEnabled := feature.Enabled(ctx, feature.VolumeSnapshots)
169+
// If VolumeSnapshots feature gate is disabled, do nothing and return early.
170+
if !feature.Enabled(ctx, feature.VolumeSnapshots) {
171+
return nil, nil
172+
}
169173

170174
// Set appropriate labels for dedicated snapshot volume
171175
labelMap := map[string]string{
@@ -192,7 +196,7 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
192196

193197
// If snapshots are disabled, delete the PVC if it exists and return early.
194198
// Check the client cache first using Get.
195-
if cluster.Spec.Backups.Snapshots == nil || !volumeSnapshotsFeatureEnabled {
199+
if cluster.Spec.Backups.Snapshots == nil {
196200
key := client.ObjectKeyFromObject(pvc)
197201
err := errors.WithStack(r.Client.Get(ctx, key, pvc))
198202
if err == nil {

‎internal/controller/postgrescluster/snapshots_test.go‎

Lines changed: 38 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -18,13 +18,14 @@ import (
1818
"k8s.io/apimachinery/pkg/api/resource"
1919
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2020
"k8s.io/client-go/discovery"
21-
"k8s.io/client-go/tools/record"
2221
"sigs.k8s.io/controller-runtime/pkg/client"
2322

23+
"github.com/crunchydata/postgres-operator/internal/controller/runtime"
2424
"github.com/crunchydata/postgres-operator/internal/feature"
2525
"github.com/crunchydata/postgres-operator/internal/initialize"
2626
"github.com/crunchydata/postgres-operator/internal/naming"
2727
"github.com/crunchydata/postgres-operator/internal/testing/cmp"
28+
"github.com/crunchydata/postgres-operator/internal/testing/events"
2829
"github.com/crunchydata/postgres-operator/internal/testing/require"
2930
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
3031

@@ -38,7 +39,7 @@ func TestReconcileVolumeSnapshots(t *testing.T) {
3839
discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
3940
assert.NilError(t, err)
4041

41-
recorder := record.NewFakeRecorder(100)
42+
recorder := events.NewRecorder(t, runtime.Scheme)
4243
r := &Reconciler{
4344
Client: cc,
4445
Owner: client.FieldOwner(t.Name()),
@@ -47,8 +48,15 @@ func TestReconcileVolumeSnapshots(t *testing.T) {
4748
}
4849
ns := setupNamespace(t, cc)
4950

51+
// Enable snapshots feature gate
52+
gate := feature.NewGate()
53+
assert.NilError(t, gate.SetFromMap(map[string]bool{
54+
feature.VolumeSnapshots: true,
55+
}))
56+
ctx = feature.NewContext(ctx, gate)
57+
5058
t.Run("SnapshotsDisabledDeleteSnapshots", func(t *testing.T) {
51-
// Create cluster
59+
// Create cluster (without snapshots spec)
5260
cluster := testCluster()
5361
cluster.Namespace = ns.Name
5462
cluster.ObjectMeta.UID = "the-uid-123"
@@ -96,11 +104,11 @@ func TestReconcileVolumeSnapshots(t *testing.T) {
96104
})
97105

98106
t.Run("SnapshotsEnabledTablespacesEnabled", func(t *testing.T) {
99-
// Enable snapshots feature gate
107+
// Enable both tablespaces and snapshots feature gates
100108
gate := feature.NewGate()
101109
assert.NilError(t, gate.SetFromMap(map[string]bool{
102-
feature.VolumeSnapshots: true,
103110
feature.TablespaceVolumes: true,
111+
feature.VolumeSnapshots: true,
104112
}))
105113
ctx := feature.NewContext(ctx, gate)
106114

@@ -126,20 +134,17 @@ func TestReconcileVolumeSnapshots(t *testing.T) {
126134
err = r.reconcileVolumeSnapshots(ctx, cluster, pvc)
127135
assert.NilError(t, err)
128136

129-
// Assert warning event was created
130-
event, ok := <-recorder.Events
131-
assert.Equal(t, ok, true)
132-
assert.Assert(t, cmp.Contains(event, "IncompatibleFeatures"))
137+
// Assert warning event was created and has expected attributes
138+
if assert.Check(t, len(recorder.Events) > 0) {
139+
assert.Equal(t, recorder.Events[0].Type, "Warning")
140+
assert.Equal(t, recorder.Events[0].Regarding.Kind, "PostgresCluster")
141+
assert.Equal(t, recorder.Events[0].Regarding.Name, "hippo")
142+
assert.Equal(t, recorder.Events[0].Reason, "IncompatibleFeatures")
143+
assert.Assert(t, cmp.Contains(recorder.Events[0].Note, "VolumeSnapshots not currently compatible with TablespaceVolumes"))
144+
}
133145
})
134146

135147
t.Run("SnapshotsEnabledNoPvcAnnotation", func(t *testing.T) {
136-
// Enable snapshots feature gate
137-
gate := feature.NewGate()
138-
assert.NilError(t, gate.SetFromMap(map[string]bool{
139-
feature.VolumeSnapshots: true,
140-
}))
141-
ctx := feature.NewContext(ctx, gate)
142-
143148
// Create a volume snapshot class
144149
volumeSnapshotClassName := "my-snapshotclass"
145150
volumeSnapshotClass := &volumesnapshotv1.VolumeSnapshotClass{
@@ -183,13 +188,6 @@ func TestReconcileVolumeSnapshots(t *testing.T) {
183188
})
184189

185190
t.Run("SnapshotsEnabledReadySnapshotsExist", func(t *testing.T) {
186-
// Enable snapshots feature gate
187-
gate := feature.NewGate()
188-
assert.NilError(t, gate.SetFromMap(map[string]bool{
189-
feature.VolumeSnapshots: true,
190-
}))
191-
ctx := feature.NewContext(ctx, gate)
192-
193191
// volumeSnapshotClass still exists
194192
// Create a cluster with snapshots enabled
195193
cluster := testCluster()
@@ -303,13 +301,6 @@ func TestReconcileVolumeSnapshots(t *testing.T) {
303301
})
304302

305303
t.Run("SnapshotsEnabledCreateSnapshot", func(t *testing.T) {
306-
// Enable snapshots feature gate
307-
gate := feature.NewGate()
308-
assert.NilError(t, gate.SetFromMap(map[string]bool{
309-
feature.VolumeSnapshots: true,
310-
}))
311-
ctx := feature.NewContext(ctx, gate)
312-
313304
// volumeSnapshotClass still exists
314305
// Create a cluster with snapshots enabled
315306
cluster := testCluster()
@@ -357,16 +348,23 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
357348
discoveryClient, err := discovery.NewDiscoveryClientForConfig(cfg)
358349
assert.NilError(t, err)
359350

360-
recorder := record.NewFakeRecorder(100)
351+
recorder := events.NewRecorder(t, runtime.Scheme)
361352
r := &Reconciler{
362353
Client: cc,
363354
Owner: client.FieldOwner(t.Name()),
364355
DiscoveryClient: discoveryClient,
365356
Recorder: recorder,
366357
}
367358

359+
// Enable snapshots feature gate
360+
gate := feature.NewGate()
361+
assert.NilError(t, gate.SetFromMap(map[string]bool{
362+
feature.VolumeSnapshots: true,
363+
}))
364+
ctx = feature.NewContext(ctx, gate)
365+
368366
t.Run("SnapshotsDisabledDeletePvc", func(t *testing.T) {
369-
// Create cluster
367+
// Create cluster without snapshots spec
370368
ns := setupNamespace(t, cc)
371369
cluster := testCluster()
372370
cluster.Namespace = ns.Name
@@ -426,13 +424,6 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
426424
})
427425

428426
t.Run("SnapshotsEnabledCreatePvcNoBackupNoRestore", func(t *testing.T) {
429-
// Enable snapshots feature gate
430-
gate := feature.NewGate()
431-
assert.NilError(t, gate.SetFromMap(map[string]bool{
432-
feature.VolumeSnapshots: true,
433-
}))
434-
ctx := feature.NewContext(ctx, gate)
435-
436427
// Create cluster with snapshots enabled
437428
ns := setupNamespace(t, cc)
438429
cluster := testCluster()
@@ -466,13 +457,6 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
466457
})
467458

468459
t.Run("SnapshotsEnabledBackupExistsCreateRestore", func(t *testing.T) {
469-
// Enable snapshots feature gate
470-
gate := feature.NewGate()
471-
assert.NilError(t, gate.SetFromMap(map[string]bool{
472-
feature.VolumeSnapshots: true,
473-
}))
474-
ctx := feature.NewContext(ctx, gate)
475-
476460
// Create cluster with snapshots enabled
477461
ns := setupNamespace(t, cc)
478462
cluster := testCluster()
@@ -524,13 +508,6 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
524508
})
525509

526510
t.Run("SnapshotsEnabledSuccessfulRestoreExists", func(t *testing.T) {
527-
// Enable snapshots feature gate
528-
gate := feature.NewGate()
529-
assert.NilError(t, gate.SetFromMap(map[string]bool{
530-
feature.VolumeSnapshots: true,
531-
}))
532-
ctx := feature.NewContext(ctx, gate)
533-
534511
// Create cluster with snapshots enabled
535512
ns := setupNamespace(t, cc)
536513
cluster := testCluster()
@@ -604,13 +581,6 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
604581
})
605582

606583
t.Run("SnapshotsEnabledFailedRestoreExists", func(t *testing.T) {
607-
// Enable snapshots feature gate
608-
gate := feature.NewGate()
609-
assert.NilError(t, gate.SetFromMap(map[string]bool{
610-
feature.VolumeSnapshots: true,
611-
}))
612-
ctx := feature.NewContext(ctx, gate)
613-
614584
// Create cluster with snapshots enabled
615585
ns := setupNamespace(t, cc)
616586
cluster := testCluster()
@@ -668,10 +638,14 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
668638
assert.NilError(t, err)
669639
assert.Assert(t, pvc != nil)
670640

671-
// Assert warning event was created
672-
event, ok := <-recorder.Events
673-
assert.Equal(t, ok, true)
674-
assert.Assert(t, cmp.Contains(event, "DedicatedSnapshotVolumeRestoreJobError"))
641+
// Assert warning event was created and has expected attributes
642+
if assert.Check(t, len(recorder.Events) > 0) {
643+
assert.Equal(t, recorder.Events[0].Type, "Warning")
644+
assert.Equal(t, recorder.Events[0].Regarding.Kind, "PostgresCluster")
645+
assert.Equal(t, recorder.Events[0].Regarding.Name, "hippo")
646+
assert.Equal(t, recorder.Events[0].Reason, "DedicatedSnapshotVolumeRestoreJobError")
647+
assert.Assert(t, cmp.Contains(recorder.Events[0].Note, "restore job failed, check the logs"))
648+
}
675649
})
676650
}
677651

0 commit comments

Comments
 (0)