Skip to content

Commit c8da193

Browse files
cbandycbandy
committed
Treat corev1.PersistentVolumeClaim as a reference type
The client.Object interface is implemented only by the pointer type. Subtle bugs can arise from passing a value when you intended to pass a reference. See: 96132b8 See: https://go.dev/wiki/MethodSets
1 parent 99a0953 commit c8da193

File tree

12 files changed

+124
-83
lines changed

12 files changed

+124
-83
lines changed

‎internal/controller/postgrescluster/cluster.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -281,7 +281,7 @@ func (r *Reconciler) reconcileClusterReplicaService(
281281
// `dataSource.pgbackrest` fields
282282
func (r *Reconciler) reconcileDataSource(ctx context.Context,
283283
cluster *v1beta1.PostgresCluster, observed *observedInstances,
284-
clusterVolumes []corev1.PersistentVolumeClaim,
284+
clusterVolumes []*corev1.PersistentVolumeClaim,
285285
rootCA *pki.RootCertificateAuthority,
286286
backupsSpecFound bool,
287287
) (bool, error) {

‎internal/controller/postgrescluster/controller.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ func (r *Reconciler) Reconcile(
162162
clusterConfigMap *corev1.ConfigMap
163163
clusterReplicationSecret *corev1.Secret
164164
clusterPodService *corev1.Service
165-
clusterVolumes []corev1.PersistentVolumeClaim
165+
clusterVolumes []*corev1.PersistentVolumeClaim
166166
instanceServiceAccount *corev1.ServiceAccount
167167
instances *observedInstances
168168
patroniLeaderService *corev1.Service

‎internal/controller/postgrescluster/instance.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -588,7 +588,7 @@ func (r *Reconciler) reconcileInstanceSets(
588588
instances *observedInstances,
589589
patroniLeaderService *corev1.Service,
590590
primaryCertificate *corev1.SecretProjection,
591-
clusterVolumes []corev1.PersistentVolumeClaim,
591+
clusterVolumes []*corev1.PersistentVolumeClaim,
592592
exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap,
593593
backupsSpecFound bool,
594594
) error {
@@ -706,12 +706,12 @@ func (r *Reconciler) cleanupPodDisruptionBudgets(
706706
// for the instance set specified that are not currently associated with an instance, and then
707707
// returning the instance names associated with those PVC's.
708708
func findAvailableInstanceNames(set v1beta1.PostgresInstanceSetSpec,
709-
observedInstances *observedInstances, clusterVolumes []corev1.PersistentVolumeClaim) []string {
709+
observedInstances *observedInstances, clusterVolumes []*corev1.PersistentVolumeClaim) []string {
710710

711711
availableInstanceNames := []string{}
712712

713713
// first identify any PGDATA volumes for the instance set specified
714-
setVolumes := []corev1.PersistentVolumeClaim{}
714+
setVolumes := []*corev1.PersistentVolumeClaim{}
715715
for _, pvc := range clusterVolumes {
716716
// ignore PGDATA PVCs that are terminating
717717
if pvc.GetDeletionTimestamp() != nil {
@@ -729,7 +729,7 @@ func findAvailableInstanceNames(set v1beta1.PostgresInstanceSetSpec,
729729
// any available PGDATA volumes for the instance set that have no corresponding WAL
730730
// volumes (which means new PVCs will simply be reconciled instead).
731731
if set.WALVolumeClaimSpec != nil {
732-
setVolumesWithWAL := []corev1.PersistentVolumeClaim{}
732+
setVolumesWithWAL := []*corev1.PersistentVolumeClaim{}
733733
for _, setVol := range setVolumes {
734734
setVolInstance := setVol.GetLabels()[naming.LabelInstance]
735735
for _, pvc := range clusterVolumes {
@@ -1066,7 +1066,7 @@ func (r *Reconciler) scaleUpInstances(
10661066
primaryCertificate *corev1.SecretProjection,
10671067
availableInstanceNames []string,
10681068
numInstancePods int,
1069-
clusterVolumes []corev1.PersistentVolumeClaim,
1069+
clusterVolumes []*corev1.PersistentVolumeClaim,
10701070
exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap,
10711071
backupsSpecFound bool,
10721072
) ([]*appsv1.StatefulSet, error) {
@@ -1141,7 +1141,7 @@ func (r *Reconciler) reconcileInstance(
11411141
primaryCertificate *corev1.SecretProjection,
11421142
instance *appsv1.StatefulSet,
11431143
numInstancePods int,
1144-
clusterVolumes []corev1.PersistentVolumeClaim,
1144+
clusterVolumes []*corev1.PersistentVolumeClaim,
11451145
exporterQueriesConfig, exporterWebConfig *corev1.ConfigMap,
11461146
backupsSpecFound bool,
11471147
) error {

‎internal/controller/postgrescluster/instance_test.go‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1758,7 +1758,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
17581758
testCases := []struct {
17591759
set v1beta1.PostgresInstanceSetSpec
17601760
fakeObservedInstances *observedInstances
1761-
fakeClusterVolumes []corev1.PersistentVolumeClaim
1761+
fakeClusterVolumes []*corev1.PersistentVolumeClaim
17621762
expectedInstanceNames []string
17631763
}{{
17641764
set: v1beta1.PostgresInstanceSetSpec{Name: "instance1"},
@@ -1769,7 +1769,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
17691769
[]appsv1.StatefulSet{{}},
17701770
[]corev1.Pod{},
17711771
),
1772-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{{}},
1772+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{{}},
17731773
expectedInstanceNames: []string{},
17741774
}, {
17751775
set: v1beta1.PostgresInstanceSetSpec{Name: "instance1"},
@@ -1783,7 +1783,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
17831783
naming.LabelInstanceSet: "instance1"}}}},
17841784
[]corev1.Pod{},
17851785
),
1786-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{{ObjectMeta: metav1.ObjectMeta{
1786+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{{ObjectMeta: metav1.ObjectMeta{
17871787
Name: "instance1-abc-def",
17881788
Labels: map[string]string{
17891789
naming.LabelRole: naming.RolePostgresData,
@@ -1802,7 +1802,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
18021802
naming.LabelInstanceSet: "instance1"}}}},
18031803
[]corev1.Pod{},
18041804
),
1805-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{},
1805+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{},
18061806
expectedInstanceNames: []string{},
18071807
}, {
18081808
set: v1beta1.PostgresInstanceSetSpec{Name: "instance1"},
@@ -1816,7 +1816,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
18161816
naming.LabelInstanceSet: "instance1"}}}},
18171817
[]corev1.Pod{},
18181818
),
1819-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{
1819+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{
18201820
{ObjectMeta: metav1.ObjectMeta{
18211821
Name: "instance1-abc-def",
18221822
Labels: map[string]string{
@@ -1843,7 +1843,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
18431843
naming.LabelInstanceSet: "instance1"}}}},
18441844
[]corev1.Pod{},
18451845
),
1846-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{{ObjectMeta: metav1.ObjectMeta{
1846+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{{ObjectMeta: metav1.ObjectMeta{
18471847
Name: "instance1-abc-def",
18481848
Labels: map[string]string{
18491849
naming.LabelRole: naming.RolePostgresData,
@@ -1863,7 +1863,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
18631863
naming.LabelInstanceSet: "instance1"}}}},
18641864
[]corev1.Pod{},
18651865
),
1866-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{
1866+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{
18671867
{ObjectMeta: metav1.ObjectMeta{
18681868
Name: "instance1-abc-def",
18691869
Labels: map[string]string{
@@ -1887,7 +1887,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
18871887
[]appsv1.StatefulSet{},
18881888
[]corev1.Pod{},
18891889
),
1890-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{
1890+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{
18911891
{ObjectMeta: metav1.ObjectMeta{
18921892
Name: "instance1-def-ghi",
18931893
Labels: map[string]string{
@@ -1911,7 +1911,7 @@ func TestFindAvailableInstanceNames(t *testing.T) {
19111911
[]appsv1.StatefulSet{},
19121912
[]corev1.Pod{},
19131913
),
1914-
fakeClusterVolumes: []corev1.PersistentVolumeClaim{{ObjectMeta: metav1.ObjectMeta{
1914+
fakeClusterVolumes: []*corev1.PersistentVolumeClaim{{ObjectMeta: metav1.ObjectMeta{
19151915
Name: "instance1-def-ghi",
19161916
Labels: map[string]string{
19171917
naming.LabelRole: naming.RolePostgresData,

‎internal/controller/postgrescluster/pgbackrest.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1499,7 +1499,7 @@ func (r *Reconciler) reconcilePGBackRest(ctx context.Context,
14991499
// for the PostgresCluster being reconciled using the backups of another PostgresCluster.
15001500
func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context,
15011501
cluster *v1beta1.PostgresCluster, dataSource *v1beta1.PostgresClusterDataSource,
1502-
configHash string, clusterVolumes []corev1.PersistentVolumeClaim,
1502+
configHash string, clusterVolumes []*corev1.PersistentVolumeClaim,
15031503
rootCA *pki.RootCertificateAuthority,
15041504
backupsSpecFound bool,
15051505
) error {
@@ -1663,7 +1663,7 @@ func (r *Reconciler) reconcilePostgresClusterDataSource(ctx context.Context,
16631663
// data source, i.e., S3, etc.
16641664
func (r *Reconciler) reconcileCloudBasedDataSource(ctx context.Context,
16651665
cluster *v1beta1.PostgresCluster, dataSource *v1beta1.PGBackRestDataSource,
1666-
configHash string, clusterVolumes []corev1.PersistentVolumeClaim) error {
1666+
configHash string, clusterVolumes []*corev1.PersistentVolumeClaim) error {
16671667

16681668
// Ensure the proper instance and instance set can be identified via the status. The
16691669
// StartupInstance and StartupInstanceSet values should be populated when the cluster

‎internal/controller/postgrescluster/postgres.go‎

Lines changed: 9 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import (
2020
corev1 "k8s.io/api/core/v1"
2121
"k8s.io/apimachinery/pkg/api/resource"
2222
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
23+
"k8s.io/apimachinery/pkg/labels"
2324
"k8s.io/apimachinery/pkg/util/sets"
2425
"k8s.io/apimachinery/pkg/util/validation/field"
2526
"sigs.k8s.io/controller-runtime/pkg/client"
@@ -569,7 +570,7 @@ func (r *Reconciler) reconcilePostgresUsersInPostgreSQL(
569570
func (r *Reconciler) reconcilePostgresDataVolume(
570571
ctx context.Context, cluster *v1beta1.PostgresCluster,
571572
instanceSpec *v1beta1.PostgresInstanceSetSpec, instance *appsv1.StatefulSet,
572-
clusterVolumes []corev1.PersistentVolumeClaim, sourceCluster *v1beta1.PostgresCluster,
573+
clusterVolumes []*corev1.PersistentVolumeClaim, sourceCluster *v1beta1.PostgresCluster,
573574
) (*corev1.PersistentVolumeClaim, error) {
574575

575576
labelMap := map[string]string{
@@ -581,10 +582,7 @@ func (r *Reconciler) reconcilePostgresDataVolume(
581582
}
582583

583584
var pvc *corev1.PersistentVolumeClaim
584-
existingPVCName, err := getPGPVCName(labelMap, clusterVolumes)
585-
if err != nil {
586-
return nil, errors.WithStack(err)
587-
}
585+
existingPVCName := getPVCName(clusterVolumes, labels.SelectorFromSet(labelMap))
588586
if existingPVCName != "" {
589587
pvc = &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{
590588
Namespace: cluster.GetNamespace(),
@@ -596,7 +594,7 @@ func (r *Reconciler) reconcilePostgresDataVolume(
596594

597595
pvc.SetGroupVersionKind(corev1.SchemeGroupVersion.WithKind("PersistentVolumeClaim"))
598596

599-
err = errors.WithStack(r.setControllerReference(cluster, pvc))
597+
err := errors.WithStack(r.setControllerReference(cluster, pvc))
600598

601599
pvc.Annotations = naming.Merge(
602600
cluster.Spec.Metadata.GetAnnotationsOrNil(),
@@ -726,7 +724,7 @@ func (r *Reconciler) setVolumeSize(ctx context.Context, cluster *v1beta1.Postgre
726724
func (r *Reconciler) reconcileTablespaceVolumes(
727725
ctx context.Context, cluster *v1beta1.PostgresCluster,
728726
instanceSpec *v1beta1.PostgresInstanceSetSpec, instance *appsv1.StatefulSet,
729-
clusterVolumes []corev1.PersistentVolumeClaim,
727+
clusterVolumes []*corev1.PersistentVolumeClaim,
730728
) (tablespaceVolumes []*corev1.PersistentVolumeClaim, err error) {
731729

732730
if !feature.Enabled(ctx, feature.TablespaceVolumes) {
@@ -747,10 +745,7 @@ func (r *Reconciler) reconcileTablespaceVolumes(
747745
}
748746

749747
var pvc *corev1.PersistentVolumeClaim
750-
existingPVCName, err := getPGPVCName(labelMap, clusterVolumes)
751-
if err != nil {
752-
return nil, errors.WithStack(err)
753-
}
748+
existingPVCName := getPVCName(clusterVolumes, labels.SelectorFromSet(labelMap))
754749
if existingPVCName != "" {
755750
pvc = &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{
756751
Namespace: cluster.GetNamespace(),
@@ -799,7 +794,7 @@ func (r *Reconciler) reconcileTablespaceVolumes(
799794
func (r *Reconciler) reconcilePostgresWALVolume(
800795
ctx context.Context, cluster *v1beta1.PostgresCluster,
801796
instanceSpec *v1beta1.PostgresInstanceSetSpec, instance *appsv1.StatefulSet,
802-
observed *Instance, clusterVolumes []corev1.PersistentVolumeClaim,
797+
observed *Instance, clusterVolumes []*corev1.PersistentVolumeClaim,
803798
) (*corev1.PersistentVolumeClaim, error) {
804799

805800
labelMap := map[string]string{
@@ -811,10 +806,7 @@ func (r *Reconciler) reconcilePostgresWALVolume(
811806
}
812807

813808
var pvc *corev1.PersistentVolumeClaim
814-
existingPVCName, err := getPGPVCName(labelMap, clusterVolumes)
815-
if err != nil {
816-
return nil, errors.WithStack(err)
817-
}
809+
existingPVCName := getPVCName(clusterVolumes, labels.SelectorFromSet(labelMap))
818810
if existingPVCName != "" {
819811
pvc = &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{
820812
Namespace: cluster.GetNamespace(),
@@ -872,7 +864,7 @@ func (r *Reconciler) reconcilePostgresWALVolume(
872864
return pvc, err
873865
}
874866

875-
err = errors.WithStack(r.setControllerReference(cluster, pvc))
867+
err := errors.WithStack(r.setControllerReference(cluster, pvc))
876868

877869
pvc.Annotations = naming.Merge(
878870
cluster.Spec.Metadata.GetAnnotationsOrNil(),

‎internal/controller/postgrescluster/snapshots.go‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
batchv1 "k8s.io/api/batch/v1"
1515
corev1 "k8s.io/api/core/v1"
1616
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
17+
"k8s.io/apimachinery/pkg/labels"
1718
"sigs.k8s.io/controller-runtime/pkg/client"
1819

1920
volumesnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
@@ -164,7 +165,7 @@ func (r *Reconciler) reconcileVolumeSnapshots(ctx context.Context,
164165
// after a successful backup.
165166
func (r *Reconciler) reconcileDedicatedSnapshotVolume(
166167
ctx context.Context, cluster *v1beta1.PostgresCluster,
167-
clusterVolumes []corev1.PersistentVolumeClaim,
168+
clusterVolumes []*corev1.PersistentVolumeClaim,
168169
) (*corev1.PersistentVolumeClaim, error) {
169170

170171
// If VolumeSnapshots feature gate is disabled, do nothing and return early.
@@ -181,10 +182,7 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
181182

182183
// If volume already exists, use existing name. Otherwise, generate a name.
183184
var pvc *corev1.PersistentVolumeClaim
184-
existingPVCName, err := getPGPVCName(labelMap, clusterVolumes)
185-
if err != nil {
186-
return nil, errors.WithStack(err)
187-
}
185+
existingPVCName := getPVCName(clusterVolumes, labels.SelectorFromSet(labelMap))
188186
if existingPVCName != "" {
189187
pvc = &corev1.PersistentVolumeClaim{ObjectMeta: metav1.ObjectMeta{
190188
Namespace: cluster.GetNamespace(),
@@ -208,7 +206,7 @@ func (r *Reconciler) reconcileDedicatedSnapshotVolume(
208206

209207
// If we've got this far, snapshots are enabled so we should create/update/get
210208
// the dedicated snapshot volume
211-
pvc, err = r.createDedicatedSnapshotVolume(ctx, cluster, labelMap, pvc)
209+
pvc, err := r.createDedicatedSnapshotVolume(ctx, cluster, labelMap, pvc)
212210
if err != nil {
213211
return pvc, err
214212
}

‎internal/controller/postgrescluster/snapshots_test.go‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -405,7 +405,7 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
405405
assert.Equal(t, len(pvcs.Items), 1)
406406

407407
// Create volumes for reconcile
408-
clusterVolumes := []corev1.PersistentVolumeClaim{*pvc}
408+
clusterVolumes := []*corev1.PersistentVolumeClaim{pvc}
409409

410410
// Reconcile
411411
returned, err := r.reconcileDedicatedSnapshotVolume(ctx, cluster, clusterVolumes)
@@ -434,7 +434,7 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
434434
t.Cleanup(func() { assert.Check(t, r.Client.Delete(ctx, cluster)) })
435435

436436
// Create volumes for reconcile
437-
clusterVolumes := []corev1.PersistentVolumeClaim{}
437+
clusterVolumes := []*corev1.PersistentVolumeClaim{}
438438

439439
// Reconcile
440440
pvc, err := r.reconcileDedicatedSnapshotVolume(ctx, cluster, clusterVolumes)
@@ -480,7 +480,7 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
480480
// Create instance set and volumes for reconcile
481481
sts := &appsv1.StatefulSet{}
482482
generateInstanceStatefulSetIntent(ctx, cluster, &cluster.Spec.InstanceSets[0], "pod-service", "service-account", sts, 1)
483-
clusterVolumes := []corev1.PersistentVolumeClaim{}
483+
clusterVolumes := []*corev1.PersistentVolumeClaim{}
484484

485485
// Reconcile
486486
pvc, err := r.reconcileDedicatedSnapshotVolume(ctx, cluster, clusterVolumes)
@@ -544,7 +544,7 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
544544
// Create instance set and volumes for reconcile
545545
sts := &appsv1.StatefulSet{}
546546
generateInstanceStatefulSetIntent(ctx, cluster, &cluster.Spec.InstanceSets[0], "pod-service", "service-account", sts, 1)
547-
clusterVolumes := []corev1.PersistentVolumeClaim{}
547+
clusterVolumes := []*corev1.PersistentVolumeClaim{}
548548

549549
// Reconcile
550550
pvc, err := r.reconcileDedicatedSnapshotVolume(ctx, cluster, clusterVolumes)
@@ -611,7 +611,7 @@ func TestReconcileDedicatedSnapshotVolume(t *testing.T) {
611611
// Setup instances and volumes for reconcile
612612
sts := &appsv1.StatefulSet{}
613613
generateInstanceStatefulSetIntent(ctx, cluster, &cluster.Spec.InstanceSets[0], "pod-service", "service-account", sts, 1)
614-
clusterVolumes := []corev1.PersistentVolumeClaim{}
614+
clusterVolumes := []*corev1.PersistentVolumeClaim{}
615615

616616
// Reconcile
617617
pvc, err := r.reconcileDedicatedSnapshotVolume(ctx, cluster, clusterVolumes)

0 commit comments

Comments
 (0)