Set correct permissions on the specified log directory #4226
Conversation
There was a problem hiding this comment.
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
| return fmt.Sprintf("%s/logs/postgres", dataMountPath) | ||
| // DataStorage returns the absolute path to the disk where cluster stores its data. | ||
| // Use [DataDirectory] for the exact directory that Postgres uses. | ||
| func DataStorage(cluster *v1beta1.PostgresCluster) string { |
There was a problem hiding this comment.
Why pass in the cluster?
There was a problem hiding this comment.
Hmm. 😬 Future-proofing, I guess. Something about being a constant does sit right with me, but it absolutely is a constant.
If I remove the arguments, should I also remove the function? Export the constant?
There was a problem hiding this comment.
🤔 I think I want things outside this package to not treat it as a constant, even though it is constant right now. Does that English make sense?
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. 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
internal/postgres/config.go
Outdated
| `(` + shell.MakeDirectories(DataDirectory(cluster), logDir) + `) ||`, | ||
| // FIXME: This error prints the wrong directory when logDir is relative (the default). | ||
| `halt "$(permissions ` + logDir + ` ||:)"`, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Is now a time to pull the thread on moving logs out of pgdata completely?
cbandy
force-pushed
the
postgres-log-directory
branch
from
d1ef339 to
5756ad3
Compare
cbandy
force-pushed
the
postgres-log-directory
branch
from
5756ad3 to
eff8b68
Compare
Checklist:
Type of Changes:
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