Skip to content

Fix conf_dir => data_dir required for preparing cluster #231

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Jun 21, 2018

Conversation

myii
Copy link
Contributor

@myii myii commented Jun 21, 2018

@noelmcloughlin @vutny Thanks for #228, that came at the right time for me as well. However, this fix is necessary, since the data_dir needs to be in place before preparing the cluster.

I've only done basic testing but there also seems to be an issue with starting the server -- otherwise the postgres.manage states fail. I resolved this by adding --start to the pg_createcluster command. What are your thoughts about this?

@myii
Copy link
Contributor Author

myii commented Jun 21, 2018

OK, I've just rebased the commit because the regression in #228 is:

  • runas: {{ postgres.prepare_cluster.user }} changed to {{ postgres.user }}

First error encountered, prompting this PR (Ubuntu minion):

stderr:
    install: cannot create directory '/var/lib/postgresql': Permission denied
    Error: could not create data directory; you might need to run this program with root privileges

Another error, even after the initial fix (fresh PostgreSQL installation):

stderr:
    Error: could not create log directory; you might need to run this program with root privileges

@myii
Copy link
Contributor Author

myii commented Jun 21, 2018

I've just added another commit in relation to the server starting before postgres.manage. Simply taking advantage of the service.restart by moving it out of the if block (for postgresql.conf changes) and adding the watch_in.

@@ -62,7 +62,7 @@ postgresql-{{ bin }}-altinstall:

postgresql-cluster-prepared:
file.directory:
- name: {{ postgres.conf_dir }}
- name: {{ postgres.data_dir }}
Copy link
Contributor

@noelmcloughlin noelmcloughlin Jun 21, 2018

Choose a reason for hiding this comment

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

Sorry for inadvertent mistake on my part.

I had correctly specified data_dir in this branch but wrong value got merged.

@@ -81,7 +81,7 @@ postgresql-cluster-prepared:
{%- endif %}
- cwd: /
- env: {{ postgres.prepare_cluster.env }}
- runas: {{ postgres.user }}
- runas: {{ postgres.prepare_cluster.user }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the previous value was postgres.prepare_cluster.user so we should revert.

Copy link
Contributor

@noelmcloughlin noelmcloughlin left a comment

Choose a reason for hiding this comment

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

Thanks for the fixes. LTGM.

@myii
Copy link
Contributor Author

myii commented Jun 21, 2018

@noelmcloughlin No problem, thanks for the review.

@noelmcloughlin
Copy link
Contributor

One general comment regarding prepare_cluster states. PR #202, #206 and #210 (& others) tried to deal with the permission problem - we do want 0700 permission on data_dir - but it remains critically important that data_dir has correct ownership (thanks for your fix) because the "prepare cluster" command is runas non-root. Hopefully we are closer to full solution with this PR.

@myii
Copy link
Contributor Author

myii commented Jun 21, 2018

@noelmcloughlin The main progress was yours in #228 but glad to have been of service.

... because the "prepare cluster" command is runas non-root ...

Except for Debian-based systems of course (due to pg_createcluster), where postgres.prepare_cluster.user evaluates to root:

prepare_cluster:
pgcommand: pg_createcluster {{ version }} main -d
user: root

@noelmcloughlin
Copy link
Contributor

@myii Good point. Even on Debian, the user/command values revert to postgres and initdb -D whenever use_upstream_repo: True is set.

@myii
Copy link
Contributor Author

myii commented Jun 21, 2018

@noelmcloughlin I'm not finding that at my end during a test run:

...

prepare_cluster:
  env: []
  pgcommand: pg_createcluster 10 testing07 -d
  pgtestfile: PG_VERSION
  user: root

...

use_upstream_repo: true

@noelmcloughlin
Copy link
Contributor

I should have checked the code before my claim. Yeah, the {% if repo.use_upstream_repo .. check in codenamemap.yaml does not affect user/client being updated by the macro. thanks for clarifying.

@noelmcloughlin noelmcloughlin merged commit 007d639 into saltstack-formulas:master Jun 21, 2018
@myii myii deleted the patch-1 branch June 28, 2018 05:12
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