Makes postgres-reload-modules conditional #216

RobRuana · 2018-06-05T18:10:21Z

This pull request makes the postgres-reload-modules conditional so it doesn't report changes during every run.

RobRuana · 2018-06-05T18:11:20Z

postgres/codenamemap.yaml

@@ -70,6 +70,7 @@
 {{ debian_codename('vivid', '9.4') }}
 {{ debian_codename('wily', '9.4') }}
 {{ debian_codename('xenial', '9.5') }}
+{{ debian_codename('bionic', '10') }}


Adds codename for Ubuntu 18.04 "bionic". This was all I needed to change to get Postgres 10 working for Ubuntu 18.04.

RobRuana · 2018-06-05T18:11:45Z

postgres/defaults.yaml

@@ -57,6 +57,8 @@ postgres:

  bake_image: False

+  manage_force_reload_modules: True


New config setting defaults to True to preserve existing behavior.

RobRuana · 2018-06-05T18:13:45Z

postgres/manage.sls

@@ -15,12 +17,16 @@ include:

 {%- endif %}

+{%- if needs_client_binaries or postgres.manage_force_reload_modules %}


I was getting tired of postgres-reload-modules running even when nothing had changed. I hid the state behind a conditional that checks if any new client binaries were installed, or if manage_force_reload_modules was set to True.

NOTE: manage_force_reload_modules defaults to True to preserve the existing behavior.

Maybemange_force_reload_modules should default to False to fix #214 @vunty what do you think?

I'm totally fine with setting mange_force_reload_modules to False by default.

myii · 2018-06-06T05:34:01Z

@RobRuana Great minds think alike... pipped you to the postgres/codenamemap.yaml change by 3 hours -- see #215. Took it a bit further by updating the list to all of the supported versions and getting rid off the deadwood.

noelmcloughlin

LGTM, I have just one outstanding query.

vutny

@RobRuana Thanks for your PR!

Since we already have a PR submitted to add support for Ubuntu 18, I propose to separate concerns and focus on fixing issue #214 here. I found it really annoys people 😃

Also I think you need to add conditional to macros.jinja file, as each state produced by format_state macro has explicit requirement on the postgres-reload-modules.

…nditional

RobRuana · 2018-06-06T18:51:16Z

Okay, I merged the codename changes from master and made manage_force_reload_modules default to False.

Furthermore, I repositioned the conditional so postgres-reload-modules always runs, so other states will always be able to reference it. The part that's conditional is whether it reports changes or not.

vutny · 2018-06-07T11:43:27Z

postgres/manage.sls

  test.succeed_with_changes:
+{%- else %}
+  test.succeed_without_changes:
+{%- endif %}


@RobRuana This seems like an obvious fix, but that wouldn't work.
Each state generated below explicitly subscribed on changes in postgres-reload-modules. So if you later change PG entities in the Pillar, it will not have any effect.
The only way I see to overcome this: you need to put conditional in format_state macro to depend on modules reload only when postgres.manage_force_reload_modules set True.

@vutny, yes, you are correct that this breaks any state using the format_state() macro.

At first I couldn't understand why onchanges: postgres-reload-modules was being used instead of require: postgres-reload-modules. But then I realized that many instances of format_state() are already using the require conditional, so there would be conflicting require keys when the state is rendered.

Correct me if I'm wrong, but I believe onchanges is being used to force correct ordering of the states, not because we want every format_state() to run when there are changes in postgres-reload-modules.

If that's correct, then we should be able to achieve the same result by using watch instead of onchanges in format_state(). I've made that change. Please let me know if you think that works.

myii · 2018-07-23T07:35:06Z

postgres/server/init.sls

@@ -184,9 +184,6 @@ postgresql-running:
  service.running:
    - name: {{ postgres.service }}
    - enable: True
-   {% if grains.os not in ('MacOS',) %}
-    - reload: True
-   {% endif %}


@RobRuana This has already been resolved by 79ab1a2 during #218:

postgres-formula/postgres/server/init.sls

Lines 141 to 151 in 332fe7b

- watch_in:

- module: postgresql-service-restart

{%- endif %}

# Restart the service where reloading is not sufficient

# Currently when the cluster is created or changes made to `postgresql.conf`

postgresql-service-restart:

module.wait:

- name: service.restart

- m_name: {{ postgres.service }}

Thanks for the notice! I'm scrambling to get everything working at the moment, and this was my quick and dirty fix. I've synced with upstream.

@RobRuana You're welcome.

Syncing with upstream

aboe76 · 2018-07-29T14:57:29Z

@vutny could you take another look and ping me if it's ok?

vutny · 2018-08-01T08:57:56Z

@aboe76 @RobRuana My in-line comment above still applies. All latter states in postgres.manage are subscribed on changes from postgres-reload-modules. So if it succeeds without changes in subsequent highstate run, there would be no further states to trigger, even if related Pillar data would be changed.

…n pg_hba.conf

RobRuana · 2018-08-01T21:14:56Z

postgres/server/init.sls

@@ -176,6 +169,15 @@ postgresql-pg_hba:
 {%- endif %}
    - require:
      - file: postgresql-config-dir
+    - watch_in:
+      - module: postgresql-service-restart


I've found that simply reloading the postgres service is not enough to pick up ACL changes in pg_hba.conf. A full restart is required.

This is on ubuntu 18.04 with postgres 10.

RobRuana · 2018-08-01T21:31:27Z

I'm going to close this pull request for now. I may revisit this issue, but for now I am okay with the postgres-reload-modules reporting changes all the time.

@RobRuana

…formulas#214 Based on the great work of @RobRuana in saltstack-formulas#216.

@RobRuana

…formulas#214 Based on the great work of @RobRuana in saltstack-formulas#216.

Adds Ubuntu "bionic" codename & makes reload-modules conditional

984cf4e

RobRuana commented Jun 5, 2018

View reviewed changes

noelmcloughlin approved these changes Jun 6, 2018

View reviewed changes

vutny requested changes Jun 6, 2018

View reviewed changes

RobRuana added 4 commits June 6, 2018 14:32

Allows postgres-reload-modules to always run, only the changes are co…

4dd99d5

…nditional

Adds Ubuntu "bionic" codename & makes reload-modules conditional

ed74e86

Allows postgres-reload-modules to always run, only the changes are co…

a9498ad

…nditional

Merge saltstack-formulas master

69fc654

RobRuana changed the title ~~Adds Ubuntu "bionic" codename & makes reload-modules conditional~~ Makes postgres-reload-modules conditional Jun 6, 2018

RobRuana added 2 commits June 6, 2018 14:48

Merge branch 'master' into master

5115e45

Removes extraneous whitespace

f6f4998

vutny reviewed Jun 7, 2018

View reviewed changes

RobRuana added 2 commits July 22, 2018 15:27

Make sure postgresql-conf runs before postgresql-running

d2408a4

Force postgresql service restart instead of reload

32f45e9

myii reviewed Jul 23, 2018

View reviewed changes

RobRuana added 2 commits July 23, 2018 12:28

Merge pull request #1 from saltstack-formulas/master

41033a2

Syncing with upstream

Reverts force service restart, as this issue was fixed upstream

42580d7

RobRuana added 2 commits August 1, 2018 10:27

Uses watch requisite for postgres-reload-modules instead of onchanges

5c77a33

Reloading the postgres service is not enough to pick up ACL changes i…

ef87358

…n pg_hba.conf

RobRuana commented Aug 1, 2018

View reviewed changes

RobRuana closed this Aug 1, 2018

myii mentioned this pull request Aug 16, 2018

Fresh install need to restart after pg_hba.conf #236

Closed

myii mentioned this pull request Jan 16, 2019

Fix #236: Restart postgres service after ACL changes in pg_hba.conf #250

Merged

alxwr added a commit to alxwr/postgres-formula that referenced this pull request Apr 30, 2019

fix(postgres.manage): only reload modules if needed; fixes saltstack-…

ad6caee

…formulas#214 Based on the great work of @RobRuana in saltstack-formulas#216.

alxwr mentioned this pull request Apr 30, 2019

fix(postgres.manage): only reload modules if needed; fixes #214 #265

Open

alxwr added a commit to alxwr/postgres-formula that referenced this pull request May 3, 2019

fix(postgres.manage): only reload modules if needed; fixes saltstack-…

feea4f1

…formulas#214 Based on the great work of @RobRuana in saltstack-formulas#216.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makes postgres-reload-modules conditional #216

Makes postgres-reload-modules conditional #216

RobRuana commented Jun 5, 2018 •

edited

Loading

RobRuana Jun 5, 2018

RobRuana Jun 5, 2018

RobRuana Jun 5, 2018 •

edited

Loading

noelmcloughlin Jun 6, 2018

vutny Jun 6, 2018

myii commented Jun 6, 2018

noelmcloughlin left a comment

vutny left a comment

RobRuana commented Jun 6, 2018

vutny Jun 7, 2018

RobRuana Aug 1, 2018

myii Jul 23, 2018

RobRuana Jul 23, 2018

myii Jul 23, 2018

aboe76 commented Jul 29, 2018

vutny commented Aug 1, 2018

RobRuana Aug 1, 2018

RobRuana commented Aug 1, 2018

		@@ -57,6 +57,8 @@ postgres:

		bake_image: False

		manage_force_reload_modules: True

		@@ -15,12 +17,16 @@ include:

		{%- endif %}

		{%- if needs_client_binaries or postgres.manage_force_reload_modules %}

	- watch_in:
	- module: postgresql-service-restart

	{%- endif %}

	# Restart the service where reloading is not sufficient
	# Currently when the cluster is created or changes made to `postgresql.conf`
	postgresql-service-restart:
	module.wait:
	- name: service.restart
	- m_name: {{ postgres.service }}

Makes postgres-reload-modules conditional #216

Makes postgres-reload-modules conditional #216

Conversation

RobRuana commented Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RobRuana Jun 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myii commented Jun 6, 2018

noelmcloughlin left a comment

Choose a reason for hiding this comment

vutny left a comment

Choose a reason for hiding this comment

RobRuana commented Jun 6, 2018

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

aboe76 commented Jul 29, 2018

vutny commented Aug 1, 2018

Choose a reason for hiding this comment

RobRuana commented Aug 1, 2018

RobRuana commented Jun 5, 2018 •

edited

Loading

RobRuana Jun 5, 2018 •

edited

Loading