Rotate pgbackrest #4108
Rotate pgbackrest #4108
Conversation
internal/collector/instance.go
Outdated
| var mkdirScript string | ||
| if dir != "" { | ||
| mkdirScript = ` | ||
| ` + shell.MakeDirectories(0o775, dir, dir+"/receiver") |
There was a problem hiding this comment.
We had a few TODOs about creating the receiver directory through the pod, not through the collector, so I did this.
There was a problem hiding this comment.
In that case, I believe we need to be able to pass in multiple directories (e.g. the collector container in the postgres instance pod needs to create directories for patroni, postgres, and pgbackrest)
There was a problem hiding this comment.
Yeah, I was wondering if the logrotate struct might help with that... I was thinking to work some of that out in the pgadmin PR
|
|
||
| // Logging | ||
| EnablePostgresLogging(ctx, inCluster, config, outParameters) | ||
| EnablePatroniLogging(ctx, inCluster, config) |
There was a problem hiding this comment.
Just re-ordering and grouping
|
|
||
| if err == nil && | ||
| (feature.Enabled(ctx, feature.OpenTelemetryLogs) && !feature.Enabled(ctx, feature.OpenTelemetryMetrics)) { | ||
| if err == nil { |
There was a problem hiding this comment.
@dsessler7 We had talked about reworking this logic a bit along these lines, here's my pass at it.
Note: this work is done with the understanding that we're not rotating postgres or patroni logs through logrotate right now.
| monitoringUserSecret := &corev1.Secret{ObjectMeta: naming.MonitoringUserSecret(cluster)} | ||
| err = errors.WithStack( | ||
| r.Client.Get(ctx, client.ObjectKeyFromObject(monitoringUserSecret), monitoringUserSecret)) | ||
| // FIXME: is this bool really asking for a backup vol? |
There was a problem hiding this comment.
Yeah, gotta figure this out tomorrow
There was a problem hiding this comment.
Looked into this -- it's not actually looking for a vol, just that backups are in the spec, so I'll use pgbackrest.RepoHostVolumeDefined
| } | ||
| template.Spec.InitContainers = append(template.Spec.InitContainers, container) | ||
|
|
||
| return pgBackRestLogPath |
There was a problem hiding this comment.
I got tired of looping through repos to get this name, so I had this func return it since I needed this name right after this func was called. I could've done that work outside this func or create a util, maybe I'll consider that tomorrow.
| includeLogrotate bool, | ||
| ) { | ||
| if !(feature.Enabled(ctx, feature.OpenTelemetryLogs) || feature.Enabled(ctx, feature.OpenTelemetryMetrics)) { | ||
| if spec == nil || |
There was a problem hiding this comment.
So we want to require the instrumentation spec, then?
There was a problem hiding this comment.
I think so, just like you said at another point: this is all feature gated, but turning on a feature gate doesn't necessarily mean you want X feature for all your clusters.
benjaminjb
force-pushed
the
benjb/pgbackrest-rotate
branch
from
7d2792d to
85d9046
Compare
Issues: [PGO-2171]
benjaminjb
force-pushed
the
benjb/pgbackrest-rotate
branch
from
85d9046 to
01ca6bb
Compare
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
PgBackRest logs don't rotate
What is the new behavior (if this is a feature change)?
PgBackRest logs -- in both postgres and repohost -- rotate.
This also includes a change where the collector container can create the
/receiverdir that it requires. (This will need to be updated if/when we have multiple logrotates in a container.)Other Information:
Issues: [PGO-2171]