Update postgresql to latest and enable it to run without root permissions #980

irvingpop · 2017-11-17T20:09:57Z

This PR updates postgresql and postgis to the latest (9.6.6 and 2.4.2 respectively) and also enables it to run without root permissions. In order to accomplish that I had to:

reorganize svc_data_path to include the pgdata and archive subfolders
remove all chown/chmod commands that require root permissions
remove all chpst statements which are no longer needed

thesentinels · 2017-11-17T20:09:58Z

Thanks for the pull request! Here is what will happen next:

Your PR will be reviewed by the maintainers
If everything looks good, one of them will approve it, and your PR will be merged.

Thank you for contributing!

irvingpop · 2017-11-17T20:12:55Z

cc: folks who have touched this plan recently: @themightychris @bodymindarts @eeyun for review

irvingpop · 2017-11-17T20:16:48Z

postgresql/config/functions.sh

@@ -59,13 +58,12 @@ master_ready() {
 bootstrap_replica_via_pg_basebackup() {
  echo 'Bootstrapping replica via pg_basebackup from leader '

-  rm -rf {{pkg.svc_data_path}}/*
-  pg_basebackup --pgdata={{pkg.svc_data_path}} --xlog-method=stream --dbname='postgres://{{cfg.replication.name}}@{{svc.leader.sys.ip}}:{{cfg.port}}/postgres'
+  rm -rf {{cfg.data_directory}}/*


Note: we should put some kind of guard around this, in case {{cfg.data_directory}} evaluates to "" and we blow away /

irvingpop · 2017-11-17T20:17:22Z

postgresql/default.toml

@@ -15,4 +16,4 @@ name = 'replication'
 password = 'replication'
 lag_health_threshold = 1048576
 enable = false
-archive_path = "{{pkg.svc_path}}/archive"
+archive_path = "hab/svc/postgresql/data/archive"


typo here, I got it

irvingpop · 2017-11-17T20:41:28Z

update: reworked the data path stuff after feedback form @elliott-davis

elliott-davis · 2017-11-17T20:50:37Z

My only other concern here is what happens to your data on upgrade?

irvingpop · 2017-11-18T00:27:30Z

super good point - this would totally break anyone who's upgrading their postgresql plan. argh!

As an alternative, what if we keep the postgres data in svc_data_path and then put the archive dir into svc_var_path? I'm unclear how much it matters for the archive directory to survive a container stop/start

elliott-davis · 2017-11-20T15:29:22Z

@irvingpop I feel like you were on to the right solution about creating a pgdata dir in data but maybe just check to see if that directory exists and if not just create and move it in the init/reconfigure hook?

irvingpop · 2017-11-20T19:26:30Z

pseudo-coding out a couple options here for the init hook, thoughts?

Auto-detect pg data in the root of svc_data_path, attempt to move it for you

if [[ -f "{{pkg.svc_data_path}}/PG_VERSION" ]]; then
  echo "PGDATA detected in the root of the data path ( {{pkg.svc_data_path}} ), relocating...."
  mkdir -p {{pkg.svc_data_path}}/pgdata
  # bash extended globbing can cleanly move everything under a subfolder http://www.linuxjournal.com/content/bash-extended-globbing
  mv {{pkg.svc_data_path}}/!(pgdata) {{pkg.svc_data_path}}/pgdata/
else
  mkdir -p {{pkg.svc_data_path}}/pgdata
fi

Auto-detect pg data in the root of svc_data_path, and exit if it exists:

if [[  -f "{{pkg.svc_data_path}}/PG_VERSION" ]]; then
  echo "PGDATA detected in the root of the data path ( {{pkg.svc_data_path}} ), please manually move it to {{pkg.svc_data_path}}/pgdata before attempting to start the service"
  exit 1
fi

irvingpop · 2017-11-20T19:55:25Z

lingering question: does the init hook have any assurances that a hab supervised service isn't running?

Update: yes, it does. we can safely relocate the data

irvingpop · 2017-11-20T21:44:38Z

okay @elliott-davis @fnichol the latest commit should make life easier for existing installations that are upgrading. In testing:

postgresql_1  | postgresql.default(SR): Initializing
postgresql_1  | postgresql.default hook[init]:(HK): PGDATA detected in the root of the data path ( /hab/svc/postgresql/data ), relocating it to /hab/svc/postgresql/data/pgdata
postgresql_1  | postgresql.default hook[init]:(HK): Making sure hab user owns var and data paths
postgresql_1  | postgresql.default(SV): Starting service as user=hab, group=hab
postgresql_1  | postgresql.default(O): Executing run hook
postgresql_1  | postgresql.default(O): Writing postgresql.local.conf file based on memory settings
postgresql_1  | postgresql.default(O): Making sure hab user owns var and data paths
postgresql_1  | postgresql.default(O): Starting PostgreSQL
postgresql_1  | postgresql.default(O): 2017-11-20 21:37:40 GMT [63]: [1-1] user=,db=,client=  (0:00000)LOG:  redirecting log output to logging collector process
postgresql_1  | postgresql.default(O): 2017-11-20 21:37:40 GMT [63]: [2-1] user=,db=,client=  (0:00000)HINT:  Future log output will appear in directory "/hab/svc/postgresql/var/pg_log".

stevendanna · 2017-11-22T18:30:03Z

postgresql/config/functions.sh

@@ -59,13 +63,14 @@ master_ready() {
 bootstrap_replica_via_pg_basebackup() {
  echo 'Bootstrapping replica via pg_basebackup from leader '

-  rm -rf {{pkg.svc_data_path}}/*
-  pg_basebackup --pgdata={{pkg.svc_data_path}} --xlog-method=stream --dbname='postgres://{{cfg.replication.name}}@{{svc.leader.sys.ip}}:{{cfg.port}}/postgres'
+  # TODO: guard this rm -rf


What kind of guard are you imagining? If there isn't a blocker, let's add it now. This rm -rf rightfully makes us nervous but it is a necessary evil with pg_basebackup.

ah, that TODO comment is probably safe to delete now that it isn't simply rm -rf {{pkg.svc_data_path}}/*.

I was thinking of this issue 🤦‍♀️ and I think the correct way to guard that is suggested by ShellCheck: https://github.com/koalaman/shellcheck/wiki/SC2115

Regardless this seems safer to me as-is now that there's the /pgdata part in the argument. even in the highly unlikely situation that Habitat goes crazy and {{pkg.svc_data_path}} evaluates to an empty string

tl;dr - removed that comment

fnichol · 2017-11-30T18:58:03Z

Okay, I'll get on trying this out--looking really good!

robbkidd · 2017-11-30T18:58:12Z

postgresql/README.md

@@ -12,7 +12,7 @@ hab start core/postgresql
 ```
 or
 ```
-docker run starkandwayne/postgresql
+docker run chefdemo/postgresql


What would the origin be here after merge?

ah, I just realized that something (bldr?) is publishing official versions of this plan to https://hub.docker.com/r/habitat/postgresql/ so that would make more sense here

stevendanna · 2017-12-01T01:42:01Z

postgresql/config/functions.sh

-  rm -rf {{pkg.svc_data_path}}/*
-  pg_basebackup --pgdata={{pkg.svc_data_path}} --xlog-method=stream --dbname='postgres://{{cfg.replication.name}}@{{svc.leader.sys.ip}}:{{cfg.port}}/postgres'
+  rm -rf {{pkg.svc_data_path}}/pgdata/*
+  pg_basebackup --pgdata={{pkg.svc_data_path}}/pgdata --xlog-method=stream --dbname='postgres://{{cfg.replication.name}}@{{svc.leader.sys.ip}}:{{cfg.port}}/postgres'


Not blocking: a basebackup can take a long time for real-world databases. -v -P will give you some more output that can be reassuring when you are on minute 5-6 (or 20-21) of bootstrapping :D.

great idea! done

rsertelon · 2017-12-13T22:06:47Z

Will this fix #887 ?

irvingpop · 2017-12-14T18:22:05Z

@rsertelon not directly because this is for the postgresql plan and that issue was filed against the postgresql94 plan. but likely these fixes are applicable.

irvingpop · 2018-01-11T17:36:50Z

…root Signed-off-by: Irving Popovetsky <[email protected]>

fnichol

Left a couple of comments, but nothing show stopping. We're close to get this merged now, testing in a couple of hours. So sorry this took so long.

fnichol · 2018-02-08T18:54:08Z

postgresql/config/functions.sh

 }

 ensure_dir_ownership() {
-  echo 'Making sure hab user owns var, config and data paths'
+  echo 'Making sure hab user owns var and data paths'
  chown -RL hab:hab {{pkg.svc_var_path}}


We should be able to get rid of this one now

fnichol · 2018-02-08T18:54:15Z

postgresql/config/functions.sh

  chown -RL hab:hab {{pkg.svc_var_path}}
-  chown -RL hab:hab {{pkg.svc_config_path}}
  chown -RL hab:hab {{pkg.svc_data_path}}


fnichol · 2018-02-08T18:55:26Z

postgresql/config/postgresql.conf

@@ -17,7 +17,7 @@ datestyle = 'iso, mdy'

 default_text_search_config = 'pg_catalog.english'

-data_directory = '{{pkg.svc_data_path}}'
+data_directory = '{{pkg.svc_data_path}}/pgdata'


While this will work, if anyone automatically updates their core/postgresql* package, the data dir will have moved. Something we'll want to think on, but despite that, I like this change.

Nevermind, I missed the init hook fix

fnichol

As we'll be testing and can resolve any issues, let's merge!

irvingpop requested review from fnichol and reset as code owners November 17, 2017 20:09

irvingpop force-pushed the update_postgres_no_root branch from 4984370 to 538edc7 Compare November 17, 2017 20:10

irvingpop commented Nov 17, 2017

View reviewed changes

irvingpop force-pushed the update_postgres_no_root branch from a806e42 to 3d56e49 Compare November 22, 2017 00:33

stevendanna reviewed Nov 22, 2017

View reviewed changes

robbkidd reviewed Nov 30, 2017

View reviewed changes

irvingpop force-pushed the update_postgres_no_root branch 2 times, most recently from 24dce3c to 9a74a47 Compare December 1, 2017 00:12

stevendanna reviewed Dec 1, 2017

View reviewed changes

eeyun approved these changes Dec 13, 2017

View reviewed changes

irvingpop mentioned this pull request Dec 21, 2017

Add libpq (postgres client library) #1098

Merged

irvingpop mentioned this pull request Jan 22, 2018

[postgresql] Fix data directory permissions #1145

Merged

irvingpop force-pushed the update_postgres_no_root branch from 222aa89 to 1ab3be7 Compare February 8, 2018 00:27

Update postgresql to 9.6.6 and enable it to run without pkg_svc_user=…

dcdf884

…root Signed-off-by: Irving Popovetsky <[email protected]>

irvingpop force-pushed the update_postgres_no_root branch from 1ab3be7 to dcdf884 Compare February 8, 2018 01:12

fnichol reviewed Feb 8, 2018

View reviewed changes

fnichol approved these changes Feb 8, 2018

View reviewed changes

fnichol merged commit 0285428 into habitat-sh:master Feb 8, 2018

rsertelon mentioned this pull request Mar 22, 2018

[postgresql94] hab svc start core/postgresql94 fails due to lack of permissions #887

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update postgresql to latest and enable it to run without root permissions #980

Update postgresql to latest and enable it to run without root permissions #980

irvingpop commented Nov 17, 2017 •

edited

Loading

thesentinels commented Nov 17, 2017

irvingpop commented Nov 17, 2017

irvingpop Nov 17, 2017

irvingpop Nov 17, 2017

irvingpop commented Nov 17, 2017

elliott-davis commented Nov 17, 2017

irvingpop commented Nov 18, 2017

elliott-davis commented Nov 20, 2017

irvingpop commented Nov 20, 2017 •

edited

Loading

irvingpop commented Nov 20, 2017 •

edited

Loading

irvingpop commented Nov 20, 2017

stevendanna Nov 22, 2017

irvingpop Nov 22, 2017

irvingpop Dec 1, 2017

fnichol commented Nov 30, 2017

robbkidd Nov 30, 2017

irvingpop Nov 30, 2017

irvingpop Dec 1, 2017

stevendanna Dec 1, 2017

irvingpop Dec 2, 2017

rsertelon commented Dec 13, 2017

irvingpop commented Dec 14, 2017

irvingpop commented Jan 11, 2018

fnichol left a comment

fnichol Feb 8, 2018

fnichol Feb 8, 2018

fnichol Feb 8, 2018

fnichol Feb 8, 2018

fnichol left a comment

Update postgresql to latest and enable it to run without root permissions #980

Update postgresql to latest and enable it to run without root permissions #980

Conversation

irvingpop commented Nov 17, 2017 • edited Loading

thesentinels commented Nov 17, 2017

irvingpop commented Nov 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

irvingpop commented Nov 17, 2017

elliott-davis commented Nov 17, 2017

irvingpop commented Nov 18, 2017

elliott-davis commented Nov 20, 2017

irvingpop commented Nov 20, 2017 • edited Loading

irvingpop commented Nov 20, 2017 • edited Loading

irvingpop commented Nov 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnichol commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsertelon commented Dec 13, 2017

irvingpop commented Dec 14, 2017

irvingpop commented Jan 11, 2018

fnichol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fnichol left a comment

Choose a reason for hiding this comment

irvingpop commented Nov 17, 2017 •

edited

Loading

irvingpop commented Nov 20, 2017 •

edited

Loading

irvingpop commented Nov 20, 2017 •

edited

Loading