Skip to content

Conversation

noelmcloughlin
Copy link
Contributor

@noelmcloughlin noelmcloughlin commented Feb 10, 2018

This PR allows different "configuration" and "data" directory, resolving some issues-

  1. Ubuntu postgresql-server stores config in /etc, and data into /var. The postgres state fails horribly if /etc/postgresql/../postgresql.conf exists, perhaps after postgres.dropped runs.

  2. User may want separation of data and configuration.

  3. This is current situation where conf_dir variable exists but not data_dir.

codenamemap.yaml:  conf_dir: /etc/postgresql/{{ version }}/main
defaults.yaml:  conf_dir: /var/lib/pgsql/data
osmap.yaml:  conf_dir: /var/lib/postgres/data
osmap.yaml:  conf_dir: /var/lib/pgsql/{{ repo.version }}/data
osmap.yaml:  conf_dir: /usr/local/var/postgres
  1. postgres install always fails after 'dropped' (or remove Add basic postgres.server.remove state #182) states, because conf_dir remains.

  2. If conf_dir and data_dir have same value, pg_ctl throws permissions error.

pg_ctl[8774]: FATAL: data directory "/var/lib/pgsql/data" has group or world access

@@ -70,6 +70,9 @@ postgresql-config-dir:
- makedirs: True
- require:
- cmd: postgresql-cluster-prepared
{%- if postgres.conf_dir == postgres.data_dir %}
- dir_mode: 0700
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good change, and the configuration directory should definitely have to be secured even if does not match the data directory. No need for if statement. Just put it above of the require items as a matter of consistent style.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was reading PG documentation regarding this (due to error in logs). I could not find any instruction that conf_dir needs to be secured. I understand you see no issues using 0700 for conf_dir so agree here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented this. Because it's allowed to have cond_dir and data_dir the same in PG implementations, and 0700 was already requirement, then 0700 applies to conf_dir too. You are correct therefore - the if should be removed.

Copy link
Contributor

@vutny vutny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having ability to define separate configuration and data directory is very good.

However, this new paramenter data_dir has not been used in any state directly. It will have no effect at all if put any random string there, since real data directory will be created with prepare_cluster.command. Internally, the data_dir jinja varible was used to modify prepare_cluster.command for upstream-based installations. Also, if you would like to change real data dir, you will need to make corresponding changes in the postgresql.conf file.

I feel like this PR is incomplete.

@noelmcloughlin
Copy link
Contributor Author

noelmcloughlin commented Feb 16, 2018

The primary PR goal is to fix 0700 permission bug. PG documentation/error complains about data_dir not configuration directory.

In default.yaml the '/var/lib/pgsql/data' string occurs 3 times to avoid Jinja, and be consistent with conf_dir parameter declarations.

In osmap.yaml the data_dir is declared as Jinja, not YAML. I'm fixing the inconsistency cautiously. It is better for everyone to address the YAML first, and SLS in later PR. Researching correct values for osmap.yaml involved significant research into various wiki's and distro docs. Can we just ensure we are happy with YAML first.

@noelmcloughlin
Copy link
Contributor Author

noelmcloughlin commented Feb 19, 2018

In hindsight, its better to do SLS now. Implementation wise, we could remove command and test from this block, and move command/test definition to SLS.

UPDATE: Below is my implementation decision regarding SLS usage of data_dir

prepare_cluster
    pgcommand: initdb --pgdata=
    pstestfile: PG_VERSION

PR updated with full support in YML and SLS files.

@noelmcloughlin
Copy link
Contributor Author

@vutny Please review this again when you have free time. thx

@noelmcloughlin
Copy link
Contributor Author

Testing this today.

@noelmcloughlin
Copy link
Contributor Author

Closing to rebase & submit smaller PRs. Comments here will be adopted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants