Set correct permissions on the specified log directory #4226

cbandy · 2025-07-30T22:19:12Z

Checklist:

Have you added an explanation of what your changes do and why you'd like them to be included?
Have you updated or added documentation for the change, as applicable?
Have you tested your changes on all related environments with successful results, as applicable?
- Have you added automated tests?

Type of Changes:

Bug fix

What is the current behavior (link to any open issues here)?

Controllers ignore the specified "log_directory" when preparing directories for Postgres. This parameter can be specified when the OpenTelemetryLogs gate is disabled.

What is the new behavior (if this is a feature change)?

Controllers create the logs directory before starting Postgres.

Other Information:

Issue: PGO-2558

benjaminjb

So when this is merged we

always run the func to set collector params (but we don't set anything if collector is not on)
create/set perms on the log dir depending on the log dir setting (which may be a relative path, e.g., when default is used)
handle relative paths in our path func

internal/postgres/config.go

cbandy · 2025-07-31T22:25:59Z

internal/postgres/config.go

+		`(` + shell.MakeDirectories(DataDirectory(cluster), logDir) + `) ||`,
+		// FIXME: This error prints the wrong directory when logDir is relative (the default).
+		`halt "$(permissions ` + logDir + ` ||:)"`,


The challenge here is that initdb is only happy populating an empty data directory.

It is happy with this:

drwxrwxr-x. 2 postgres postgres Jul 31 17:04 pgdata/pg16

It is not happy with this:

drwxrwxr-x. 3 postgres postgres Jul 31 17:04 pgdata/pg16 drwxrwxr-x. 2 postgres postgres Jul 31 17:04 pgdata/pg16/log

Is now a time to pull the thread on moving logs out of pgdata completely?

Very much "almost." I'll think about this tomorrow.

cbandy · 2025-08-18T21:54:26Z

This one is ready again. The parameter is still hard-coded when the gate is on, but configurable otherwise.

The next PR after this will be validation and more configurability.

dsessler7

I'm always a little iffy when there's this much bash-in-go, but LGTM

benjaminjb · 2025-08-22T14:24:45Z

internal/postgres/config.go

-			`else (halt Permissions!); fi ||`,
-			`halt "$(permissions "${tablespace_dir}" ||:)"`,
-		}, "\n")
+	mkdirs := make([]string, 0, 9+len(instance.TablespaceVolumes))


I'm missing something -- why 9+?

I see:

postgres_data_directory

tablespaces (the + in that make)

logdir

PatroniPGDataLogPath

PGBackRestPGDataLogPath

I feel weird when doing this. I have internalize "if you know the size before appending, use make" but I'm not finding a source for that.

I know that append one at a time doubles capacity, so from 0 → 9 is five allocations... But who cares, right? gah.

internal/postgres/config.go

benjaminjb · 2025-08-22T14:57:29Z

internal/postgres/config.go

+	if path.IsAbs(logDir) && !strings.HasPrefix(logDir, dataDir) {
+		mkdirs = append(mkdirs,
+			`(`+shell.MakeDirectories(dataMountPath, logDir)+`) ||`,
+			`halt "$(permissions `+shell.QuoteWord(logDir)+` ||:)"`,
+		)
+	} else {
+		mkdirs = append(mkdirs,
+			`[[ ! -f `+shell.QuoteWord(path.Join(dataDir, "PG_VERSION"))+` ]] ||`,
+			`(`+shell.MakeDirectories(dataDir, logDir)+`) ||`,
+			`halt "$(permissions `+shell.QuoteWord(path.Join(dataDir, logDir))+` ||:)"`,


I might need you to walk me through this, but

ll 383-385: if the logdir is absolute and isn't prefixed with datadir, we MakeDirectories with dataMountPath as the base -- but since the logdir abs path won't have the base as part of the path, MakeDirectories kinda ignores the base path/the output will not have any reference to that.

That is: if logdir is outside the datadir, create it with the correct permissions. (Passing "dataDir" into MakeDirectories was throwing me.)

ll 389-392:

we have reached this because either the logdir path is relative or includes the datadir

if the PG_VERSION file exists (and therefore initdb or similar has run?), we can then create our logdir without worrying about it interfering with initdb

Am I missing anything?

So if the version doesn't exist, we haven't run initdb/bootstrap; and then we'll run that at some time, and then re-run this startup command at some time?

ll 383-385: if the logdir is absolute and isn't prefixed with datadir, we MakeDirectories with dataMountPath as the base -- but since the logdir abs path won't have the base as part of the path, MakeDirectories kinda ignores the base path/the output will not have any reference to that.

I'm not following the back half of this. What does base as part of the path mean?

We don't pay attention to which volume the directory is on, true. If the absolute path is /volumes then it does mkdir -p and chmod of one directory.

That is: if logdir is outside the datadir, create it with the correct permissions. (Passing "dataDir" into MakeDirectories was throwing me.)

These lines pass dataMountPath.

Am I missing anything?

So if the version doesn't exist, we haven't run initdb/bootstrap; and then we'll run that at some time, and then re-run this startup command at some time?

That's right, and at some time facts bug me, but all I've got is startup and bash atm.

Ah, yeah, some of what I said was wrong, partly because I was mixing up /pgdata and /pgdata/pg##

So:

if logdir is not in datadir, we create the paths for logdir. And that's true where the log dir is in the pgdata dir or elsewhere. (The real distinction is that the logdir cannot be in /pgdata/pg##.)

and if the logdir is in datadir... yeah, "at some time" is what we've got right now.

The commands for these were intentionally similar, and now they are the same and documented together. Issue: PGO-2558

This function needed defined behavior for relative paths. For example, the default log directory for Postgres is just "log" which it interprets relative to the data directory. Issue: PGO-2558

Controllers ignored the specified "log_directory" when preparing directories for Postgres. This parameter can be specified when the OpenTelemetryLogs gate is disabled. A relative path of more than one directory continues to fail during bootstrap. This does not change what happens when the OpenTelemetryLogs gate is enabled. In that case, controllers override the spec with their own value and prepare that directory. Issue: PGO-2558

benjaminjb reviewed Jul 31, 2025

View reviewed changes

internal/postgres/config.go Show resolved Hide resolved

cbandy commented Jul 31, 2025

View reviewed changes

cbandy force-pushed the postgres-log-directory branch 2 times, most recently from 5756ad3 to eff8b68 Compare August 1, 2025 16:00

cbandy mentioned this pull request Aug 1, 2025

Move pgbackrest-restore test to Kyverno Chainsaw #4228

Merged

5 tasks

benjaminjb force-pushed the postgres-log-directory branch from eff8b68 to 354a00d Compare August 8, 2025 17:54

cbandy force-pushed the postgres-log-directory branch from 354a00d to 9642615 Compare August 18, 2025 21:42

cbandy marked this pull request as ready for review August 18, 2025 21:52

dsessler7 approved these changes Aug 20, 2025

View reviewed changes

benjaminjb reviewed Aug 22, 2025

View reviewed changes

internal/postgres/config.go Outdated Show resolved Hide resolved

benjaminjb reviewed Aug 22, 2025

View reviewed changes

internal/postgres/config.go Show resolved Hide resolved

benjaminjb reviewed Aug 22, 2025

View reviewed changes

cbandy added 4 commits August 22, 2025 15:08

Add a Bash function to create Postgres tablespace and data directories

5c69762

The commands for these were intentionally similar, and now they are the same and documented together. Issue: PGO-2558

Expand relative paths in shell.MakeDirectories

bc2fc7c

This function needed defined behavior for relative paths. For example, the default log directory for Postgres is just "log" which it interprets relative to the data directory. Issue: PGO-2558

FIXUP: more comments around log dir creation

3ac69fe

cbandy force-pushed the postgres-log-directory branch from 9642615 to 3ac69fe Compare August 22, 2025 20:09

benjaminjb approved these changes Aug 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Set correct permissions on the specified log directory #4226

Set correct permissions on the specified log directory #4226

Uh oh!

cbandy commented Jul 30, 2025 •

edited

Loading

Uh oh!

benjaminjb left a comment

Uh oh!

Uh oh!

cbandy Jul 31, 2025

Uh oh!

benjaminjb Aug 1, 2025

Uh oh!

cbandy Aug 18, 2025

Uh oh!

cbandy commented Aug 18, 2025

Uh oh!

dsessler7 left a comment

Uh oh!

benjaminjb Aug 22, 2025

Uh oh!

cbandy Aug 22, 2025

Uh oh!

Uh oh!

Uh oh!

benjaminjb Aug 22, 2025

Uh oh!

cbandy Aug 22, 2025

Uh oh!

benjaminjb Aug 22, 2025

Uh oh!

Uh oh!

Set correct permissions on the specified log directory #4226

Are you sure you want to change the base?

Set correct permissions on the specified log directory #4226

Uh oh!

Conversation

cbandy commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjaminjb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cbandy commented Aug 18, 2025

Uh oh!

dsessler7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cbandy commented Jul 30, 2025 •

edited

Loading