Allow conf/data directory separation #228
Conversation
noelmcloughlin
force-pushed
the
confdata1
branch
4 times, most recently
from
e6b4bee to
24d6900
Compare
Sample output on Ubuntu |
postgres/codenamemap.yaml
Outdated
| {% else %} | ||
| {% set fromrepo = name %} | ||
| {% set conf_dir = defaults.conf_dir %} | ||
| {% set data_dir = defaults.data_dir %} |
There was a problem hiding this comment.
I have checked Ubuntu docs on PostgreSQL, and it appears that "downstream" packages also store all configuration files in /etc as well. Tried to install PG on Debian: same thing there.
So we need to separate conf and data directory for the whole Debian OS family.
There was a problem hiding this comment.
I checked my commits on #187 (which was tested heavily) and you're correct. Fixed.
postgres/codenamemap.yaml
Outdated
| @@ -40,13 +46,19 @@ | |||
| {% if repo.use_upstream_repo == true %} | |||
| {% set fromrepo = repo.fromrepo|default(name ~ '-pgdg', true) %} | |||
| {% set version = repo.version %} | |||
| {% set conf_dir = '/etc/postgresql/' + version|string + '/main' %} | |||
| {% set data_dir = '/var/lib/postgresql/' + version|string + '/main' %} | |||
There was a problem hiding this comment.
Is this really applicable to Fedora? I'm not sure.
There was a problem hiding this comment.
I checked my commits on #187 (which was tested heavily) and you're correct. Not needed. Fixed.
postgres/codenamemap.yaml
Outdated
| @@ -40,13 +46,19 @@ | |||
| {% if repo.use_upstream_repo == true %} | |||
| {% set fromrepo = repo.fromrepo|default(name ~ '-pgdg', true) %} | |||
There was a problem hiding this comment.
I guess I missed this before, but the fromrepo for RedHat family should be equal to pkg_repo.name value (pgdg{{ release }}). This need to be corrected.
postgres/server/init.sls
Outdated
| - name: {{ postgres.prepare_cluster.command }} | ||
| - unless: {{ postgres.prepare_cluster.test }} | ||
| {%- else %} | ||
| - name: {{ postgres.prepare_cluster.pgcommand + ' ' }} {{ postgres.data_dir }} |
There was a problem hiding this comment.
I saw you have reconstructed the pgcommand defaults... I like that.
But just consider to use short option for all platforms except Debians: initdb -D. In such case adding that space in the template would be unnecessary.
There was a problem hiding this comment.
So I can replace {{ postgres.prepare_cluster.pgcommand + ' ' }} with initdb -D for all except Debian?
postgres/defaults.yaml
Outdated
| @@ -15,12 +15,13 @@ postgres: | |||
| group: postgres | |||
|
|
|||
| prepare_cluster: | |||
| command: initdb --pgdata=/var/lib/pgsql/data | |||
| test: test -f /var/lib/pgsql/data/PG_VERSION | |||
| pgcommand: initdb --pgdata= | |||
There was a problem hiding this comment.
Having initdb -D here will simplify your final command for the postgresql-cluster-prepared state.
There was a problem hiding this comment.
Cool!! I'm only realizing now initdb -D and initdb --pgdata= are equilivent. ;-)
postgres/osfamilymap.yaml
Outdated
| command: initdb -D /var/lib/postgres/data | ||
| test: test -f /var/lib/postgres/data/PG_VERSION | ||
| pgcommand: initdb -D | ||
| pgtestfile: PG_VERSION |
There was a problem hiding this comment.
Since these values would exactly match the default ones, no need to override them here. Same for other distros below.
There was a problem hiding this comment.
Good point. I'll fix that. thanks @vutny
noelmcloughlin
force-pushed
the
confdata1
branch
2 times, most recently
from
248cb85 to
9349335
Compare
There was a problem hiding this comment.
@noelmcloughlin This looks promising, just need to finish some cleanup and fixup for Fedora. Thanks!
postgres/codenamemap.yaml
Outdated
| @@ -1,6 +1,7 @@ | |||
| ### Set parameters based on PostgreSQL version supplied with particular distro | |||
|
|
|||
| {% import_yaml "postgres/repo.yaml" as repo %} | |||
| {% import_yaml "postgres/defaults.yaml" as defaults %} | |||
There was a problem hiding this comment.
This line becomes unnecessary.
There was a problem hiding this comment.
yes, thanks.
There was a problem hiding this comment.
ha ha - think I nailed it this time!! ;-)
|
|
||
| {{ codename|default(name, true) }}: | ||
| # PostgreSQL packages are mostly downloaded from `main` repo component | ||
| conf_dir: {{ conf_dir }} | ||
| data_dir: {{ data_dir }} |
There was a problem hiding this comment.
Since these values become global for all Debian derivatives, we should move them to the OS family map.
There was a problem hiding this comment.
The version calculated by oscodename is passed to fedora_codename macro and used ..
{% set conf_dir = '/etc/postgresql/' + version|string + '/main' %}
{% set data_dir = '/var/lib/postgresql/' + version|string + '/main' %}
If version calculated by OS map is different to version calculated here, problems will result? What do you think?
There was a problem hiding this comment.
Oh, you're right, absolutely. I've completely forgot that the version for distro-provided packages is handled here as well. Thanks, keep insisting on right things! 👍
postgres/codenamemap.yaml
Outdated
| @@ -46,7 +48,7 @@ | |||
|
|
|||
| {{ codename|default(name, true) }}: | |||
| # PostgreSQL packages are mostly downloaded from `main` repo component | |||
| fromrepo: {{ name }} | |||
| fromrepo: {{ fromrepo }} | |||
There was a problem hiding this comment.
This really needs to be fixed. The fromrepo for "downstream" installations should be None by default, but for the upstream it should match the pkg_repo.name value across RedHat-based distros. I see that it is already done at upper mapping level in the OS family map. So we can just remove manipulations on fromrepo here.
There was a problem hiding this comment.
Yes, its handled correctly for "upstream" repos in OS family map file. The fromrepo: '' is defaults.yaml should evaluate to None, for Debian it should be name but for Fedora/Redhat what should the "downstream" fromrepo value be?
There was a problem hiding this comment.
I think we should leave that default value and do not handle the fromrepo parameter for downstream at all. That's rarely applicable use-case and would be to complex to handle for now.
There was a problem hiding this comment.
Okay, so fromrepo is not used for Fedora. For Ubuntu I do need to set fromrepo jinja variable to some value, either {% set fromrepo = name %} or {% set fromrepo = '' %} or {% set fromrepo = None %} in codenamemap.yaml. The SLS files will check {% if postgres.fromrepo } so just need to set correct value so condition fails. Is this how you see it too?
noelmcloughlin
force-pushed
the
confdata1
branch
2 times, most recently
from
7b13803 to
194370a
Compare
There was a problem hiding this comment.
@noelmcloughlin Keep up the good work! Just that import of defaults to remove and we go merge this.
|
|
||
| {{ codename|default(name, true) }}: | ||
| # PostgreSQL packages are mostly downloaded from `main` repo component | ||
| conf_dir: {{ conf_dir }} | ||
| data_dir: {{ data_dir }} |
There was a problem hiding this comment.
Oh, you're right, absolutely. I've completely forgot that the version for distro-provided packages is handled here as well. Thanks, keep insisting on right things! 👍
postgres/codenamemap.yaml
Outdated
| {% if repo.use_upstream_repo == true %} | ||
| {% set version = repo.version %} | ||
| {% set fromrepo = repo.fromrepo|default(name ~ '-pgdg', true) %} | ||
| {% else %} | ||
| {% set fromrepo = name %} | ||
| {% endif %} | ||
| {% set conf_dir = '/etc/postgresql/' + version|string + '/main' %} | ||
| {% set data_dir = '/var/lib/postgresql/' + version|string + '/main' %} |
There was a problem hiding this comment.
That's OK, but generally better to use Jinja concatenation operator ~ (tilde) or file.join Salt's function here instead of type casting on variables.
There was a problem hiding this comment.
Forgot about tilde - very useful operand in this case. thx
~
Converts all operands into strings and concatenates them.
postgres/codenamemap.yaml
Outdated
| @@ -1,6 +1,7 @@ | |||
| ### Set parameters based on PostgreSQL version supplied with particular distro | |||
|
|
|||
| {% import_yaml "postgres/repo.yaml" as repo %} | |||
| {% import_yaml "postgres/defaults.yaml" as defaults %} | |||
noelmcloughlin
force-pushed
the
confdata1
branch
2 times, most recently
from
8865637 to
fef23e6
Compare
noelmcloughlin
force-pushed
the
confdata1
branch
from
fef23e6 to
764863f
Compare
|
One question. If we choose non-standard |
There was a problem hiding this comment.
Looks good to me. Thanks @noelmcloughlin
Regarding security context. If AppArmor or SELinux are enforcing policies provided by the distribution, any path or listening port customizations would require modifications of those policies. It is applicable to any service, not just PotgreSQL.
I think our formula has nothing to do about that, because security hardening automation has too much broad scope and very specific to a platform in particular environment. Here we provide a baseline of how do things directly related to PG operations. Everything else could be build on top, with or without the help of Salt.
|
@noelmcloughlin and @vutny merged! |
|
Cool thanks!! |
This PR adds PostgreSQL support for different
data_dirandconf_dirdirectories.Supersedes #187 which listed following advantages-
User may want separation of data and configuration.
Ubuntu postgresql-server stores config in
/etc, and data into/var. Thepostgresstate fails horribly if/etc/postgresql/../postgresql.confexists, perhaps afterpostgres.droppedruns.This is current situation where
conf_dirvariable exists but notdata_dir.postgresinstall always fails after 'dropped' (or remove Add basic postgres.server.remove state #182) states, because conf_dir remains.If
conf_diranddata_dirhave same value,pg_ctlthrows permissions error.pg_ctl[8774]: FATAL: data directory "/var/lib/pgsql/data" has group or world accessBackwards Compatibility: Depreciated
postgres.create_cluster.commandpillar is supported.