Skip to content

Commit cc71591

Browse files
dsessler7dsessler7
committed
Adjust cloud backup volume behavior such that additional volumes take precedence when there is a naming collision with the pgbackrest-cloud-log-volume annotation. If annotation is present and no log path is given via the spec, use the log path that the annotation logic yields. Add/adjust tests appropriately.
1 parent ed15859 commit cc71591

File tree

2 files changed

+342
-13
lines changed

2 files changed

+342
-13
lines changed

‎internal/controller/postgrescluster/pgbackrest.go‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -892,9 +892,29 @@ func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCl
892892
jobSpec.Template.Spec.SecurityContext = postgres.PodSecurityContext(postgresCluster)
893893
pgbackrest.AddConfigToCloudBackupJob(postgresCluster, &jobSpec.Template)
894894

895-
// Mount the PVC named in the "pgbackrest-cloud-log-volume" annotation, if any.
895+
// If the "pgbackrest-cloud-log-volume" annotation has a value, check if it is the
896+
// same as any of the additional volume names. If there is a collision of names,
897+
// create a warning event. If there is no name collision, mount the volume referenced
898+
// by the annotation.
896899
if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
897-
util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolume)
900+
var collisionFound bool
901+
if jobs != nil && jobs.Volumes != nil {
902+
for _, volume := range jobs.Volumes.Additional {
903+
if volume.Name == logVolume {
904+
collisionFound = true
905+
r.Recorder.Event(postgresCluster, corev1.EventTypeWarning,
906+
"DuplicateCloudBackupVolume", "The volume name specified in the "+
907+
"pgbackrest-cloud-log-volume annotation is the same as one "+
908+
"specified in spec.backups.pgbackrest.jobs.volumes.additional. "+
909+
"Cannot mount duplicate volume names. Defaulting to the "+
910+
"additional volume.")
911+
break
912+
}
913+
}
914+
}
915+
if !collisionFound {
916+
util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolume)
917+
}
898918
}
899919
}
900920

@@ -3346,21 +3366,22 @@ func authorizeBackupRemovalAnnotationPresent(postgresCluster *v1beta1.PostgresCl
33463366
}
33473367

33483368
// getCloudLogPath is responsible for determining the appropriate log path for pgbackrest
3349-
// in cloud backup jobs. If the user has specified a PVC to use as a log volume for cloud
3350-
// backups via the PGBackRestCloudLogVolume annotation, set the cloud log path accordingly.
3351-
// If the user has not set the PGBackRestCloudLogVolume annotation, but has set a log path
3352-
// via the spec, use that.
3353-
// TODO: Make sure this is what we want (i.e. annotation to take precedence over spec)
3369+
// in cloud backup jobs. If the user specified a log path via the spec, use it. Otherwise,
3370+
// if the user specified a log volume for cloud backups via the PGBackRestCloudLogVolume
3371+
// annotation, we will use that. If neither scenario is true, return an empty string.
33543372
//
33553373
// This function assumes that the backups/pgbackrest spec is present in postgresCluster.
33563374
func getCloudLogPath(postgresCluster *v1beta1.PostgresCluster) string {
33573375
cloudLogPath := ""
3358-
if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
3359-
cloudLogPath = "/volumes/" + logVolume
3360-
} else if postgresCluster.Spec.Backups.PGBackRest.Jobs != nil &&
3376+
if postgresCluster.Spec.Backups.PGBackRest.Jobs != nil &&
33613377
postgresCluster.Spec.Backups.PGBackRest.Jobs.Log != nil &&
33623378
postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path != "" {
3379+
// TODO: I know it should be caught by CEL validation, but is it worthwhile to also
3380+
// check that Log.Path ~= "/volumes/" + existingAdditionalVolume.name here??
3381+
33633382
cloudLogPath = filepath.Clean(postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path)
3383+
} else if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
3384+
cloudLogPath = "/volumes/" + logVolume
33643385
}
33653386
return cloudLogPath
33663387
}

‎internal/controller/postgrescluster/pgbackrest_test.go‎

Lines changed: 311 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3130,6 +3130,312 @@ volumes:
31303130
// No events created
31313131
assert.Equal(t, len(recorder.Events), 0)
31323132
})
3133+
3134+
t.Run("AdditionalVolumesMissingContainers", func(t *testing.T) {
3135+
recorder := events.NewRecorder(t, runtime.Scheme)
3136+
r.Recorder = recorder
3137+
3138+
cluster := cluster.DeepCopy()
3139+
cluster.Namespace = ns.Name
3140+
cluster.Annotations = map[string]string{}
3141+
cluster.Spec.Backups.PGBackRest.Jobs = &v1beta1.BackupJobs{
3142+
Log: &v1beta1.LoggingConfiguration{
3143+
Path: "/volumes/stuff/log",
3144+
},
3145+
Volumes: &v1beta1.PGBackRestVolumesSpec{
3146+
Additional: []v1beta1.AdditionalVolume{
3147+
{
3148+
ClaimName: "additional-pvc",
3149+
Containers: []v1beta1.DNS1123Label{
3150+
"pgbackrest",
3151+
"non-existent-container",
3152+
},
3153+
Name: "stuff",
3154+
},
3155+
},
3156+
},
3157+
}
3158+
3159+
spec := r.generateBackupJobSpecIntent(ctx,
3160+
cluster, v1beta1.PGBackRestRepo{},
3161+
"",
3162+
nil, nil,
3163+
)
3164+
3165+
assert.Assert(t, cmp.MarshalMatches(spec.Template.Spec, `
3166+
containers:
3167+
- command:
3168+
- sh
3169+
- -c
3170+
- --
3171+
- mkdir -p '/volumes/stuff/log' && { chmod 0775 '/volumes/stuff/log' || :; }; exec
3172+
"$@"
3173+
- --
3174+
- /bin/pgbackrest
3175+
- backup
3176+
- --stanza=db
3177+
- --repo=
3178+
name: pgbackrest
3179+
resources: {}
3180+
securityContext:
3181+
allowPrivilegeEscalation: false
3182+
capabilities:
3183+
drop:
3184+
- ALL
3185+
privileged: false
3186+
readOnlyRootFilesystem: true
3187+
runAsNonRoot: true
3188+
seccompProfile:
3189+
type: RuntimeDefault
3190+
volumeMounts:
3191+
- mountPath: /etc/pgbackrest/conf.d
3192+
name: pgbackrest-config
3193+
readOnly: true
3194+
- mountPath: /tmp
3195+
name: tmp
3196+
- mountPath: /volumes/stuff
3197+
name: volumes-stuff
3198+
enableServiceLinks: false
3199+
restartPolicy: Never
3200+
securityContext:
3201+
fsGroup: 26
3202+
fsGroupChangePolicy: OnRootMismatch
3203+
volumes:
3204+
- name: pgbackrest-config
3205+
projected:
3206+
sources:
3207+
- configMap:
3208+
items:
3209+
- key: pgbackrest_cloud.conf
3210+
path: pgbackrest_cloud.conf
3211+
name: hippo-test-pgbackrest-config
3212+
- secret:
3213+
items:
3214+
- key: pgbackrest.ca-roots
3215+
path: ~postgres-operator/tls-ca.crt
3216+
- key: pgbackrest-client.crt
3217+
path: ~postgres-operator/client-tls.crt
3218+
- key: pgbackrest-client.key
3219+
mode: 384
3220+
path: ~postgres-operator/client-tls.key
3221+
name: hippo-test-pgbackrest
3222+
- emptyDir:
3223+
sizeLimit: 16Mi
3224+
name: tmp
3225+
- name: volumes-stuff
3226+
persistentVolumeClaim:
3227+
claimName: additional-pvc`))
3228+
3229+
// Missing containers warning event created
3230+
assert.Equal(t, len(recorder.Events), 1)
3231+
assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
3232+
assert.Equal(t, recorder.Events[0].Reason, "SpecifiedContainerNotFound")
3233+
assert.Equal(t, recorder.Events[0].Note, "The following Backup Job Pod "+
3234+
"containers were specified for additional volumes but cannot be "+
3235+
"found: [non-existent-container].")
3236+
})
3237+
3238+
t.Run("AnnotationAndAdditionalVolumeWithPath", func(t *testing.T) {
3239+
recorder := events.NewRecorder(t, runtime.Scheme)
3240+
r.Recorder = recorder
3241+
3242+
cluster := cluster.DeepCopy()
3243+
cluster.Namespace = ns.Name
3244+
cluster.Annotations = map[string]string{}
3245+
cluster.Annotations[naming.PGBackRestCloudLogVolume] = "stuff"
3246+
3247+
cluster.Spec.Backups.PGBackRest.Jobs = &v1beta1.BackupJobs{
3248+
Log: &v1beta1.LoggingConfiguration{
3249+
Path: "/volumes/stuff/log",
3250+
},
3251+
Volumes: &v1beta1.PGBackRestVolumesSpec{
3252+
Additional: []v1beta1.AdditionalVolume{
3253+
{
3254+
ClaimName: "additional-pvc",
3255+
Name: "stuff",
3256+
},
3257+
},
3258+
},
3259+
}
3260+
3261+
spec := r.generateBackupJobSpecIntent(ctx,
3262+
cluster, v1beta1.PGBackRestRepo{},
3263+
"",
3264+
nil, nil,
3265+
)
3266+
3267+
assert.Assert(t, cmp.MarshalMatches(spec.Template.Spec, `
3268+
containers:
3269+
- command:
3270+
- sh
3271+
- -c
3272+
- --
3273+
- mkdir -p '/volumes/stuff/log' && { chmod 0775 '/volumes/stuff/log' || :; }; exec
3274+
"$@"
3275+
- --
3276+
- /bin/pgbackrest
3277+
- backup
3278+
- --stanza=db
3279+
- --repo=
3280+
name: pgbackrest
3281+
resources: {}
3282+
securityContext:
3283+
allowPrivilegeEscalation: false
3284+
capabilities:
3285+
drop:
3286+
- ALL
3287+
privileged: false
3288+
readOnlyRootFilesystem: true
3289+
runAsNonRoot: true
3290+
seccompProfile:
3291+
type: RuntimeDefault
3292+
volumeMounts:
3293+
- mountPath: /etc/pgbackrest/conf.d
3294+
name: pgbackrest-config
3295+
readOnly: true
3296+
- mountPath: /tmp
3297+
name: tmp
3298+
- mountPath: /volumes/stuff
3299+
name: volumes-stuff
3300+
enableServiceLinks: false
3301+
restartPolicy: Never
3302+
securityContext:
3303+
fsGroup: 26
3304+
fsGroupChangePolicy: OnRootMismatch
3305+
volumes:
3306+
- name: pgbackrest-config
3307+
projected:
3308+
sources:
3309+
- configMap:
3310+
items:
3311+
- key: pgbackrest_cloud.conf
3312+
path: pgbackrest_cloud.conf
3313+
name: hippo-test-pgbackrest-config
3314+
- secret:
3315+
items:
3316+
- key: pgbackrest.ca-roots
3317+
path: ~postgres-operator/tls-ca.crt
3318+
- key: pgbackrest-client.crt
3319+
path: ~postgres-operator/client-tls.crt
3320+
- key: pgbackrest-client.key
3321+
mode: 384
3322+
path: ~postgres-operator/client-tls.key
3323+
name: hippo-test-pgbackrest
3324+
- emptyDir:
3325+
sizeLimit: 16Mi
3326+
name: tmp
3327+
- name: volumes-stuff
3328+
persistentVolumeClaim:
3329+
claimName: additional-pvc`))
3330+
3331+
// Annotation/additional volume collision warning event created
3332+
assert.Equal(t, len(recorder.Events), 1)
3333+
assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
3334+
assert.Equal(t, recorder.Events[0].Reason, "DuplicateCloudBackupVolume")
3335+
assert.Equal(t, recorder.Events[0].Note, "The volume name specified in "+
3336+
"the pgbackrest-cloud-log-volume annotation is the same as one "+
3337+
"specified in spec.backups.pgbackrest.jobs.volumes.additional. Cannot "+
3338+
"mount duplicate volume names. Defaulting to the additional volume.")
3339+
})
3340+
3341+
t.Run("AnnotationAndAdditionalVolumeNoPath", func(t *testing.T) {
3342+
recorder := events.NewRecorder(t, runtime.Scheme)
3343+
r.Recorder = recorder
3344+
3345+
cluster := cluster.DeepCopy()
3346+
cluster.Namespace = ns.Name
3347+
cluster.Annotations = map[string]string{}
3348+
cluster.Annotations[naming.PGBackRestCloudLogVolume] = "stuff"
3349+
3350+
cluster.Spec.Backups.PGBackRest.Jobs = &v1beta1.BackupJobs{
3351+
Volumes: &v1beta1.PGBackRestVolumesSpec{
3352+
Additional: []v1beta1.AdditionalVolume{
3353+
{
3354+
ClaimName: "additional-pvc",
3355+
Name: "stuff",
3356+
},
3357+
},
3358+
},
3359+
}
3360+
3361+
spec := r.generateBackupJobSpecIntent(ctx,
3362+
cluster, v1beta1.PGBackRestRepo{},
3363+
"",
3364+
nil, nil,
3365+
)
3366+
3367+
assert.Assert(t, cmp.MarshalMatches(spec.Template.Spec, `
3368+
containers:
3369+
- command:
3370+
- sh
3371+
- -c
3372+
- --
3373+
- mkdir -p '/volumes/stuff' && { chmod 0775 '/volumes/stuff' || :; }; exec "$@"
3374+
- --
3375+
- /bin/pgbackrest
3376+
- backup
3377+
- --stanza=db
3378+
- --repo=
3379+
name: pgbackrest
3380+
resources: {}
3381+
securityContext:
3382+
allowPrivilegeEscalation: false
3383+
capabilities:
3384+
drop:
3385+
- ALL
3386+
privileged: false
3387+
readOnlyRootFilesystem: true
3388+
runAsNonRoot: true
3389+
seccompProfile:
3390+
type: RuntimeDefault
3391+
volumeMounts:
3392+
- mountPath: /etc/pgbackrest/conf.d
3393+
name: pgbackrest-config
3394+
readOnly: true
3395+
- mountPath: /tmp
3396+
name: tmp
3397+
- mountPath: /volumes/stuff
3398+
name: volumes-stuff
3399+
enableServiceLinks: false
3400+
restartPolicy: Never
3401+
securityContext:
3402+
fsGroup: 26
3403+
fsGroupChangePolicy: OnRootMismatch
3404+
volumes:
3405+
- name: pgbackrest-config
3406+
projected:
3407+
sources:
3408+
- configMap:
3409+
items:
3410+
- key: pgbackrest_cloud.conf
3411+
path: pgbackrest_cloud.conf
3412+
name: hippo-test-pgbackrest-config
3413+
- secret:
3414+
items:
3415+
- key: pgbackrest.ca-roots
3416+
path: ~postgres-operator/tls-ca.crt
3417+
- key: pgbackrest-client.crt
3418+
path: ~postgres-operator/client-tls.crt
3419+
- key: pgbackrest-client.key
3420+
mode: 384
3421+
path: ~postgres-operator/client-tls.key
3422+
name: hippo-test-pgbackrest
3423+
- emptyDir:
3424+
sizeLimit: 16Mi
3425+
name: tmp
3426+
- name: volumes-stuff
3427+
persistentVolumeClaim:
3428+
claimName: additional-pvc`))
3429+
3430+
// Annotation/additional volume collision warning event created
3431+
assert.Equal(t, len(recorder.Events), 1)
3432+
assert.Equal(t, recorder.Events[0].Regarding.Name, cluster.Name)
3433+
assert.Equal(t, recorder.Events[0].Reason, "DuplicateCloudBackupVolume")
3434+
assert.Equal(t, recorder.Events[0].Note, "The volume name specified in "+
3435+
"the pgbackrest-cloud-log-volume annotation is the same as one "+
3436+
"specified in spec.backups.pgbackrest.jobs.volumes.additional. Cannot "+
3437+
"mount duplicate volume names. Defaulting to the additional volume.")
3438+
})
31333439
}
31343440

31353441
func TestGenerateRepoHostIntent(t *testing.T) {
@@ -4599,15 +4905,17 @@ func TestGetCloudLogPath(t *testing.T) {
45994905
Spec: v1beta1.PostgresClusterSpec{
46004906
Backups: v1beta1.Backups{
46014907
PGBackRest: v1beta1.PGBackRestArchive{
4602-
Log: &v1beta1.LoggingConfiguration{
4603-
Path: "/volumes/test/log",
4908+
Jobs: &v1beta1.BackupJobs{
4909+
Log: &v1beta1.LoggingConfiguration{
4910+
Path: "/volumes/test/log/",
4911+
},
46044912
},
46054913
},
46064914
},
46074915
},
46084916
}
46094917
postgrescluster.Annotations = map[string]string{}
46104918
postgrescluster.Annotations[naming.PGBackRestCloudLogVolume] = "another-pvc"
4611-
assert.Equal(t, getCloudLogPath(postgrescluster), "/volumes/another-pvc")
4919+
assert.Equal(t, getCloudLogPath(postgrescluster), "/volumes/test/log")
46124920
})
46134921
}

0 commit comments

Comments
 (0)