Skip to content

Commit f9d9c42

Browse files
dsessler7dsessler7
committed
Add LoggingConfiguration struct for providing log path to pgbackrest processes.
Add ability to send pgbackrest logs to additional volume for instance sidecar, repo host, and backups jobs. Add validation to prevent incorrect log paths or setting pgbackrest log-path via global. Add appropriate unit and integration tests.
1 parent 7e38f44 commit f9d9c42

File tree

20 files changed

+941
-96
lines changed

20 files changed

+941
-96
lines changed

‎config/crd/bases/postgres-operator.crunchydata.com_postgresclusters.yaml‎

Lines changed: 73 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -151,7 +151,7 @@ spec:
151151
properties:
152152
configuration:
153153
description: |-
154-
Projected volumes containing custom pgBackRest configuration. These files are mounted
154+
Projected volumes containing custom pgBackRest configuration. These files are mounted
155155
under "/etc/pgbackrest/conf.d" alongside any pgBackRest configuration generated by the
156156
PostgreSQL Operator:
157157
https://pgbackrest.org/configuration.html
@@ -1424,6 +1424,14 @@ spec:
14241424
x-kubernetes-list-type: atomic
14251425
type: object
14261426
type: object
1427+
log:
1428+
description: Logging configuration for pgbackrest processes
1429+
running in Backup Job Pods.
1430+
properties:
1431+
path:
1432+
maxLength: 256
1433+
type: string
1434+
type: object
14271435
priorityClassName:
14281436
description: |-
14291437
Priority class name for the pgBackRest backup Job pods.
@@ -1583,6 +1591,14 @@ spec:
15831591
x-kubernetes-list-type: map
15841592
type: object
15851593
type: object
1594+
log:
1595+
description: Logging configuration for pgbackrest processes
1596+
running in postgres instance pods.
1597+
properties:
1598+
path:
1599+
maxLength: 256
1600+
type: string
1601+
type: object
15861602
manual:
15871603
description: Defines details for manual pgBackRest backup
15881604
Jobs
@@ -2551,6 +2567,14 @@ spec:
25512567
x-kubernetes-list-type: atomic
25522568
type: object
25532569
type: object
2570+
log:
2571+
description: Logging configuration for pgbackrest processes
2572+
running in the repo host pod.
2573+
properties:
2574+
path:
2575+
maxLength: 256
2576+
type: string
2577+
type: object
25542578
priorityClassName:
25552579
description: |-
25562580
Priority class name for the pgBackRest repo host pod. Changing this value
@@ -4562,6 +4586,21 @@ spec:
45624586
required:
45634587
- repos
45644588
type: object
4589+
x-kubernetes-validations:
4590+
- message: pgbackrest sidecar log path is restricted to an existing
4591+
additional volume
4592+
rule: '!self.?log.path.hasValue() || self.log.path.startsWith("/volumes/")'
4593+
- message: repo host log path is restricted to an existing additional
4594+
volume
4595+
rule: '!self.?repoHost.log.path.hasValue() || self.repoHost.volumes.additional.exists(x,
4596+
self.repoHost.log.path.startsWith("/volumes/"+x.name))'
4597+
- message: backup jobs log path is restricted to an existing additional
4598+
volume
4599+
rule: '!self.?jobs.log.path.hasValue() || self.jobs.volumes.additional.exists(x,
4600+
self.jobs.log.path.startsWith("/volumes/"+x.name))'
4601+
- message: pgbackrest log-path must be set via the various log.path
4602+
fields in the spec
4603+
rule: '!self.?global["log-path"].hasValue()'
45654604
snapshots:
45664605
description: VolumeSnapshot configuration
45674606
properties:
@@ -11209,6 +11248,7 @@ spec:
1120911248
type: object
1121011249
type: array
1121111250
volumes:
11251+
description: Volumes to be added to the instance set.
1121211252
properties:
1121311253
additional:
1121411254
description: Additional pre-existing volumes to add to the
@@ -18416,6 +18456,12 @@ spec:
1841618456
|| !v.startsWith("/volumes") || self.instances.all(i, i.?volumes.additional.hasValue()
1841718457
&& i.volumes.additional.exists(volume, v.startsWith("/volumes/" +
1841818458
volume.name)))).orValue(true)
18459+
- fieldPath: .backups.pgbackrest.log.path
18460+
message: all instances need an additional volume for pgbackrest sidecar
18461+
to log in "/volumes"
18462+
rule: self.?backups.pgbackrest.log.path.optMap(v, !v.startsWith("/volumes")
18463+
|| self.instances.all(i, i.?volumes.additional.hasValue() && i.volumes.additional.exists(volume,
18464+
v.startsWith("/volumes/" + volume.name)))).orValue(true)
1841918465
status:
1842018466
description: PostgresClusterStatus defines the observed state of PostgresCluster
1842118467
properties:
@@ -18951,7 +18997,7 @@ spec:
1895118997
properties:
1895218998
configuration:
1895318999
description: |-
18954-
Projected volumes containing custom pgBackRest configuration. These files are mounted
19000+
Projected volumes containing custom pgBackRest configuration. These files are mounted
1895519001
under "/etc/pgbackrest/conf.d" alongside any pgBackRest configuration generated by the
1895619002
PostgreSQL Operator:
1895719003
https://pgbackrest.org/configuration.html
@@ -20224,6 +20270,14 @@ spec:
2022420270
x-kubernetes-list-type: atomic
2022520271
type: object
2022620272
type: object
20273+
log:
20274+
description: Logging configuration for pgbackrest processes
20275+
running in Backup Job Pods.
20276+
properties:
20277+
path:
20278+
maxLength: 256
20279+
type: string
20280+
type: object
2022720281
priorityClassName:
2022820282
description: |-
2022920283
Priority class name for the pgBackRest backup Job pods.
@@ -20383,6 +20437,14 @@ spec:
2038320437
x-kubernetes-list-type: map
2038420438
type: object
2038520439
type: object
20440+
log:
20441+
description: Logging configuration for pgbackrest processes
20442+
running in postgres instance pods.
20443+
properties:
20444+
path:
20445+
maxLength: 256
20446+
type: string
20447+
type: object
2038620448
manual:
2038720449
description: Defines details for manual pgBackRest backup
2038820450
Jobs
@@ -21351,6 +21413,14 @@ spec:
2135121413
x-kubernetes-list-type: atomic
2135221414
type: object
2135321415
type: object
21416+
log:
21417+
description: Logging configuration for pgbackrest processes
21418+
running in the repo host pod.
21419+
properties:
21420+
path:
21421+
maxLength: 256
21422+
type: string
21423+
type: object
2135421424
priorityClassName:
2135521425
description: |-
2135621426
Priority class name for the pgBackRest repo host pod. Changing this value
@@ -30002,6 +30072,7 @@ spec:
3000230072
type: object
3000330073
type: array
3000430074
volumes:
30075+
description: Volumes to be added to the instance set.
3000530076
properties:
3000630077
additional:
3000730078
description: Additional pre-existing volumes to add to the

‎internal/collector/pgbackrest.go‎

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"context"
99
_ "embed"
1010
"encoding/json"
11-
"fmt"
1211
"slices"
1312

1413
"github.com/crunchydata/postgres-operator/internal/naming"
@@ -25,19 +24,12 @@ func NewConfigForPgBackrestRepoHostPod(
2524
ctx context.Context,
2625
spec *v1beta1.InstrumentationSpec,
2726
repos []v1beta1.PGBackRestRepo,
27+
directory string,
2828
) *Config {
2929
config := NewConfig(spec)
3030

3131
if OpenTelemetryLogsEnabled(ctx, spec) {
3232

33-
var directory string
34-
for _, repo := range repos {
35-
if repo.Volume != nil {
36-
directory = fmt.Sprintf(naming.PGBackRestRepoLogPath, repo.Name)
37-
break
38-
}
39-
}
40-
4133
// We should only enter this function if a PVC is assigned for a dedicated repohost
4234
// but if we don't have one, exit early.
4335
if directory == "" {

‎internal/collector/pgbackrest_test.go‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -30,8 +30,7 @@ func TestNewConfigForPgBackrestRepoHostPod(t *testing.T) {
3030
}
3131
var instrumentation *v1beta1.InstrumentationSpec
3232
require.UnmarshalInto(t, &instrumentation, `{}`)
33-
34-
config := NewConfigForPgBackrestRepoHostPod(ctx, instrumentation, repos)
33+
config := NewConfigForPgBackrestRepoHostPod(ctx, instrumentation, repos, "/test/directory")
3534

3635
result, err := config.ToYAML()
3736
assert.NilError(t, err)
@@ -43,7 +42,7 @@ exporters:
4342
extensions:
4443
file_storage/pgbackrest_logs:
4544
create_directory: false
46-
directory: /pgbackrest/repo1/log/receiver
45+
directory: /test/directory/receiver
4746
fsync: true
4847
processors:
4948
batch/1s:
@@ -101,8 +100,8 @@ processors:
101100
receivers:
102101
filelog/pgbackrest_log:
103102
include:
104-
- /pgbackrest/repo1/log/*.log
105-
- /pgbackrest/repo1/log/*.log.1
103+
- /test/directory/*.log
104+
- /test/directory/*.log.1
106105
multiline:
107106
line_start_pattern: ^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d{3}|^-{19}
108107
storage: file_storage/pgbackrest_logs
@@ -136,8 +135,7 @@ service:
136135
Volume: new(v1beta1.RepoPVC),
137136
},
138137
}
139-
140-
config := NewConfigForPgBackrestRepoHostPod(ctx, testInstrumentationSpec(), repos)
138+
config := NewConfigForPgBackrestRepoHostPod(ctx, testInstrumentationSpec(), repos, "/another/directory")
141139

142140
result, err := config.ToYAML()
143141
assert.NilError(t, err)
@@ -153,7 +151,7 @@ exporters:
153151
extensions:
154152
file_storage/pgbackrest_logs:
155153
create_directory: false
156-
directory: /pgbackrest/repo1/log/receiver
154+
directory: /another/directory/receiver
157155
fsync: true
158156
processors:
159157
batch/1s:
@@ -211,8 +209,8 @@ processors:
211209
receivers:
212210
filelog/pgbackrest_log:
213211
include:
214-
- /pgbackrest/repo1/log/*.log
215-
- /pgbackrest/repo1/log/*.log.1
212+
- /another/directory/*.log
213+
- /another/directory/*.log.1
216214
multiline:
217215
line_start_pattern: ^\d{4}-\d{2}-\d{2} \d{2}:\d{2}:\d{2}\.\d{3}|^-{19}
218216
storage: file_storage/pgbackrest_logs

‎internal/collector/postgres.go‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import (
1616

1717
"github.com/crunchydata/postgres-operator/internal/naming"
1818
"github.com/crunchydata/postgres-operator/internal/postgres"
19+
"github.com/crunchydata/postgres-operator/internal/util"
1920
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
2021
)
2122

@@ -244,8 +245,9 @@ func EnablePostgresLogging(
244245
}
245246

246247
// pgBackRest pipeline
248+
pgBackRestLogPath := util.GetPGBackRestLogPathForInstance(inCluster)
247249
outConfig.Extensions["file_storage/pgbackrest_logs"] = map[string]any{
248-
"directory": naming.PGBackRestPGDataLogPath + "/receiver",
250+
"directory": pgBackRestLogPath + "/receiver",
249251
"create_directory": false,
250252
"fsync": true,
251253
}
@@ -258,8 +260,8 @@ func EnablePostgresLogging(
258260
// a log record or two to the old file while rotation is occurring.
259261
// The collector knows not to create duplicate logs.
260262
"include": []string{
261-
naming.PGBackRestPGDataLogPath + "/*.log",
262-
naming.PGBackRestPGDataLogPath + "/*.log.1",
263+
pgBackRestLogPath + "/*.log",
264+
pgBackRestLogPath + "/*.log.1",
263265
},
264266
"storage": "file_storage/pgbackrest_logs",
265267

‎internal/controller/postgrescluster/instance.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1205,7 +1205,7 @@ func (r *Reconciler) reconcileInstance(
12051205
// TODO(sidecar): Create these directories sometime other than startup.
12061206
collector.AddToPod(ctx, cluster.Spec.Instrumentation, cluster.Spec.ImagePullPolicy, instanceConfigMap, &instance.Spec.Template,
12071207
[]corev1.VolumeMount{postgres.DataVolumeMount()}, pgPassword,
1208-
[]string{naming.PGBackRestPGDataLogPath}, includeLogrotate, true)
1208+
[]string{util.GetPGBackRestLogPathForInstance(cluster)}, includeLogrotate, true)
12091209
}
12101210

12111211
// Add postgres-exporter to the instance Pod spec
@@ -1433,7 +1433,7 @@ func (r *Reconciler) reconcileInstanceConfigMap(
14331433
collector.AddLogrotateConfigs(ctx, cluster.Spec.Instrumentation,
14341434
instanceConfigMap,
14351435
[]collector.LogrotateConfig{{
1436-
LogFiles: []string{naming.PGBackRestPGDataLogPath + "/*.log"},
1436+
LogFiles: []string{util.GetPGBackRestLogPathForInstance(cluster) + "/*.log"},
14371437
}})
14381438
}
14391439
}

‎internal/controller/postgrescluster/pgbackrest.go‎

Lines changed: 32 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"context"
99
"fmt"
1010
"io"
11+
"path/filepath"
1112
"reflect"
1213
"regexp"
1314
"sort"
@@ -38,6 +39,7 @@ import (
3839
"github.com/crunchydata/postgres-operator/internal/pgbackrest"
3940
"github.com/crunchydata/postgres-operator/internal/pki"
4041
"github.com/crunchydata/postgres-operator/internal/postgres"
42+
"github.com/crunchydata/postgres-operator/internal/shell"
4143
"github.com/crunchydata/postgres-operator/internal/util"
4244
"github.com/crunchydata/postgres-operator/pkg/apis/postgres-operator.crunchydata.com/v1beta1"
4345
)
@@ -821,7 +823,13 @@ func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCl
821823
{Name: "SELECTOR", Value: naming.PGBackRestDedicatedSelector(postgresCluster.GetName()).String()},
822824
}
823825
} else {
824-
container.Command = []string{"/bin/pgbackrest", "backup"}
826+
mkdirCommand := ""
827+
cloudLogPath := getCloudLogPath(postgresCluster)
828+
if cloudLogPath != "" {
829+
mkdirCommand += shell.MakeDirectories(cloudLogPath, cloudLogPath) + "; "
830+
}
831+
832+
container.Command = []string{"sh", "-c", "--", mkdirCommand + `exec "$@"`, "--", "/bin/pgbackrest", "backup"}
825833
container.Command = append(container.Command, cmdOpts...)
826834
}
827835

@@ -885,8 +893,8 @@ func (r *Reconciler) generateBackupJobSpecIntent(ctx context.Context, postgresCl
885893
pgbackrest.AddConfigToCloudBackupJob(postgresCluster, &jobSpec.Template)
886894

887895
// Mount the PVC named in the "pgbackrest-cloud-log-volume" annotation, if any.
888-
if logVolumeName := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolumeName != "" {
889-
util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolumeName)
896+
if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
897+
util.AddCloudLogVolumeToPod(&jobSpec.Template.Spec, logVolume)
890898
}
891899
}
892900

@@ -2075,28 +2083,7 @@ func (r *Reconciler) reconcilePGBackRestConfig(ctx context.Context,
20752083
repoHostName, configHash, serviceName, serviceNamespace string,
20762084
instanceNames []string) error {
20772085

2078-
// If the user has specified a PVC to use as a log volume for cloud backups via the
2079-
// PGBackRestCloudLogVolume annotation, check for the PVC. If we find it, set the cloud
2080-
// log path. If the user has specified a PVC, but we can't find it, create a warning event.
2081-
cloudLogPath := ""
2082-
if logVolumeName := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolumeName != "" {
2083-
logVolume := &corev1.PersistentVolumeClaim{
2084-
ObjectMeta: metav1.ObjectMeta{
2085-
Name: logVolumeName,
2086-
Namespace: postgresCluster.GetNamespace(),
2087-
},
2088-
}
2089-
err := errors.WithStack(r.Client.Get(ctx,
2090-
client.ObjectKeyFromObject(logVolume), logVolume))
2091-
if err != nil {
2092-
// PVC not retrieved, create warning event
2093-
r.Recorder.Event(postgresCluster, corev1.EventTypeWarning,
2094-
"PGBackRestCloudLogVolumeNotFound", err.Error())
2095-
} else {
2096-
// We successfully found the specified PVC, so we will set the log path
2097-
cloudLogPath = "/volumes/" + logVolumeName
2098-
}
2099-
}
2086+
cloudLogPath := getCloudLogPath(postgresCluster)
21002087

21012088
backrestConfig, err := pgbackrest.CreatePGBackRestConfigMapIntent(ctx, postgresCluster, repoHostName,
21022089
configHash, serviceName, serviceNamespace, cloudLogPath, instanceNames)
@@ -3351,3 +3338,23 @@ func authorizeBackupRemovalAnnotationPresent(postgresCluster *v1beta1.PostgresCl
33513338
}
33523339
return false
33533340
}
3341+
3342+
// getCloudLogPath is responsible for determining the appropriate log path for pgbackrest
3343+
// in cloud backup jobs. If the user has specified a PVC to use as a log volume for cloud
3344+
// backups via the PGBackRestCloudLogVolume annotation, set the cloud log path accordingly.
3345+
// If the user has not set the PGBackRestCloudLogVolume annotation, but has set a log path
3346+
// via the spec, use that.
3347+
// TODO: Make sure this is what we want (i.e. annotation to take precedence over spec)
3348+
//
3349+
// This function assumes that the backups/pgbackrest spec is present in postgresCluster.
3350+
func getCloudLogPath(postgresCluster *v1beta1.PostgresCluster) string {
3351+
cloudLogPath := ""
3352+
if logVolume := postgresCluster.Annotations[naming.PGBackRestCloudLogVolume]; logVolume != "" {
3353+
cloudLogPath = "/volumes/" + logVolume
3354+
} else if postgresCluster.Spec.Backups.PGBackRest.Jobs != nil &&
3355+
postgresCluster.Spec.Backups.PGBackRest.Jobs.Log != nil &&
3356+
postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path != "" {
3357+
cloudLogPath = filepath.Clean(postgresCluster.Spec.Backups.PGBackRest.Jobs.Log.Path)
3358+
}
3359+
return cloudLogPath
3360+
}

0 commit comments

Comments
 (0)